arm64: swiotlb: Don’t shrink default buffer when bounce is forced
Subsystems (2)
PATCH 1/1 arm64: swiotlb: Don’t shrink default buffer when bounce is forced
arm64 reduces the default swiotlb size (for unaligned kmalloc() bouncing) when it detects that no swiotlb bouncing is needed.
If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
--- arch/arm64/mm/init.c | 3 ++- include/linux/swiotlb.h | 7 +++++++ kernel/dma/swiotlb.c | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-)
arch/arm64/mm/init.c +2 -1
@@ -345,7 +345,8 @@ void __init arch_mm_preinit(void) flags |=SWIOTLB_FORCE ; } -if (IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) && !swiotlb) { +if (IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) && + !(swiotlb ||force_swiotlb_bounce ())) {/* * If no bouncing needed for ZONE_DMA, reduce the swiotlb * buffer for kmalloc() bouncing to 1MB per 1GB of RAM.
include/linux/swiotlb.h +7 -0
@@ -185,6 +185,7 @@ bool is_swiotlb_active(struct device *dev);void __init swiotlb_adjust_size (unsigned long size);phys_addr_t default_swiotlb_base (void );phys_addr_t default_swiotlb_limit (void ); +bool force_swiotlb_bounce (void );#else static inline void swiotlb_init (bool addressing_limited,unsigned int flags) {
@@ -234,6 +235,12 @@ static inline phys_addr_t default_swiotlb_limit(void) {return 0 ; } + +static inline bool force_swiotlb_bounce (void ) +{ +return false; +} +#endif /* CONFIG_SWIOTLB */ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,
kernel/dma/swiotlb.c +5 -0
@@ -1646,6 +1646,11 @@ phys_addr_t default_swiotlb_base(void)return io_tlb_default_mem.defpool.start; } +bool force_swiotlb_bounce (void ) +{ +return swiotlb_force_bounce; +} +/** * default_swiotlb_limit() - get the address limit of the default SWIOTLB *
Signature · 1 line
2.43.0
Quoted 7 lines · Aneesh Kumar K.V (Arm)
arm64 reduces the default swiotlb size (for unaligned
kmalloc()bouncing) when it detects that no swiotlb bouncing is needed.If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
Makes sense not to reduce the SWIOTLB buffer size when being forced by the administrator.
Quoted 13 lines
Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/mm/init.c | 3 ++- include/linux/swiotlb.h | 7 +++++++ kernel/dma/swiotlb.c | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-)arch/arm64/mm/init.c +0
-0
arch/arm64/mm/init.c void __init arch_mm_preinit(void) · line 352
Quoted 9 lines
arch/arm64/mm/init.c +2
-1@@ -345,7 +345,8 @@ void __init arch_mm_preinit(void) flags |=SWIOTLB_FORCE ; } -if (IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) && !swiotlb) { +if (IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) && + !(swiotlb ||force_swiotlb_bounce ())) {/* * If no bouncing needed for ZONE_DMA, reduce the swiotlb * buffer for kmalloc() bouncing to 1MB per 1GB of RAM.
Should the comment here be updated as well ?
Quoted 41 lines
include/linux/swiotlb.h +7
-0@@ -185,6 +185,7 @@ bool is_swiotlb_active(struct device *dev);void __init swiotlb_adjust_size (unsigned long size);phys_addr_t default_swiotlb_base (void );phys_addr_t default_swiotlb_limit (void ); +bool force_swiotlb_bounce (void );#else static inline void swiotlb_init (bool addressing_limited,unsigned int flags) {@@ -234,6 +235,12 @@ static inline phys_addr_t default_swiotlb_limit(void) {return 0 ; } + +static inline bool force_swiotlb_bounce (void ) +{ +return false; +} +#endif /* CONFIG_SWIOTLB */ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,kernel/dma/swiotlb.c +5
-0@@ -1646,6 +1646,11 @@ phys_addr_t default_swiotlb_base(void)return io_tlb_default_mem.defpool.start; } +bool force_swiotlb_bounce (void ) +{ +return swiotlb_force_bounce; +} +/** * default_swiotlb_limit() - get the address limit of the default SWIOTLB *
Robin Murphy
reviewerQuoted 7 lines · Aneesh Kumar K.V (Arm)
arm64 reduces the default swiotlb size (for unaligned
kmalloc()bouncing) when it detects that no swiotlb bouncing is needed.If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
This doesn't appear to be an arm64-specific concern though... Since swiotlb_adjust_size() already prevents resizing if the user requests a specific size on the command line, it seems logical enough to also not reduce the size (but I guess still allow it to be enlarged) there if force is requested.
(Although realistically, anyone requesting force is quite likely to want to request a larger default size anyway...)
Thanks,
Robin.
Quoted 62 lines
Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/mm/init.c | 3 ++- include/linux/swiotlb.h | 7 +++++++ kernel/dma/swiotlb.c | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-)arch/arm64/mm/init.c +2
-1@@ -345,7 +345,8 @@ void __init arch_mm_preinit(void) flags |=SWIOTLB_FORCE ; } -if (IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) && !swiotlb) { +if (IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) && + !(swiotlb ||force_swiotlb_bounce ())) {/* * If no bouncing needed for ZONE_DMA, reduce the swiotlb * buffer for kmalloc() bouncing to 1MB per 1GB of RAM. include/linux/swiotlb.h +7
-0@@ -185,6 +185,7 @@ bool is_swiotlb_active(struct device *dev);void __init swiotlb_adjust_size (unsigned long size);phys_addr_t default_swiotlb_base (void );phys_addr_t default_swiotlb_limit (void ); +bool force_swiotlb_bounce (void );#else static inline void swiotlb_init (bool addressing_limited,unsigned int flags) {@@ -234,6 +235,12 @@ static inline phys_addr_t default_swiotlb_limit(void) {return 0 ; } + +static inline bool force_swiotlb_bounce (void ) +{ +return false; +} +#endif /* CONFIG_SWIOTLB */ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,kernel/dma/swiotlb.c +5
-0@@ -1646,6 +1646,11 @@ phys_addr_t default_swiotlb_base(void)return io_tlb_default_mem.defpool.start; } +bool force_swiotlb_bounce (void ) +{ +return swiotlb_force_bounce; +} +/** * default_swiotlb_limit() - get the address limit of the default SWIOTLB *
Quoted 9 lines · Robin Murphy
Quoted 7 lines · Aneesh Kumar K.V (Arm)
arm64 reduces the default swiotlb size (for unaligned
kmalloc()bouncing) when it detects that no swiotlb bouncing is needed.If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
Quoted 6 lines
This doesn't appear to be an arm64-specific concern though... Since
swiotlb_adjust_size()already prevents resizing if the user requests a specific size on the command line, it seems logical enough to also not reduce the size (but I guess still allow it to be enlarged) there if force is requested.
Something like the below? I am wondering whether we are doing more than what the function name suggests. Not allowing the size to be adjusted when the kernel parameter specifies a swiotlb size seems fine. However, I am not sure whether adding the force_bounce check is a good idea. I only found RISC-V doing a similar size adjustment to arm64. Maybe we can fix both architectures?
@@ -211,6 +211,8 @@ unsigned long swiotlb_size_or_default(void)
void __init swiotlb_adjust_size(unsigned long size)
{
+ unsigned long nslabs;
+
/*
* If swiotlb parameter has not been specified, give a chance to
* architectures such as those supporting memory encryption to
@@ -220,7 +222,13 @@ void __init swiotlb_adjust_size(unsigned long size)
return;
size = ALIGN(size, IO_TLB_SIZE);
- default_nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
+ nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
+ /*
+ * Don't allow to reduce size if we are forcing swiotlb bounce.
+ */
+ if (swiotlb_force_bounce && nslabs < default_nslabs)
+ return;
+ default_nslabs = nslabs;
if (round_up_default_nslabs())
size = default_nslabs << IO_TLB_SHIFT;
pr_info("SWIOTLB bounce buffer size adjusted to %luMB", size >> 20);
Catalin Marinas
maintainerQuoted 7 lines · Aneesh Kumar K.V (Arm)
arm64 reduces the default swiotlb size (for unaligned
kmalloc()bouncing) when it detects that no swiotlb bouncing is needed.If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
I think the logic you proposed in reply to Robin might work better but have you actually hit a problem that triggered this patch? Do people passing swiotlb=force expect a specific size for the buffer?
Signature · 1 line
Catalin
Quoted 4 lines · Catalin Marinas
Quoted 3 lines · Aneesh Kumar K.V (Arm)
arm64 reduces the default swiotlb size (for unaligned
kmalloc()bouncing) when it detects that no swiotlb bouncing is needed.
Quoted 9 lines
Quoted 4 lines
If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
I think the logic you proposed in reply to Robin might work better but have you actually hit a problem that triggered this patch? Do people passing swiotlb=force expect a specific size for the buffer?
This issue was observed while implementing swiotlb for a trusted device. I was testing the protected swiotlb space using the swiotlb=force option, which causes the device to use swiotlb even in protected mode. As per Robin, an end user using the swiotlb=force option will also specify a custom swiotlb size
-aneesh
Marek Szyprowski
maintainerQuoted 13 lines · Aneesh Kumar K.V
Quoted 4 lines · Catalin Marinas
Quoted 3 lines · Aneesh Kumar K.V (Arm)
arm64 reduces the default swiotlb size (for unaligned
kmalloc()bouncing) when it detects that no swiotlb bouncing is needed.Quoted 8 lines
Quoted 4 lines
If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
I think the logic you proposed in reply to Robin might work better but have you actually hit a problem that triggered this patch? Do people passing swiotlb=force expect a specific size for the buffer?
Quoted 5 lines
This issue was observed while implementing swiotlb for a trusted device. I was testing the protected swiotlb space using the swiotlb=force option, which causes the device to use swiotlb even in protected mode. As per Robin, an end user using the swiotlb=force option will also specify a custom swiotlb size
Does the above mean that it works fine when user provides both swiotlb=force and custom swiotlb size, so no changes in the code are actually needed?
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
Quoted 20 lines · Marek Szyprowski
Quoted 13 lines · Aneesh Kumar K.V
Quoted 4 lines · Catalin Marinas
Quoted 3 lines · Aneesh Kumar K.V (Arm)
arm64 reduces the default swiotlb size (for unaligned
kmalloc()bouncing) when it detects that no swiotlb bouncing is needed.Quoted 8 lines
Quoted 4 lines
If swiotlb bouncing is explicitly forced via the command line (swiotlb=force), this heuristic must not apply. Add a swiotlb helper to query the forced-bounce state and use it to skip the resize when bouncing is forced.
I think the logic you proposed in reply to Robin might work better but have you actually hit a problem that triggered this patch? Do people passing swiotlb=force expect a specific size for the buffer?
Quoted 5 lines
This issue was observed while implementing swiotlb for a trusted device. I was testing the protected swiotlb space using the swiotlb=force option, which causes the device to use swiotlb even in protected mode. As per Robin, an end user using the swiotlb=force option will also specify a custom swiotlb size
Quoted 4 lines
Does the above mean that it works fine when user provides both swiotlb=force and custom swiotlb size, so no changes in the code are actually needed?
swiotlb_adjust_size() checks whether the default_nslabs value has changed and avoids updating the SWIOTLB size based on different subsystem logic.
void __init swiotlb_adjust_size(unsigned long size)
{
/*
* If swiotlb parameter has not been specified, give a chance to
* architectures such as those supporting memory encryption to
* adjust/expand SWIOTLB size for their use.
*/
if (default_nslabs != IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT)
return;To handle swiotlb_force alone we can do
modified kernel/dma/swiotlb.c
@@ -209,6 +209,8 @@ unsigned long swiotlb_size_or_default(void)
void __init swiotlb_adjust_size(unsigned long size)
{
+ unsigned long nslabs;
+
/*
* If swiotlb parameter has not been specified, give a chance to
* architectures such as those supporting memory encryption to
@@ -218,7 +220,13 @@ void __init swiotlb_adjust_size(unsigned long size)
return;
size = ALIGN(size, IO_TLB_SIZE);
- default_nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
+ nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
+ /*
+ * Don't allow to reduce size if we are forcing swiotlb bounce.
+ */
+ if (swiotlb_force_bounce && nslabs < default_nslabs)
+ return;
+ default_nslabs = nslabs;
if (round_up_default_nslabs())
size = default_nslabs << IO_TLB_SHIFT;
pr_info("SWIOTLB bounce buffer size adjusted to %luMB", size >> 20);