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