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.

In AMD code re-order conditionals to have the config space accesses
after (cheaper) flag checks.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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.

--- 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 )
         return;
 
     if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
@@ -340,7 +340,7 @@ static void _amd_iommu_flush_pages(struc
         flush_command_buffer(iommu, 0);
     }
 
-    if ( ats_enabled )
+    if ( opt_ats > 0 )
     {
         amd_iommu_flush_all_iotlbs(d, daddr, order);
 
--- 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) )
     {
         if ( devfn == pdev->devfn )
--- a/xen/drivers/passthrough/ats.c
+++ b/xen/drivers/passthrough/ats.c
@@ -18,8 +18,8 @@
 #include <xen/pci_regs.h>
 #include "ats.h"
 
-bool __read_mostly ats_enabled;
-boolean_param("ats", ats_enabled);
+int8_t __ro_after_init opt_ats = -1;
+boolean_param("ats", opt_ats);
 
 int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list)
 {
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -22,7 +22,7 @@
 #define ATS_QUEUE_DEPTH_MASK     0x1f
 #define ATS_ENABLE               (1<<15)
 
-extern bool ats_enabled;
+extern int8_t opt_ats;
 
 int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list);
 void disable_ats_device(struct pci_dev *pdev);
@@ -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;
 
     return pci_find_ext_capability(PCI_SBDF(seg, bus, devfn),
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1543,7 +1543,7 @@ int domain_context_mapping_one(
         }
 
         context_set_address_root(lctxt, root);
-        if ( ats_enabled && ecap_dev_iotlb(iommu->ecap) )
+        if ( opt_ats > 0 && ecap_dev_iotlb(iommu->ecap) )
             context_set_translation_type(lctxt, CONTEXT_TT_DEV_IOTLB);
         else
             context_set_translation_type(lctxt, CONTEXT_TT_MULTI_LEVEL);
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -46,7 +46,7 @@ int ats_device(const struct pci_dev *pde
     struct acpi_drhd_unit *ats_drhd;
     int pos;
 
-    if ( !ats_enabled || !iommu_qinval )
+    if ( opt_ats <= 0 || !iommu_qinval )
         return 0;
 
     if ( !ecap_queued_inval(drhd->iommu->ecap) ||

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.

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.

> In AMD code re-order conditionals to have the config space accesses
> after (cheaper) flag checks.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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().

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()).

> 
> --- 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 )
>          return;
>  
>      if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
> @@ -340,7 +340,7 @@ static void _amd_iommu_flush_pages(struc
>          flush_command_buffer(iommu, 0);
>      }
>  
> -    if ( ats_enabled )
> +    if ( opt_ats > 0 )
>      {
>          amd_iommu_flush_all_iotlbs(d, daddr, order);
>  
> --- 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.

Thanks, Roger.

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@suse.com>
>> ---
>> 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

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.

> > 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@suse.com>
> >> ---
> >> 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.

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?

Thanks, Roger.

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

On Mon, Feb 12, 2024 at 11:45:33AM +0100, Jan Beulich wrote:
> 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.

Or assign to domIO?

> >>>> --- 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".

IMO we should prefer avoiding code repetition, even if at the cost
of having a handler that have a maybe not ideal naming, but I can't
force you to do that.

Thanks, Roger.

On 12.02.2024 16:38, Roger Pau Monné wrote:
> On Mon, Feb 12, 2024 at 11:45:33AM +0100, Jan Beulich wrote:
>> 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.
> 
> Or assign to domIO?

Not behind Dom0's back - I can't see how that would work if then Dom0
tried to load a driver for the device.

Jan