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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2023 16:38, Andrew Cooper wrote:
> On 19/05/2023 7:00 am, Jan Beulich wrote:
>> 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.
> 
> Quite the contrary.  It stays because this patch, and changing the
> semantics of the print block are unrelated things and should not be
> mixed together.

Hmm. On one hand I can see your point, yet otoh we move things in a longer
term intended direction in other cases where we need to touch code anyway.
While I'm not going to refuse to ack this change just because of this, I
don't fell like you've answered the original question. In particular I
don't see how taking the value from a memory location we've already cached
it in is changing any semantics here. While some masking may apply even to
the raw policy (to zap unknown bits), this should be meaningless here. No
bit used here should be unmentioned in the policy.

>>> 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?
> 
> What is init_host_cpu_policies() ?

Oops. I did use my own tree as reference during review. See the long
pending "x86/xstate: drop xstate_offsets[] and xstate_sizes[]" [1]. Maybe
you simply didn't tell me yet ...

> I have a plan for what it's going to be if prior MSR work hadn't ground
> to a halt, but it's a bit too late for that now.
> 
> (To answer the question properly, no the policies aren't set up until
> just before building dom0, and that's not something that is trivial to
> change.)

... that what I'm doing there is too simplistic?

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2021-04/msg01335.html