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
