Re: [PATCH 25/34] AMD IOMMU: add dma_ops mapping functions for single mappings

[Joerg Roedel <joerg.roedel@amd.com>]

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