[PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

Currently, gart IOMMU ignores device's dma_mask when it does virtual
mappings. So it could give a device a virtual address that the device
can't access to.

Some IOMMUs, in x86 IOMMUs GART and Calgary, have this issue. This
patchset fixes GART.

The first and second patches add helper functions, useful for some
IOMMUs (both are taken from POWER IOMMU code). I'll convert some
IOMMUs to use the functions after they are merged into mainline (to
avoid complicated dependence on multiple trees).

This is against tip/x86/iommu plus a patch that I sent yesterday:

http://lkml.org/lkml/2008/9/11/147.

[PATCH 1/3] add iommu_device_max_index IOMMU helper function

[FUJITA Tomonori]

This function helps IOMMUs to know the highest address that a device
can access to.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 include/linux/iommu-helper.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/iommu-helper.h b/include/linux/iommu-helper.h
index c975caf..58f4110 100644
--- a/include/linux/iommu-helper.h
+++ b/include/linux/iommu-helper.h
@@ -1,3 +1,13 @@
+static inline unsigned long iommu_device_max_index(unsigned long size,
+						   unsigned long offset,
+						   u64 dma_mask)
+{
+	if (size + offset > dma_mask)
+		return dma_mask - offset + 1;
+	else
+		return size;
+}
+
 extern int iommu_is_span_boundary(unsigned int index, unsigned int nr,
 				  unsigned long shift,
 				  unsigned long boundary_size);
-- 
1.5.5.GIT

[PATCH 2/3] add dma_get_mask helper function

[FUJITA Tomonori]

Several IOMMUs do the same thing to get the dma_mask of a device. This
adds a helper function to do the same thing to sweep them.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 include/linux/dma-mapping.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 6ed50c1..0dba743 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -63,6 +63,13 @@ static inline int is_buffer_dma_capable(u64 mask, dma_addr_t addr, size_t size)
 #define dma_sync_single		dma_sync_single_for_cpu
 #define dma_sync_sg		dma_sync_sg_for_cpu
 
+static inline u64 dma_get_mask(struct device *dev)
+{
+	if (dev->dma_mask && *dev->dma_mask)
+		return *dev->dma_mask;
+	return DMA_32BIT_MASK;
+}
+
 extern u64 dma_get_required_mask(struct device *dev);
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
1.5.5.GIT

[PATCH 3/3] x86: make GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

Currently, GART IOMMU ingores device's dma_mask when it does virtual
mappings. So it could give a device a virtual address that the device
can't access to.

This patch fixes the above problem.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-gart_64.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index 47abe43..9739d56 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -83,23 +83,34 @@ static unsigned long next_bit;  /* protected by iommu_bitmap_lock */
 static int need_flush;		/* global flush state. set for each gart wrap */
 
 static unsigned long alloc_iommu(struct device *dev, int size,
-				 unsigned long align_mask)
+				 unsigned long align_mask, u64 dma_mask)
 {
 	unsigned long offset, flags;
 	unsigned long boundary_size;
 	unsigned long base_index;
+	unsigned long limit;
 
 	base_index = ALIGN(iommu_bus_base & dma_get_seg_boundary(dev),
 			   PAGE_SIZE) >> PAGE_SHIFT;
 	boundary_size = ALIGN((unsigned long long)dma_get_seg_boundary(dev) + 1,
 			      PAGE_SIZE) >> PAGE_SHIFT;
 
+	limit = iommu_device_max_index(iommu_pages,
+				       DIV_ROUND_UP(iommu_bus_base, PAGE_SIZE),
+				       dma_mask >> PAGE_SHIFT);
+
 	spin_lock_irqsave(&iommu_bitmap_lock, flags);
-	offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, next_bit,
+
+	if (limit <= next_bit) {
+		need_flush = 1;
+		next_bit = 0;
+	}
+
+	offset = iommu_area_alloc(iommu_gart_bitmap, limit, next_bit,
 				  size, base_index, boundary_size, align_mask);
-	if (offset == -1) {
+	if (offset == -1 && next_bit) {
 		need_flush = 1;
-		offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, 0,
+		offset = iommu_area_alloc(iommu_gart_bitmap, limit, 0,
 					  size, base_index, boundary_size,
 					  align_mask);
 	}
@@ -228,12 +239,14 @@ nonforced_iommu(struct device *dev, unsigned long addr, size_t size)
  * Caller needs to check if the iommu is needed and flush.
  */
 static dma_addr_t dma_map_area(struct device *dev, dma_addr_t phys_mem,
-				size_t size, int dir, unsigned long align_mask)
+			       size_t size, int dir, unsigned long align_mask,
+			       u64 dma_mask)
 {
 	unsigned long npages = iommu_num_pages(phys_mem, size);
-	unsigned long iommu_page = alloc_iommu(dev, npages, align_mask);
+	unsigned long iommu_page;
 	int i;
 
+	iommu_page = alloc_iommu(dev, npages, align_mask, dma_mask);
 	if (iommu_page == -1) {
 		if (!nonforced_iommu(dev, phys_mem, size))
 			return phys_mem;
@@ -263,7 +276,7 @@ gart_map_single(struct device *dev, phys_addr_t paddr, size_t size, int dir)
 	if (!need_iommu(dev, paddr, size))
 		return paddr;
 
-	bus = dma_map_area(dev, paddr, size, dir, 0);
+	bus = dma_map_area(dev, paddr, size, dir, 0, dma_get_mask(dev));
 	flush_gart();
 
 	return bus;
@@ -314,6 +327,7 @@ static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
 {
 	struct scatterlist *s;
 	int i;
+	u64 dma_mask = dma_get_mask(dev);
 
 #ifdef CONFIG_IOMMU_DEBUG
 	printk(KERN_DEBUG "dma_map_sg overflow\n");
@@ -323,7 +337,8 @@ static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
 		unsigned long addr = sg_phys(s);
 
 		if (nonforced_iommu(dev, addr, s->length)) {
-			addr = dma_map_area(dev, addr, s->length, dir, 0);
+			addr = dma_map_area(dev, addr, s->length, dir, 0,
+					    dma_mask);
 			if (addr == bad_dma_address) {
 				if (i > 0)
 					gart_unmap_sg(dev, sg, i, dir);
@@ -345,14 +360,16 @@ static int __dma_map_cont(struct device *dev, struct scatterlist *start,
 			  int nelems, struct scatterlist *sout,
 			  unsigned long pages)
 {
-	unsigned long iommu_start = alloc_iommu(dev, pages, 0);
-	unsigned long iommu_page = iommu_start;
+	unsigned long iommu_start;
+	unsigned long iommu_page;
 	struct scatterlist *s;
 	int i;
 
+	iommu_start = alloc_iommu(dev, pages, 0, dma_get_mask(dev));
 	if (iommu_start == -1)
 		return -1;
 
+	iommu_page = iommu_start;
 	for_each_sg(start, s, nelems, i) {
 		unsigned long pages, addr;
 		unsigned long phys_addr = s->dma_address;
@@ -505,7 +522,7 @@ gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
 	align_mask = (1UL << get_order(size)) - 1;
 
 	*dma_addr = dma_map_area(dev, paddr, size, DMA_BIDIRECTIONAL,
-				 align_mask);
+				 align_mask, dma_mask);
 	flush_gart();
 
 	if (*dma_addr != bad_dma_address)
-- 
1.5.5.GIT

Re: [PATCH 3/3] x86: make GART to respect device's dma_mask about virtual mappings

[Joerg Roedel]

On Fri, Sep 12, 2008 at 07:42:35PM +0900, FUJITA Tomonori wrote:
> Currently, GART IOMMU ingores device's dma_mask when it does virtual
> mappings. So it could give a device a virtual address that the device
> can't access to.
> 
> This patch fixes the above problem.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  arch/x86/kernel/pci-gart_64.c |   39 ++++++++++++++++++++++++++++-----------
>  1 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
> index 47abe43..9739d56 100644
> --- a/arch/x86/kernel/pci-gart_64.c
> +++ b/arch/x86/kernel/pci-gart_64.c
> @@ -83,23 +83,34 @@ static unsigned long next_bit;  /* protected by iommu_bitmap_lock */
>  static int need_flush;		/* global flush state. set for each gart wrap */
>  
>  static unsigned long alloc_iommu(struct device *dev, int size,
> -				 unsigned long align_mask)
> +				 unsigned long align_mask, u64 dma_mask)

You can calculate the dma_mask in this function from the dev parameter.
There is no need to pass it two levels down to this function extending
various parameter lists.

>  {
>  	unsigned long offset, flags;
>  	unsigned long boundary_size;
>  	unsigned long base_index;
> +	unsigned long limit;
>  
>  	base_index = ALIGN(iommu_bus_base & dma_get_seg_boundary(dev),
>  			   PAGE_SIZE) >> PAGE_SHIFT;
>  	boundary_size = ALIGN((unsigned long long)dma_get_seg_boundary(dev) + 1,
>  			      PAGE_SIZE) >> PAGE_SHIFT;
>  
> +	limit = iommu_device_max_index(iommu_pages,
> +				       DIV_ROUND_UP(iommu_bus_base, PAGE_SIZE),
> +				       dma_mask >> PAGE_SHIFT);
> +
>  	spin_lock_irqsave(&iommu_bitmap_lock, flags);
> -	offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, next_bit,
> +
> +	if (limit <= next_bit) {
> +		need_flush = 1;
> +		next_bit = 0;
> +	}
> +
> +	offset = iommu_area_alloc(iommu_gart_bitmap, limit, next_bit,
>  				  size, base_index, boundary_size, align_mask);
> -	if (offset == -1) {
> +	if (offset == -1 && next_bit) {
>  		need_flush = 1;
> -		offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, 0,
> +		offset = iommu_area_alloc(iommu_gart_bitmap, limit, 0,
>  					  size, base_index, boundary_size,
>  					  align_mask);
>  	}
> @@ -228,12 +239,14 @@ nonforced_iommu(struct device *dev, unsigned long addr, size_t size)
>   * Caller needs to check if the iommu is needed and flush.
>   */
>  static dma_addr_t dma_map_area(struct device *dev, dma_addr_t phys_mem,
> -				size_t size, int dir, unsigned long align_mask)
> +			       size_t size, int dir, unsigned long align_mask,
> +			       u64 dma_mask)
>  {
>  	unsigned long npages = iommu_num_pages(phys_mem, size);
> -	unsigned long iommu_page = alloc_iommu(dev, npages, align_mask);
> +	unsigned long iommu_page;
>  	int i;
>  
> +	iommu_page = alloc_iommu(dev, npages, align_mask, dma_mask);
>  	if (iommu_page == -1) {
>  		if (!nonforced_iommu(dev, phys_mem, size))
>  			return phys_mem;
> @@ -263,7 +276,7 @@ gart_map_single(struct device *dev, phys_addr_t paddr, size_t size, int dir)
>  	if (!need_iommu(dev, paddr, size))
>  		return paddr;
>  
> -	bus = dma_map_area(dev, paddr, size, dir, 0);
> +	bus = dma_map_area(dev, paddr, size, dir, 0, dma_get_mask(dev));
>  	flush_gart();
>  
>  	return bus;
> @@ -314,6 +327,7 @@ static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
>  {
>  	struct scatterlist *s;
>  	int i;
> +	u64 dma_mask = dma_get_mask(dev);
>  
>  #ifdef CONFIG_IOMMU_DEBUG
>  	printk(KERN_DEBUG "dma_map_sg overflow\n");
> @@ -323,7 +337,8 @@ static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
>  		unsigned long addr = sg_phys(s);
>  
>  		if (nonforced_iommu(dev, addr, s->length)) {
> -			addr = dma_map_area(dev, addr, s->length, dir, 0);
> +			addr = dma_map_area(dev, addr, s->length, dir, 0,
> +					    dma_mask);
>  			if (addr == bad_dma_address) {
>  				if (i > 0)
>  					gart_unmap_sg(dev, sg, i, dir);
> @@ -345,14 +360,16 @@ static int __dma_map_cont(struct device *dev, struct scatterlist *start,
>  			  int nelems, struct scatterlist *sout,
>  			  unsigned long pages)
>  {
> -	unsigned long iommu_start = alloc_iommu(dev, pages, 0);
> -	unsigned long iommu_page = iommu_start;
> +	unsigned long iommu_start;
> +	unsigned long iommu_page;
>  	struct scatterlist *s;
>  	int i;
>  
> +	iommu_start = alloc_iommu(dev, pages, 0, dma_get_mask(dev));
>  	if (iommu_start == -1)
>  		return -1;
>  
> +	iommu_page = iommu_start;
>  	for_each_sg(start, s, nelems, i) {
>  		unsigned long pages, addr;
>  		unsigned long phys_addr = s->dma_address;
> @@ -505,7 +522,7 @@ gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
>  	align_mask = (1UL << get_order(size)) - 1;
>  
>  	*dma_addr = dma_map_area(dev, paddr, size, DMA_BIDIRECTIONAL,
> -				 align_mask);
> +				 align_mask, dma_mask);
>  	flush_gart();
>  
>  	if (*dma_addr != bad_dma_address)
> -- 
> 1.5.5.GIT
> 
> 

-- 
           |           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

Re: [PATCH 3/3] x86: make GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Fri, 12 Sep 2008 16:52:27 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Fri, Sep 12, 2008 at 07:42:35PM +0900, FUJITA Tomonori wrote:
> > Currently, GART IOMMU ingores device's dma_mask when it does virtual
> > mappings. So it could give a device a virtual address that the device
> > can't access to.
> > 
> > This patch fixes the above problem.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  arch/x86/kernel/pci-gart_64.c |   39 ++++++++++++++++++++++++++++-----------
> >  1 files changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
> > index 47abe43..9739d56 100644
> > --- a/arch/x86/kernel/pci-gart_64.c
> > +++ b/arch/x86/kernel/pci-gart_64.c
> > @@ -83,23 +83,34 @@ static unsigned long next_bit;  /* protected by iommu_bitmap_lock */
> >  static int need_flush;		/* global flush state. set for each gart wrap */
> >  
> >  static unsigned long alloc_iommu(struct device *dev, int size,
> > -				 unsigned long align_mask)
> > +				 unsigned long align_mask, u64 dma_mask)
> 
> You can calculate the dma_mask in this function from the dev parameter.
> There is no need to pass it two levels down to this function extending
> various parameter lists.

No, we can't because we need to use dev->coherent_dma_mask for
alloc_coherent and dev->dma_mask for the rest.

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> Currently, gart IOMMU ignores device's dma_mask when it does virtual
> mappings. So it could give a device a virtual address that the device
> can't access to.
> 
> Some IOMMUs, in x86 IOMMUs GART and Calgary, have this issue. This
> patchset fixes GART.
> 
> The first and second patches add helper functions, useful for some
> IOMMUs (both are taken from POWER IOMMU code). I'll convert some
> IOMMUs to use the functions after they are merged into mainline (to
> avoid complicated dependence on multiple trees).
> 
> This is against tip/x86/iommu plus a patch that I sent yesterday:
> 
> http://lkml.org/lkml/2008/9/11/147.

applied them to tip/x86/iommu:

bee44f2: x86: make GART to respect device's dma_mask about virtual mappings
589fc9a: iommu: add dma_get_mask helper function
eecfffc: iommu: add iommu_device_max_index IOMMU helper function

thanks!

	Ingo

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:

> Currently, gart IOMMU ignores device's dma_mask when it does virtual
> mappings. So it could give a device a virtual address that the device
> can't access to.

Huh? That is what the need_iommu() logic in gart_map_sg()
does. An I'm not aware of any bugs in this area.

Did you actually see that failure in practice? I don't see
how it could happen.

-Andi
-- 
ak@linux.intel.com

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Tue, 16 Sep 2008 02:54:32 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:
> 
> > Currently, gart IOMMU ignores device's dma_mask when it does virtual
> > mappings. So it could give a device a virtual address that the device
> > can't access to.
> 
> Huh? That is what the need_iommu() logic in gart_map_sg()
> does. An I'm not aware of any bugs in this area.

What the need_iommu() does is seeing if GART needs to do virtual
mappings or not.

(After need_iommu() checking) What this patchset does is to guarantee
that GART provides a virtual address that a device can access to.


> Did you actually see that failure in practice? I don't see
> how it could happen.

No, I did not. This patchset does the right thing theoretically, I
think, but if such problem never happens for GART, I'll drop the patch
for GART. Joerg?

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

> What the need_iommu() does is seeing if GART needs to do virtual
> mappings or not.
> 
> (After need_iommu() checking) What this patchset does is to guarantee
> that GART provides a virtual address that a device can access to.

Ah you care about masks < 32bit?

Those always are handled elsewhere in the block layer (using the bounce_pfn
mechanism) or in various other ways in other subsystems. e.g. on networking
the rule is that you just don't announce direct SG when you have 
less than 32bit mask. And the others like sound generally don't use 
map_sg()/map_single(), but instead pre allocate something with 
dma_alloc_coherent() or similar.

Also what would you do if it this check fails? There is no suitable
fallback path.

Can you describe a concrete use case your patch fixes? 

Anyways I'm aware the semantics are a little non untuitive (I didn't
invent them, but inherited them from other 64bit IOMMU implementations),
but fully general handling of < 32bit masks would be extremly complicated
because you would always need to have a full arbitary fallback
swiotlb implemention that is able to allocate arbitary low memory.

-Andi

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Tue, 16 Sep 2008 15:43:35 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> > What the need_iommu() does is seeing if GART needs to do virtual
> > mappings or not.
> > 
> > (After need_iommu() checking) What this patchset does is to guarantee
> > that GART provides a virtual address that a device can access to.
> 
> Ah you care about masks < 32bit?

Yes,


> Those always are handled elsewhere in the block layer (using the bounce_pfn
> mechanism)

I don't think that the bounce guarantees that dma_alloc_coherent()
returns an address that a device can access to.


> or in various other ways in other subsystems. e.g. on networking
> the rule is that you just don't announce direct SG when you have 
> less than 32bit mask.

I'm not familiar with what the networking does, for example, seems
that b44 sets dev->dma_mask to DMA_30BIT_MASK and b44_start_xmit()
does:

mapping = ssb_dma_map_single(bp->sdev, skb->data, len, DMA_TO_DEVICE);
if (ssb_dma_mapping_error(bp->sdev, mapping) || mapping + len > DMA_30BIT_MASK) {
	struct sk_buff *bounce_skb;

	/* Chip can't handle DMA to/from >1GB, use bounce buffer */
	if (!ssb_dma_mapping_error(bp->sdev, mapping))
		ssb_dma_unmap_single(bp->sdev, mapping, len,
				     DMA_TO_DEVICE);

IOMMUs can try to return an address that the NIC can access to.

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

> > Those always are handled elsewhere in the block layer (using the bounce_pfn
> > mechanism)
> 
> I don't think that the bounce guarantees that dma_alloc_coherent()
> returns an address that a device can access to.

dma_alloc_coherent() is not used for block IO data. And dma_alloc_coherent()
does handle masks > 24bit < 32bits just fine.


> I'm not familiar with what the networking does, for example, seems
> that b44 sets dev->dma_mask to DMA_30BIT_MASK and b44_start_xmit()
> does:
> 

b44 (and related designs like the bcm wireless chipset) 
has its own bouncing scheme. IT doesn't need any hacks in map_sg
 
> IOMMUs can try to return an address that the NIC can access to.

It's not worth to handle this strange case. The drivers do anyways.
These are very cheap devices which are only rarely used in systems
with >2GB and for those cases the existing bouncing setup works fine.

-Andi

-- 
ak@linux.intel.com

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Tue, 16 Sep 2008 19:58:24 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> > > Those always are handled elsewhere in the block layer (using the bounce_pfn
> > > mechanism)
> > 
> > I don't think that the bounce guarantees that dma_alloc_coherent()
> > returns an address that a device can access to.
> 
> dma_alloc_coherent() is not used for block IO data. And dma_alloc_coherent()
> does handle masks > 24bit < 32bits just fine.

What do you mean? For example, some aacraid cards have 31bit dma
mask. What guarantees that IOMMUs's dma_alloc_coherent don't return a
virtual address > 31bit < 32bit?


> > I'm not familiar with what the networking does, for example, seems
> > that b44 sets dev->dma_mask to DMA_30BIT_MASK and b44_start_xmit()
> > does:
> > 
> 
> b44 (and related designs like the bcm wireless chipset) 
> has its own bouncing scheme. IT doesn't need any hacks in map_sg
>  
> > IOMMUs can try to return an address that the NIC can access to.
> 
> It's not worth to handle this strange case. The drivers do anyways.
> These are very cheap devices which are only rarely used in systems
> with >2GB and for those cases the existing bouncing setup works fine.

I think that the patch is a pretty straightforward, it just the same
thing that IOMMUs with > 32bits virtual address space do. We can do
better with the simple patch. But I'm ok with dropping the patch for
GART since we can live without the patch, as you said.

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

On Wed, Sep 17, 2008 at 08:53:42AM +0900, FUJITA Tomonori wrote:
> On Tue, 16 Sep 2008 19:58:24 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > > Those always are handled elsewhere in the block layer (using the bounce_pfn
> > > > mechanism)
> > > 
> > > I don't think that the bounce guarantees that dma_alloc_coherent()
> > > returns an address that a device can access to.
> > 
> > dma_alloc_coherent() is not used for block IO data. And dma_alloc_coherent()
> > does handle masks > 24bit < 32bits just fine.
> 
> What do you mean? For example, some aacraid cards have 31bit dma
> mask. What guarantees that IOMMUs's dma_alloc_coherent don't return a
> virtual address > 31bit < 32bit?

At least the old IOMMU implementations (GART, non GART) handled this
by falling back to GFP_DMA. I haven't checked if that didn't get broken
in the recent reorganization, but if it got it should be fixed of course.
But hopefully it still works.

But that has nothing to do with _map_sg() anyways. The semantics here
are quite different, matching traditional i386 semantics. On i386
_alloc_coherent always did this logic, and map_sg/map_single didn't
do anything.


> 
> 
> > > I'm not familiar with what the networking does, for example, seems
> > > that b44 sets dev->dma_mask to DMA_30BIT_MASK and b44_start_xmit()
> > > does:
> > > 
> > 
> > b44 (and related designs like the bcm wireless chipset) 
> > has its own bouncing scheme. IT doesn't need any hacks in map_sg
> >  
> > > IOMMUs can try to return an address that the NIC can access to.
> > 
> > It's not worth to handle this strange case. The drivers do anyways.
> > These are very cheap devices which are only rarely used in systems
> > with >2GB and for those cases the existing bouncing setup works fine.
> 
> I think that the patch is a pretty straightforward, it just the same
> thing that IOMMUs with > 32bits virtual address space do. We can do
> better with the simple patch. But I'm ok with dropping the patch for
> GART since we can live without the patch, as you said.

The semantics are a little weird and non intuitive I agree. Perhaps it would 
make sense to document them properly? 

With the DMA allocator rework I did some time ago it would be actually
relatively straight forward to do. But to do it properly would
require doing it for 32bit x86 too, and frankly I don't think it's
worth doing there. Devices with such weird masks are on their way out
and after we survived this plague for years and finally things are 
getting better I don't think it's worth complicating basic infrastructure
for this.

-Andi

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Wed, 17 Sep 2008 02:24:04 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> On Wed, Sep 17, 2008 at 08:53:42AM +0900, FUJITA Tomonori wrote:
> > On Tue, 16 Sep 2008 19:58:24 +0200
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > > > Those always are handled elsewhere in the block layer (using the bounce_pfn
> > > > > mechanism)
> > > > 
> > > > I don't think that the bounce guarantees that dma_alloc_coherent()
> > > > returns an address that a device can access to.
> > > 
> > > dma_alloc_coherent() is not used for block IO data. And dma_alloc_coherent()
> > > does handle masks > 24bit < 32bits just fine.
> > 
> > What do you mean? For example, some aacraid cards have 31bit dma
> > mask. What guarantees that IOMMUs's dma_alloc_coherent don't return a
> > virtual address > 31bit < 32bit?
> 
> At least the old IOMMU implementations (GART, non GART) handled this
> by falling back to GFP_DMA. I haven't checked if that didn't get broken
> in the recent reorganization, but if it got it should be fixed of course.
> But hopefully it still works.

The falling back mechanism was moved to pci-nommu from the common code
since it doesn't work for other IOMMUs that always need virtual
mappings. Calgary needs this dma_mask trick too but I guess that it's
unlikely that the IBM servers with Calgary have weird hardware.

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

> The falling back mechanism was moved to pci-nommu from the common code
> since it doesn't work for other IOMMUs that always need virtual

There's no fallback for _map_sg/_map_single. All the fallback to GFP
only works for coherent allocations, but not for streaming mappings.

To make this "fully robust" for masks < 32bit you would need to implement 
a new swiotlb that uses GFP_DMA allocations as fallback (or use the DMA 
allocator's swiotlb which can actually handle this)

So you're right now basically checking for something that you cannot
fix. And also you try to check for (but not handle) something that even 
32bit x86 doesn't handle. So if some driver relied on you checking
for it on 64bit it wouldn't work on 32bit x86 which would be a bad 
thing.

> mappings. Calgary needs this dma_mask trick too but I guess that it's
> unlikely that the IBM servers with Calgary have weird hardware.

On a full IOMMU like calgary it's easier to do because you don't
 need to deal with GFP_DMA at least. But as you say it's unlikely
to be worth the effort on these systems.

Also the earlier problem that it wouldn't work on 32bit x86 and other
systems would make it also not too useful. Having drivers that only
work on Calgary wouldn't be a good thing.

-Andi

-- 
ak@linux.intel.com

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Thu, 18 Sep 2008 20:20:29 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> > The falling back mechanism was moved to pci-nommu from the common code
> > since it doesn't work for other IOMMUs that always need virtual
> 
> There's no fallback for _map_sg/_map_single. All the fallback to GFP
> only works for coherent allocations, but not for streaming mappings.

Yeah, so the falling back mechanism was moved to pci-nommu's
alloc_coherent.


> To make this "fully robust" for masks < 32bit you would need to implement 
> a new swiotlb that uses GFP_DMA allocations as fallback (or use the DMA 
> allocator's swiotlb which can actually handle this)

Do you mean if GART's alloc_coherent can't find a virtual address that
a device can access to, it should try GFP_DMA allocations as fallback?

GART could but why GART should do? If full IOMMUs' alloc_coherent
can't find a virtual address that a device can access to, it's
failure. No fallback is for them. Why can't GART use the same logic?
Yeah, GART is not a full IOMMU, so it can have a fallback for this
case. But why can't GART work in the same way other IOMMUs?


> So you're right now basically checking for something that you cannot
> fix. And also you try to check for (but not handle) something that even 
> 32bit x86 doesn't handle. So if some driver relied on you checking
> for it on 64bit it wouldn't work on 32bit x86 which would be a bad 
> thing.

What does '32bit x86 doesn't handle' mean? pci-nommu's alloc_coherent
can handle < 32bit bit mask in the fallback path.

Or you are talking about '_map_sg/_map_single'? If so, as we
discussed, < 32bit bit mask can be handled in else where. The patch
just tries to return an address that such tricks are not necessary
with.

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

On Fri, Sep 19, 2008 at 07:15:59AM +0900, FUJITA Tomonori wrote:
> On Thu, 18 Sep 2008 20:20:29 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > The falling back mechanism was moved to pci-nommu from the common code
> > > since it doesn't work for other IOMMUs that always need virtual
> > 
> > There's no fallback for _map_sg/_map_single. All the fallback to GFP
> > only works for coherent allocations, but not for streaming mappings.
> 
> Yeah, so the falling back mechanism was moved to pci-nommu's
> alloc_coherent.

Sure, but that doesn't help for map_single/map_sg. The two cases are
quite different.

> > To make this "fully robust" for masks < 32bit you would need to implement 
> > a new swiotlb that uses GFP_DMA allocations as fallback (or use the DMA 
> > allocator's swiotlb which can actually handle this)
> 
> Do you mean if GART's alloc_coherent can't find a virtual address that
> a device can access to, it should try GFP_DMA allocations as fallback?

It used to at least, that is how I wrote it. That is it did all GFP_DMA,
GFP_DMA32, swiotlb, ZONE_NORMAL based on a fallback scheme.



> 
> GART could but why GART should do? If full IOMMUs' alloc_coherent

The GART is somewhere in the 4GB range so you cannot use it to 
map anything < 4GB.

Also GART is pretty small (and it's not a isolating) IOMMU so 
if you can get direct memory allocation that fits you should 
definitely do that.


> can't find a virtual address that a device can access to, it's
> failure. No fallback is for them. Why can't GART use the same logic?

GART uses the same logic, but only for alloc_cohernet, not for
 map_sg/map_single and masks < 4GB.

> Yeah, GART is not a full IOMMU, so it can have a fallback for this
> case. But why can't GART work in the same way other IOMMUs?

Because GART cannot remap to addresses < 4GB reliably.

The big difference to the other IOMMUs is that it's only a tiny memory
hole somewhere near the 4GB boundary, not a full remapper of the full 
4GB space.


> 
> > So you're right now basically checking for something that you cannot
> > fix. And also you try to check for (but not handle) something that even 
> > 32bit x86 doesn't handle. So if some driver relied on you checking
> > for it on 64bit it wouldn't work on 32bit x86 which would be a bad 
> > thing.
> 
> What does '32bit x86 doesn't handle' mean? pci-nommu's alloc_coherent
> can handle < 32bit bit mask in the fallback path.

Yes it does, just map_sg/map_single doesn't.  And your patch changed
that in GART and that is what I objected too.

> 
> Or you are talking about '_map_sg/_map_single'? If so, as we
> discussed, < 32bit bit mask can be handled in else where. The patch

I don't hink it can, unless you want to write another swiotlb using
GFP_DMA (or use the dma allocator). That is because the swiotlb
has the same limitation as GART. It cannot reliably remap to < 4GB.

-Andi

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Fri, 19 Sep 2008 02:44:31 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> On Fri, Sep 19, 2008 at 07:15:59AM +0900, FUJITA Tomonori wrote:
> > On Thu, 18 Sep 2008 20:20:29 +0200
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > > The falling back mechanism was moved to pci-nommu from the common code
> > > > since it doesn't work for other IOMMUs that always need virtual
> > > 
> > > There's no fallback for _map_sg/_map_single. All the fallback to GFP
> > > only works for coherent allocations, but not for streaming mappings.
> > 
> > Yeah, so the falling back mechanism was moved to pci-nommu's
> > alloc_coherent.
> 
> Sure, but that doesn't help for map_single/map_sg. The two cases are
> quite different.

Sure, I have no intention to mix two cases.


> > > To make this "fully robust" for masks < 32bit you would need to implement 
> > > a new swiotlb that uses GFP_DMA allocations as fallback (or use the DMA 
> > > allocator's swiotlb which can actually handle this)
> > 
> > Do you mean if GART's alloc_coherent can't find a virtual address that
> > a device can access to, it should try GFP_DMA allocations as fallback?
> 
> It used to at least, that is how I wrote it. That is it did all GFP_DMA,
> GFP_DMA32, swiotlb, ZONE_NORMAL based on a fallback scheme.

Ok, after you told me that GART cannot remap to addresses < 4GB
reliably, I understand that GART's alloc_coherent needs to work in the
old way.

I'll take care of it. Well, I need to take care of SWIOTLB about this
issue, I guess.


> > GART could but why GART should do? If full IOMMUs' alloc_coherent
> 
> The GART is somewhere in the 4GB range so you cannot use it to 
> map anything < 4GB.
> 
> Also GART is pretty small (and it's not a isolating) IOMMU so 
> if you can get direct memory allocation that fits you should 
> definitely do that.
> 
> 
> > can't find a virtual address that a device can access to, it's
> > failure. No fallback is for them. Why can't GART use the same logic?
> 
> GART uses the same logic, but only for alloc_cohernet, not for
>  map_sg/map_single and masks < 4GB.
> 
> > Yeah, GART is not a full IOMMU, so it can have a fallback for this
> > case. But why can't GART work in the same way other IOMMUs?
> 
> Because GART cannot remap to addresses < 4GB reliably.
> 
> The big difference to the other IOMMUs is that it's only a tiny memory
> hole somewhere near the 4GB boundary, not a full remapper of the full 
> 4GB space.
> 
> 
> > 
> > > So you're right now basically checking for something that you cannot
> > > fix. And also you try to check for (but not handle) something that even 
> > > 32bit x86 doesn't handle. So if some driver relied on you checking
> > > for it on 64bit it wouldn't work on 32bit x86 which would be a bad 
> > > thing.
> > 
> > What does '32bit x86 doesn't handle' mean? pci-nommu's alloc_coherent
> > can handle < 32bit bit mask in the fallback path.
> 
> Yes it does, just map_sg/map_single doesn't.  And your patch changed
> that in GART and that is what I objected too.

Sure, pci-nommu's map_sg/map_single doesn't handle it. But we handle
this issue somewhere else (like b44 keeps own DMA buffer)?

If I understand your logic correctly,

1. not all map_sg/map_single (e.g. pci-nommu) can't handle it.
2. we already have workarounds for it somewhere else so
map_sg/map_single don't need to handle it.
3. I changed GART map_sg/map_signle to handle it. I thought if it
can handle it, for example, b44 doesn't go the workaround path. It
would be a good thing.
4. But GART cannot remap to addresses < 4GB reliably, so my above
argument doesn't always work.
5. Then my patch doesn't break anything but it's almost meaningless.


Again GART cannot remap to addresses < 4GB reliably, then I have to
say that I agree with you.


> > Or you are talking about '_map_sg/_map_single'? If so, as we
> > discussed, < 32bit bit mask can be handled in else where. The patch
> 
> I don't hink it can, unless you want to write another swiotlb using
> GFP_DMA (or use the dma allocator). That is because the swiotlb
> has the same limitation as GART. It cannot reliably remap to < 4GB.

As I wrote above, 'else where' meant to something like b44's own DMA
hack.

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

On Tue, Sep 23, 2008 at 04:12:33AM +0900, FUJITA Tomonori wrote:
> Sure, pci-nommu's map_sg/map_single doesn't handle it. But we handle
> this issue somewhere else (like b44 keeps own DMA buffer)?

Yes, b44 handles it on its own. It has to for 32bit which always
has a nop map_sg/single. Also some other subsystems like the block layer
do it implicitely.

> 1. not all map_sg/map_single (e.g. pci-nommu) can't handle it.

and especially i386

> 2. we already have workarounds for it somewhere else so
> map_sg/map_single don't need to handle it.
> 3. I changed GART map_sg/map_signle to handle it. I thought if it
> can handle it, for example, b44 doesn't go the workaround path. It
> would be a good thing.
> 4. But GART cannot remap to addresses < 4GB reliably, so my above
> argument doesn't always work.
> 5. Then my patch doesn't break anything but it's almost meaningless.

Correct.

-Andi
-- 
ak@linux.intel.com

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[FUJITA Tomonori]

On Mon, 22 Sep 2008 22:35:18 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> On Tue, Sep 23, 2008 at 04:12:33AM +0900, FUJITA Tomonori wrote:
> > Sure, pci-nommu's map_sg/map_single doesn't handle it. But we handle
> > this issue somewhere else (like b44 keeps own DMA buffer)?
> 
> Yes, b44 handles it on its own. It has to for 32bit which always
> has a nop map_sg/single. Also some other subsystems like the block layer
> do it implicitely.

Yeah, 'somewhere else' includes the block layer bounce.


> > 1. not all map_sg/map_single (e.g. pci-nommu) can't handle it.
> 
> and especially i386
> 
> > 2. we already have workarounds for it somewhere else so
> > map_sg/map_single don't need to handle it.
> > 3. I changed GART map_sg/map_signle to handle it. I thought if it
> > can handle it, for example, b44 doesn't go the workaround path. It
> > would be a good thing.
> > 4. But GART cannot remap to addresses < 4GB reliably, so my above
> > argument doesn't always work.
> > 5. Then my patch doesn't break anything but it's almost meaningless.
> 
> Correct.

Thanks for the clarification.

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Tue, 16 Sep 2008 19:58:24 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > > Those always are handled elsewhere in the block layer (using the bounce_pfn
> > > > mechanism)
> > > 
> > > I don't think that the bounce guarantees that dma_alloc_coherent()
> > > returns an address that a device can access to.
> > 
> > dma_alloc_coherent() is not used for block IO data. And dma_alloc_coherent()
> > does handle masks > 24bit < 32bits just fine.
> 
> What do you mean? For example, some aacraid cards have 31bit dma
> mask. What guarantees that IOMMUs's dma_alloc_coherent don't return a
> virtual address > 31bit < 32bit?
> 
> 
> > > I'm not familiar with what the networking does, for example, seems
> > > that b44 sets dev->dma_mask to DMA_30BIT_MASK and b44_start_xmit()
> > > does:
> > > 
> > 
> > b44 (and related designs like the bcm wireless chipset) 
> > has its own bouncing scheme. IT doesn't need any hacks in map_sg
> >  
> > > IOMMUs can try to return an address that the NIC can access to.
> > 
> > It's not worth to handle this strange case. The drivers do anyways. 
> > These are very cheap devices which are only rarely used in systems 
> > with >2GB and for those cases the existing bouncing setup works 
> > fine.
> 
> I think that the patch is a pretty straightforward, it just the same 
> thing that IOMMUs with > 32bits virtual address space do. We can do 
> better with the simple patch. But I'm ok with dropping the patch for 
> GART since we can live without the patch, as you said.

no - any extra layer of robustness is good in such a critical piece of 
code. Perhaps also emit a one-time printk if you really consider the use 
as a potential sign of bugs, but dont remove robustness.

	Ingo

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

> no - any extra layer of robustness is good in such a critical piece of 

robustness would require to handle it everywhere (as in always fall
back to 24bit DMA bouncing if someone tries a map_* on a < 32bit mapping), 
but this patch doesn't do that at all.

-Andi
-- 
ak@linux.intel.com

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Joerg Roedel]

On Tue, Sep 16, 2008 at 10:20:40PM +0900, FUJITA Tomonori wrote:
> On Tue, 16 Sep 2008 02:54:32 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:
> > 
> > > Currently, gart IOMMU ignores device's dma_mask when it does virtual
> > > mappings. So it could give a device a virtual address that the device
> > > can't access to.
> > 
> > Huh? That is what the need_iommu() logic in gart_map_sg()
> > does. An I'm not aware of any bugs in this area.
> 
> What the need_iommu() does is seeing if GART needs to do virtual
> mappings or not.
> 
> (After need_iommu() checking) What this patchset does is to guarantee
> that GART provides a virtual address that a device can access to.
> 
> 
> > Did you actually see that failure in practice? I don't see
> > how it could happen.
> 
> No, I did not. This patchset does the right thing theoretically, I
> think, but if such problem never happens for GART, I'll drop the patch
> for GART. Joerg?

I am not aware of any failures which are fixed by these patches. But in
theory there could be failures.

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

Re: [PATCH 0/3] fix GART to respect device's dma_mask about virtual mappings

[Andi Kleen]

On Tue, Sep 16, 2008 at 05:52:11PM +0200, Joerg Roedel wrote:
> On Tue, Sep 16, 2008 at 10:20:40PM +0900, FUJITA Tomonori wrote:
> > On Tue, 16 Sep 2008 02:54:32 +0200
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:
> > > 
> > > > Currently, gart IOMMU ignores device's dma_mask when it does virtual
> > > > mappings. So it could give a device a virtual address that the device
> > > > can't access to.
> > > 
> > > Huh? That is what the need_iommu() logic in gart_map_sg()
> > > does. An I'm not aware of any bugs in this area.
> > 
> > What the need_iommu() does is seeing if GART needs to do virtual
> > mappings or not.
> > 
> > (After need_iommu() checking) What this patchset does is to guarantee
> > that GART provides a virtual address that a device can access to.
> > 
> > 
> > > Did you actually see that failure in practice? I don't see
> > > how it could happen.
> > 
> > No, I did not. This patchset does the right thing theoretically, I
> > think, but if such problem never happens for GART, I'll drop the patch
> > for GART. Joerg?
> 
> I am not aware of any failures which are fixed by these patches. 

AFAIK all subsystems deal with it on their own. That is because i386
is the same (no remapping pci_map_* at all) and subsystems are usually 
written to i386 semantics.

> But in
> theory there could be failures.

They will stay failures because GFP_DMA bouncing can not be really
done today in the pci_map_* layer. With a lot of effort you could
probably fix all that, but I doubt it would be worth the effort for
the few devices left with DMA masks < 32bit. 

-Andi

> 
> 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
> 

-- 
ak@linux.intel.com