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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/25/25 10:11 AM, Roger Pau Monné wrote:

> On Thu, Sep 25, 2025 at 09:41:43AM +0200, Jan Beulich wrote:
> > On 25.09.2025 09:40, Roger Pau Monné wrote:
> > > On Thu, Sep 25, 2025 at 09:37:46AM +0200, Jan Beulich wrote:
> > > > 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.
> > > >           
> > > Oleksii, can I please get a release-ack for this to go in?
> > >         
> > Do bug fixes actually need release-acks just yet?
> >       
> I always err on the side of caution and ask for them.  Maybe Oleksii
> can state if/when he formally wants release-acks for bugfixes.
>     
I am okay not to have release-acks for bugfixes until the end of code
freeze.

When I will announce a next stages of release process,
I would put such the information explicitly.

Thanks.

~ Oleksii
> Regards, Roger.
>