Re: [PATCH] x86/S3: restore MCE (APs) and add MTRR (BSP) init

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2026 12:16, Roger Pau Monné wrote:
> On Wed, Mar 04, 2026 at 02:39:01PM +0100, Jan Beulich wrote:
>> MCE init for APs was broken when CPU feature re-checking was added. MTRR
>> (re)init for the BSP looks to never have been there on the resume path.
> 
> I'm not sure the statement about MTRR init is correct, AFAICT
> mtrr_aps_sync_end() will also re-init the MTRRs on the BSP, and hence
> the added mtrr_ap_init() seems to duplicate what's already done in
> mtrr_aps_sync_end().

Hmm, right you are. Had I been asked, I would have confirmed that I checked
the code past the "enable_cpu" label, but clearly I must not have, or I was
blind at that time. Let me strip that out.

>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -642,16 +642,21 @@ void identify_cpu(struct cpuinfo_x86 *c)
>>                             smp_processor_id());
>>      }
>>  
>> -    if (system_state == SYS_STATE_resume)
>> -            return;
>> +    if (system_state == SYS_STATE_resume) {
>> +            unsigned int cpu = smp_processor_id();
>>  
>> +            if (cpu)
>> +                    mcheck_init(&cpu_data[cpu], false);
>> +            else /* Yes, the BSP needs to use the AP function here. */
>> +                    mtrr_ap_init();
> 
> For symmetry with the BSP path, is it really needed to init MCE so
> early for the BSP by calling it directly in enter_state(), or could it
> also be done here?

To be honest, I would put the question the other way around: Is it really
okay to do it this late for APs (during boot also for the BSP [1])? Iirc
an #MC prior to mcheck_init() is going to be deadly to the system. Moving
it earlier may, however, be a more intrusive change.

Jan

[1] Us crashing (rebooting) during boot is perhaps less of an issue than
us doing so during S3 resume: In that latter case it may mean data loss
(or maybe even data corruption).

Jan