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é <[email protected]>
> Signed-off-by: Jan Beulich <[email protected]>
> ---
> 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.
