From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 1 of 3] apic: record local apic state on boot Date: Wed, 18 May 2011 16:43:09 -0400 Message-ID: <20110518204309.GA7647@dumpdata.com> References: <62a8ce6595ad940a76db.1305742094@andrewcoop> <20110518184950.GC14013@dumpdata.com> <4DD42BCC.6020106@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <4DD42BCC.6020106@citrix.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: Andrew Cooper Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On Wed, May 18, 2011 at 09:27:56PM +0100, Andrew Cooper wrote: > > > 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 MP tables? Those don't get used when ACPI is used.. > >>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 Hmm..This is what I see in the MAINTAINERS file: KDUMP M: Vivek Goyal M: Haren Myneni L: kexec@lists.infradead.org W: http://lse.sourceforge.net/kdump/ S: Maintained F: Documentation/kdump/ > address. http://lmgtfy.com/?q=Vivek+Goyal+kexec > >>+ */ > >>+ 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. Ok. > >>+} > >>+ > >> 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. Could be my mailer..