Re: [Xen-devel] [PATCH 4 of 7] APIC: record local APIC state on boot

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 13.06.11 at 19:02, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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 kexecing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

With a couple of minor adjustments (see inline comments),

Acked-by: Jan Beulich <jbeulich@xxxxxxxxxx>

> diff -r 68efd418b6f1 -r 615db56671fa xen/arch/x86/apic.c
> --- a/xen/arch/x86/apic.c     Mon Jun 13 17:45:43 2011 +0100
> +++ b/xen/arch/x86/apic.c     Mon Jun 13 17:45:43 2011 +0100
> @@ -74,6 +74,8 @@ u8 __read_mostly apic_verbosity;
>  static bool_t __initdata opt_x2apic = 1;
>  boolean_param("x2apic", opt_x2apic);
>  
> +enum apic_mode apic_boot_mode = APIC_MODE_INVALID;

static ... __read_mostly ...

> +
>  bool_t __read_mostly x2apic_enabled = 0;
>  bool_t __read_mostly directed_eoi_enabled = 0;
>  
> @@ -1438,6 +1440,62 @@ int __init APIC_init_uniprocessor (void)
>      return 0;
>  }
>  
> +/* Needs to be called during startup.  It records the state the BIOS
> + * leaves the local APIC so we can undo upon kexec.
> + */
> +void __init record_boot_APIC_mode(void)
> +{
> +    /* Sanity check - we should only ever run once, but could possibly
> +     * be called several times */
> +    if ( APIC_MODE_INVALID != apic_boot_mode )
> +        return;
> +
> +    apic_boot_mode = current_local_apic_mode();
> +
> +    apic_printk(APIC_DEBUG, "APIC boot state is '%s'\n",
> +                apic_mode_to_str(apic_boot_mode));
> +}
> +
> +/* Look at the bits in MSR_IA32_APICBASE and work out which
> + * APIC mode we are in */
> +enum apic_mode current_local_apic_mode(void)

Logically (within this patch) this function should also be static, but
I see it is being referenced by a later patch. Read on there.

> +{
> +    u64 msr_contents;
> +
> +    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) )
> +        return APIC_MODE_X2APIC;
> +
> +    /* EN bit should always be valid as long as we can read the MSR
> +     */
> +    if ( msr_contents & MSR_IA32_APICBASE_ENABLE )
> +        return APIC_MODE_XAPIC;
> +
> +    return APIC_MODE_DISABLED;
> +}
> +
> +
> +const char * apic_mode_to_str(const enum apic_mode mode)

static ... __init ...

Jan

> +{
> +    switch ( mode )
> +    {
> +        case APIC_MODE_INVALID:
> +            return "invalid";
> +        case APIC_MODE_DISABLED:
> +            return "disabled";
> +        case APIC_MODE_XAPIC:
> +            return "xapic";
> +        case APIC_MODE_X2APIC:
> +            return "x2apic";
> +        default:
> +            return "unrecognised";
> +    }
> +}
> +
>  void check_for_unexpected_msi(unsigned int vector)
>  {
>      unsigned long v = apic_read(APIC_ISR + ((vector & ~0x1f) >> 1));
> diff -r 68efd418b6f1 -r 615db56671fa xen/arch/x86/genapic/probe.c
> --- a/xen/arch/x86/genapic/probe.c    Mon Jun 13 17:45:43 2011 +0100
> +++ b/xen/arch/x86/genapic/probe.c    Mon Jun 13 17:45:43 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 68efd418b6f1 -r 615db56671fa xen/include/asm-x86/apic.h
> --- a/xen/include/asm-x86/apic.h      Mon Jun 13 17:45:43 2011 +0100
> +++ b/xen/include/asm-x86/apic.h      Mon Jun 13 17:45:43 2011 +0100
> @@ -21,6 +21,23 @@
>  #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, /* If uniprocessor, or smp in
> +                                      * uniprocessor mode */
> +                 APIC_MODE_XAPIC,    /* xAPIC mode - default upon
> +                                      * chipset reset */
> +                 APIC_MODE_X2APIC    /* x2APIC mode - common for large
> +                                      * smp machines */
> +};
> +
> +/* Bootstrap processor local APIC boot mode - so we can undo our changes
> + * to the APIC state */
> +extern enum apic_mode apic_boot_mode;
> +
> +/* enum apic_mode -> str function for logging/debug */
> +const char * apic_mode_to_str(const enum apic_mode);
> +
>  extern u8 apic_verbosity;
>  extern bool_t x2apic_enabled;
>  extern bool_t directed_eoi_enabled;
> @@ -203,6 +220,8 @@ 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 enum apic_mode current_local_apic_mode(void);
>  
>  extern int check_nmi_watchdog (void);
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx 
> http://lists.xensource.com/xen-devel 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel