Skip to contentSkip to conversation

arm64: swiotlb: Don’t shrink default buffer when bounce is forced

PATCH In review Aneesh Kumar K.V (Arm) 5 participants 8 messages Jan 20 – Mar 17

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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
index 524d34a0e921..7046241b47b8 100644
@@ -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
index 3dae0f592063..513a93dcbdbc 100644
@@ -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
index 0d37da3d95b6..85e31f228cc9 100644
@@ -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
#1 lore raw
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
---
 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
index 524d34a0e921..7046241b47b8 100644

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
index 3dae0f592063..513a93dcbdbc 100644
@@ -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
index 0d37da3d95b6..85e31f228cc9 100644
@@ -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
  *
reviewer
#2 lore raw
In reply to Aneesh Kumar K.V (Arm)
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.

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
---
  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
index 524d34a0e921..7046241b47b8 100644
@@ -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
index 3dae0f592063..513a93dcbdbc 100644
@@ -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
index 0d37da3d95b6..85e31f228cc9 100644
@@ -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
   *
#3 lore raw
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);

maintainer
#4 lore raw
In reply to Aneesh Kumar K.V (Arm)
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.

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
#5 lore raw
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

maintainer
#6 lore raw
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

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
#7 lore raw
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);