Re: [PATCH 1/6] x86/boot: Rework dom0 feature configuration

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/6] x86/boot: Rework dom0 feature configuration
Date: Tue, 16 May 2023 09:58:34 +0200	[thread overview]
Message-ID: <97825c89-87c5-1156-5621-9d03286fd865@suse.com> (raw)
In-Reply-To: <20230515144259.1009245-2-andrew.cooper3@citrix.com>

On 15.05.2023 16:42, Andrew Cooper wrote:
> Right now, dom0's feature configuration is split between between the common
> path and a dom0-specific one.  This mostly is by accident, and causes some
> very subtle bugs.
> 
> First, start by clearly defining init_dom0_cpuid_policy() to be the domain
> that Xen builds automatically.  The late hwdom case is still constructed in a
> mostly normal way, with the control domain having full discretion over the CPU
> policy.
> 
> Identifying this highlights a latent bug - the two halves of the MSR_ARCH_CAPS
> bodge are asymmetric with respect to the hardware domain.  This means that
> shim, or a control-only dom0 sees the MSR_ARCH_CAPS CPUID bit but none of the
> MSR content.  This in turn declares the hardware to be retpoline-safe by
> failing to advertise the {R,}RSBA bits appropriately.  Restrict this logic to
> the hardware domain, although the special case will cease to exist shortly.
> 
> For the CPUID Faulting adjustment, the comment in ctxt_switch_levelling()
> isn't actually relevant.  Provide a better explanation.
> 
> Move the recalculate_cpuid_policy() call outside of the dom0-cpuid= case.
> This is no change for now, but will become necessary shortly.
> 
> Finally, place the second half of the MSR_ARCH_CAPS bodge after the
> recalculate_cpuid_policy() call.  This is necessary to avoid transiently
> breaking the hardware domain's view while the handling is cleaned up.  This
> special case will cease to exist shortly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one question / suggestion:

> @@ -858,7 +839,7 @@ void __init init_dom0_cpuid_policy(struct domain *d)
>       * so dom0 can turn off workarounds as appropriate.  Temporary, until the
>       * domain policy logic gains a better understanding of MSRs.
>       */
> -    if ( cpu_has_arch_caps )
> +    if ( is_hardware_domain(d) && cpu_has_arch_caps )
>          p->feat.arch_caps = true;

As a result of this, ...

> @@ -876,8 +857,32 @@ void __init init_dom0_cpuid_policy(struct domain *d)
>          }
>  
>          x86_cpu_featureset_to_policy(fs, p);
> +    }
> +
> +    /*
> +     * PV Control domains used to require unfiltered CPUID.  This was fixed in
> +     * Xen 4.13, but there is an cmdline knob to restore the prior behaviour.
> +     *
> +     * If the domain is getting unfiltered CPUID, don't let the guest kernel
> +     * play with CPUID faulting either, as Xen's CPUID path won't cope.
> +     */
> +    if ( !opt_dom0_cpuid_faulting && is_control_domain(d) && is_pv_domain(d) )
> +        p->platform_info.cpuid_faulting = false;
>  
> -        recalculate_cpuid_policy(d);
> +    recalculate_cpuid_policy(d);
> +
> +    if ( is_hardware_domain(d) && cpu_has_arch_caps )

... it would feel slightly more logical if p->feat.arch_caps was used here.
Whether that's to replace the entire condition or merely the right side of
the && depends on what the subsequent changes require (which I haven't
looked at yet).

Jan

> +    {
> +        uint64_t val;
> +
> +        rdmsrl(MSR_ARCH_CAPABILITIES, val);
> +
> +        p->arch_caps.raw = val &
> +            (ARCH_CAPS_RDCL_NO | ARCH_CAPS_IBRS_ALL | ARCH_CAPS_RSBA |
> +             ARCH_CAPS_SSB_NO | ARCH_CAPS_MDS_NO | ARCH_CAPS_IF_PSCHANGE_MC_NO |
> +             ARCH_CAPS_TAA_NO | ARCH_CAPS_SBDR_SSDP_NO | ARCH_CAPS_FBSDP_NO |
> +             ARCH_CAPS_PSDP_NO | ARCH_CAPS_FB_CLEAR | ARCH_CAPS_RRSBA |
> +             ARCH_CAPS_BHI_NO | ARCH_CAPS_PBRSB_NO);
>      }
>  }
>  