| Message ID | 7f11ca06-9bed-443b-9c79-0e62b71a1f96@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VT-d: SATC handling; ATS: tidying | expand |
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@suse.com> 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) || >
--- 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) ||
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@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. --- v2: Re-base over new earlier patches.