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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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<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

Hmm..This is what I see in the MAINTAINERS file:
KDUMP
M:      Vivek Goyal <vgoyal@redhat.com>
M:      Haren Myneni <hbabu@us.ibm.com>
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..