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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2023 15:51, Andrew Cooper wrote:
> 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.

Hmm, I see. Not very intuitive, but I think I follow.

>> 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()?

With your reply further down also sufficiently clarifying things for
me (in particular pointing the one oversight of mine), the question
above is the sole part remaining before I'd be okay giving my R-b here.

Jan

>>> @@ -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