[PATCH 1/2] x86 swiotlb: Verify we can perform the remapping requested.

[PATCH 1/2] x86 swiotlb: Verify we can perform the remapping requested.

[Eric W. Biederman]

Recently I had a driver try with a peculiar 2G dma memory limit.
It failed in weird and strange ways because my bounce buffers were
being allocated above 2G where the driver could not reach, and
no error was reported when the mappings were setup.

Use the swiotlb_dma_supported to avoid silent problems like this
in the future.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86/kernel/pci-swiotlb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
index 8f972cb..6a802fa 100644
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -38,7 +38,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.unmap_sg = swiotlb_unmap_sg_attrs,
 	.map_page = swiotlb_map_page,
 	.unmap_page = swiotlb_unmap_page,
-	.dma_supported = NULL,
+	.dma_supported = swiotlb_dma_supported,
 };
 
 /*
-- 
1.7.2.5

[PATCH 2/2] x86 amd_gart_64: Verify we can perform the remapping requested

[Eric W. Biederman]

Recently I had a driver try with a peculiar 2G dma memory limit.
The driver failed in weird and strange ways because the GART remapping
apperture had been allocated above 2G where the driver cound not reach,
and no error was reported when the mappings were setup.

Implement gart_dma_supported to test for this problem case and to return
and error if we can not support the remapping.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86/kernel/amd_gart_64.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
index 8a439d3..66279cb 100644
--- a/arch/x86/kernel/amd_gart_64.c
+++ b/arch/x86/kernel/amd_gart_64.c
@@ -519,6 +519,14 @@ static int gart_mapping_error(struct device *dev, dma_addr_t dma_addr)
 	return (dma_addr == bad_dma_addr);
 }
 
+static int gart_dma_supported(struct device *dev, u64 mask)
+{
+	unsigned long iommu_max_addr = iommu_bus_base + iommu_size - 1;
+
+	/* Fail if the gart window is too high to fit in the devices dma mask */
+	return iommu_max_addr <= mask;
+}
+
 static int no_agp;
 
 static __init unsigned long check_iommu_size(unsigned long aper, u64 aper_size)
@@ -703,6 +711,7 @@ static struct dma_map_ops gart_dma_ops = {
 	.alloc_coherent			= gart_alloc_coherent,
 	.free_coherent			= gart_free_coherent,
 	.mapping_error			= gart_mapping_error,
+	.dma_supported			= gart_dma_supported,
 };
 
 static void gart_iommu_shutdown(void)
-- 
1.7.2.5

Re: [PATCH 2/2] x86 amd_gart_64: Verify we can perform the remapping requested

[Joerg Roedel]

On Mon, Oct 17, 2011 at 02:20:15PM -0700, Eric W. Biederman wrote:
> 
> Recently I had a driver try with a peculiar 2G dma memory limit.
> The driver failed in weird and strange ways because the GART remapping
> apperture had been allocated above 2G where the driver cound not reach,
> and no error was reported when the mappings were setup.
> 
> Implement gart_dma_supported to test for this problem case and to return
> and error if we can not support the remapping.

You do basically the same as for swiotlb, so it must be good :)

> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

For both patches:

Acked-by: Joerg Roedel <joerg.roedel@amd.com>

> ---
>  arch/x86/kernel/amd_gart_64.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
> index 8a439d3..66279cb 100644
> --- a/arch/x86/kernel/amd_gart_64.c
> +++ b/arch/x86/kernel/amd_gart_64.c
> @@ -519,6 +519,14 @@ static int gart_mapping_error(struct device *dev, dma_addr_t dma_addr)
>  	return (dma_addr == bad_dma_addr);
>  }
>  
> +static int gart_dma_supported(struct device *dev, u64 mask)
> +{
> +	unsigned long iommu_max_addr = iommu_bus_base + iommu_size - 1;
> +
> +	/* Fail if the gart window is too high to fit in the devices dma mask */
> +	return iommu_max_addr <= mask;
> +}
> +
>  static int no_agp;
>  
>  static __init unsigned long check_iommu_size(unsigned long aper, u64 aper_size)
> @@ -703,6 +711,7 @@ static struct dma_map_ops gart_dma_ops = {
>  	.alloc_coherent			= gart_alloc_coherent,
>  	.free_coherent			= gart_free_coherent,
>  	.mapping_error			= gart_mapping_error,
> +	.dma_supported			= gart_dma_supported,
>  };
>  
>  static void gart_iommu_shutdown(void)
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Nothing great was ever achieved without enthusiasm.

Re: [PATCH 2/2] x86 amd_gart_64: Verify we can perform the remapping requested

[Joerg Roedel]

On Mon, Oct 24, 2011 at 12:07:36PM +0200, Joerg Roedel wrote:
> On Mon, Oct 17, 2011 at 02:20:15PM -0700, Eric W. Biederman wrote:
> > 
> > Recently I had a driver try with a peculiar 2G dma memory limit.
> > The driver failed in weird and strange ways because the GART remapping
> > apperture had been allocated above 2G where the driver cound not reach,
> > and no error was reported when the mappings were setup.
> > 
> > Implement gart_dma_supported to test for this problem case and to return
> > and error if we can not support the remapping.
> 
> You do basically the same as for swiotlb, so it must be good :)

Okay, thinking more about this, I am not so sure anymore it is a really
good idea to fix it this way. It basically signals the driver that DMA
is not possible with the device if the aperture does not fit into
the dma_mask. But DMA within the ZONE_DMA might still work, no?

So I think the right way to fix it is to return a dma-mapping error if
and only if the address needs to be remapped by the aperture (of swiotlb or
gart).

This still allows the driver to use GFP_DMA allocations.


	Joerg

Re: [PATCH 2/2] x86 amd_gart_64: Verify we can perform the remapping requested

[Eric W. Biederman]

Joerg Roedel <joro@8bytes.org> writes:

> On Mon, Oct 24, 2011 at 12:07:36PM +0200, Joerg Roedel wrote:
>> On Mon, Oct 17, 2011 at 02:20:15PM -0700, Eric W. Biederman wrote:
>> > 
>> > Recently I had a driver try with a peculiar 2G dma memory limit.
>> > The driver failed in weird and strange ways because the GART remapping
>> > apperture had been allocated above 2G where the driver cound not reach,
>> > and no error was reported when the mappings were setup.
>> > 
>> > Implement gart_dma_supported to test for this problem case and to return
>> > and error if we can not support the remapping.
>> 
>> You do basically the same as for swiotlb, so it must be good :)
>
> Okay, thinking more about this, I am not so sure anymore it is a really
> good idea to fix it this way. It basically signals the driver that DMA
> is not possible with the device if the aperture does not fit into
> the dma_mask. But DMA within the ZONE_DMA might still work, no?
>
> So I think the right way to fix it is to return a dma-mapping error if
> and only if the address needs to be remapped by the aperture (of swiotlb or
> gart).

Perhaps I am mistaken but since the method is iommu type specific if we
don't actually use the iommu we should not call the method so I think
this is patch is already implementing what you want.

> This still allows the driver to use GFP_DMA allocations.

I definitely agree on that.

Eric

Re: [PATCH 2/2] x86 amd_gart_64: Verify we can perform the remapping requested

[Joerg Roedel]

On Fri, Nov 11, 2011 at 10:59:33AM -0800, Eric W. Biederman wrote:
> Joerg Roedel <joro@8bytes.org> writes:
> > Okay, thinking more about this, I am not so sure anymore it is a really
> > good idea to fix it this way. It basically signals the driver that DMA
> > is not possible with the device if the aperture does not fit into
> > the dma_mask. But DMA within the ZONE_DMA might still work, no?
> >
> > So I think the right way to fix it is to return a dma-mapping error if
> > and only if the address needs to be remapped by the aperture (of swiotlb or
> > gart).
> 
> Perhaps I am mistaken but since the method is iommu type specific if we
> don't actually use the iommu we should not call the method so I think
> this is patch is already implementing what you want.

No it does not. With GART or SWIOTLB all DMA-mapping functions call
directly into the IOMMU driver. The driver then checks whether it needs
to remap or not (see dma_capable() in SWIOTLB or need_iommu() in GART).

I think the check you want to add needs to happen after these checks in
each of the IOMMU drivers.


	Joerg

Re: [PATCH 1/2] x86 swiotlb: Verify we can perform the remapping requested.

[Konrad Rzeszutek Wilk]

On Mon, Oct 17, 2011 at 02:19:18PM -0700, Eric W. Biederman wrote:
> 
> Recently I had a driver try with a peculiar 2G dma memory limit.
> It failed in weird and strange ways because my bounce buffers were
> being allocated above 2G where the driver could not reach, and
> no error was reported when the mappings were setup.

OK, so the overflow buffer was used instead.. which presumarily
also was allocated above the 2G? That seems to point that
alloc_bootmem_low_pages is not doing its job?

> 
> Use the swiotlb_dma_supported to avoid silent problems like this
> in the future.

Which driver was it that had this limit?

> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

I also CC-ed Fujita on this as he is the swiotlb maintainer.

> ---
>  arch/x86/kernel/pci-swiotlb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
> index 8f972cb..6a802fa 100644
> --- a/arch/x86/kernel/pci-swiotlb.c
> +++ b/arch/x86/kernel/pci-swiotlb.c
> @@ -38,7 +38,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
>  	.unmap_sg = swiotlb_unmap_sg_attrs,
>  	.map_page = swiotlb_map_page,
>  	.unmap_page = swiotlb_unmap_page,
> -	.dma_supported = NULL,
> +	.dma_supported = swiotlb_dma_supported,
>  };
>  
>  /*
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] x86 swiotlb: Verify we can perform the remapping requested.

[Eric W. Biederman]

Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:

> On Mon, Oct 17, 2011 at 02:19:18PM -0700, Eric W. Biederman wrote:
>> 
>> Recently I had a driver try with a peculiar 2G dma memory limit.
>> It failed in weird and strange ways because my bounce buffers were
>> being allocated above 2G where the driver could not reach, and
>> no error was reported when the mappings were setup.
>
> OK, so the overflow buffer was used instead.. which presumarily
> also was allocated above the 2G? That seems to point that
> alloc_bootmem_low_pages is not doing its job?

I just looked alloc_bootmem_low_pages allocates memory below
ARCH_ADDRESS_LIMIT which for everything except s390 is 4G.

I know I was mostly using the amd gart driver.  So I may be mistaken
that the swiotlb driver had the same issue.  However my only solution
at the time was to boot with mem=2G.  So I believe the swiotlb did
have this issue.

Mostly the patch was.  Hmm.  That looks stupid not wiring up the
swiotlb address space limit check when someone has already written it.

>> Use the swiotlb_dma_supported to avoid silent problems like this
>> in the future.
>
> Which driver was it that had this limit?

An unmerged driver.  The driver has since been fixed to use it's
hardware in a way that doesn't hit this limit.

At the time that I looked drivers in the kernel with a dma limit
above 16MB and blow 4GB only existed on exotic architectures in
very special cases.

Unfortunately I no longer have access to this hardware to test with.

Eric

Re: [PATCH 1/2] x86 swiotlb: Verify we can perform the remapping requested.

[FUJITA Tomonori]

On Mon, 24 Oct 2011 08:07:31 -0700
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:
> 
> > On Mon, Oct 17, 2011 at 02:19:18PM -0700, Eric W. Biederman wrote:
> >> 
> >> Recently I had a driver try with a peculiar 2G dma memory limit.
> >> It failed in weird and strange ways because my bounce buffers were
> >> being allocated above 2G where the driver could not reach, and
> >> no error was reported when the mappings were setup.
> >
> > OK, so the overflow buffer was used instead.. which presumarily
> > also was allocated above the 2G? That seems to point that
> > alloc_bootmem_low_pages is not doing its job?
> 
> I just looked alloc_bootmem_low_pages allocates memory below
> ARCH_ADDRESS_LIMIT which for everything except s390 is 4G.
> 
> I know I was mostly using the amd gart driver.  So I may be mistaken
> that the swiotlb driver had the same issue.  However my only solution
> at the time was to boot with mem=2G.  So I believe the swiotlb did
> have this issue.
> 
> Mostly the patch was.  Hmm.  That looks stupid not wiring up the
> swiotlb address space limit check when someone has already written it.

http://www.gossamer-threads.com/lists/linux/kernel/1333685?do=post_view_threaded#1333685

swiotlb isn't designed to handle hardware having odd dma mask (e.g. 2G
or something). Such device needs the own bouncing mechanism.

Re: [PATCH 1/2] x86 swiotlb: Verify we can perform the remapping requested.

[Eric W. Biederman]

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

> On Mon, 24 Oct 2011 08:07:31 -0700
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:
>> 
>> > On Mon, Oct 17, 2011 at 02:19:18PM -0700, Eric W. Biederman wrote:
>> >> 
>> >> Recently I had a driver try with a peculiar 2G dma memory limit.
>> >> It failed in weird and strange ways because my bounce buffers were
>> >> being allocated above 2G where the driver could not reach, and
>> >> no error was reported when the mappings were setup.
>> >
>> > OK, so the overflow buffer was used instead.. which presumarily
>> > also was allocated above the 2G? That seems to point that
>> > alloc_bootmem_low_pages is not doing its job?
>> 
>> I just looked alloc_bootmem_low_pages allocates memory below
>> ARCH_ADDRESS_LIMIT which for everything except s390 is 4G.
>> 
>> I know I was mostly using the amd gart driver.  So I may be mistaken
>> that the swiotlb driver had the same issue.  However my only solution
>> at the time was to boot with mem=2G.  So I believe the swiotlb did
>> have this issue.
>> 
>> Mostly the patch was.  Hmm.  That looks stupid not wiring up the
>> swiotlb address space limit check when someone has already written it.
>
> http://www.gossamer-threads.com/lists/linux/kernel/1333685?do=post_view_threaded#1333685
>
> swiotlb isn't designed to handle hardware having odd dma mask (e.g. 2G
> or something). Such device needs the own bouncing mechanism.

I have no problem with that with not supporting weird dma masks.

There is a perfectly fine method to report an unsupported dma mask
programmatically that just needs a trivial code change to be wired up.
I just want the method to be wired up on x86 so that we fail gracefully,
instead of mysteriously.

Eric