linux-kernel - Re: [PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20080904130800O.fujita.tomonori@lab.ntt.co.jp>
Date:	Thu, 4 Sep 2008 13:11:46 +0900
From:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
To:	joro@...tes.org
Cc:	fujita.tomonori@....ntt.co.jp, linux-kernel@...r.kernel.org,
	tony.luck@...el.com, iommu@...ts.linux-foundation.org,
	mingo@...hat.com, kamezawa.hiroyu@...fujitsu.com
Subject: Re: [PATCH 1/3] x86: remove the NULL device hack in dma-mapping.h

On Wed, 3 Sep 2008 22:01:14 +0200
Joerg Roedel <joro@...tes.org> wrote:

> On Thu, Sep 04, 2008 at 03:04:23AM +0900, FUJITA Tomonori wrote:
> > dma_alloc_coherent in dma-mapping.h has a hack to use
> > x86_dma_fallback_dev if a pointer to a device is NULL. Some of IOMMUs
> > don't need such hack. The hack also makes it difficult for IOMMUs to
> > make a proper decision because the hack hides the information.
> 
> I don't think its the right way to work around shortcomings of the
> generic code in the architecture specific implementations. Especially
> when the generic code can be easily fixed like in this case.

Well, the generic code should not have such work around.

As I wrote in another mail, you try to bring back the tricks used in
arch/x86/kernel/pci-dma.c to lib/swiotlb.c. I try to bring back them
to arch/x86/kernel/pci-swiotlb.c.


> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
> > ---
> >  arch/x86/kernel/pci-swiotlb_64.c |   15 ++++++++++++++-
> >  include/asm-x86/dma-mapping.h    |    9 +--------
> >  2 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/pci-swiotlb_64.c b/arch/x86/kernel/pci-swiotlb_64.c
> > index c4ce033..f3d8d0e 100644
> > --- a/arch/x86/kernel/pci-swiotlb_64.c
> > +++ b/arch/x86/kernel/pci-swiotlb_64.c
> > @@ -11,6 +11,8 @@
> >  
> >  int swiotlb __read_mostly;
> >  
> > +extern struct device x86_dma_fallback_dev;
> > +
> >  static dma_addr_t
> >  swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
> >  			int direction)
> > @@ -18,9 +20,20 @@ swiotlb_map_single_phys(struct device *hwdev, phys_addr_t paddr, size_t size,
> >  	return swiotlb_map_single(hwdev, phys_to_virt(paddr), size, direction);
> >  }
> >  
> > +static void *x86_swiotlb_alloc_coherent(struct device *dev, size_t size,
> > +					dma_addr_t *dma_handle, gfp_t gfp)
> > +{
> > +	if (!dev) {
> > +		dev = &x86_dma_fallback_dev;
> > +		gfp |= GFP_DMA;
> > +	}
> 
> This really should be checked in the generic x86 dma_alloc_coherent
> function.

I don't think so. Any motherboards with the recent IOMMUs support ISA?


> > +
> > +	return swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> > +}
> > +
> >  struct dma_mapping_ops swiotlb_dma_ops = {
> >  	.mapping_error = swiotlb_dma_mapping_error,
> > -	.alloc_coherent = swiotlb_alloc_coherent,
> > +	.alloc_coherent = x86_swiotlb_alloc_coherent,
> >  	.free_coherent = swiotlb_free_coherent,
> >  	.map_single = swiotlb_map_single_phys,
> >  	.unmap_single = swiotlb_unmap_single,
> > diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
> > index bc6c8df..3463702 100644
> > --- a/include/asm-x86/dma-mapping.h
> > +++ b/include/asm-x86/dma-mapping.h
> > @@ -14,7 +14,6 @@
> >  
> >  extern dma_addr_t bad_dma_address;
> >  extern int iommu_merge;
> > -extern struct device x86_dma_fallback_dev;
> >  extern int panic_on_overflow;
> >  extern int force_iommu;
> >  
> > @@ -251,14 +250,8 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
> >  	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
> >  		return memory;
> >  
> > -	if (!dev) {
> > -		dev = &x86_dma_fallback_dev;
> > -		gfp |= GFP_DMA;
> > -	}
> 
> Why do you move this check to swiotlb implemenation? This will break
> existing IOMMU implementations which don't check for a valid dev
> pointer?

As I wrote above, I don't think so (though I forgot to add this to
pci-nommu.c).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/