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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.05.2023 16:14, Andrew Cooper wrote:
> 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.

But you wouldn't alter the raw featureset, would you? As much as ...

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

... setup_force_cpu_cap() doesn't affect raw.

Jan