Re: [PATCH 4/4] x86/spec-ctrl: Remove opencoded MSR_ARCH_CAPS check

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.05.2023 18:35, Andrew Cooper wrote:
> On 17/05/2023 3:47 pm, Jan Beulich wrote:
>> On 16.05.2023 16:53, Andrew Cooper wrote:
>>> @@ -401,6 +400,8 @@ static void __init print_details(enum ind_thunk thunk, 
>>> uint64_t caps)
>>>          cpuid_count(7, 2, &tmp, &tmp, &tmp, &_7d2);
>>>      if ( boot_cpu_data.extended_cpuid_level >= 0x80000008 )
>>>          cpuid(0x80000008, &tmp, &e8b, &tmp, &tmp);
>>> +    if ( cpu_has_arch_caps )
>>> +        rdmsrl(MSR_ARCH_CAPABILITIES, caps);
>> Why do you read the MSR again? I would have expected this to come out
>> of raw_cpu_policy now (and incrementally the CPUID pieces as well,
>> later on).
> 
> Consistency with the surrounding logic.

I view this as relevant only when the code invoking CPUID directly is
intended to stay.

> Also because the raw and host policies don't get sorted until much later
> in boot.

identify_cpu(), which invokes init_host_cpu_policies(), is called
ahead of init_speculation_mitigations(), isn't it?

>> Apart from this, with all the uses further down gone, perhaps there's
>> not even a need for the raw value, if you used the bitfields in the
>> printk(). Which in turn raises the question whether the #define-s in
>> msr-index.h are of much use then anymore.
> 
> One of the next phases of work is synthesizing these in the host policy
> for CPUs which didn't receive microcode updates (for whatever reason).
> 
> There is a valid discussion for whether we ought to render the raw or
> host info here (currently we do raw), but I'm not adjusting that in this
> patch.

In the end I think both have their merits to log. So far it was my
assumption that "Hardware {hints,features}:" was intended to cover
raw, while "Xen settings:" was meaning to be close to "host" (but of
course there's quite a bit of a delta).

Jan