Re: [PATCH for 4.19?] x86/Intel: unlock CPUID earlier for the BSP

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/06/2024 12:12 pm, Jan Beulich wrote:
> On 14.06.2024 12:14, Andrew Cooper wrote:
>> On 14/06/2024 7:27 am, Jan Beulich wrote:
>>> On 13.06.2024 18:17, Andrew Cooper wrote:
>>>> On 13/06/2024 9:19 am, Jan Beulich wrote:
>>>>> Intel CPUs have a MSR bit to limit CPUID enumeration to leaf two. If
>>>>> this bit is set by the BIOS then CPUID evaluation does not work when
>>>>> data from any leaf greater than two is needed; early_cpu_init() in
>>>>> particular wants to collect leaf 7 data.
>>>>>
>>>>> Cure this by unlocking CPUID right before evaluating anything which
>>>>> depends on the maximum CPUID leaf being greater than two.
>>>>>
>>>>> Inspired by (and description cloned from) Linux commit 0c2f6d04619e
>>>>> ("x86/topology/intel: Unlock CPUID before evaluating anything").
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> ---
>>>>> While I couldn't spot anything, it kind of feels as if I'm overlooking
>>>>> further places where we might be inspecting in particular leaf 7 yet
>>>>> earlier.
>>>>>
>>>>> No Fixes: tag(s), as imo it would be too many that would want
>>>>> enumerating.
>>>> I also saw that go by, but concluded that Xen doesn't need it, hence why
>>>> I left it alone.
>>>>
>>>> The truth is that only the BSP needs it.  APs sort it out in the
>>>> trampoline via trampoline_misc_enable_off, because they need to clear
>>>> XD_DISABLE in prior to enabling paging, so we should be taking it out of
>>>> early_init_intel().
>>> Except for the (odd) case also mentioned to Roger, where the BSP might have
>>> the bit clear but some (or all) AP(s) have it set.
>> Fine I suppose.  It's a single MSR adjustment once per CPU.
>>
>>>> But, we don't have an early BSP-only early hook, and I'm not overwhelmed
>>>> at the idea of exporting it from intel.c
>>>>
>>>> I was intending to leave it alone until I can burn this whole
>>>> infrastructure to the ground and make it work nicely with policies, but
>>>> that's not a job for this point in the release...
>>> This last part reads like the rest of your reply isn't an objection to me
>>> putting this in with Roger's R-b, but it would be nice if you could
>>> confirm this understanding of mine. Without this last part it, especially
>>> the 2nd from last paragraph, certainly reads a little like an objection.
>> I'm -1 to this generally.  It's churn without fixing anything AFAICT,
> How that? We clearly do the adjustment too late right now for the BSP.
> All the leaf-7 stuff added to early_cpu_init() (iirc in part in the course
> of speculation work) is useless on a system where firmware set that flag.

After discussing this at the x86 maintainers meeting, I agree that it is
fixing a potential bug.

So, while I really dislike the patch, it's the right approach in the
short term, and should go in.

~Andrew