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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/05/2023 8:10 am, Jan Beulich wrote:
> 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.

The very next thing I'm going to need to do is start synthesizing arch
caps bits for the hardware with known properties but without appropriate
enumerations.  This is necessary to make migration work.

Because we have not taken a decision about the what printed block means,
it needs to not change when I start using setup_force_cpu_cap().

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

Raw is fine.  I found complexities with Host when doing that.

~Andrew