Re: [PATCH v5 5/6] dma: swiotlb: Add an overridable architecture pool opt-out

[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] dma: swiotlb: default opt-out incorrectly overrides iommu=soft
> --
>
> commit 8a081ec4054b9ec450c0c293130d0ceb6b8d20fa
> Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>
> dma: swiotlb: Add an overridable architecture pool opt-out
>
> This commit introduces SWIOTLB_INIT_DEFAULT_OFF to allow an architecture
> to opt out of SWIOTLB pool initialization without losing placement
> constraints.
>
>> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
>> index 0cff255827baf..ce257b20ea9cf 100644
>> --- a/arch/x86/kernel/pci-dma.c
>> +++ b/arch/x86/kernel/pci-dma.c
>> @@ -44,9 +44,13 @@ static 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_DEFAULT_OFF;
>
> [Severity: High]
> When this is combined with iommu=soft (which sets
> SWIOTLB_INIT_ADDRESSING_LIMIT), doesn't this flag overwrite the user's
> explicit fallback request?
>
> [ ... ]
>
>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>> index c19675846fc1d..46b789eb08063 100644
>> --- a/kernel/dma/swiotlb.c
>> +++ b/kernel/dma/swiotlb.c
>> @@ -563,10 +563,16 @@ swiotlb_select_pool_policy(unsigned int flags)
>>  	if (flags & SWIOTLB_INIT_REMAP)
>>  		return SWIOTLB_POOL_DEFAULT;
>>  
>> -	if (flags & SWIOTLB_INIT_ADDRESSING_LIMIT)
>> +	if (swiotlb_force_bounce)
>>  		return SWIOTLB_POOL_DEFAULT;
>>  
>> -	if (swiotlb_force_bounce)
>> +	/*
>> +	 * Explicit requirements above override an architecture's default opt-out.
>> +	 */
>> +	if (flags & SWIOTLB_INIT_DEFAULT_OFF)
>> +		return SWIOTLB_POOL_NONE;
>> +
>> +	if (flags & SWIOTLB_INIT_ADDRESSING_LIMIT)
>>  		return SWIOTLB_POOL_DEFAULT;
>>  
>>  	if (swiotlb_kmalloc_needs_bounce())
>
> [Severity: High]
> By evaluating SWIOTLB_INIT_DEFAULT_OFF before SWIOTLB_INIT_ADDRESSING_LIMIT
> and swiotlb_kmalloc_needs_bounce(), does the default opt-out incorrectly
> override explicit requests for a software bounce pool and unaligned kmalloc
> bouncing?
>
> For instance, if a user boots an x86 system with iommu=off iommu=soft,
> pci_swiotlb_detect() sets SWIOTLB_INIT_DEFAULT_OFF because of no_iommu=1.
> The iommu=soft parameter also sets SWIOTLB_INIT_ADDRESSING_LIMIT.
>

I don't think combining iommu=off with iommu=soft is valid.

>
> Because the opt-out is checked first here in swiotlb_select_pool_policy(),
> it returns SWIOTLB_POOL_NONE and prevents the pool from being allocated.
> Could this lead to DMA mapping failures for devices that cannot address all
> system memory, contradicting the commit message which states that iommu=soft
> retains the addressing limit?
>

-aneesh