| Message ID | 99086452-43a8-2d93-ab4d-0343a0259259@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOMMU: superpage support when not sharing pagetables | expand |
On Thu, Jun 09, 2022 at 12:17:23PM +0200, Jan Beulich wrote: > Before actually enabling their use, provide a means to suppress it in > case of problems. Note that using the option can also affect the sharing > of page tables in the VT-d / EPT combination: If EPT would use large > page mappings but the option is in effect, page table sharing would be > suppressed (to properly fulfill the admin request). > > Requested-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v6: New. > > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -1405,7 +1405,7 @@ detection of systems known to misbehave > > ### iommu > = List of [ <bool>, verbose, debug, force, required, quarantine[=scratch-page], > - sharept, intremap, intpost, crash-disable, > + sharept, superpages, intremap, intpost, crash-disable, > snoop, qinval, igfx, amd-iommu-perdev-intremap, > dom0-{passthrough,strict} ] > > @@ -1481,6 +1481,12 @@ boolean (e.g. `iommu=no`) can override t > > This option is ignored on ARM, and the pagetables are always shared. > > +* The `superpages` boolean controls whether superpage mappings may be used > + in IOMMU page tables. If using this option is necessary to fix an issue, > + please report a bug. > + > + This option is only valid on x86. > + > * The `intremap` boolean controls the Interrupt Remapping sub-feature, and > is active by default on compatible hardware. On x86 systems, the first > generation of IOMMUs only supported DMA remapping, and Interrupt Remapping > --- a/xen/arch/x86/include/asm/iommu.h > +++ b/xen/arch/x86/include/asm/iommu.h > @@ -132,7 +132,7 @@ extern bool untrusted_msi; > int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq, > const uint8_t gvec); > > -extern bool iommu_non_coherent; > +extern bool iommu_non_coherent, iommu_superpages; > > static inline void iommu_sync_cache(const void *addr, unsigned int size) > { > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -88,6 +88,8 @@ static int __init cf_check parse_iommu_p > iommu_igfx = val; > else if ( (val = parse_boolean("qinval", s, ss)) >= 0 ) > iommu_qinval = val; > + else if ( (val = parse_boolean("superpages", s, ss)) >= 0 ) > + iommu_superpages = val; > #endif > else if ( (val = parse_boolean("verbose", s, ss)) >= 0 ) > iommu_verbose = val; > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2213,7 +2213,8 @@ static bool __init vtd_ept_page_compatib > if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) > return false; > > - return (ept_has_2mb(ept_cap) && opt_hap_2mb) <= cap_sps_2mb(vtd_cap) && > + return iommu_superpages && > + (ept_has_2mb(ept_cap) && opt_hap_2mb) <= cap_sps_2mb(vtd_cap) && > (ept_has_1gb(ept_cap) && opt_hap_1gb) <= cap_sps_1gb(vtd_cap); Isn't this too strict in requesting iommu superpages to be enabled regardless of whether EPT has superpage support? Shouldn't this instead be: return iommu_superpages ? ((ept_has_2mb(ept_cap) && opt_hap_2mb) <= cap_sps_2mb(vtd_cap) && (ept_has_1gb(ept_cap) && opt_hap_1gb) <= cap_sps_1gb(vtd_cap)) : !((ept_has_2mb(ept_cap) && opt_hap_2mb) || (ept_has_1gb(ept_cap) && opt_hap_1gb)); I think we want to introduce some local variables to store EPT superpage availability, as the lines are too long. The rest LGTM. Thanks, Roger.
> From: Roger Pau Monné <roger.pau@citrix.com> > Sent: Tuesday, June 28, 2022 8:52 PM > > On Thu, Jun 09, 2022 at 12:17:23PM +0200, Jan Beulich wrote: > > Before actually enabling their use, provide a means to suppress it in > > case of problems. Note that using the option can also affect the sharing > > of page tables in the VT-d / EPT combination: If EPT would use large > > page mappings but the option is in effect, page table sharing would be > > suppressed (to properly fulfill the admin request). > > > > Requested-by: Roger Pau Monné <roger.pau@citrix.com> > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- > > v6: New. > > > > --- a/docs/misc/xen-command-line.pandoc > > +++ b/docs/misc/xen-command-line.pandoc > > @@ -1405,7 +1405,7 @@ detection of systems known to misbehave > > > > ### iommu > > = List of [ <bool>, verbose, debug, force, required, quarantine[=scratch- > page], > > - sharept, intremap, intpost, crash-disable, > > + sharept, superpages, intremap, intpost, crash-disable, > > snoop, qinval, igfx, amd-iommu-perdev-intremap, > > dom0-{passthrough,strict} ] > > > > @@ -1481,6 +1481,12 @@ boolean (e.g. `iommu=no`) can override t > > > > This option is ignored on ARM, and the pagetables are always shared. > > > > +* The `superpages` boolean controls whether superpage mappings may > be used > > + in IOMMU page tables. If using this option is necessary to fix an issue, > > + please report a bug. > > + > > + This option is only valid on x86. > > + > > * The `intremap` boolean controls the Interrupt Remapping sub-feature, > and > > is active by default on compatible hardware. On x86 systems, the first > > generation of IOMMUs only supported DMA remapping, and Interrupt > Remapping > > --- a/xen/arch/x86/include/asm/iommu.h > > +++ b/xen/arch/x86/include/asm/iommu.h > > @@ -132,7 +132,7 @@ extern bool untrusted_msi; > > int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq, > > const uint8_t gvec); > > > > -extern bool iommu_non_coherent; > > +extern bool iommu_non_coherent, iommu_superpages; > > > > static inline void iommu_sync_cache(const void *addr, unsigned int size) > > { > > --- a/xen/drivers/passthrough/iommu.c > > +++ b/xen/drivers/passthrough/iommu.c > > @@ -88,6 +88,8 @@ static int __init cf_check parse_iommu_p > > iommu_igfx = val; > > else if ( (val = parse_boolean("qinval", s, ss)) >= 0 ) > > iommu_qinval = val; > > + else if ( (val = parse_boolean("superpages", s, ss)) >= 0 ) > > + iommu_superpages = val; > > #endif > > else if ( (val = parse_boolean("verbose", s, ss)) >= 0 ) > > iommu_verbose = val; > > --- a/xen/drivers/passthrough/vtd/iommu.c > > +++ b/xen/drivers/passthrough/vtd/iommu.c > > @@ -2213,7 +2213,8 @@ static bool __init vtd_ept_page_compatib > > if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) > > return false; > > > > - return (ept_has_2mb(ept_cap) && opt_hap_2mb) <= > cap_sps_2mb(vtd_cap) && > > + return iommu_superpages && > > + (ept_has_2mb(ept_cap) && opt_hap_2mb) <= > cap_sps_2mb(vtd_cap) && > > (ept_has_1gb(ept_cap) && opt_hap_1gb) <= cap_sps_1gb(vtd_cap); > > Isn't this too strict in requesting iommu superpages to be enabled > regardless of whether EPT has superpage support? > > Shouldn't this instead be: > > return iommu_superpages ? ((ept_has_2mb(ept_cap) && opt_hap_2mb) <= > cap_sps_2mb(vtd_cap) && > (ept_has_1gb(ept_cap) && opt_hap_1gb) <= > cap_sps_1gb(vtd_cap)) > : !((ept_has_2mb(ept_cap) && opt_hap_2mb) || > (ept_has_1gb(ept_cap) && opt_hap_1gb)); > > I think we want to introduce some local variables to store EPT > superpage availability, as the lines are too long. > Or to be pair with ept side checks: return (ept_has_2mb(ept_cap) && opt_hap_2mb) <= (cap_sps_2mb(vtd_cap) && iommu_superpages) && (ept_has_1gb(ept_cap) && opt_hap_1gb) <= (cap_sps_1gb(vtd_cap) && iommu_superpages);
--- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -1405,7 +1405,7 @@ detection of systems known to misbehave ### iommu = List of [ <bool>, verbose, debug, force, required, quarantine[=scratch-page], - sharept, intremap, intpost, crash-disable, + sharept, superpages, intremap, intpost, crash-disable, snoop, qinval, igfx, amd-iommu-perdev-intremap, dom0-{passthrough,strict} ] @@ -1481,6 +1481,12 @@ boolean (e.g. `iommu=no`) can override t This option is ignored on ARM, and the pagetables are always shared. +* The `superpages` boolean controls whether superpage mappings may be used + in IOMMU page tables. If using this option is necessary to fix an issue, + please report a bug. + + This option is only valid on x86. + * The `intremap` boolean controls the Interrupt Remapping sub-feature, and is active by default on compatible hardware. On x86 systems, the first generation of IOMMUs only supported DMA remapping, and Interrupt Remapping --- a/xen/arch/x86/include/asm/iommu.h +++ b/xen/arch/x86/include/asm/iommu.h @@ -132,7 +132,7 @@ extern bool untrusted_msi; int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq, const uint8_t gvec); -extern bool iommu_non_coherent; +extern bool iommu_non_coherent, iommu_superpages; static inline void iommu_sync_cache(const void *addr, unsigned int size) { --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -88,6 +88,8 @@ static int __init cf_check parse_iommu_p iommu_igfx = val; else if ( (val = parse_boolean("qinval", s, ss)) >= 0 ) iommu_qinval = val; + else if ( (val = parse_boolean("superpages", s, ss)) >= 0 ) + iommu_superpages = val; #endif else if ( (val = parse_boolean("verbose", s, ss)) >= 0 ) iommu_verbose = val; --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2213,7 +2213,8 @@ static bool __init vtd_ept_page_compatib if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) return false; - return (ept_has_2mb(ept_cap) && opt_hap_2mb) <= cap_sps_2mb(vtd_cap) && + return iommu_superpages && + (ept_has_2mb(ept_cap) && opt_hap_2mb) <= cap_sps_2mb(vtd_cap) && (ept_has_1gb(ept_cap) && opt_hap_1gb) <= cap_sps_1gb(vtd_cap); } --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -31,6 +31,7 @@ const struct iommu_init_ops *__initdata iommu_init_ops; struct iommu_ops __ro_after_init iommu_ops; bool __read_mostly iommu_non_coherent; +bool __initdata iommu_superpages = true; enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full; @@ -104,8 +105,13 @@ int __init iommu_hardware_setup(void) mask_IO_APIC_setup(ioapic_entries); } + if ( !iommu_superpages ) + iommu_ops.page_sizes &= PAGE_SIZE_4K; + rc = iommu_init_ops->setup(); + ASSERT(iommu_superpages || iommu_ops.page_sizes == PAGE_SIZE_4K); + if ( ioapic_entries ) { restore_IO_APIC_setup(ioapic_entries, rc);
Before actually enabling their use, provide a means to suppress it in case of problems. Note that using the option can also affect the sharing of page tables in the VT-d / EPT combination: If EPT would use large page mappings but the option is in effect, page table sharing would be suppressed (to properly fulfill the admin request). Requested-by: Roger Pau Monné <roger.pau@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v6: New.