Re: [PATCH v3 4/4] dma: swiotlb: Remove SWIOTLB_ANY

[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] Inverting the default SWIOTLB allocation behavior from `memblock_alloc_low()` to `memblock_alloc()` breaks DMA for legacy 32-bit devices on x86 systems with `iommu=off` when memory encryption or `swiotlb=force` is used.
> --
>
> commit 35955ff88ad01e61df3c901065f4e509abaab7d0
> Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>
> dma: swiotlb: Remove SWIOTLB_ANY
>
> SWIOTLB_ANY permits the swiotlb pool to be allocated outside the
> architecture's low address limit. This is already implied when
> SWIOTLB_INIT_ADDRESSING_LIMIT is not set, making SWIOTLB_ANY redundant.
>
> Use SWIOTLB_INIT_ADDRESSING_LIMIT as the sole indication that the
> default pool and any dynamically allocated pools must reside below
> ARCH_LOW_ADDRESS_LIMIT. Otherwise, allow the pools to use any directly
> mapped memory.
>
>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>> index c4611b2c45403..2715833a21f8a 100644
>> --- a/kernel/dma/swiotlb.c
>> +++ b/kernel/dma/swiotlb.c
>> @@ -448,15 +448,10 @@ static void __init *swiotlb_memblock_alloc(unsigned long nslabs,
>>  	size_t bytes = PAGE_ALIGN(nslabs << IO_TLB_SHIFT);
>>  	void *tlb;
>>  
>> -	/*
>> -	 * By default allocate the bounce buffer memory from low memory, but
>> -	 * allow to pick a location everywhere for hypervisors with guest
>> -	 * memory encryption.
>> -	 */
>> -	if (flags & SWIOTLB_ANY)
>> -		tlb = memblock_alloc(bytes, PAGE_SIZE);
>> -	else
>> +	if (flags & SWIOTLB_INIT_ADDRESSING_LIMIT)
>>  		tlb = memblock_alloc_low(bytes, PAGE_SIZE);
>> +	else
>> +		tlb = memblock_alloc(bytes, PAGE_SIZE);
>
> [Severity: High]
> Does inverting the default SWIOTLB allocation behavior from memblock_alloc_low()
> to memblock_alloc() break DMA for legacy 32-bit devices on x86 systems?
>
> If a system boots with iommu=off and swiotlb=force (or memory encryption),
> pci_swiotlb_detect() leaves x86_swiotlb_flags as 0 because no_iommu is true:
>
> arch/x86/kernel/pci-dma.c:pci_swiotlb_detect() {
> 	/* don't initialize swiotlb if iommu=off (no_iommu=1) */
> 	if (!no_iommu && max_possible_pfn > MAX_DMA32_PFN) {
> 		x86_swiotlb_enable = true;
> 		x86_swiotlb_flags |= SWIOTLB_INIT_ADDRESSING_LIMIT;
> 	}
>     ...
> }
>
> Because SWIOTLB_INIT_ADDRESSING_LIMIT is not set, this new logic uses
> memblock_alloc() instead of the previous memblock_alloc_low(), placing the
> bounce buffer pool anywhere, including above 4GB.
>
> Legacy 32-bit devices will then fail DMA mappings because they receive bounce
> buffers allocated above 4GB, failing dma_capable() checks and causing
> swiotlb_map() to return DMA_MAPPING_ERROR.
>

I can fix this by adding a SWIOTLB_INIT_FORCE_DISABLE flag, which will
be checked after swiotlb_force_disable and swiotlb_force_bounce.

modified   arch/x86/kernel/pci-dma.c
@@ -43,10 +43,13 @@ unsigned int x86_swiotlb_flags;
 
 static void __init pci_swiotlb_detect(void)
 {
-	/* don't initialize swiotlb if iommu=off (no_iommu=1) */
-	if (!no_iommu && max_possible_pfn > MAX_DMA32_PFN) {
-		x86_swiotlb_enable = true;
+	if (no_iommu)
+		x86_swiotlb_flags |= SWIOTLB_INIT_FORCE_DISABLE;
+
+	if (max_possible_pfn > MAX_DMA32_PFN) {
 		x86_swiotlb_flags |= SWIOTLB_INIT_ADDRESSING_LIMIT;
+		if (!no_iommu)
+			x86_swiotlb_enable = true;
 	}
 

Should we do this, or simply remove the !no_iommu check from the
conditional? Is it OK to create a swiotlb pool when no_iommu is true?

>
> [ ... ]
>
>> @@ -598,10 +593,10 @@ void __init swiotlb_init_remap(unsigned int flags,
>>  #ifdef CONFIG_SWIOTLB_DYNAMIC
>>  	if (!remap)
>>  		io_tlb_default_mem.can_grow = true;
>> -	if (flags & SWIOTLB_ANY)
>> -		io_tlb_default_mem.phys_limit = virt_to_phys(high_memory - 1);
>> -	else
>> +	if (flags & SWIOTLB_INIT_ADDRESSING_LIMIT)
>>  		io_tlb_default_mem.phys_limit = ARCH_LOW_ADDRESS_LIMIT;
>> +	else
>> +		io_tlb_default_mem.phys_limit = virt_to_phys(high_memory - 1);
>>  #endif
>
> [Severity: High]
> Similarly, does this incorrectly record an unrestricted phys_limit for the pool
> when SWIOTLB_INIT_ADDRESSING_LIMIT is missing under the same iommu=off
> conditions?
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904101810.1193505-1-aneesh.kumar@kernel.org?part=4

-aneesh