[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 02/12] IOMMU/x86: new command line option to suppress use of superpage mappings
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@xxxxxxxxxx> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > --- > 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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |