Re: [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2024 14:42, Roger Pau Monné wrote:
> On Thu, Feb 15, 2024 at 11:15:39AM +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.
> 
> I guess I need to look at further patches, as given the feedback on
> the past version I think we agreed we want to set ATS unconditionally
> disabled by default, and hence I'm not sure I see the point of the
> tri-state if enabling ATS will require an explicit opt-in on the
> command line (ats=1).

With the present wording in the VT-d spec (which we've now had vague
indication that it may not be meant that way) there needs to be
tristate behavior:
- With "ats=0" ATS won't be used.
- With "ats=1" ATS will be used for all ATS-capable devices.
- Without either option ATS will be used for devices where firmware
  mandates its use.
If the alternative reading was confirmed (and preferably the text also
adjusted), we should be able to get away with a simple boolean again.
At which point this patch may end up being purely renaming, and hence
could then as well be left out.

>> The only place it retains boolean nature is pci_ats_device(), for now.
>>
>> 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.
> 
> Is it maybe fine do set DEV_IOTLB unconditionally as long as the IOMMU
> supports it, and then let the device decide whether it wants to issue
> translated or non-translated requests depending on whether it supports
> (and enables) ATS?

This might be the reason why it is what it is now.

> I think it would be best to limit this strictly to devices that have
> ATS enabled.

And this was the reason for me putting the remark here.

>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>> @@ -282,7 +282,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con
>>      struct amd_iommu *iommu;
>>      unsigned int req_id, queueid, maxpend;
>>  
>> -    if ( !ats_enabled )
>> +    if ( opt_ats <= 0 )
> 
> If having a tri-state is required, won't it be best to decide on a
> binary value at init time, so that runtime functions can handle
> opt_ats as a boolean?

As per above, unlike for other options we can't consolidate into a
boolean, as runtime behavior wants to be different with all three
possible settings.

> Otherwise we are open coding the default expectation of what -1
> implies (disabled) in all use sites.

That's pretty much unavoidable with the different meaning of 1 and -1.

>> @@ -196,7 +196,7 @@ static int __must_check amd_iommu_setup_
>>          dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, 
>> ACPI_IVHD_SYSTEM_MGMT);
>>  
>>          if ( use_ats(pdev, iommu, ivrs_dev) )
>> -            dte->i = ats_enabled;
>> +            dte->i = true;
> 
> Might be easier to just use:
> 
> dte->i = use_ats(pdev, iommu, ivrs_dev);

I'm hesitant here, as in principle we might be overwriting a "true" by
"false" then.

>> @@ -257,7 +257,7 @@ static int __must_check amd_iommu_setup_
>>                                           ACPI_IVHD_SYSTEM_MGMT));
>>  
>>          if ( use_ats(pdev, iommu, ivrs_dev) )
>> -            ASSERT(dte->i == ats_enabled);
>> +            ASSERT(dte->i);
> 
> ASSERT(dte->i == use_ats(pdev, iommu, ivrs_dev));

I'm okay switching here, but better to the precise logical equivalent of
the earlier code:

ASSERT(dte->i || !use_ats(pdev, iommu, ivrs_dev));

>> @@ -43,7 +43,7 @@ static inline int pci_ats_enabled(int se
>>  
>>  static inline int pci_ats_device(int seg, int bus, int devfn)
>>  {
>> -    if ( !ats_enabled )
>> +    if ( !opt_ats )
>>          return 0;
> 
> Can't you remove that check altogether now, since you are adding an
> opt_ats check to use_ats()?

Two reasons why not: For one this isn't AMD-specific code, and hence
shouldn't be tied to the AMD-specific use_ats(). In principle VT-d
code should be okay to call here, too. And then
amd_iommu_disable_domain_device() doesn't use use_ats(), but does call
here.

Jan