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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.02.2024 10:39, Roger Pau Monné wrote:
> On Thu, Feb 08, 2024 at 04:49:46PM +0100, Jan Beulich wrote:
>> 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 have to admit I'm slightly concerned about this soft-off.  Given the
> current status of ATS itself in Xen, and the technology itself, I
> think a user should always opt-in to ATS usage.

The plan is to follow your suggestion in patch 3 and require explicit
enabling for passing through of such devices. For Dom0, however, I
think it is important that we respect the firmware request by default.
The only viable(?!) alternative would be to panic() instead.

>>>> --- 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.
> 
> But if ATS is soft-disabled (-1) or hard disabled (0), it's fine to
> consider the devices as not ATS capable for the context here?

I don't like mixing capability and policy aspects into a resulting
"capable".

Jan