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

[FUJITA Tomonori]

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);
+}
--
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/