Re: [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.02.2024 11:15, 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.
> 
> In AMD code re-order conditionals to have the config space accesses
> after (cheaper) flag checks.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

I'm sorry, forgot to Cc Andrew for the AMD IOMMU part of the changes.

Jan

> ---
> 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.
> ---
> v2: Re-base over new earlier patches.
> 
> --- 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
> @@ -119,7 +119,7 @@ static bool use_ats(
>      const struct amd_iommu *iommu,
>      const struct ivrs_mappings *ivrs_dev)
>  {
> -    return !ivrs_dev->block_ats &&
> +    return opt_ats > 0 && !ivrs_dev->block_ats &&
>             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
>             pci_ats_device(iommu->seg, pdev->bus, pdev->devfn);
>  }
> @@ -196,7 +196,7 @@ static int __must_check amd_iommu_setup_
>          dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, 
> ACPI_IVHD_SYSTEM_MGMT);
>  
>          if ( use_ats(pdev, iommu, ivrs_dev) )
> -            dte->i = ats_enabled;
> +            dte->i = true;
>  
>          spin_unlock_irqrestore(&iommu->lock, flags);
>  
> @@ -257,7 +257,7 @@ static int __must_check amd_iommu_setup_
>                                           ACPI_IVHD_SYSTEM_MGMT));
>  
>          if ( use_ats(pdev, iommu, ivrs_dev) )
> -            ASSERT(dte->i == ats_enabled);
> +            ASSERT(dte->i);
>  
>          spin_unlock_irqrestore(&iommu->lock, flags);
>  
> --- a/xen/drivers/passthrough/ats.c
> +++ b/xen/drivers/passthrough/ats.c
> @@ -18,8 +18,8 @@
>  #include <xen/pci_regs.h>
>  #include "ats.h"
>  
> -bool __read_mostly ats_enabled;
> -boolean_param("ats", ats_enabled);
> +int8_t __ro_after_init opt_ats = -1;
> +boolean_param("ats", opt_ats);
>  
>  int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list)
>  {
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -22,7 +22,7 @@
>  #define ATS_QUEUE_DEPTH_MASK     0x1f
>  #define ATS_ENABLE               (1<<15)
>  
> -extern bool ats_enabled;
> +extern int8_t opt_ats;
>  
>  int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list);
>  void disable_ats_device(struct pci_dev *pdev);
> @@ -43,7 +43,7 @@ static inline int pci_ats_enabled(int se
>  
>  static inline int pci_ats_device(int seg, int bus, int devfn)
>  {
> -    if ( !ats_enabled )
> +    if ( !opt_ats )
>          return 0;
>  
>      return pci_find_ext_capability(PCI_SBDF(seg, bus, devfn),
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1543,7 +1543,7 @@ int domain_context_mapping_one(
>          }
>  
>          context_set_address_root(lctxt, root);
> -        if ( ats_enabled && ecap_dev_iotlb(iommu->ecap) )
> +        if ( opt_ats > 0 && ecap_dev_iotlb(iommu->ecap) )
>              context_set_translation_type(lctxt, CONTEXT_TT_DEV_IOTLB);
>          else
>              context_set_translation_type(lctxt, CONTEXT_TT_MULTI_LEVEL);
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -46,7 +46,7 @@ int ats_device(const struct pci_dev *pde
>      struct acpi_drhd_unit *ats_drhd;
>      unsigned int pos, expfl = 0;
>  
> -    if ( !ats_enabled || !iommu_qinval )
> +    if ( opt_ats <= 0 || !iommu_qinval )
>          return 0;
>  
>      if ( !ecap_queued_inval(drhd->iommu->ecap) ||
>