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

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

+x86@kernel.org and maintainers

Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:

> 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