RE: [PATCH v6 02/12] IOMMU/x86: new command line option to suppress use of superpage mappings

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> 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);