Re: [PATCH 1 of 3] apic: record local apic state on boot

[Keir Fraser <keir.xen@gmail.com>]

On 18/05/2011 19:08, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> Xen does not store the boot local apic state which leads to problems
> when shutting down for a kexec jump.  This patch records the boot
> state so we can return to the boot state when kexec'ing.
> 
> This is per CPU because all 3 bioses on the boxes I have tested dont
> enabled all local apics on boot.

You mean some CPUs have APIC totally disabled? Impossible: you need the APIC
enabled to be able to INIT-SIPI bootstrap the secondary processors. And Xen
itself has no code to enable disabled APICs (except some very legacy code
that executes only in the uniprocessor case, which I'm pretty sure you're
not touching).

You call your new function so late in secondary CPU bringup that our own
APIC setup has already run (via
smp_callin->{x2apic_ap_setup,setup_local_APIC}). Which would indicate that
the boot state of the secondary CPUs is not of importance to you, since you
I'm sure tested your patch as working okay in this current form, and that
would imply that you don't need to record per-cpu boot state.

The singleshot BUG_ON in record_boot_APIC_mode is broken, since secondary
CPUs can be started up multiple times (we can offline/online them). You need
to bail on repeated invocations. Or not be recording boot state for
secondary cpus since apparently it isn't needed...

We're going to be going through these patches very slowly and painfully,
because frankly the arguments and assertions seem to be plenty full of
holes.

 -- Keir

> As a result, we have to return to
> the bios state so the ACPI tables match up with the hardware state
> for the booting kernel.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/apic.c
> --- a/xen/arch/x86/apic.c Tue May 17 17:32:19 2011 +0100
> +++ b/xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100
> @@ -74,6 +74,8 @@ u8 __read_mostly apic_verbosity;
>  static bool_t __initdata opt_x2apic = 1;
>  boolean_param("x2apic", opt_x2apic);
>  
> +DEFINE_PER_CPU_READ_MOSTLY(enum apic_mode, apic_boot_mode) =
> APIC_MODE_INVALID;
> +
>  bool_t __read_mostly x2apic_enabled = 0;
>  bool_t __read_mostly directed_eoi_enabled = 0;
>  
> @@ -1437,6 +1439,41 @@ int __init APIC_init_uniprocessor (void)
>      return 0;
>  }
>  
> +/* Needs to be called once per CPU during startup.  It records the state the
> BIOS
> + * leaves the local APIC so we can tare back down upon shutdown/crash
> + */
> +void __init record_boot_APIC_mode(void)
> +{
> +    enum apic_mode this_apic_mode;
> +    u64 msr_contents;
> +
> +    this_apic_mode = APIC_MODE_INVALID;
> +
> +    /* Sanity check - we should only ever run once */
> +    BUG_ON( APIC_MODE_INVALID != this_cpu(apic_boot_mode) );
> +
> +    rdmsrl(MSR_IA32_APICBASE, msr_contents);
> +
> +    /* Reading EXTD bit from the MSR is only valid if CPUID says so, else
> reserved */
> +    if ( cpu_has(&current_cpu_data, X86_FEATURE_X2APIC)
> +         && (msr_contents & MSR_IA32_APICBASE_EXTD) )
> +        this_apic_mode = APIC_MODE_X2APIC;
> +    else
> +        {
> +            /* EN bit should always be valid as long as we can read the MSR
> +             * Can anyone confirm this?
> +             */
> +            if ( msr_contents & MSR_IA32_APICBASE_ENABLE )
> +                this_apic_mode = APIC_MODE_XAPIC;
> +            else
> +                this_apic_mode = APIC_MODE_DISABLED;
> +        }
> +
> +    this_cpu(apic_boot_mode) = this_apic_mode;
> +    apic_printk(APIC_DEBUG, "APIC boot state is %d on core #%d\n",
> +                this_apic_mode, smp_processor_id());
> +}
> +
>  void check_for_unexpected_msi(unsigned int vector)
>  {
>      unsigned long v = apic_read(APIC_ISR + ((vector & ~0x1f) >> 1));
> diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/genapic/probe.c
> --- a/xen/arch/x86/genapic/probe.c Tue May 17 17:32:19 2011 +0100
> +++ b/xen/arch/x86/genapic/probe.c Wed May 18 19:00:13 2011 +0100
> @@ -60,6 +60,8 @@ void __init generic_apic_probe(void)
>  { 
> int i, changed;
>  
> +    record_boot_APIC_mode();
> +
> check_x2apic_preenabled();
> cmdline_apic = changed = (genapic != NULL);
>  
> diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c Tue May 17 17:32:19 2011 +0100
> +++ b/xen/arch/x86/smpboot.c Wed May 18 19:00:13 2011 +0100
> @@ -388,6 +388,9 @@ void start_secondary(void *unused)
>  
>      microcode_resume_cpu(cpu);
>  
> +    /* Record boot apic state */
> +    record_boot_APIC_mode();
> +
>      wmb();
>      startup_cpu_idle_loop();
>  }
> diff -r f531ed84b066 -r 62a8ce6595ad xen/include/asm-x86/apic.h
> --- a/xen/include/asm-x86/apic.h Tue May 17 17:32:19 2011 +0100
> +++ b/xen/include/asm-x86/apic.h Wed May 18 19:00:13 2011 +0100
> @@ -21,6 +21,16 @@
>  #define IO_APIC_REDIR_DEST_LOGICAL 0x00800
>  #define IO_APIC_REDIR_DEST_PHYSICAL 0x00000
>  
> +/* Possible APIC states */
> +enum apic_mode { APIC_MODE_INVALID,  /* Not set yet */
> +                 APIC_MODE_DISABLED, /* Some bioses disable by default for
> compatability */
> +                 APIC_MODE_XAPIC,    /* xAPIC mode - default upon chipset
> reset */
> +                 APIC_MODE_X2APIC    /* x2APIC mode - common for large smp
> machines */
> +};
> +
> +/* PerCPU local APIC boot mode - so we can taredown to bios state */
> +DECLARE_PER_CPU(enum apic_mode, apic_boot_mode);
> +
>  extern u8 apic_verbosity;
>  extern bool_t x2apic_enabled;
>  extern bool_t directed_eoi_enabled;
> @@ -203,6 +213,7 @@ extern void disable_APIC_timer(void);
>  extern void enable_APIC_timer(void);
>  extern int lapic_suspend(void);
>  extern int lapic_resume(void);
> +extern void record_boot_APIC_mode(void);
>  
>  extern int check_nmi_watchdog (void);
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel