Re: [PATCH 2/7] IOMMU: rename and re-type ats_enabled

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.02.2024 12:49, Roger Pau Monné wrote:
> On Mon, Feb 05, 2024 at 02:55:43PM +0100, Jan Beulich wrote:
>> Make the variable a tristate, with (as done elsewhere) a negative value
>> meaning "default". Since all use sites need looking at, also rename it
>> to match our usual "opt_*" pattern. While touching it, also move it to
>> .data.ro_after_init.
>>
>> The only place it retains boolean nature is pci_ats_device(), for now.
> 
> Why does it retain the boolean nature in pci_ats_device()?
> 
> I assume this is to avoid having to touch the line again in a further
> patch, as given the current logic pci_ats_device() would also want to
> treat -1 as ATS disabled.

No, then I would need to touch the line. The function wants to treat
-1 as "maybe enabled", so the caller can know whether a device is an
ATS device regardless of whether ATS use is fully off, or only
"soft-off".

> I think this is all fine because you add additional opt_ats > 0 checks
> before the call to pci_ats_device(), but would be good to know this is
> the intention.

Note how amd_iommu_disable_domain_device() does not gain such a
check, for exactly the reason named above: The function would better
turn off ATS whenever enabled, no matter for what reason.

And of course - none of this "soft-off" vs "fully off" matters for
AMD (which is the only user of the function) right now anyway, seeing
they don't have an equivalent of the ATC_REQUIRED flag.

>> In AMD code re-order conditionals to have the config space accesses
>> after (cheaper) flag checks.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> In domain_context_mapping_one() I'm a little puzzled that translation
>> type is selected based on only IOMMU and global properties, i.e. not
>> taking the device itself into account.
> 
> That seems like a bug to me, we should check that the device supports
> ATS (and has it enabled) before setting the translation type to
> CONTEXT_TT_DEV_IOTLB unconditionally.  We should likely use
> ats_device() instead of ats_enabled in domain_context_mapping_one().

Will try to remember to add yet another patch then.

> There's also IMO a second bug here, which is that we possibly attempt
> to flush the device IOTLB before having ATS enabled.  We flush the
> device TLB in domain_context_mapping_one(), yet ATS is enabled by the
> caller afterwards (see domain_context_mapping()).

You may be right with this; I'd need to read up on whether such
flushing is permissible.

>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -185,10 +185,11 @@ static int __must_check amd_iommu_setup_
>>          dte->ex = ivrs_dev->dte_allow_exclusion;
>>          dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, 
>> ACPI_IVHD_SYSTEM_MGMT);
>>  
>> -        if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
>> +        if ( opt_ats > 0 &&
>>               !ivrs_dev->block_ats &&
>> -             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
>> -            dte->i = ats_enabled;
>> +             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
>> +             pci_ats_device(iommu->seg, bus, pdev->devfn) )
>> +            dte->i = true;
>>  
>>          spin_unlock_irqrestore(&iommu->lock, flags);
>>  
>> @@ -248,10 +249,11 @@ static int __must_check amd_iommu_setup_
>>          ASSERT(dte->sys_mgt == MASK_EXTR(ivrs_dev->device_flags,
>>                                           ACPI_IVHD_SYSTEM_MGMT));
>>  
>> -        if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
>> +        if ( opt_ats > 0 &&
>>               !ivrs_dev->block_ats &&
>> -             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
>> -            ASSERT(dte->i == ats_enabled);
>> +             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
>> +             pci_ats_device(iommu->seg, bus, pdev->devfn) )
>> +            ASSERT(dte->i);
>>  
>>          spin_unlock_irqrestore(&iommu->lock, flags);
>>  
>> @@ -268,9 +270,10 @@ static int __must_check amd_iommu_setup_
>>  
>>      ASSERT(pcidevs_locked());
>>  
>> -    if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
>> +    if ( opt_ats > 0 &&
>>           !ivrs_dev->block_ats &&
>>           iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
>> +         pci_ats_device(iommu->seg, bus, pdev->devfn) &&
>>           !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
> 
> Seeing that this same set of conditions is used in 3 different checks,
> could we add a wrapper for it?
> 
> opt_ats > 0 && !ivrs_dev->block_ats &&
> iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
> pci_ats_device(iommu->seg, bus, pdev->devfn)
> 
> pci_device_ats_capable()? or some such.

I was pondering that, yes (iirc already once when adding block_ats).
Problem is the name. "capable" isn't quite right when considering
the tristate opt_ats. And pci_device_may_use_ats() reads, well,
clumsy to me. If you have any good idea for a name that's fully
applicable and not odd or overly long, I can certainly introduce
such a helper.

Jan