Re: [PATCH for-4.21] x86/cpu: populate CPUID 0x1.edx features early for self-snoop detection

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2025 09:34, Roger Pau Monné wrote:
> On Thu, Sep 25, 2025 at 09:03:06AM +0200, Jan Beulich wrote:
>> On 24.09.2025 15:40, Roger Pau Monné wrote:
>>> On Wed, Sep 24, 2025 at 11:50:02AM +0000, Andrew Cooper wrote:
>>>> On 24/09/2025 4:00 am, Roger Pau Monne wrote:
>>>>> Otherwise the check for the SS feature in
>>>>> check_memory_type_self_snoop_errata() fails unconditionally, which leads 
>>>>> to
>>>>> X86_FEATURE_XEN_SELFSNOOP never being set.
>>>>>
>>>>> We could also avoid this by not doing the reset_cpuinfo() for the BSP in
>>>>> identify_cpu(), because SS detection uses boot_cpu_data.
>>>>
>>>> Doesn't this, mean ...
>>>
>>> Well, that's the reason for the rant here.  The reset at the top of
>>> identify_cpu() has been there since 2005.  It's arguably to make sure
>>> the BSP and the APs have the same empty state in the passed
>>> cpuinfo_x86 struct, as for the BSP this would be already partially
>>> initialized due to what's done in early_cpu_init().
>>>
>>> The underlying question is whether we would rather prefer to not do
>>> the reset for the BSP, but that would lead to differences in the
>>> contents of cpuinfo_x86 struct between the BSP and the APs.  In the
>>> past we have arranged for leaves needed early to be populated in
>>> generic_identify(), like FEATURESET_e21a, hence the proposed patch
>>> does that for FEATURESET_1d.
>>>
>>>>>   However that
>>>>> creates an imbalance on the state of the BSP versus the APs in the
>>>>> identify_cpu() code.
>>>>>
>>>>> I've opted for the less controversial solution of populating FEATURESET_1d
>>>>> in generic_identify(), as the value is already there.  The same is done 
>>>>> for
>>>>> the AMD faulting probe code.
>>>>>
>>>>> Fixes: f2663ca2e520 ("x86/cpu/intel: Clear cache self-snoop capability in 
>>>>> CPUs with known errata")
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>
>>>> ... this Fixes tag is incorrect?
>>>
>>> I think the Fixes tag is accurate; the code was OK before that change.
>>> Nothing in c_early_init hooks depended on (some of) the x86_capability
>>> fields being populated, which is required after the change.
>>
>> I agree. Hence:
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> I wonder though whether while there we wouldn't want to also store ecx if
>> we already have it. (Really there is the question of whether we haven't
>> other cpu_has_* uses which similarly come "too early".)
> 
> Yeah, I was about to do it, but it's not strictly needed for
> c_early_init, and it's done anyway just after the call to
> c_early_init.  I can set that field also, but then I would need to
> tweak the comment ahead, something like:

Sure, i.e. fine with me.

Jan

>       /*
>        * Early init of Self Snoop support requires 0x1.edx, while
>        * there also set 0x1.ecx as the value is already in context.
>        */
>       c->x86_capability[FEATURESET_1d] = edx;
>       c->x86_capability[FEATURESET_1c] = ecx;
> 
> Thanks, Roger.