Re: [PATCH 6/6] x86/boot: Expose MSR_ARCH_CAPS data in guest max policies

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 6/6] x86/boot: Expose MSR_ARCH_CAPS data in guest max policies
Date: Tue, 16 May 2023 14:51:02 +0100	[thread overview]
Message-ID: <25421dbc-5889-a33c-37dd-d82476d56ce4@citrix.com> (raw)
In-Reply-To: <a545a6c9-db48-9d91-c23b-59ea97def769@suse.com>

On 16/05/2023 2:06 pm, Jan Beulich wrote:
> On 15.05.2023 16:42, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -408,6 +408,25 @@ static void __init calculate_host_policy(void)
>>      p->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
>>  }
>>  
>> +static void __init guest_common_max_feature_adjustments(uint32_t *fs)
>> +{
>> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
>> +    {
>> +        /*
>> +         * MSR_ARCH_CAPS is just feature data, and we can offer it to guests
>> +         * unconditionally, although limit it to Intel systems as it is highly
>> +         * uarch-specific.
>> +         *
>> +         * In particular, the RSBA and RRSBA bits mean "you might migrate to a
>> +         * system where RSB underflow uses alternative predictors (a.k.a
>> +         * Retpoline not safe)", so these need to be visible to a guest in all
>> +         * cases, even when it's only some other server in the pool which
>> +         * suffers the identified behaviour.
>> +         */
>> +        __set_bit(X86_FEATURE_ARCH_CAPS, fs);
>> +    }
>> +}
> The comment reads as if it wasn't applying to "max" only, but rather to
> "default". Reading this I'm therefore now (and perhaps even more so in
> the future, when coming across it) wondering whether it's misplaced, or
> and hence whether the commented code is also misplaced and/or wrong.

On migrate-in, we (well - toolstacks that understand multiple hosts)
check the cpu policy the VM saw against the appropriate PV/HVM max
policy to determine whether it can safely run.

So this is very intentionally for the max policy.  We need (I think -
still pending an clarification from Intel because there's pending work
still not published) to set RSBA unconditionally, and RRSBA conditional
on EIBRS being available, in max even on pre-Skylake hardware such that
we can migrate-in a VM which previously ran on Skylake or later hardware.

Activating this by default for VMs is just a case of swapping the CPUID
ARCH_CAPS bit from 'a' to 'A', without any adjustment to this logic.

> Further is even just non-default exposure of all the various bits okay
> to other than Dom0? IOW is there indeed no further adjustment necessary
> to guest_rdmsr()?
>
>> @@ -828,7 +845,10 @@ void __init init_dom0_cpuid_policy(struct domain *d)
>>       * domain policy logic gains a better understanding of MSRs.
>>       */
>>      if ( is_hardware_domain(d) && cpu_has_arch_caps )
>> +    {
>>          p->feat.arch_caps = true;
>> +        p->arch_caps.raw = host_cpu_policy.arch_caps.raw;
>> +    }
> Doesn't this expose all the bits, irrespective of their exposure
> annotations in the public header?

No, because of ...

>  I.e. even more than just the two
> bits that become 'A' in patch 4, but weren't ...
>
>> @@ -858,20 +878,6 @@ void __init init_dom0_cpuid_policy(struct domain *d)
>>          p->platform_info.cpuid_faulting = false;
>>  
>>      recalculate_cpuid_policy(d);

... this recalculate_cpuid_policy() (which was moved in patch 1), which
applies the appropriate pv/hvm max mask over the inherited bits.

More generally, this is how *all* opting-into-non-default features needs
to work when it's more than just turning on a single feature bit.  It's
also why doing full-policy levelling in the toolstack is much harder
than it appears on paper.

All domains get the default policy, so zero out all non-default
information.  It has to be recovered from somewhere.  Generally that
would be the appropriate max policy, but the host policy here is fine
because there's nothing to do other than applying the appropriate max mask.

When arch-caps becomes default, the full block feeding arch caps back
into dom0 will be dropped, but there's still a lot of work to do first.

~Andrew