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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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

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.