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 <[email protected]>
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) ||
>
