Re: [PATCH 2/4] x86/MCE: restrict allocation of thermal and CMCI vector to BSP

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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