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

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

[Roger Pau Monne]

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.  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@citrix.com>
---
 xen/arch/x86/cpu/common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 530b9eb39abc..35dcdf0c8801 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -490,6 +490,9 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
 	c->phys_proc_id = c->apicid;
 
+	/* Early init of Self Snoop support requires 0x1.edx. */
+	c->x86_capability[FEATURESET_1d] = edx;
+
 	eax = cpuid_eax(0x80000000);
 	if ((eax >> 16) == 0x8000)
 		c->extended_cpuid_level = eax;
-- 
2.51.0

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

[Andrew Cooper]

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

>   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@citrix.com>

... this Fixes tag is incorrect?

~Andrew

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

[Roger Pau Monné]

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@citrix.com>
> 
> ... 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.

Thanks, Roger.

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

[Jan Beulich]

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@citrix.com>
>>
>> ... 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@suse.com>

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

Jan

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

[Roger Pau Monné]

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@citrix.com>
> >>
> >> ... 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@suse.com>
> 
> 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:

	/*
	 * 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.

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

[Jan Beulich]

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@citrix.com>
>>>>
>>>> ... 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@suse.com>
>>
>> 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.

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

[Roger Pau Monné]

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@citrix.com>
> >>>>
> >>>> ... 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@suse.com>
> >>
> >> 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.

Thanks!

Oleksii, can I please get a release-ack for this to go in?

Regards, Roger.

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

[Jan Beulich]

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@citrix.com>
>>>>>>
>>>>>> ... 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@suse.com>
>>>>
>>>> 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?

Jan

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

[Roger Pau Monné]

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@citrix.com>
> >>>>>>
> >>>>>> ... 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@suse.com>
> >>>>
> >>>> 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.

Regards, Roger.

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 3543 bytes --]


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@citrix.com>
>>>>>>>> ... 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@suse.com>
>>>>>>
>>>>>> 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.

[-- Attachment #2: Type: text/html, Size: 5528 bytes --]