Messages in this thread | | | Date | Thu, 10 Jul 2008 14:44:34 +0200 | From | Joerg Roedel <> | Subject | Re: [PATCH 17/34] AMD IOMMU: add generic defines and structures for mapping code |
| |
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@linux-foundation.org> wrote: > > > On Thu, 26 Jun 2008 21:27:53 +0200 Joerg Roedel <joerg.roedel@amd.com> wrote: > > > > > This patch adds generic stuff used by the mapping and unmapping code for the > > > AMD IOMMU. > > > > > > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> > > > --- > > > 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@amd.com> > > > + * Leo Duran <leo.duran@amd.com> > > > + * > > > + * 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
| |