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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/05/2023 8:58 am, Jan Beulich wrote:
> 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@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> 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).

I'd really prefer to leave it as-is.

You're likely right, but this entire block is deleted in patch 6 and it
has been a giant pain already making this series bisectable WRT all our
special cases.

For the sake of a few patches, it's safer to go with it in the
pre-existing form.

~Andrew