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

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

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

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.