There's no need to do this for every AP; just do it once when setting up
the BSP. Then both vector variables can also validly become ro-after-init.
While touching intel_init_thermal(), constify its 1st parameter, which
in turn requires touching intel_thermal_supported() as well.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This contextually (but not functionally) collides with "x86/MCE: adjust S3
resume handling".
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -37,7 +37,7 @@ bool is_mc_panic;
DEFINE_PER_CPU_READ_MOSTLY(unsigned int, nr_mce_banks);
unsigned int __read_mostly firstbank;
unsigned int __read_mostly ppin_msr;
-uint8_t __read_mostly cmci_apic_vector;
+uint8_t __ro_after_init cmci_apic_vector;
bool __read_mostly cmci_support;
/* If mce_force_broadcast == 1, lmce_support will be disabled forcibly. */
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -87,7 +87,7 @@ static void cf_check intel_thermal_inter
}
/* Thermal monitoring depends on APIC, ACPI and clock modulation */
-static bool intel_thermal_supported(struct cpuinfo_x86 *c)
+static bool intel_thermal_supported(const struct cpuinfo_x86 *c)
{
if ( !cpu_has_apic )
return false;
@@ -110,13 +110,13 @@ static void __init mcheck_intel_therm_in
}
/* P4/Xeon Thermal regulation detect and init */
-static void intel_init_thermal(struct cpuinfo_x86 *c)
+static void intel_init_thermal(const struct cpuinfo_x86 *c, bool bsp)
{
uint64_t msr_content;
uint32_t val;
int tm2 = 0;
unsigned int cpu = smp_processor_id();
- static uint8_t thermal_apic_vector;
+ static uint8_t __ro_after_init thermal_apic_vector;
if ( !intel_thermal_supported(c) )
return; /* -ENODEV */
@@ -160,7 +160,8 @@ static void intel_init_thermal(struct cp
return; /* -EBUSY */
}
- alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
+ if ( bsp )
+ alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
/* The temperature transition interrupt handler setup */
val = thermal_apic_vector; /* our delivery vector */
@@ -667,7 +668,7 @@ static void cf_check cmci_interrupt(void
mctelem_dismiss(mctc);
}
-static void intel_init_cmci(struct cpuinfo_x86 *c)
+static void intel_init_cmci(struct cpuinfo_x86 *c, bool bsp)
{
u32 l, apic;
int cpu = smp_processor_id();
@@ -687,7 +688,8 @@ static void intel_init_cmci(struct cpuin
return;
}
- alloc_direct_apic_vector(&cmci_apic_vector, cmci_interrupt);
+ if ( bsp )
+ alloc_direct_apic_vector(&cmci_apic_vector, cmci_interrupt);
apic = cmci_apic_vector;
apic |= (APIC_DM_FIXED | APIC_LVT_MASKED);
@@ -993,9 +995,9 @@ enum mcheck_type intel_mcheck_init(struc
intel_init_mce(bsp);
- intel_init_cmci(c);
+ intel_init_cmci(c, bsp);
- intel_init_thermal(c);
+ intel_init_thermal(c, bsp);
intel_init_ppin(c);On 19/11/2025 10:50 am, Jan Beulich wrote:
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -110,13 +110,13 @@ static void __init mcheck_intel_therm_in
> }
>
> /* P4/Xeon Thermal regulation detect and init */
> -static void intel_init_thermal(struct cpuinfo_x86 *c)
> +static void intel_init_thermal(const struct cpuinfo_x86 *c, bool bsp)
> {
> uint64_t msr_content;
> uint32_t val;
> int tm2 = 0;
> unsigned int cpu = smp_processor_id();
> - static uint8_t thermal_apic_vector;
> + static uint8_t __ro_after_init thermal_apic_vector;
>
> if ( !intel_thermal_supported(c) )
> return; /* -ENODEV */
> @@ -160,7 +160,8 @@ static void intel_init_thermal(struct cp
> return; /* -EBUSY */
> }
>
> - alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
> + if ( bsp )
> + alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
We really don't want both c and bsp passed in. That can only go wrong.
Furthermore, this function has 2 other examples generating bsp locally.
The function is in desperate need of cleanup (MSRs, variable and
constant names), but right now this makes it worse.
Please either use c == &boot_cpu_data, and I'll do some cleanup later,
or generate bsp = c == &boot_cpu_data and fix up all users in the function.
~Andrew
On 20.11.2025 12:51, Andrew Cooper wrote:
> On 19/11/2025 10:50 am, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/mcheck/mce.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>> @@ -110,13 +110,13 @@ static void __init mcheck_intel_therm_in
>> }
>>
>> /* P4/Xeon Thermal regulation detect and init */
>> -static void intel_init_thermal(struct cpuinfo_x86 *c)
>> +static void intel_init_thermal(const struct cpuinfo_x86 *c, bool bsp)
>> {
>> uint64_t msr_content;
>> uint32_t val;
>> int tm2 = 0;
>> unsigned int cpu = smp_processor_id();
>> - static uint8_t thermal_apic_vector;
>> + static uint8_t __ro_after_init thermal_apic_vector;
>>
>> if ( !intel_thermal_supported(c) )
>> return; /* -ENODEV */
>> @@ -160,7 +160,8 @@ static void intel_init_thermal(struct cp
>> return; /* -EBUSY */
>> }
>>
>> - alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
>> + if ( bsp )
>> + alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
>
> We really don't want both c and bsp passed in. That can only go wrong.
>
> Furthermore, this function has 2 other examples generating bsp locally.
>
> The function is in desperate need of cleanup (MSRs, variable and
> constant names), but right now this makes it worse.
>
> Please either use c == &boot_cpu_data, and I'll do some cleanup later,
> or generate bsp = c == &boot_cpu_data and fix up all users in the function.
No, throughout mce/ this won't work as long as acpi/power.c:enter_state() has
mcheck_init(&boot_cpu_data, false);
Jan
On 20/11/2025 12:11 pm, Jan Beulich wrote:
> On 20.11.2025 12:51, Andrew Cooper wrote:
>> On 19/11/2025 10:50 am, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpu/mcheck/mce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>>> @@ -110,13 +110,13 @@ static void __init mcheck_intel_therm_in
>>> }
>>>
>>> /* P4/Xeon Thermal regulation detect and init */
>>> -static void intel_init_thermal(struct cpuinfo_x86 *c)
>>> +static void intel_init_thermal(const struct cpuinfo_x86 *c, bool bsp)
>>> {
>>> uint64_t msr_content;
>>> uint32_t val;
>>> int tm2 = 0;
>>> unsigned int cpu = smp_processor_id();
>>> - static uint8_t thermal_apic_vector;
>>> + static uint8_t __ro_after_init thermal_apic_vector;
>>>
>>> if ( !intel_thermal_supported(c) )
>>> return; /* -ENODEV */
>>> @@ -160,7 +160,8 @@ static void intel_init_thermal(struct cp
>>> return; /* -EBUSY */
>>> }
>>>
>>> - alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
>>> + if ( bsp )
>>> + alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
>> We really don't want both c and bsp passed in. That can only go wrong.
>>
>> Furthermore, this function has 2 other examples generating bsp locally.
>>
>> The function is in desperate need of cleanup (MSRs, variable and
>> constant names), but right now this makes it worse.
>>
>> Please either use c == &boot_cpu_data, and I'll do some cleanup later,
>> or generate bsp = c == &boot_cpu_data and fix up all users in the function.
> No, throughout mce/ this won't work as long as acpi/power.c:enter_state() has
>
> mcheck_init(&boot_cpu_data, false);
How's not not already broken then? As said, intel_init_thermal() is
already using c == &boot_cpu_data.
This patch introduces a conflicting idea of bsp in this function, and
that's what I really want to avoid.
This looks like it wants splitting properly as {bsp,percpu}_init_$FOO()
like we have elsewhere.
~Andrew
On 20.11.2025 18:25, Andrew Cooper wrote:
> On 20/11/2025 12:11 pm, Jan Beulich wrote:
>> On 20.11.2025 12:51, Andrew Cooper wrote:
>>> On 19/11/2025 10:50 am, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/cpu/mcheck/mce.c
>>>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>>>> @@ -110,13 +110,13 @@ static void __init mcheck_intel_therm_in
>>>> }
>>>>
>>>> /* P4/Xeon Thermal regulation detect and init */
>>>> -static void intel_init_thermal(struct cpuinfo_x86 *c)
>>>> +static void intel_init_thermal(const struct cpuinfo_x86 *c, bool bsp)
>>>> {
>>>> uint64_t msr_content;
>>>> uint32_t val;
>>>> int tm2 = 0;
>>>> unsigned int cpu = smp_processor_id();
>>>> - static uint8_t thermal_apic_vector;
>>>> + static uint8_t __ro_after_init thermal_apic_vector;
>>>>
>>>> if ( !intel_thermal_supported(c) )
>>>> return; /* -ENODEV */
>>>> @@ -160,7 +160,8 @@ static void intel_init_thermal(struct cp
>>>> return; /* -EBUSY */
>>>> }
>>>>
>>>> - alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
>>>> + if ( bsp )
>>>> + alloc_direct_apic_vector(&thermal_apic_vector, intel_thermal_interrupt);
>>> We really don't want both c and bsp passed in. That can only go wrong.
>>>
>>> Furthermore, this function has 2 other examples generating bsp locally.
>>>
>>> The function is in desperate need of cleanup (MSRs, variable and
>>> constant names), but right now this makes it worse.
>>>
>>> Please either use c == &boot_cpu_data, and I'll do some cleanup later,
>>> or generate bsp = c == &boot_cpu_data and fix up all users in the function.
>> No, throughout mce/ this won't work as long as acpi/power.c:enter_state() has
>>
>> mcheck_init(&boot_cpu_data, false);
>
> How's not not already broken then? As said, intel_init_thermal() is
> already using c == &boot_cpu_data.
That's two printk()s, so not overly severe a bug. And being fixed by "x86/MCE:
adjust S3 resume handling" posted months ago. There I'm actually putting up
the question whether one of the two could/should stay as is.
> This patch introduces a conflicting idea of bsp in this function, and
> that's what I really want to avoid.
>
> This looks like it wants splitting properly as {bsp,percpu}_init_$FOO()
> like we have elsewhere.
Without detailed checking, I'm not sure that would properly cover things.
Right now we have three modes: BSP (boot), BSP (resume), and AP.
More importantly, though: This would be more than enough content for another
series, i.e. shouldn't block the work here (which really moves things to a
more consistent state, within cpu/mcheck/, just like that other patch also
aims at doing).
Jan
© 2016 - 2025 Red Hat, Inc.