From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1 of 3] apic: record local apic state on boot Date: Wed, 18 May 2011 21:27:56 +0100 Message-ID: <4DD42BCC.6020106@citrix.com> References: <62a8ce6595ad940a76db.1305742094@andrewcoop> <20110518184950.GC14013@dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20110518184950.GC14013@dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Konrad Rzeszutek Wilk Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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 >> >> 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(¤t_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