[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
> From: Roger Pau Monné <roger.pau@xxxxxxxxxx> > 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@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. > 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);
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |