Re: [PATCH 1/3] x86/amd: Use setup_force_cpu_cap() for BTC_NO

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2025 17:33, Andrew Cooper wrote:
> On 26/11/2025 3:25 pm, Jan Beulich wrote:
>> On 26.11.2025 16:12, Andrew Cooper wrote:
>>> On 26/11/2025 2:19 pm, Jan Beulich wrote:
>>>> On 26.11.2025 14:22, Andrew Cooper wrote:
>>>>> When re-scanning features,
>>>> What exactly do you mean with this, outside of XenServer (i.e. upstream)? 
>>>> The
>>>> only thing I can think of is recheck_cpu_features(), which calls 
>>>> identify_cpu()
>>>> and hence init_amd(). Thus ...
>>>>
>>>>> forced caps are taken into account but unforced
>>>>> such as this are not.  This causes BTC_NO to go missing, and for the 
>>>>> system to
>>>>> appear to have lost features.
>>>> ... I don't really follow where features might be lost.
>>> Well - it's a feature that we started upstreaming and I still hope to
>>> finish in some copious free time.
>>>
>>> Already upstream, we rescan the Raw CPU policy after microcode load. 
>>> That has had fixes such as dis-engaging CPUID Masking/Overriding so the
>>> Raw policy comes out accurate.
>> Yet that doesn't take forced features into account afaics. So at the very
>> least this needs to come with a description which more accurately describes
>> what (if anything) is actually being fixed / altered upstream.
> 
> I don't know what more you want me to say.  It's not a problem per say
> in upstream, but it does come about because BTC_NO is handled
> inconsistently to the other FOO_NO bits.

First, it still is unclear to me how "When re-scanning features, forced caps
are taken into account" applies to upstream. As a a result, "This causes
BTC_NO to go missing" is unclear as well. Both aspects need to be clear from
the description alone.

Further, while indeed BTC_NO is the only *_NO one set by means of __set_bit(),
an (apparently) similar issue exists for SRSO_US_NO, in it being cleared by
__clear_bit(). How that goes together with the feature elsewhere being forced
on I can't immediately tell.

> recheck_cpu_features() papers over the issue by re-invoking
> identify_cpu().

This isn't papering over anything. We do the normal identification, to then
compare its results with the ones obtained during boot.

>  It's necessary for S3 resume because all of
> init_$VENDOR() really is needed, but it looks bogus in
> smp_store_cpu_info() because it's repeating work done immediately prior
> in start_secondary().

What is it that is done there "immediately prior"? I see smp_callin()
calling smp_store_cpu_info() alone, which in turn calls either
identify_cpu() or recheck_cpu_features(). Nothing being repeated afaics.
There is an earlier call to cpu_init(), but that doesn't collect any
feature info. (Perhaps we have a latent ordering problem there, as in
principle feature dependent setup would better come after determining what
features are available. The only feature dependent operations there look to
be PKRU clearing and skinit_enable_intr(), so hopefully not an active issue;
certainly not as long as the system is symmetric.)

Jan