lkml.org 
[lkml]   [2008]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 25/34] AMD IOMMU: add dma_ops mapping functions for single mappings
On Wed, Jul 09, 2008 at 07:26:06PM -0700, Andrew Morton wrote:
> On Thu, 26 Jun 2008 21:28:01 +0200 Joerg Roedel <joerg.roedel@amd.com> wrote:
>
> > This patch adds the dma_ops specific mapping functions for single mappings.
> >
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> > arch/x86/kernel/amd_iommu.c | 59 +++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 59 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index e00a3e7..b4079f6 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -40,6 +40,11 @@ struct command {
> > static int dma_ops_unity_map(struct dma_ops_domain *dma_dom,
> > struct unity_map_entry *e);
> >
> > +static int iommu_has_npcache(struct amd_iommu *iommu)
> > +{
> > + return iommu->cap & IOMMU_CAP_NPCACHE;
> > +}
> > +
> > static int __iommu_queue_command(struct amd_iommu *iommu, struct command *cmd)
> > {
> > u32 tail, head;
> > @@ -641,3 +646,57 @@ static void __unmap_single(struct amd_iommu *iommu,
> > dma_ops_free_addresses(dma_dom, dma_addr, pages);
> > }
> >
> > +static dma_addr_t map_single(struct device *dev, phys_addr_t paddr,
> > + size_t size, int dir)
> > +{
> > + unsigned long flags;
> > + struct amd_iommu *iommu;
> > + struct protection_domain *domain;
> > + u16 devid;
> > + dma_addr_t addr;
> > +
> > + get_device_resources(dev, &iommu, &domain, &devid);
> > +
> > + if (iommu == NULL || domain == NULL)
> > + return (dma_addr_t)paddr;
>
> OK, a test case. A reader of your code (ie: me) wants to find out what
> this code is doing. What is the *meaning* of iommu == NULL || domain ==
> NULL here?
>
> I go look at the (undocumented) get_device_resources() and I see that this:
>
> if (_bdf >= amd_iommu_last_bdf) {
>
> happened. I don't know what that semantically means and I gave up.
>
> I'm not saying that the code is unmaintainable, but I would assert that it
> is a heck of a lot harder to maintain than it could be, and than it should
> be.
>
> get_device_resources() has an open-coded copy of your DEVID() macro, btw.

Ok, thats a bit weird. If the check above fails it means that we have a
device the IOMMU is not translating. In this case the code just returns
the physical address of the buffer. I will add a comment for that too.

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



\
 
 \ /
  Last update: 2008-07-10 15:23    [W:0.435 / U:0.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site