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
