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