SWIOTLB fixes for 4.20
Robin Murphy
reviewercover SWIOTLB fixes for 4.20
This is what I have so far, which at least resolves the most ovbious problems. I still haven't got very far with the USB corruption issue I see on Juno with -rc1, but I'm yet to confirm whether that's actually attributable to the SWIOTLB changes or something else entirely.
Robin.
Robin Murphy (2):
swiotlb: Make DIRECT_MAPPING_ERROR viable
swiotlb: Skip cache maintenance on map error
include/linux/dma-direct.h | 2 +- kernel/dma/swiotlb.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)
Signature · 1 line
2.19.1.dirty
Robin Murphy
reviewerPATCH 1/2 swiotlb: Make DIRECT_MAPPING_ERROR viable
With the overflow buffer removed, we no longer have a unique address which is guaranteed not to be a valid DMA target to use as an error token. The DIRECT_MAPPING_ERROR value of 0 tries to at least represent an unlikely DMA target, but unfortunately there are already SWIOTLB users with DMA-able memory at physical address 0 which now gets falsely treated as a mapping failure and leads to all manner of misbehaviour.
The best we can do to mitigate that is flip DIRECT_MAPPING_ERROR to the commonly-used all-bits-set value, since the last single byte of memory is by far the least-likely-valid DMA target.
--- include/linux/dma-direct.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/dma-direct.h +1 -1
@@ -5,7 +5,7 @@#include <linux/dma-mapping.h> #include <linux/mem_encrypt.h> -#define DIRECT_MAPPING_ERROR 0 +#define DIRECT_MAPPING_ERROR ~(dma_addr_t)0#ifdef CONFIG_ARCH_HAS_PHYS_TO_DMA #include <asm/dma-direct.h>
Signature · 1 line
2.19.1.dirty
Robin Murphy
reviewerPATCH 2/2 swiotlb: Skip cache maintenance on map error
In reply to Robin MurphyIf swiotlb_bounce_page() failed, calling arch_sync_dma_for_device() may lead to such delights as performing cache maintenance on whatever address phys_to_virt(SWIOTLB_MAP_ERROR) looks like, which is typically outside the kernel memory map and goes about as well as expected.
Don't do that.
--- kernel/dma/swiotlb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/dma/swiotlb.c +2 -1
@@ -679,7 +679,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, }if (!dev_is_dma_coherent (dev) && - (attrs &DMA_ATTR_SKIP_CPU_SYNC ) ==0 ) + (attrs &DMA_ATTR_SKIP_CPU_SYNC ) ==0 && + dev_addr !=DIRECT_MAPPING_ERROR )arch_sync_dma_for_device (dev, phys, size, dir);return dev_addr;
Signature · 1 line
2.19.1.dirty
Re: [PATCH 1/2] swiotlb: Make DIRECT_MAPPING_ERROR viable
In reply to Robin MurphyQuoted 10 lines · Robin Murphy
With the overflow buffer removed, we no longer have a unique address which is guaranteed not to be a valid DMA target to use as an error token. The
DIRECT_MAPPING_ERRORvalue of 0 tries to at least represent an unlikely DMA target, but unfortunately there are already SWIOTLB users with DMA-able memory at physical address 0 which now gets falsely treated as a mapping failure and leads to all manner of misbehaviour.The best we can do to mitigate that is flip
DIRECT_MAPPING_ERRORto the commonly-used all-bits-set value, since the last single byte of memory is by far the least-likely-valid DMA target.
Are all the callers checking for DIRECT_MAPPING_ERROR or is it more of a comparison (as in if (!ret)) ?
Quoted 24 lines
Fixes dff8d6c1ed58 ("swiotlb: remove the overflow buffer")]Reported-by John Stultz <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- include/linux/dma-direct.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/linux/dma-direct.h +1
-1@@ -5,7 +5,7 @@#include <linux/dma-mapping.h> #include <linux/mem_encrypt.h> -#define DIRECT_MAPPING_ERROR 0 +#define DIRECT_MAPPING_ERROR ~(dma_addr_t)0#ifdef CONFIG_ARCH_HAS_PHYS_TO_DMA #include <asm/dma-direct.h> Signature · 1 line
2.19.1.dirty
Re: [PATCH 2/2] swiotlb: Skip cache maintenance on map error
In reply to Robin MurphyQuoted 6 lines · Robin Murphy
If
swiotlb_bounce_page()failed, callingarch_sync_dma_for_device()may lead to such delights as performing cache maintenance on whatever addressphys_to_virt(SWIOTLB_MAP_ERROR)looks like, which is typically outside the kernel memory map and goes about as well as expected.Don't do that.
+Stefano
Quoted 24 lines
Fixes a4a4330db46a ("swiotlb: add support for non-coherent DMA")Signed-off-by Robin Murphy <[email protected]>--- kernel/dma/swiotlb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/dma/swiotlb.c +2
-1@@ -679,7 +679,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, }if (!dev_is_dma_coherent (dev) && - (attrs &DMA_ATTR_SKIP_CPU_SYNC ) ==0 ) + (attrs &DMA_ATTR_SKIP_CPU_SYNC ) ==0 && + dev_addr !=DIRECT_MAPPING_ERROR )arch_sync_dma_for_device (dev, phys, size, dir);return dev_addr;Signature · 1 line
2.19.1.dirty
Robin Murphy
reviewerRe: [PATCH 1/2] swiotlb: Make DIRECT_MAPPING_ERROR viable
In reply to Konrad Rzeszutek WilkQuoted 8 lines · Konrad Rzeszutek Wilk
Quoted 7 lines · Robin Murphy
With the overflow buffer removed, we no longer have a unique address which is guaranteed not to be a valid DMA target to use as an error token. The
DIRECT_MAPPING_ERRORvalue of 0 tries to at least represent an unlikely DMA target, but unfortunately there are already SWIOTLB users with DMA-able memory at physical address 0 which now gets falsely treated as a mapping failure and leads to all manner of misbehaviour.
Quoted 6 lines
Quoted 3 lines
The best we can do to mitigate that is flip
DIRECT_MAPPING_ERRORto the commonly-used all-bits-set value, since the last single byte of memory is by far the least-likely-valid DMA target.Are all the callers checking for
DIRECT_MAPPING_ERRORor is it more of a comparison (as in if (!ret)) ?
dma_direct_map_page() and dma_direct_mapping_error() were already doing the right thing, and external callers must rely on the latter via dma_mapping_error() rather than trying to inspect the actual value themselves, since that varies between implementations anyway. AFAICS all the new return paths from swiotlb_map_page() are also robust in referencing the macro explicitly, so I think we're good.
Thanks,
Robin.
Quoted 23 lines
Quoted 23 lines
Fixes dff8d6c1ed58 ("swiotlb: remove the overflow buffer")]Reported-by John Stultz <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- include/linux/dma-direct.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/linux/dma-direct.h +1
-1@@ -5,7 +5,7 @@#include <linux/dma-mapping.h> #include <linux/mem_encrypt.h> -#define DIRECT_MAPPING_ERROR 0 +#define DIRECT_MAPPING_ERROR ~(dma_addr_t)0#ifdef CONFIG_ARCH_HAS_PHYS_TO_DMA #include <asm/dma-direct.h> Signature · 1 line
2.19.1.dirty
Re: [PATCH 1/2] swiotlb: Make DIRECT_MAPPING_ERROR viable
In reply to Robin MurphyThe subject line should say dma-direct. This isn't really swiotlb specific.
Quoted 1 line
+#define
DIRECT_MAPPING_ERROR~(dma_addr_t)0
Please add braces around the value like the other *MAPPING_ERROR defintions so that it can be safely used in any context.
Otherwise looks good:
Re: [PATCH 2/2] swiotlb: Skip cache maintenance on map error
In reply to Robin MurphyQuoted 9 lines · Robin Murphy
If
swiotlb_bounce_page()failed, callingarch_sync_dma_for_device()may lead to such delights as performing cache maintenance on whatever addressphys_to_virt(SWIOTLB_MAP_ERROR)looks like, which is typically outside the kernel memory map and goes about as well as expected.Don't do that.
Fixes a4a4330db46a ("swiotlb: add support for non-coherent DMA")Signed-off-by Robin Murphy <[email protected]>
Looks good,
Quoted 4 lines · Robin Murphy
This is what I have so far, which at least resolves the most ovbious problems. I still haven't got very far with the USB corruption issue I see on Juno with -rc1, but I'm yet to confirm whether that's actually attributable to the SWIOTLB changes or something else entirely.
This looks good modulo the minor nitpicks.
Konrad, are you ok with me picking up both through the dma-mapping tree?
Konrad Rzeszutek Wilk
Acked-byRe: [PATCH 1/2] swiotlb: Make DIRECT_MAPPING_ERROR viable
In reply to Robin MurphyQuoted 16 lines · Robin Murphy
Quoted 8 lines · Konrad Rzeszutek Wilk
Quoted 7 lines · Robin Murphy
With the overflow buffer removed, we no longer have a unique address which is guaranteed not to be a valid DMA target to use as an error token. The
DIRECT_MAPPING_ERRORvalue of 0 tries to at least represent an unlikely DMA target, but unfortunately there are already SWIOTLB users with DMA-able memory at physical address 0 which now gets falsely treated as a mapping failure and leads to all manner of misbehaviour.Quoted 6 lines
Quoted 3 lines
The best we can do to mitigate that is flip
DIRECT_MAPPING_ERRORto the commonly-used all-bits-set value, since the last single byte of memory is by far the least-likely-valid DMA target.Are all the callers checking for
DIRECT_MAPPING_ERRORor is it more of a comparison (as in if (!ret)) ?
Quoted 6 lines
dma_direct_map_page()anddma_direct_mapping_error()were already doing the right thing, and external callers must rely on the latter viadma_mapping_error()rather than trying to inspect the actual value themselves, since that varies between implementations anyway. AFAICS all the new return paths fromswiotlb_map_page()are also robust in referencing the macro explicitly, so I think we're good.
Cool! Thank you for checking.
Thank you!
Quoted 25 lines
Thanks,
Robin.Quoted 21 lines
Quoted 21 lines
Fixes dff8d6c1ed58 ("swiotlb: remove the overflow buffer")]Reported-by John Stultz <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- include/linux/dma-direct.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/linux/dma-direct.h +1
-1@@ -5,7 +5,7 @@#include <linux/dma-mapping.h> #include <linux/mem_encrypt.h> -#define DIRECT_MAPPING_ERROR 0 +#define DIRECT_MAPPING_ERROR ~(dma_addr_t)0#ifdef CONFIG_ARCH_HAS_PHYS_TO_DMA #include <asm/dma-direct.h> Signature · 1 line
2.19.1.dirty
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Robin Murphy
This is what I have so far, which at least resolves the most ovbious problems. I still haven't got very far with the USB corruption issue I see on Juno with -rc1, but I'm yet to confirm whether that's actually attributable to the SWIOTLB changes or something else entirely.
This looks good modulo the minor nitpicks.
Konrad, are you ok with me picking up both through the dma-mapping tree?
Yes, albeit I would want Stefano to take a peek at patch #2 just in case.
Quoted 8 lines · Robin Murphy
This is what I have so far, which at least resolves the most ovbious problems. I still haven't got very far with the USB corruption issue I see on Juno with -rc1, but I'm yet to confirm whether that's actually attributable to the SWIOTLB changes or something else entirely.
Robin.
Quoted 7 lines
Robin Murphy (2):
swiotlb: MakeDIRECT_MAPPING_ERRORviable
swiotlb: Skip cache maintenance on map errorinclude/linux/dma-direct.h | 2 +- kernel/dma/swiotlb.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)
Thanks so much for chasing this down!
Unfortunately AOSP is giving me grief this week, so I've not been able to test the full environment, but I don't seem to be hitting the io hangs I was seeing earlier with this patch set.
For both:
thanks
-john
Quoted 4 lines · Konrad Rzeszutek Wilk
Quoted 2 lines
Konrad, are you ok with me picking up both through the dma-mapping tree?
Yes, albeit I would want Stefano to take a peek at patch #2 just in case.
Stefano, can you take a look asap? This is a pretty trivial fix for a nasty bug that breaks boot on real life systems. I'd like to merge it by tomorrow so that I can send it off to Linus for the next rc (I will be offline for about two days after Friday morning)
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Konrad Rzeszutek Wilk
Quoted 2 lines
Konrad, are you ok with me picking up both through the dma-mapping tree?
Yes, albeit I would want Stefano to take a peek at patch #2 just in case.
Stefano, can you take a look asap? This is a pretty trivial fix for a nasty bug that breaks boot on real life systems. I'd like to merge it by tomorrow so that I can send it off to Linus for the next rc (I will be offline for about two days after Friday morning)
It is Turkey Day in US so he may be busy catching those pesky turkeys. How about Tuesday? And in can also send the GIT PULL.
Quoted 7 lines · Konrad Rzeszutek Wilk
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · Konrad Rzeszutek Wilk
Quoted 2 lines
Konrad, are you ok with me picking up both through the dma-mapping tree?
Yes, albeit I would want Stefano to take a peek at patch #2 just in case.
Quoted 7 lines
Quoted 4 lines
Stefano, can you take a look asap? This is a pretty trivial fix for a nasty bug that breaks boot on real life systems. I'd like to merge it by tomorrow so that I can send it off to Linus for the next rc (I will be offline for about two days after Friday morning)
It is Turkey Day in US so he may be busy catching those pesky turkeys. How about Tuesday? And in can also send the GIT PULL.
I have just come back to the office. I'll give it a look today.
Quoted 13 lines · Stefano Stabellini
Quoted 12 lines · Konrad Rzeszutek Wilk
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Konrad Rzeszutek Wilk
Quoted 2 lines
Konrad, are you ok with me picking up both through the dma-mapping tree?
Yes, albeit I would want Stefano to take a peek at patch #2 just in case.
Stefano, can you take a look asap? This is a pretty trivial fix for a nasty bug that breaks boot on real life systems. I'd like to merge it by tomorrow so that I can send it off to Linus for the next rc (I will be offline for about two days after Friday morning)
Quoted 4 lines
Quoted 2 lines
It is Turkey Day in US so he may be busy catching those pesky turkeys. How about Tuesday? And in can also send the GIT PULL.
I have just come back to the office. I'll give it a look today.
Everything looks good to me, thanks for keeping me in the loop.