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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/05/11 19:49, Konrad Rzeszutek Wilk wrote:
> On Wed, May 18, 2011 at 07:08:14PM +0100, Andrew Cooper 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.  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.
> Which ACPI table requires this?
Cant remember offhand but linux (2.6.32) was doing finger pointing at 
the multi-processor tables
>> 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
> tare?
I fail at spelling - I meant "tear down"
>> + */
>> +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?
> Email Vivek Goyal. He is the kexec/kdump maintainer.
He doesn't appear in the maintainers file, and I don't have an email 
address.
>> +             */
>> +            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());
> This begs of a function to convert those enums to strings..
True - this was left over from my debugging - I shall fix that tomorrow.
>> +}
>> +
>>   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();
>> +
> The spacing looks odd.
Does it?  Looks fine for me - I have been following the 1 tab to 4 
spaces convention in half the codebase.
>>   	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 */
> taredown?
same comment as above re. spelling
>> +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

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com