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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/05/2023 3:06 pm, Jan Beulich wrote:
> 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.

Oh sorry.  Yes, it is sufficient.  Because VMs (other than dom0) don't
get the ARCH_CAPS CPUID bit, reads of MSR_ARCH_CAPS will #GP.

Right now, you can set cpuid = "host:arch-caps" in an xl.cfg file.  If
you do this, you get to keep both pieces, as you'll end up advertising
the MSR but with a value of 0 because of the note in patch 4.  libxl
still only understand the xend CPUID format and can't express any MSR
data at all.

~Andrew