Re: [PATCH] virtio_mmio: Set DMA masks appropriately

Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Arnd Bergmann]

On Tuesday, January 10, 2017 12:26:01 PM CET Robin Murphy wrote:
> @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>         if (vm_dev->version == 1)
>                 writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
>  
> +       rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +       if (rc)
> +               rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

You don't seem to do anything different when 64-bit DMA is unsupported.
How do you prevent the use of kernel buffers that are above the first 4G
here?

> +       else if (vm_dev->version == 1)
> +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));

Why is this limitation only for the coherent mask?

	Arnd

Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Russell King - ARM Linux]

On Tue, Jan 10, 2017 at 02:15:57PM +0100, Arnd Bergmann wrote:
> On Tuesday, January 10, 2017 12:26:01 PM CET Robin Murphy wrote:
> > @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >         if (vm_dev->version == 1)
> >                 writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
> >  
> > +       rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> > +       if (rc)
> > +               rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> 
> You don't seem to do anything different when 64-bit DMA is unsupported.
> How do you prevent the use of kernel buffers that are above the first 4G
> here?
> 
> > +       else if (vm_dev->version == 1)
> > +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));
> 
> Why is this limitation only for the coherent mask?

It looks wrong for two reasons:

1. It is calling dma_set_coherent_mask(), so only the coherent mask
   is being updated.  What about streaming DMA?

   Maybe include the comment from the commit you refer to (a0be1db4304f)
   which explains this, which would help reviewers understand why you're
   only changing the coherent mask.

2. It fails to check whether the coherent mask was accepted... which
   I guess is okay, as the coherent allocation mask won't be updated
   so you should get coherent memory below 4GB.  Nevertheless, drivers
   are expected to try setting a 32-bit coherent mask if setting a
   larger mask fails.  See examples in Documentation/DMA-API-HOWTO.txt.

Of course, if setting a 32-bit coherent mask fails, then the driver
should probably fail to initialise.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Robin Murphy]

On 10/01/17 13:15, Arnd Bergmann wrote:
> On Tuesday, January 10, 2017 12:26:01 PM CET Robin Murphy wrote:
>> @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>>         if (vm_dev->version == 1)
>>                 writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
>>  
>> +       rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +       if (rc)
>> +               rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> 
> You don't seem to do anything different when 64-bit DMA is unsupported.
> How do you prevent the use of kernel buffers that are above the first 4G
> here?

That's the token "give up and rely on SWIOTLB/IOMMU" point, which as we
already know won't necessarily work very well (because it's already the
situation without this patch), but is still arguably better than
nothing. As I've just replied elsewhere, I personally hate this idiom,
but it's the done thing given the current DMA mask API.

>> +       else if (vm_dev->version == 1)
>> +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));
> 
> Why is this limitation only for the coherent mask?

AIUI, the "32-bit pointers to pages" limitation of legacy virtio only
applies to the location of the vring itself, which is allocated via
dma_alloc_coherent - the descriptors themselves hold full 64-bit
addresses pointing at the actual data, which is mapped using streaming
DMA. It relies on the API guarantee that if we've managed to set a
64-bit streaming mask, then setting a smaller coherent mask cannot fail
(DMA-API-HOWTO.txt:257)

This is merely an amalgamation of the logic already in place for
virtio-pci, I just skimped on duplicating all the rationale (I know
there's a mail thread somewhere I could probably dig up).

Robin.

Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Arnd Bergmann]

On Tuesday, January 10, 2017 1:44:37 PM CET Robin Murphy wrote:
> On 10/01/17 13:15, Arnd Bergmann wrote:
> > On Tuesday, January 10, 2017 12:26:01 PM CET Robin Murphy wrote:
> >> @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >>         if (vm_dev->version == 1)
> >>                 writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
> >>  
> >> +       rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> >> +       if (rc)
> >> +               rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > 
> > You don't seem to do anything different when 64-bit DMA is unsupported.
> > How do you prevent the use of kernel buffers that are above the first 4G
> > here?
> 
> That's the token "give up and rely on SWIOTLB/IOMMU" point, which as we
> already know won't necessarily work very well (because it's already the
> situation without this patch), but is still arguably better than
> nothing. As I've just replied elsewhere, I personally hate this idiom,
> but it's the done thing given the current DMA mask API.

Ah, I think I understand now. This is actually unrelated to SWIOTLB, which
should always succeed, but it is used on some architectures (x86, and
sparc in particular) to control whether the iommu is allowed to hand
out IOVA above 32-bit. Sorry for assuming in my earlier mail that all
IOMMUs only do 32-bit VA addressing in the dma-mapping API.

The related case on PowerPC is that DMA_BIT_MASK(64) has a special
meaning of bypassing the IOMMU entirely to optimize for performance.
Again that should not fail, but it will switch the dma_map_ops between
iommu and direct, using the IOMMU only when the device can't address
the entire space.

> >> +       else if (vm_dev->version == 1)
> >> +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));
> > 
> > Why is this limitation only for the coherent mask?
> 
> AIUI, the "32-bit pointers to pages" limitation of legacy virtio only
> applies to the location of the vring itself, which is allocated via
> dma_alloc_coherent - the descriptors themselves hold full 64-bit
> addresses pointing at the actual data, which is mapped using streaming
> DMA. It relies on the API guarantee that if we've managed to set a
> 64-bit streaming mask, then setting a smaller coherent mask cannot fail
> (DMA-API-HOWTO.txt:257)
> 
> This is merely an amalgamation of the logic already in place for
> virtio-pci, I just skimped on duplicating all the rationale (I know
> there's a mail thread somewhere I could probably dig up).

Ok, got it.

	Arnd

Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Michael S. Tsirkin]

On Tue, Jan 10, 2017 at 12:26:01PM +0000, Robin Murphy wrote:
> Once DMA API usage is enabled, it becomes apparent that virtio-mmio is
> inadvertently relying on the default 32-bit DMA mask, which leads to
> problems like rapidly exhausting SWIOTLB bounce buffers.
> 
> Ensure that we set the appropriate 64-bit DMA mask whenever possible,
> with the coherent mask suitably limited for the legacy vring as per
> a0be1db4304f ("virtio_pci: Limit DMA mask to 44 bits for legacy virtio
> devices").
> 
> Reported-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Fixes: b42111382f0e ("virtio_mmio: Use the DMA API if enabled")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/virtio/virtio_mmio.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 48bfea91dbca..b5c5d49ca598 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -59,6 +59,7 @@
>  #define pr_fmt(fmt) "virtio-mmio: " fmt
>  
>  #include <linux/acpi.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/highmem.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -497,6 +498,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>  	struct virtio_mmio_device *vm_dev;
>  	struct resource *mem;
>  	unsigned long magic;
> +	int rc;
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!mem)
> @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>  	if (vm_dev->version == 1)
>  		writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
>  
> +	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +	if (rc)
> +		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +	else if (vm_dev->version == 1)
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));

That's a very convoluted way to do this, for version 1 you
set coherent mask to 64 then override it.
why not

if (vm_dev->version == 1) {
	dma_set_mask
	dma_set_coherent_mask
} else {
	dma_set_mask_and_coherent
}

if (rc)
	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

> +	if (rc)
> +		dev_warn(&pdev->dev, "Failed to enable 64-bit or 32-bit DMA.  Trying to continue, but this might not work.\n");
> +

is there a chance it actually still might work?

>  	platform_set_drvdata(pdev, vm_dev);
>  
>  	return register_virtio_device(&vm_dev->vdev);
> -- 
> 2.10.2.dirty

Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Robin Murphy]

On 10/01/17 14:54, Michael S. Tsirkin wrote:
> On Tue, Jan 10, 2017 at 12:26:01PM +0000, Robin Murphy wrote:
>> Once DMA API usage is enabled, it becomes apparent that virtio-mmio is
>> inadvertently relying on the default 32-bit DMA mask, which leads to
>> problems like rapidly exhausting SWIOTLB bounce buffers.
>>
>> Ensure that we set the appropriate 64-bit DMA mask whenever possible,
>> with the coherent mask suitably limited for the legacy vring as per
>> a0be1db4304f ("virtio_pci: Limit DMA mask to 44 bits for legacy virtio
>> devices").
>>
>> Reported-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> Fixes: b42111382f0e ("virtio_mmio: Use the DMA API if enabled")
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/virtio/virtio_mmio.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> index 48bfea91dbca..b5c5d49ca598 100644
>> --- a/drivers/virtio/virtio_mmio.c
>> +++ b/drivers/virtio/virtio_mmio.c
>> @@ -59,6 +59,7 @@
>>  #define pr_fmt(fmt) "virtio-mmio: " fmt
>>  
>>  #include <linux/acpi.h>
>> +#include <linux/dma-mapping.h>
>>  #include <linux/highmem.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>> @@ -497,6 +498,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>>  	struct virtio_mmio_device *vm_dev;
>>  	struct resource *mem;
>>  	unsigned long magic;
>> +	int rc;
>>  
>>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	if (!mem)
>> @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>>  	if (vm_dev->version == 1)
>>  		writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
>>  
>> +	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +	if (rc)
>> +		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> +	else if (vm_dev->version == 1)
>> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));
> 
> That's a very convoluted way to do this, for version 1 you
> set coherent mask to 64 then override it.
> why not
> 
> if (vm_dev->version == 1) {
> 	dma_set_mask
> 	dma_set_coherent_mask
> } else {
> 	dma_set_mask_and_coherent
> }
> 
> if (rc)
> 	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

Purely because it's fewer lines of code - if you'd prefer separate
legacy vs. modern flows for clarity that's fine by me.

>> +	if (rc)
>> +		dev_warn(&pdev->dev, "Failed to enable 64-bit or 32-bit DMA.  Trying to continue, but this might not work.\n");
>> +
> 
> is there a chance it actually still might work?

If we're not actually using the DMA API, we may still get away with it,
otherwise it's a fairly sure bet that the subsequent dma_map/dma_alloc
calls will fail and we'll get nowhere. If I change this to be a probe
failure condition (and correspondingly in the PCI drivers too), would
you rather that be predicated on vring_use_dma_api(), or always (given
the TODO in virtio_ring.c)?

Robin.

>>  	platform_set_drvdata(pdev, vm_dev);
>>  
>>  	return register_virtio_device(&vm_dev->vdev);
>> -- 
>> 2.10.2.dirty

Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Michael S. Tsirkin]

On Tue, Jan 10, 2017 at 03:55:31PM +0000, Robin Murphy wrote:
> On 10/01/17 14:54, Michael S. Tsirkin wrote:
> > On Tue, Jan 10, 2017 at 12:26:01PM +0000, Robin Murphy wrote:
> >> Once DMA API usage is enabled, it becomes apparent that virtio-mmio is
> >> inadvertently relying on the default 32-bit DMA mask, which leads to
> >> problems like rapidly exhausting SWIOTLB bounce buffers.
> >>
> >> Ensure that we set the appropriate 64-bit DMA mask whenever possible,
> >> with the coherent mask suitably limited for the legacy vring as per
> >> a0be1db4304f ("virtio_pci: Limit DMA mask to 44 bits for legacy virtio
> >> devices").
> >>
> >> Reported-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> >> Fixes: b42111382f0e ("virtio_mmio: Use the DMA API if enabled")
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >>  drivers/virtio/virtio_mmio.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> >> index 48bfea91dbca..b5c5d49ca598 100644
> >> --- a/drivers/virtio/virtio_mmio.c
> >> +++ b/drivers/virtio/virtio_mmio.c
> >> @@ -59,6 +59,7 @@
> >>  #define pr_fmt(fmt) "virtio-mmio: " fmt
> >>  
> >>  #include <linux/acpi.h>
> >> +#include <linux/dma-mapping.h>
> >>  #include <linux/highmem.h>
> >>  #include <linux/interrupt.h>
> >>  #include <linux/io.h>
> >> @@ -497,6 +498,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >>  	struct virtio_mmio_device *vm_dev;
> >>  	struct resource *mem;
> >>  	unsigned long magic;
> >> +	int rc;
> >>  
> >>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>  	if (!mem)
> >> @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >>  	if (vm_dev->version == 1)
> >>  		writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
> >>  
> >> +	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> >> +	if (rc)
> >> +		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> >> +	else if (vm_dev->version == 1)
> >> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));
> > 
> > That's a very convoluted way to do this, for version 1 you
> > set coherent mask to 64 then override it.
> > why not
> > 
> > if (vm_dev->version == 1) {
> > 	dma_set_mask
> > 	dma_set_coherent_mask
> > } else {
> > 	dma_set_mask_and_coherent
> > }
> > 
> > if (rc)
> > 	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> 
> Purely because it's fewer lines of code - if you'd prefer separate
> legacy vs. modern flows for clarity that's fine by me.

Yes please. It's less legacy vs modern and more that I don't like
line 2 undoig what line 1 did.

> >> +	if (rc)
> >> +		dev_warn(&pdev->dev, "Failed to enable 64-bit or 32-bit DMA.  Trying to continue, but this might not work.\n");
> >> +
> > 
> > is there a chance it actually still might work?
> 
> If we're not actually using the DMA API, we may still get away with it,
> otherwise it's a fairly sure bet that the subsequent dma_map/dma_alloc
> calls will fail and we'll get nowhere. If I change this to be a probe
> failure condition (and correspondingly in the PCI drivers too), would
> you rather that be predicated on vring_use_dma_api(), or always (given
> the TODO in virtio_ring.c)?
> 
> Robin.

Oh I see you are just mirroring what pci does. I think it's fine.

> >>  	platform_set_drvdata(pdev, vm_dev);
> >>  
> >>  	return register_virtio_device(&vm_dev->vdev);
> >> -- 
> >> 2.10.2.dirty

[PATCH] virtio_mmio: Set DMA masks appropriately

[Robin Murphy]

Once DMA API usage is enabled, it becomes apparent that virtio-mmio is
inadvertently relying on the default 32-bit DMA mask, which leads to
problems like rapidly exhausting SWIOTLB bounce buffers.

Ensure that we set the appropriate 64-bit DMA mask whenever possible,
with the coherent mask suitably limited for the legacy vring as per
a0be1db4304f ("virtio_pci: Limit DMA mask to 44 bits for legacy virtio
devices").

Reported-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Fixes: b42111382f0e ("virtio_mmio: Use the DMA API if enabled")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/virtio/virtio_mmio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 48bfea91dbca..b5c5d49ca598 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -59,6 +59,7 @@
 #define pr_fmt(fmt) "virtio-mmio: " fmt
 
 #include <linux/acpi.h>
+#include <linux/dma-mapping.h>
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -497,6 +498,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 	struct virtio_mmio_device *vm_dev;
 	struct resource *mem;
 	unsigned long magic;
+	int rc;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem)
@@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 	if (vm_dev->version == 1)
 		writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
 
+	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (rc)
+		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	else if (vm_dev->version == 1)
+		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));
+	if (rc)
+		dev_warn(&pdev->dev, "Failed to enable 64-bit or 32-bit DMA.  Trying to continue, but this might not work.\n");
+
 	platform_set_drvdata(pdev, vm_dev);
 
 	return register_virtio_device(&vm_dev->vdev);
-- 
2.10.2.dirty