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