Re: [PATCH 17/34] AMD IOMMU: add generic defines and structures for mapping code

From: Joerg Roedel
Date: Thu Jul 10 2008 - 08:45:34 EST


On Thu, Jul 10, 2008 at 01:25:26PM +0900, FUJITA Tomonori wrote:
> On Wed, 9 Jul 2008 19:01:40 -0700
> Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Thu, 26 Jun 2008 21:27:53 +0200 Joerg Roedel <joerg.roedel@xxxxxxx> wrote:
> >
> > > This patch adds generic stuff used by the mapping and unmapping code for the
> > > AMD IOMMU.
> > >
> > > Signed-off-by: Joerg Roedel <joerg.roedel@xxxxxxx>
> > > ---
> > > arch/x86/kernel/amd_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > > 1 files changed, 40 insertions(+), 0 deletions(-)
> > > create mode 100644 arch/x86/kernel/amd_iommu.c
> > >
> > > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > > new file mode 100644
> > > index 0000000..90392c7
> > > --- /dev/null
> > > +++ b/arch/x86/kernel/amd_iommu.c
> > > @@ -0,0 +1,40 @@
> > > +/*
> > > + * Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
> > > + * Author: Joerg Roedel <joerg.roedel@xxxxxxx>
> > > + * Leo Duran <leo.duran@xxxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU General Public License version 2 as published
> > > + * by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, write to the Free Software
> > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> > > + */
> > > +
> > > +#include <linux/pci.h>
> > > +#include <linux/gfp.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/iommu-helper.h>
> > > +#include <asm/proto.h>
> > > +#include <asm/gart.h>
> > > +#include <asm/amd_iommu_types.h>
> > > +
> > > +#define CMD_SET_TYPE(cmd, t) ((cmd)->data[1] |= ((t) << 28))
> >
> > Doesn't need to be implemented in a macro, hence should be implemented in C,
> > please.
> >
> > > +#define to_pages(addr, size) \
> > > + (round_up(((addr) & ~PAGE_MASK) + (size), PAGE_SIZE) >> PAGE_SHIFT)
> >
> > Does this patchset add any comments *at all*? Is it really all so obvious
> > that this was justifiable?
> >
> > I don't know what this function does, but it shouldn't be doing it here.
> > Either it duplicates an existing helper or it should be implemented in mm.h
>
> I think that this calculates the number of IOMMU pages for a
> request. Every IOMMUs has a similar function so it might be better to
> add a helper function to include/linux/iommu-helper.h.
>
> Someone except for IOMMU need this? If so, it would be better to add
> this to a different header file.
>
>
> diff --git a/include/linux/iommu-helper.h b/include/linux/iommu-helper.h
> index c975caf..9c29e65 100644
> --- a/include/linux/iommu-helper.h
> +++ b/include/linux/iommu-helper.h
> @@ -8,3 +8,14 @@ extern unsigned long iommu_area_alloc(unsigned long *map, unsigned long size,
> unsigned long align_mask);
> extern void iommu_area_free(unsigned long *map, unsigned long start,
> unsigned int nr);
> +
> +static inline unsigned long iommu_num_pages(unsigned long addr,
> + unsigned long len)
> +{
> + unsigned long npages;
> +
> + npages = ALIGN(addr + len, 1UL << IOMMU_PAGE_SHIFT) -
> + (addr & ~((1UL << IOMMU_PAGE_SHIFT) - 1));
> +
> + return (npages >> IOMMU_PAGE_SHIFT);
> +}
>

Yeah. I fully agree that this function would make sense in the
iommu-helper code. Will you send a patch introducing that function
upstream? I will change my code to use it if it is merged.

Thanks,

Joerg

--
| AMD Saxony Limited Liability Company & Co. KG
Operating | Wilschdorfer Landstr. 101, 01109 Dresden, Germany
System | Register Court Dresden: HRA 4896
Research | General Partner authorized to represent:
Center | AMD Saxony LLC (Wilmington, Delaware, US)
| General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/