virtio_mmio: Set DMA masks appropriately
PATCH 1/1 virtio_mmio: Set DMA masks appropriately
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").
--- drivers/virtio/virtio_mmio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/virtio/virtio_mmio.c +10 -0
@@ -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);
Signature · 1 line
2.10.2.dirty
PATCH 1/1 virtio_mmio: Set DMA masks appropriately
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 555
Quoted 7 lines · CET Robin Murphy
drivers/virtio/virtio_mmio.c +3
-0@@ -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?
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 2 lines
drivers/virtio/virtio_mmio.c +2
-0+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
PATCH 1/1 virtio_mmio: Set DMA masks appropriately
Quoted 13 lines · Arnd Bergmann
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 555
Quoted 7 lines · CET Robin Murphy
drivers/virtio/virtio_mmio.c +3
-0@@ -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?
Quoted 4 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 2 lines
drivers/virtio/virtio_mmio.c +2
-0+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.
Signature · 3 lines
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.
PATCH 1/1 virtio_mmio: Set DMA masks appropriately
In reply to Arnd BergmannQuoted 5 lines · Arnd Bergmann
Quoted 4 lines · CET Robin Murphy
drivers/virtio/virtio_mmio.c +0
-0@@ -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 );
Quoted 7 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 555
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+ 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.
Quoted 4 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 2 lines
drivers/virtio/virtio_mmio.c +2
-0+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.
PATCH 1/1 virtio_mmio: Set DMA masks appropriately
Quoted 14 lines · CET Robin Murphy
Quoted 5 lines · Arnd Bergmann
Quoted 4 lines · CET Robin Murphy
drivers/virtio/virtio_mmio.c +0
-0@@ -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 );Quoted 7 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 555
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+ 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?
Quoted 5 lines
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.
Quoted 13 lines
Quoted 4 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 2 lines
drivers/virtio/virtio_mmio.c +2
-0+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)
Quoted 3 lines
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
Quoted 14 lines · CET Robin Murphy
Quoted 5 lines · Arnd Bergmann
Quoted 4 lines · CET Robin Murphy
drivers/virtio/virtio_mmio.c +0
-0@@ -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 );Quoted 7 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 555
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+ 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?
Quoted 5 lines
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.
Quoted 13 lines
Quoted 4 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 2 lines
drivers/virtio/virtio_mmio.c +2
-0+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)
Quoted 3 lines
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
Quoted 5 lines · Arnd Bergmann
Quoted 4 lines · CET Robin Murphy
drivers/virtio/virtio_mmio.c +0
-0@@ -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 );
Quoted 7 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 555
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+ 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.
Quoted 4 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 2 lines
drivers/virtio/virtio_mmio.c +2
-0+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.
Michael S. Tsirkin
maintainerPATCH 1/1 virtio_mmio: Set DMA masks appropriately
In reply to Robin MurphyQuoted 37 lines · 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 <[email protected]>Fixes b42111382f0e ("virtio_mmio: Use the DMA API if enabled")Signed-off-by Robin Murphy <[email protected]>--- drivers/virtio/virtio_mmio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/virtio/virtio_mmio.c +2
-0@@ -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)
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 8 lines
drivers/virtio/virtio_mmio.c +5
-0@@ -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));
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 560
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+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?
Quoted 5 lines
drivers/virtio/virtio_mmio.c +0
-0platform_set_drvdata (pdev, vm_dev);return register_virtio_device (&vm_dev->vdev);Signature · 1 line
2.10.2.dirty
PATCH 1/1 virtio_mmio: Set DMA masks appropriately
Quoted 58 lines · Michael S. Tsirkin
Quoted 37 lines · 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 <[email protected]>Fixes b42111382f0e ("virtio_mmio: Use the DMA API if enabled")Signed-off-by Robin Murphy <[email protected]>--- drivers/virtio/virtio_mmio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/virtio/virtio_mmio.c +2
-0@@ -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)drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 8 lines
drivers/virtio/virtio_mmio.c +5
-0@@ -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 notif (
vm_dev->version== 1) {
dma_set_mask
dma_set_coherent_mask
} else {
dma_set_mask_and_coherent
}
Quoted 2 lines
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.
Quoted 5 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 560
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+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.
Quoted 5 lines
Quoted 5 lines
drivers/virtio/virtio_mmio.c +0
-0platform_set_drvdata (pdev, vm_dev);return register_virtio_device (&vm_dev->vdev);Signature · 1 line
2.10.2.dirty
Michael S. Tsirkin
maintainerPATCH 1/1 virtio_mmio: Set DMA masks appropriately
Quoted 59 lines · Robin Murphy
Quoted 58 lines · Michael S. Tsirkin
Quoted 37 lines · 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 <[email protected]>Fixes b42111382f0e ("virtio_mmio: Use the DMA API if enabled")Signed-off-by Robin Murphy <[email protected]>--- drivers/virtio/virtio_mmio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/virtio/virtio_mmio.c +2
-0@@ -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)drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 8 lines
drivers/virtio/virtio_mmio.c +5
-0@@ -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 notif (
vm_dev->version== 1) {
dma_set_mask
dma_set_coherent_mask
} else {
dma_set_mask_and_coherent
}
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 5 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 560
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+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?
Quoted 8 lines
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 invirtio_ring.c)?Robin.
Oh I see you are just mirroring what pci does. I think it's fine.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/virtio/virtio_mmio.c +0
-0platform_set_drvdata (pdev, vm_dev);return register_virtio_device (&vm_dev->vdev);Signature · 1 line
2.10.2.dirty
Quoted 58 lines · Michael S. Tsirkin
Quoted 37 lines · 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 <[email protected]>Fixes b42111382f0e ("virtio_mmio: Use the DMA API if enabled")Signed-off-by Robin Murphy <[email protected]>--- drivers/virtio/virtio_mmio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/virtio/virtio_mmio.c +2
-0@@ -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)drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 557
Quoted 8 lines
drivers/virtio/virtio_mmio.c +5
-0@@ -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 notif (
vm_dev->version== 1) {
dma_set_mask
dma_set_coherent_mask
} else {
dma_set_mask_and_coherent
}
Quoted 2 lines
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.
Quoted 5 lines
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 560
Quoted 3 lines
drivers/virtio/virtio_mmio.c +3
-0+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.
Quoted 5 lines
Quoted 5 lines
drivers/virtio/virtio_mmio.c +0
-0platform_set_drvdata (pdev, vm_dev);return register_virtio_device (&vm_dev->vdev);Signature · 1 line
2.10.2.dirty