Re: [PATCH 08/22] x86/traps: Introduce ap_early_traps_init() and set up exception handling earlier

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/08/2025 9:41 am, Jan Beulich wrote:
> On 08.08.2025 22:23, Andrew Cooper wrote:
>> --- a/xen/arch/x86/acpi/wakeup_prot.S
>> +++ b/xen/arch/x86/acpi/wakeup_prot.S
>> @@ -63,6 +63,9 @@ LABEL(s3_resume)
>>          pushq   %rax
>>          lretq
>>  1:
>> +        /* Set up early exceptions and CET before entering C properly. */
>> +        call    ap_early_traps_init
> But this is the BSP?

By the end of the cleanup, what we have is:

At boot only:
 * really early init, basic exception handling only
 * regular init (inc syscall trampolines)
 * late re-init as we change the stack linear address

For everything else (APs, S3, hot-online):
 * early, full exception handling
 * regular init (inc syscall trampolines)


Currently, these are named:
 * bsp_early_traps_init()
 * traps_init()
 * bsp_traps_reinit()

and
 * ap_early_traps_init()
 * percpu_traps_init()


Perhaps ap_early_traps_init() should be named
percpu_early_traps_init()?  But I'm open to suggestions.

>
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -327,12 +327,7 @@ void asmlinkage start_secondary(void)
>>      struct cpu_info *info = get_cpu_info();
>>      unsigned int cpu = smp_processor_id();
>>  
>> -    /* Critical region without IDT or TSS.  Any fault is deadly! */
>> -
>> -    set_current(idle_vcpu[cpu]);
>> -    this_cpu(curr_vcpu) = idle_vcpu[cpu];
>>      rdmsrl(MSR_EFER, this_cpu(efer));
>> -    init_shadow_spec_ctrl_state(info);
>>  
>>      /*
>>       * Just as during early bootstrap, it is convenient here to disable
>> @@ -352,14 +347,6 @@ void asmlinkage start_secondary(void)
>>       */
>>      spin_debug_disable();
>>  
>> -    get_cpu_info()->use_pv_cr3 = false;
>> -    get_cpu_info()->xen_cr3 = 0;
>> -    get_cpu_info()->pv_cr3 = 0;
>> -
>> -    load_system_tables();
>> -
>> -    /* Full exception support from here on in. */
>> -
>>      if ( cpu_has_pks )
>>          wrpkrs_and_cache(0); /* Must be before setting CR4.PKS */
>>  
>> @@ -1064,8 +1051,12 @@ static int cpu_smpboot_alloc(unsigned int cpu)
>>              goto out;
>>  
>>      info = get_cpu_info_from_stack((unsigned long)stack_base[cpu]);
>> +    memset(info, 0, sizeof(*info));
> Why do we suddenly need this? Or is this just out of an abundance of
> caution (while making the individual ->*_cr3 writes unnecessary)?

cpu_alloc_stack() explicitly uses alloc_xenheap_pages() which uses
MEMF_no_scrub.  It will usually be zeroed memory because we allocate
them all at the start of day, but it also has a habbit of being 0xc2'd
when running under Xen.

Also yes, I do dislike the ad-hoc zeroes of misc fields.

>
>> +    init_shadow_spec_ctrl_state(info);
> May I suggest to move this further down a little, at least ...
>
>>      info->processor_id = cpu;
> ... past here? Just in case other values in the struct may be needed
> in the function at some point.

Ok.

>
>>      info->per_cpu_offset = __per_cpu_offset[cpu];
>> +    info->current_vcpu = idle_vcpu[cpu];
> To be able to spot this, I think it wants /* set_current() */ or some
> such.

Ok.

>
>> +    per_cpu(curr_vcpu, cpu) = idle_vcpu[cpu];
> It's a little odd to do this early (and remotely), but it looks all fine
> with how the variable is currently used.

It did take a little while for me to conclude that it is safe, but yes -
it does relax a lot of ordering constraints for AP bringup.

~Andrew