Re: [XEN PATCH 5/9] x86/smp: call x2apic_ap_setup() earlier

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2023 18:50, Krystian Hebel wrote:
> It used to be called from smp_callin(), however BUG_ON() was invoked on
> multiple occasions before that. It may end up calling machine_restart()
> which tries to get APIC ID for CPU running this code. If BSP detected
> that x2APIC is enabled, get_apic_id() will try to use it for all CPUs.
> Enabling x2APIC on secondary CPUs earlier protects against an endless
> loop of #GP exceptions caused by attempts to read IA32_X2APIC_APICID
> MSR while x2APIC is disabled in IA32_APIC_BASE.
> 
> Signed-off-by: Krystian Hebel <krystian.hebel@xxxxxxxxx>
> ---
>  xen/arch/x86/smpboot.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 8ae65ab1769f..a3895dafa267 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -184,7 +184,6 @@ static void smp_callin(void)
>       * update until we finish. We are free to set up this CPU: first the 
> APIC.
>       */
>      Dprintk("CALLIN, before setup_local_APIC().\n");
> -    x2apic_ap_setup();
>      setup_local_APIC(false);
>  
>      /* Save our processor parameters. */
> @@ -351,6 +350,14 @@ void start_secondary(void *unused)
>      get_cpu_info()->xen_cr3 = 0;
>      get_cpu_info()->pv_cr3 = 0;
>  
> +    /*
> +     * BUG_ON() used in load_system_tables() and later code may end up 
> calling
> +     * machine_restart() which tries to get APIC ID for CPU running this 
> code.
> +     * If BSP detected that x2APIC is enabled, get_apic_id() will try to use 
> it
> +     * for _all_ CPUs. Enable x2APIC on secondary CPUs now so we won't end up
> +     * with endless #GP loop.
> +     */
> +    x2apic_ap_setup();
>      load_system_tables();

While I find the argument convincing, I seem to recall that there was a
firm plan to have load_system_tables() as early as possible. Andrew?

Jan