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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.