Re: [PATCH v5 4/6] dma: swiotlb: Centralize memory-encryption pool sizing

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] swiotlb: 64-bit division build error and 32-bit multiplication overflow
> --
>
> commit 7f27b828f817aa2b9787b04060d6febbcd685bcf
> Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>
> dma: swiotlb: Centralize memory-encryption pool sizing
>
> The patch moves the x86 guest-sizing policy into the SWIOTLB core, sizing
> the SWIOTLB_POOL_CC_GUEST pool to 6% of guest memory. It also provides a
> way for early users like the crash kernel to view the adjusted pool size
> before SWIOTLB initialization.
>
>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>> --- a/kernel/dma/swiotlb.c
>> +++ b/kernel/dma/swiotlb.c
>> @@ -480,6 +481,46 @@ static bool __init swiotlb_kmalloc_needs_bounce(void)
>>  
>> +/**
>> + * swiotlb_adjusted_size() - get the prospective adjusted SWIOTLB size
>> + *
> [ ... ]
>> + */
>> +unsigned long __init swiotlb_adjusted_size(void)
>> +{
>> +	unsigned long nslabs, size = swiotlb_size_or_default();
>> +
>> +	if (swiotlb_default_size_changed() ||
>> +	    !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
>> +		return size;
>
> [Severity: High]
> This is a pre-existing issue, but returning `size` directly here bypasses
> area-aware alignment for explicitly configured sizes and non-CC_GUEST setups.
>
> Could this discrepancy, along with the behavior of `swiotlb_aligned_nslabs()`
> during early boot, cause the crash kernel to under-reserve memory?
>
> The crash kernel reserves low memory based on the prospective size from
> `swiotlb_adjusted_size()` (or explicit `swiotlb=` sizes). However, early
> in boot, `default_nareas` is 0, so `swiotlb_aligned_nslabs()` merely aligns
> to segments and skips the area-aware power-of-two rounding:
>
> kernel/dma/swiotlb.c:swiotlb_aligned_nslabs() {
> 	...
> 	if (!default_nareas)
> 		return nslabs;
>
> 	if (nslabs < IO_TLB_SEGSIZE * default_nareas)
> 		nslabs = IO_TLB_SEGSIZE * default_nareas;
> 	else if (!is_power_of_2(nslabs))
> 		nslabs = roundup_pow_of_two(nslabs);
> 	...
> }
>
> Later, `swiotlb_init_remap()` initializes `default_nareas` and calls
> `swiotlb_adjust_nareas()`, triggering `round_up_default_nslabs()` to round
> the pool up to the next power of two.
>
> As a result, the allocated SWIOTLB pool can be up to ~50% larger than the
> reserved size (e.g., 600MB rounded to 1024MB). When the crash kernel
> attempts to allocate this rounded-up size, will it exceed its reserved
> memory and OOM?
>

I added a new patch that consolidates the slab rounding and addresses
this issue.

>
>> +	/*
>> +	 * For SEV and TDX and CCA, all DMA has to occur via
> [ ... ]
>> +	 * The percentage of guest memory used here for SWIOTLB buffers is
>> +	 * more of an approximation of the static adjustment which 64MB for
>> +	 * <1G, and ~128M to 256M for 1G-to-4G, i.e., the 6%
>> +	 */
>> +	size = memblock_phys_mem_size() * 6 / 100;
>> +	size = clamp_val(size, IO_TLB_DEFAULT_SIZE, SZ_1G);
>
> [Severity: High]
> Does this calculation cause an undefined reference to `__udivdi3` on 32-bit
> PAE builds, and overflow on 32-bit non-PAE builds?
>
> The guest sizing logic has been relocated into `kernel/dma/swiotlb.c`, which
> is compiled for all architectures. On 32-bit kernels with PAE, `phys_addr_t`
> is 64-bit, so `memblock_phys_mem_size() * 6 / 100` performs a 64-bit division
> by 100. GCC will emit a call to `__udivdi3`, which is missing from the
> kernel's libgcc, causing a fatal build link error.
>
> On 32-bit kernels without PAE, `phys_addr_t` is 32-bit. If physical memory
> exceeds ~715MB (`UINT_MAX / 6`), the multiplication `memblock_phys_mem_size()
> * 6` overflows the 32-bit integer, resulting in a silently truncated and
> incorrect pool size.
>
> Should this be refactored to cast the operand to `u64` and use `div_u64()` to
> be safe across all platforms?
>

Switched to div_u64() as shown below.

unsigned long __init swiotlb_adjusted_size(void)
{
	unsigned long nslabs;
	u64 size = swiotlb_size_or_default();

	if (!swiotlb_cmdline_size_set &&
	    cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
		/*
		 * For SEV and TDX and CCA, all DMA has to occur via
		 * shared/unencrypted pages. Kernel uses SWIOTLB to make this
		 * happen without changing device drivers. However, depending on
		 * the workload being run, the default 64MB of SWIOTLB may not be
		 * enough and SWIOTLB may run out of buffers for DMA, resulting in
		 * I/O errors and/or performance degradation especially with high
		 * I/O workloads.
		 *
		 * Adjust the default size of SWIOTLB using a percentage of guest
		 * memory for SWIOTLB buffers.
		 *
		 * The percentage of guest memory used here for SWIOTLB buffers is
		 * more of an approximation of the static adjustment which 64MB for
		 * <1G, and ~128M to 256M for 1G-to-4G, i.e., the 6%
		 */
		size = div_u64((u64)memblock_phys_mem_size() * 6, 100);
		size = clamp_val(size, IO_TLB_DEFAULT_SIZE, SZ_1G);
	}

	nslabs = swiotlb_calc_nslabs(size, default_nareas);

	return nslabs << IO_TLB_SHIFT;
}

-aneesh