Fix kexec path in xen (take 2)

This set of patches is the minimal subset required to get the kexec path working again on Xen 4.x

kdump kernels can't boot if x2apic mode is enabled and the ACPI tables dont state this fact. They also cant boot at all with interrupt remapping enabled.

These patches cause xen to track the local apic boot state and return to it before kexec'ing to a new kernel. It also makes sure to disable IO virtualisation.

Signed-off-by Andrew Cooper <[email protected]>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

kdump kernels are unable to boot with IOMMU enabled, this patch disabled IOMMU mode and removes some of the generic code from the shutdown path which doesnt work after other CPUs have been shot down.

Also, leave local interrupts disabled when jumping into pugatory

as we have no idea whats in there and really dont want to be servicing interrupts when our entire state is invalid.

Signed-off-by Andrew Cooper <[email protected]>

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/crash.c

xen/arch/x86/crash.c +2 -0

@@ -27,6 +27,8 @@
 #include <asm/hvm/support.h>
 #include <asm/apic.h>
 #include <asm/io_apic.h>
+#include <xen/iommu.h>
+#include <asm/hvm/iommu.h>
 
 static atomic_t waiting_for_crash_ipi;
 static unsigned int crashing_cpu;

xen/arch/x86/crash.c +8 -1

     atomic_dec(&waiting_for_crash_ipi);
 

@@ -56,6 +61,7 @@ static int crash_nmi_callback(struct cpu
 static void nmi_shootdown_cpus(void)
 {
     unsigned long msecs;
+    u64 msr_contents;
 
     local_irq_disable();
 

@@ -77,18 +83,43 @@ static void nmi_shootdown_cpus(void)
         msecs--;
     }
 
-    __stop_this_cpu();
+    disable_local_APIC();
+    hvm_cpu_down();
+    clts();
+    asm volatile ( "fninit" );
+
+    /* This is a bit of a hack but there is no other way to shutdown correctly
+     * without a significant refactoring of the APIC code */

xen/arch/x86/crash.c +0 -0

 }
 
 void machine_crash_shutdown(void)
 {
     crash_xen_info_t *info;

xen/arch/x86/crash.c +2 -0

+
+
     info = kexec_crash_save_info();
     info->xen_phys_start = xen_phys_start;
     info->dom0_pfn_to_mfn_frame_list_list =

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/hpet.c

xen/arch/x86/hpet.c +27 -0

@@ -670,6 +670,33 @@ void hpet_disable_legacy_broadcast(void)
     smp_send_event_check_mask(&cpu_online_map);
 }
 
+/* This function is similar to the regular
+ * hpet_disable_legacy_broadcast function, except it is called
+ * on the crash path with only the current processor up, so we
+ * can forget the locks and really cant send an event check IPI
+ * to the other processors */
+void crash_hpet_disable_legacy_broadcast(void)
+{
+    u32 cfg;
+
+    if ( !hpet_events || !(hpet_events->flags & HPET_EVT_LEGACY) )
+        return;
+
+    hpet_events->flags |= HPET_EVT_DISABLE;
+
+    /* disable HPET T0 */
+    cfg = hpet_read32(HPET_Tn_CFG(0));
+    cfg &= ~HPET_TN_ENABLE;
+    hpet_write32(cfg, HPET_Tn_CFG(0));
+
+    /* Stop HPET legacy interrupts */
+    cfg = hpet_read32(HPET_CFG);
+    cfg &= ~HPET_CFG_LEGACY;
+    hpet_write32(cfg, HPET_CFG);
+
+}
+
+
 void hpet_broadcast_enter(void)
 {
     unsigned int cpu = smp_processor_id();

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/machine_kexec.c

xen/arch/x86/machine_kexec.c +1 -1

@@ -97,7 +97,7 @@ void machine_kexec(xen_kexec_image_t *im
     };
 
     if ( hpet_broadcast_is_available() )
-        hpet_disable_legacy_broadcast();
+        crash_hpet_disable_legacy_broadcast();
 
     /*
      * compat_machine_kexec() returns to idle pagetables, which requires us

diff -r e80b5280fe2f -r aaf44d1a903d xen/include/asm-x86/hpet.h

xen/include/asm-x86/hpet.h +1 -0

@@ -73,5 +73,6 @@ void hpet_broadcast_enter(void);
 void hpet_broadcast_exit(void);
 int hpet_broadcast_is_available(void);
 void hpet_disable_legacy_broadcast(void);
+void crash_hpet_disable_legacy_broadcast(void);
 
 #endif /* __X86_HPET_H__ */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

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.

Signed-off-by Andrew Cooper <[email protected]>

diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/apic.c

xen/arch/x86/apic.c +2 -0

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

xen/arch/x86/apic.c +11 -0

+ */
+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) );
+

xen/arch/x86/apic.c +2 -0

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

xen/arch/x86/genapic/probe.c +0 -0

 	check_x2apic_preenabled();
 	cmdline_apic = changed = (genapic != NULL);
 

diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/smpboot.c

xen/arch/x86/smpboot.c +3 -0

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

xen/include/asm-x86/apic.h +0 -0

@@ -21,6 +21,16 @@
 #define IO_APIC_REDIR_DEST_LOGICAL	0x00800

xen/include/asm-x86/apic.h +3 -0

+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
[email protected]
http://lists.xensource.com/xen-devel

The use of this varable was only sensible when the choice for lapic mode

was between enabled or disabled. Now that x2apic is about, it is wrong, and causes a protection fault in certain cases when trying to tare down

x2apic mode.

The only place where its use is relevent in the code is in disable_local_APIC

which has been changed to correctly tare down the local APIC without a

protection fault (which leads to a general protection fault).

Signed-off-by Andrew Cooper <[email protected]>

diff -r 62a8ce6595ad -r e80b5280fe2f xen/arch/x86/apic.c

xen/arch/x86/apic.c +4 -6

@@ -165,8 +165,6 @@ void __init apic_intr_init(void)
 /* Using APIC to generate smp_local_timer_interrupt? */
 static bool_t __read_mostly using_apic_timer;
 
-static bool_t __read_mostly enabled_via_apicbase;
-
 int get_physical_broadcast(void)
 {
     if (modern_apic())

@@ -330,6 +328,9 @@ void disconnect_bsp_APIC(int virt_wire_s
 
 void disable_local_APIC(void)
 {
+    u64 msr_contents;
+    enum apic_mode current_mode;
+
     clear_local_APIC();
 
     /*

@@ -339,10 +340,54 @@ void disable_local_APIC(void)
     apic_write_around(APIC_SPIV,
         apic_read(APIC_SPIV) & ~APIC_SPIV_APIC_ENABLED);
 
-    if (enabled_via_apicbase) {
-        uint64_t msr_content;
-        rdmsrl(MSR_IA32_APICBASE, msr_content);
-        wrmsrl(MSR_IA32_APICBASE, msr_content & ~MSR_IA32_APICBASE_ENABLE);
+    /* Work out what apic mode we are in */

xen/arch/x86/apic.c +37 -1

+             */
+            if ( msr_contents & MSR_IA32_APICBASE_ENABLE )
+                current_mode = APIC_MODE_XAPIC;
+            else
+                current_mode = APIC_MODE_DISABLED;
+        }
+
+    /* See what (if anything) we need to do to revert to boot mode */
+    if( current_mode != this_cpu(apic_boot_mode) )
+    {
+        rdmsrl(MSR_IA32_APICBASE, msr_contents);
+        msr_contents &= ~ ( MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD );
+        wrmsrl(MSR_IA32_APICBASE, msr_contents);
+
+        switch(this_cpu(apic_boot_mode))
+        {
+        case APIC_MODE_DISABLED:
+            break; /* Nothing to do - we did this above */
+        case APIC_MODE_XAPIC:
+        {
+            msr_contents |= MSR_IA32_APICBASE_ENABLE;
+            wrmsrl(MSR_IA32_APICBASE, msr_contents);
+            break;
+        }
+        case APIC_MODE_X2APIC:
+        {
+            msr_contents |= ( MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD );
+            wrmsrl(MSR_IA32_APICBASE, msr_contents);
+            break;
+        }
+        default:
+        {
+            printk("Hit default case when reverting lapic to boot state on core #%d\n",
+                   smp_processor_id());
+            break;
+        }
+        }
     }
 }
 

@@ -874,7 +919,6 @@ static int __init detect_init_APIC (void
             wrmsrl(MSR_IA32_APICBASE,
                 msr_content | MSR_IA32_APICBASE_ENABLE
                 | APIC_DEFAULT_PHYS_BASE);
-            enabled_via_apicbase = 1;
         }
     }
     /*

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 8 lines · Andrew Cooper

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?

Quoted 14 lines

Signed-off-by Andrew Cooper<[email protected]>

diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/apic.c

xen/arch/x86/apic.c +2 -0

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

xen/arch/x86/apic.c int __init APIC_init_uniprocessor (void) · line 1443

Quoted 6 lines

xen/arch/x86/apic.c +2 -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?

Quoted 14 lines

xen/arch/x86/apic.c +14 -0

+ */
+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 */

xen/arch/x86/apic.c int __init APIC_init_uniprocessor (void) · line 1464

Quoted 7 lines

xen/arch/x86/apic.c +7 -0

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

xen/arch/x86/apic.c int __init APIC_init_uniprocessor (void) · line 1474

Quoted 10 lines

xen/arch/x86/apic.c +10 -0

+             */
+            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..

Quoted 12 lines

xen/arch/x86/apic.c +2 -0

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

xen/arch/x86/genapic/probe.c +0 -0

@@ -60,6 +60,8 @@ void __init generic_apic_probe(void)
  {
  	int i, changed;
 

xen/arch/x86/genapic/probe.c void __init generic_apic_probe(void) · line 64

Quoted 2 lines

xen/arch/x86/genapic/probe.c +2 -0

+    record_boot_APIC_mode();
+

The spacing looks odd.

Quoted 23 lines

xen/arch/x86/genapic/probe.c +0 -0

  	check_x2apic_preenabled();
  	cmdline_apic = changed = (genapic != NULL);
 

diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/smpboot.c

xen/arch/x86/smpboot.c +3 -0

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

xen/include/asm-x86/apic.h +0 -0

@@ -21,6 +21,16 @@
  #define IO_APIC_REDIR_DEST_LOGICAL	0x00800
  #define IO_APIC_REDIR_DEST_PHYSICAL	0x00000
 

xen/include/asm-x86/apic.h line 31

Quoted 8 lines

xen/include/asm-x86/apic.h +8 -0

+/* 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?

Quoted 18 lines

xen/include/asm-x86/apic.h +3 -0

+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
[email protected]
http://lists.xensource.com/xen-devel

Quoted 1 line · Andrew Cooper

The use of this varable was only sensible when the choice for lapic mode

variable

Quoted 2 lines

was between enabled or disabled. Now that x2apic is about, it is wrong, and causes a protection fault in certain cases when trying to tare down

tare?

Quoted 2 lines

x2apic mode.

Quoted 1 line

The only place where its use is relevent in the code is in disable_local_APIC

^- is ^^^ take that out.

Quoted 1 line

which has been changed to correctly tare down the local APIC without a

teardown?

Quoted 1 line

protection fault (which leads to a general protection fault).

So if you don't have x2apic, then it is wrong to disable the LAPIC mode? What about older hardware?

Quoted 33 lines

Signed-off-by Andrew Cooper<[email protected]>

diff -r 62a8ce6595ad -r e80b5280fe2f xen/arch/x86/apic.c

xen/arch/x86/apic.c +4 -6

@@ -165,8 +165,6 @@ void __init apic_intr_init(void)
  /* Using APIC to generate smp_local_timer_interrupt? */
  static bool_t __read_mostly using_apic_timer;
 
-static bool_t __read_mostly enabled_via_apicbase;
-
  int get_physical_broadcast(void)
  {
      if (modern_apic())

@@ -330,6 +328,9 @@ void disconnect_bsp_APIC(int virt_wire_s
 
  void disable_local_APIC(void)
  {
+    u64 msr_contents;
+    enum apic_mode current_mode;
+
      clear_local_APIC();
 
      /*

@@ -339,10 +340,54 @@ void disable_local_APIC(void)
      apic_write_around(APIC_SPIV,
          apic_read(APIC_SPIV)&  ~APIC_SPIV_APIC_ENABLED);
 
-    if (enabled_via_apicbase) {
-        uint64_t msr_content;
-        rdmsrl(MSR_IA32_APICBASE, msr_content);
-        wrmsrl(MSR_IA32_APICBASE, msr_content&  ~MSR_IA32_APICBASE_ENABLE);
+    /* Work out what apic mode we are in */

xen/arch/x86/apic.c void disable_local_APIC(void) · line 353

Quoted 10 lines

xen/arch/x86/apic.c +10 -0

+    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) )
+        current_mode = APIC_MODE_X2APIC;
+    else
+        {
+            /* EN bit should always be valid as long as we can read the MSR
+             * Can anyone confirm this?

Might want to email Vivek Goyal..

Quoted 53 lines

xen/arch/x86/apic.c +37 -1

+             */
+            if ( msr_contents&  MSR_IA32_APICBASE_ENABLE )
+                current_mode = APIC_MODE_XAPIC;
+            else
+                current_mode = APIC_MODE_DISABLED;
+        }
+
+    /* See what (if anything) we need to do to revert to boot mode */
+    if( current_mode != this_cpu(apic_boot_mode) )
+    {
+        rdmsrl(MSR_IA32_APICBASE, msr_contents);
+        msr_contents&= ~ ( MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD );
+        wrmsrl(MSR_IA32_APICBASE, msr_contents);
+
+        switch(this_cpu(apic_boot_mode))
+        {
+        case APIC_MODE_DISABLED:
+            break; /* Nothing to do - we did this above */
+        case APIC_MODE_XAPIC:
+        {
+            msr_contents |= MSR_IA32_APICBASE_ENABLE;
+            wrmsrl(MSR_IA32_APICBASE, msr_contents);
+            break;
+        }
+        case APIC_MODE_X2APIC:
+        {
+            msr_contents |= ( MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD );
+            wrmsrl(MSR_IA32_APICBASE, msr_contents);
+            break;
+        }
+        default:
+        {
+            printk("Hit default case when reverting lapic to boot state on core #%d\n",
+                   smp_processor_id());
+            break;
+        }
+        }
      }
  }
 

@@ -874,7 +919,6 @@ static int __init detect_init_APIC (void
              wrmsrl(MSR_IA32_APICBASE,
                  msr_content | MSR_IA32_APICBASE_ENABLE
                  | APIC_DEFAULT_PHYS_BASE);
-            enabled_via_apicbase = 1;
          }
      }
      /*

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 5 lines · Konrad Rzeszutek Wilk

Quoted 4 lines · Andrew Cooper

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.

Quoted 5 lines

Quoted 4 lines

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

Quoted 14 lines

Quoted 14 lines

Signed-off-by Andrew Cooper<[email protected]>

diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/apic.c

xen/arch/x86/apic.c +2 -0

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

Quoted 7 lines

xen/arch/x86/apic.c int __init APIC_init_uniprocessor (void) · line 1443

Quoted 6 lines

xen/arch/x86/apic.c +2 -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"

Quoted 14 lines

Quoted 14 lines

xen/arch/x86/apic.c +14 -0

+ */
+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 */

Quoted 8 lines

xen/arch/x86/apic.c int __init APIC_init_uniprocessor (void) · line 1464

Quoted 7 lines

xen/arch/x86/apic.c +7 -0

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

Quoted 10 lines

Quoted 10 lines

xen/arch/x86/apic.c +10 -0

+             */
+            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());

Quoted 1 line

This begs of a function to convert those enums to strings..

True - this was left over from my debugging - I shall fix that tomorrow.

Quoted 12 lines

Quoted 12 lines

xen/arch/x86/apic.c +2 -0

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

xen/arch/x86/genapic/probe.c +0 -0

@@ -60,6 +60,8 @@ void __init generic_apic_probe(void)
 {
 	int i, changed;
 

Quoted 3 lines

xen/arch/x86/genapic/probe.c void __init generic_apic_probe(void) · line 64

Quoted 2 lines

xen/arch/x86/genapic/probe.c +2 -0

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

Quoted 2 lines

So if you don't have x2apic, then it is wrong to disable the LAPIC mode? What about older hardware?

I guess I wasn't very clear in my description. In older hardware without x2apic, it is correct to simply twiddle the ENABLE bit in the APICBASE MSR. However, with x2apic mode enabled, setting the ENABLE bit from 1 to 0 while leaving the EXTD bit set will result in a protection fault which will propagate to a general protection fault the same codepath will be called in the fault handler. As a result, the current code in disable_local_APIC will result in a GPF if the BIOS boots with LAPICs disabled (fine as per the spec for compatibility) and xen decided to take advantage of x2apic mode.

Quoted 6 lines · Andrew Cooper

kdump kernels are unable to boot with IOMMU enabled, this patch disabled IOMMU mode and removes some of the generic code from the shutdown path which doesnt work after other CPUs have been shot down.

Also, leave local interrupts disabled when jumping into pugatory

purgatory?

Quoted 21 lines

as we have no idea whats in there and really dont want to be servicing interrupts when our entire state is invalid.

Signed-off-by Andrew Cooper<[email protected]>

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/crash.c

xen/arch/x86/crash.c +2 -0

@@ -27,6 +27,8 @@
  #include<asm/hvm/support.h>
  #include<asm/apic.h>
  #include<asm/io_apic.h>
+#include<xen/iommu.h>
+#include<asm/hvm/iommu.h>
 
  static atomic_t waiting_for_crash_ipi;
  static unsigned int crashing_cpu;

@@ -43,7 +45,10 @@ static int crash_nmi_callback(struct cpu
 
      kexec_crash_save_cpu();
 

xen/arch/x86/crash.c static int crash_nmi_callback(struct cpu · line 51

Quoted 5 lines

xen/arch/x86/crash.c +4 -1

-    __stop_this_cpu();
+    disable_local_APIC();
+    hvm_cpu_down();
+    clts();
+    asm volatile ( "fninit" );

Can you provide a comment why you are using fninit and clt?
Is this what the Linux kernel does too when it goes through the kexec path?

Quoted 26 lines

xen/arch/x86/crash.c +11 -1

      atomic_dec(&waiting_for_crash_ipi);
 

@@ -56,6 +61,7 @@ static int crash_nmi_callback(struct cpu
  static void nmi_shootdown_cpus(void)
  {
      unsigned long msecs;
+    u64 msr_contents;
 
      local_irq_disable();
 

@@ -77,18 +83,43 @@ static void nmi_shootdown_cpus(void)
          msecs--;
      }
 
-    __stop_this_cpu();
+    disable_local_APIC();
+    hvm_cpu_down();
+    clts();
+    asm volatile ( "fninit" );
+
+    /* This is a bit of a hack but there is no other way to shutdown correctly
+     * without a significant refactoring of the APIC code */
+    rdmsrl(MSR_IA32_APICBASE, msr_contents);
+    if ( cpu_has(&current_cpu_data, X86_FEATURE_X2APIC)
+&&  (msr_contents&  MSR_IA32_APICBASE_EXTD) )

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 83

Quoted 7 lines

xen/arch/x86/crash.c +4 -2

+        x2apic_enabled = 1;
+    else
+        x2apic_enabled = 0;
+
      disable_IO_APIC();
-
-    local_irq_enable();

Why?

Quoted 9 lines

xen/arch/x86/crash.c +1 -0

  }
 
  void machine_crash_shutdown(void)
  {
      crash_xen_info_t *info;
+    const struct iommu_ops * ops;
 
      nmi_shootdown_cpus();
 

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 115

Quoted 6 lines

xen/arch/x86/crash.c +6 -0

+    /* Yes i know this is hacky but it is the easiest solution.  I should add an iommu_ops
+     * function called crash() or so which just disables the iommu 'fun' without saving state
+     */
+    ops = iommu_get_ops();
+    if(ops)
+        ops->suspend();

Uh, no checking if ops->suspend exists?

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 118

Quoted 3 lines

xen/arch/x86/crash.c +3 -0

+
+    /* Yes i know this is from driver/passthrough/vtd/ but it appears to be architecture
+     * independant, and also bears little/no relation to x2apic.  Needs cleaning up

What about AMD VI IOMMUs? Does it work when that IOMMU is used?

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 120

Quoted 2 lines

xen/arch/x86/crash.c +2 -0

+     */
+    iommu_disable_x2apic_IR();

Can't that function be done in the suspend code of the IOMMU?

Quoted 69 lines

xen/arch/x86/crash.c +2 -0

+
+
      info = kexec_crash_save_info();
      info->xen_phys_start = xen_phys_start;
      info->dom0_pfn_to_mfn_frame_list_list =

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/hpet.c

xen/arch/x86/hpet.c +27 -0

@@ -670,6 +670,33 @@ void hpet_disable_legacy_broadcast(void)
      smp_send_event_check_mask(&cpu_online_map);
  }
 
+/* This function is similar to the regular
+ * hpet_disable_legacy_broadcast function, except it is called
+ * on the crash path with only the current processor up, so we
+ * can forget the locks and really cant send an event check IPI
+ * to the other processors */
+void crash_hpet_disable_legacy_broadcast(void)
+{
+    u32 cfg;
+
+    if ( !hpet_events || !(hpet_events->flags&  HPET_EVT_LEGACY) )
+        return;
+
+    hpet_events->flags |= HPET_EVT_DISABLE;
+
+    /* disable HPET T0 */
+    cfg = hpet_read32(HPET_Tn_CFG(0));
+    cfg&= ~HPET_TN_ENABLE;
+    hpet_write32(cfg, HPET_Tn_CFG(0));
+
+    /* Stop HPET legacy interrupts */
+    cfg = hpet_read32(HPET_CFG);
+    cfg&= ~HPET_CFG_LEGACY;
+    hpet_write32(cfg, HPET_CFG);
+
+}
+
+
  void hpet_broadcast_enter(void)
  {
      unsigned int cpu = smp_processor_id();

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/machine_kexec.c

xen/arch/x86/machine_kexec.c +1 -1

@@ -97,7 +97,7 @@ void machine_kexec(xen_kexec_image_t *im
      };
 
      if ( hpet_broadcast_is_available() )
-        hpet_disable_legacy_broadcast();
+        crash_hpet_disable_legacy_broadcast();
 
      /*
       * compat_machine_kexec() returns to idle pagetables, which requires us

diff -r e80b5280fe2f -r aaf44d1a903d xen/include/asm-x86/hpet.h

xen/include/asm-x86/hpet.h +1 -0

@@ -73,5 +73,6 @@ void hpet_broadcast_enter(void);
  void hpet_broadcast_exit(void);
  int hpet_broadcast_is_available(void);
  void hpet_disable_legacy_broadcast(void);
+void crash_hpet_disable_legacy_broadcast(void);
 
  #endif /* __X86_HPET_H__ */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 6 lines · Konrad Rzeszutek Wilk

Quoted 5 lines · Andrew Cooper

kdump kernels are unable to boot with IOMMU enabled, this patch disabled IOMMU mode and removes some of the generic code from the shutdown path which doesnt work after other CPUs have been shot down.

Quoted 2 lines

Quoted 1 line

Also, leave local interrupts disabled when jumping into pugatory

purgatory?

purgatory is the bit of code which the a crashing kernel jumps into, which pretends to do minimal bootloader things to book the kdump kernel. It is part of the kexec-tools package.

Quoted 22 lines

Quoted 22 lines

as we have no idea whats in there and really dont want to be servicing interrupts when our entire state is invalid.

Signed-off-by Andrew Cooper<[email protected]>

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/crash.c

xen/arch/x86/crash.c +2 -0

@@ -27,6 +27,8 @@
 #include<asm/hvm/support.h>
 #include<asm/apic.h>
 #include<asm/io_apic.h>
+#include<xen/iommu.h>
+#include<asm/hvm/iommu.h>
 
 static atomic_t waiting_for_crash_ipi;
 static unsigned int crashing_cpu;

@@ -43,7 +45,10 @@ static int crash_nmi_callback(struct cpu
 
     kexec_crash_save_cpu();
 

Quoted 7 lines

xen/arch/x86/crash.c static int crash_nmi_callback(struct cpu · line 51

Quoted 5 lines

xen/arch/x86/crash.c +4 -1

-    __stop_this_cpu();
+    disable_local_APIC();
+    hvm_cpu_down();
+    clts();
+    asm volatile ( "fninit" );

Can you provide a comment why you are using fninit and clt?
Is this what the Linux kernel does too when it goes through the kexec path?

I was replacing __stop_this_cpu() with the safe subset of its contents - it was a verbatim copy minus the SMP stuff which the regular __stop_this_cpu() does. I suppose I could have split __stop_this_cpu() to __crash_stop_this_cpu() but it didn't seem worth making such a trivially small function.

Quoted 26 lines

Quoted 26 lines

xen/arch/x86/crash.c +11 -1

     atomic_dec(&waiting_for_crash_ipi);
 

@@ -56,6 +61,7 @@ static int crash_nmi_callback(struct cpu
 static void nmi_shootdown_cpus(void)
 {
     unsigned long msecs;
+    u64 msr_contents;
 
     local_irq_disable();
 

@@ -77,18 +83,43 @@ static void nmi_shootdown_cpus(void)
         msecs--;
     }
 
-    __stop_this_cpu();
+    disable_local_APIC();
+    hvm_cpu_down();
+    clts();
+    asm volatile ( "fninit" );
+
+    /* This is a bit of a hack but there is no other way to shutdown correctly
+     * without a significant refactoring of the APIC code */
+    rdmsrl(MSR_IA32_APICBASE, msr_contents);
+    if ( cpu_has(&current_cpu_data, X86_FEATURE_X2APIC)
+&&  (msr_contents&  MSR_IA32_APICBASE_EXTD) )

Quoted 8 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 83

Quoted 7 lines

xen/arch/x86/crash.c +4 -2

+        x2apic_enabled = 1;
+    else
+        x2apic_enabled = 0;
+
     disable_IO_APIC();
-
-    local_irq_enable();

Why?

Because that local_irq_enable() results in the interrupt flag being set when jumping into purgatory, which (at the moment) doesn't touch interrupts at all. The result is that interrupts from PCI devices which are unaware of the crash are (potentially) being serviced by the xen handlers even though we have left the Xen context for good.

Quoted 9 lines

Quoted 9 lines

xen/arch/x86/crash.c +1 -0

 }
 
 void machine_crash_shutdown(void)
 {
     crash_xen_info_t *info;
+    const struct iommu_ops * ops;
 
     nmi_shootdown_cpus();
 

Quoted 8 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 115

Quoted 6 lines

xen/arch/x86/crash.c +6 -0

+    /* Yes i know this is hacky but it is the easiest solution.  I should add an iommu_ops
+     * function called crash() or so which just disables the iommu 'fun' without saving state
+     */
+    ops = iommu_get_ops();
+    if(ops)
+        ops->suspend();

Uh, no checking if ops->suspend exists?

True - at the moment both intel and amd iommu_ops are fully implemented but I will add an extra condition to the if statement.

Quoted 5 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 118

Quoted 3 lines

xen/arch/x86/crash.c +3 -0

+
+    /* Yes i know this is from driver/passthrough/vtd/ but it appears to be architecture
+     * independant, and also bears little/no relation to x2apic.  Needs cleaning up

What about AMD VI IOMMUs? Does it work when that IOMMU is used?

It worked on the AMD box I tested the code on. Like the comment says - as far as I can tell, it is architecture independent code.

Quoted 3 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 120

Quoted 2 lines

xen/arch/x86/crash.c +2 -0

+     */
+    iommu_disable_x2apic_IR();

Can't that function be done in the suspend code of the IOMMU?

There is a comment in iommu suspend stating that it cant and isn't done, but rather is left for the local/ioapic_suspend functions which dont properly work in the kexec path.

Quoted 8 lines · Andrew Cooper

Quoted 6 lines · Konrad Rzeszutek Wilk

Quoted 5 lines · Andrew Cooper

kdump kernels are unable to boot with IOMMU enabled, this patch disabled IOMMU mode and removes some of the generic code from the shutdown path which doesnt work after other CPUs have been shot down.

Quoted 5 lines

Quoted 2 lines

Quoted 1 line

Also, leave local interrupts disabled when jumping into pugatory

purgatory?

purgatory is the bit of code which the a crashing kernel jumps into, which pretends to do minimal bootloader things to book the kdump kernel. It is part of the kexec-tools package.

Ok. Might want to include that in the description.

Quoted 28 lines

Quoted 28 lines

Quoted 18 lines

as we have no idea whats in there and really dont want to be servicing interrupts when our entire state is invalid.

Signed-off-by Andrew Cooper<[email protected]>

diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/crash.c

xen/arch/x86/crash.c +2 -0

@@ -27,6 +27,8 @@
  #include<asm/hvm/support.h>
  #include<asm/apic.h>
  #include<asm/io_apic.h>
+#include<xen/iommu.h>
+#include<asm/hvm/iommu.h>
 
  static atomic_t waiting_for_crash_ipi;
  static unsigned int crashing_cpu;

xen/arch/x86/crash.c static int crash_nmi_callback(struct cpu · line 51

Quoted 8 lines

xen/arch/x86/crash.c +4 -1

@@ -43,7 +45,10 @@ static int crash_nmi_callback(struct cpu
 
      kexec_crash_save_cpu();
 
-    __stop_this_cpu();
+    disable_local_APIC();
+    hvm_cpu_down();
+    clts();
+    asm volatile ( "fninit" );

Can you provide a comment why you are using fninit and clt?
Is this what the Linux kernel does too when it goes through the kexec path?

Quoted 5 lines

I was replacing __stop_this_cpu() with the safe subset of its contents - it was a verbatim copy minus the SMP stuff which the regular __stop_this_cpu() does. I suppose I could have split __stop_this_cpu() to __crash_stop_this_cpu() but it didn't seem worth making such a trivially small function.

Why can't you use the SMP version? I know you are not running in SMP mode, but does it hurt?

Quoted 33 lines

Quoted 25 lines

Quoted 25 lines

xen/arch/x86/crash.c +11 -1

      atomic_dec(&waiting_for_crash_ipi);
 

@@ -56,6 +61,7 @@ static int crash_nmi_callback(struct cpu
  static void nmi_shootdown_cpus(void)
  {
      unsigned long msecs;
+    u64 msr_contents;
 
      local_irq_disable();
 

@@ -77,18 +83,43 @@ static void nmi_shootdown_cpus(void)
          msecs--;
      }
 
-    __stop_this_cpu();
+    disable_local_APIC();
+    hvm_cpu_down();
+    clts();
+    asm volatile ( "fninit" );
+
+    /* This is a bit of a hack but there is no other way to shutdown correctly
+     * without a significant refactoring of the APIC code */
+    rdmsrl(MSR_IA32_APICBASE, msr_contents);
+    if ( cpu_has(&current_cpu_data, X86_FEATURE_X2APIC)
+&&   (msr_contents&   MSR_IA32_APICBASE_EXTD) )

Quoted 8 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 83

Quoted 7 lines

xen/arch/x86/crash.c +4 -2

+        x2apic_enabled = 1;
+    else
+        x2apic_enabled = 0;
+
      disable_IO_APIC();
-
-    local_irq_enable();

Why?

Quoted 5 lines

Because that local_irq_enable() results in the interrupt flag being set when jumping into purgatory, which (at the moment) doesn't touch interrupts at all. The result is that interrupts from PCI devices which are unaware of the crash are (potentially) being serviced by the xen handlers even though we have left the Xen context for good.

Yikes. Please do explain this in the code right there were you remove the local_irq_enable..

Quoted 26 lines

Quoted 9 lines

Quoted 9 lines

xen/arch/x86/crash.c +1 -0

  }
 
  void machine_crash_shutdown(void)
  {
      crash_xen_info_t *info;
+    const struct iommu_ops * ops;
 
      nmi_shootdown_cpus();
 

Quoted 8 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 115

Quoted 6 lines

xen/arch/x86/crash.c +6 -0

+    /* Yes i know this is hacky but it is the easiest solution.  I should add an iommu_ops
+     * function called crash() or so which just disables the iommu 'fun' without saving state
+     */
+    ops = iommu_get_ops();
+    if(ops)
+        ops->suspend();

Uh, no checking if ops->suspend exists?

True - at the moment both intel and amd iommu_ops are fully implemented but I will add an extra condition to the if statement.

Quoted 5 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 118

Quoted 3 lines

xen/arch/x86/crash.c +3 -0

+
+    /* Yes i know this is from driver/passthrough/vtd/ but it appears to be architecture
+     * independant, and also bears little/no relation to x2apic.  Needs cleaning up

What about AMD VI IOMMUs? Does it work when that IOMMU is used?

It worked on the AMD box I tested the code on. Like the comment says - as far as I can tell, it is architecture independent code.

Quoted 6 lines

Quoted 3 lines

xen/arch/x86/crash.c static void nmi_shootdown_cpus(void) · line 120

Quoted 2 lines

xen/arch/x86/crash.c +2 -0

+     */
+    iommu_disable_x2apic_IR();

Can't that function be done in the suspend code of the IOMMU?

There is a comment in iommu suspend stating that it cant and isn't done, but rather is left for the local/ioapic_suspend functions which dont properly work in the kexec path.

OK, how about just moving it out of driver/passthrought/vtd then?

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.

Signed-off-by Andrew Cooper <[email protected]>

diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/apic.c

xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/apic.c Tue May 17 17:32:19 2011 +0100 +3 -0

@@ -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
+ */

xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/apic.c Tue May 17 17:32:19 2011 +0100 +13 -0

+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 */

xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/apic.c Tue May 17 17:32:19 2011 +0100 +19 -0

+    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?
+             */
+            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());
+}
+
 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

xen/arch/x86/genapic/probe.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/genapic/probe.c Tue May 17 17:32:19 2011 +0100 +2 -0

@@ -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 f531ed84b066 -r 62a8ce6595ad xen/arch/x86/smpboot.c

xen/arch/x86/smpboot.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/smpboot.c Tue May 17 17:32:19 2011 +0100 +3 -0

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

xen/include/asm-x86/apic.h Wed May 18 19:00:13 2011 +0100 renamed from xen/include/asm-x86/apic.h Tue May 17 17:32:19 2011 +0100 +3 -0

@@ -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 */

xen/include/asm-x86/apic.h Wed May 18 19:00:13 2011 +0100 renamed from xen/include/asm-x86/apic.h Tue May 17 17:32:19 2011 +0100 +6 -0

+};
+
+/* PerCPU local APIC boot mode - so we can taredown to bios state */
+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
[email protected]
http://lists.xensource.com/xen-devel

From: Andrew Cooper
Sent: Thursday, May 19, 2011 4:28 AM

Quoted 5 lines · Konrad Rzeszutek Wilk

Quoted 4 lines · Andrew Cooper

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.

Quoted 5 lines

Quoted 4 lines

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

From: Konrad Rzeszutek Wilk
Sent: Thursday, May 19, 2011 4:43 AM

Quoted 12 lines · Andrew Cooper

Quoted 9 lines · Konrad Rzeszutek Wilk

Quoted 8 lines · Andrew Cooper

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.

Quoted 3 lines

Quoted 1 line

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

From: Andrew Cooper
Sent: Thursday, May 19, 2011 4:36 AM

Quoted 7 lines

Quoted 3 lines

which has been changed to correctly tare down the local APIC without a

teardown?

Quoted 1 line

protection fault (which leads to a general protection fault).

So if you don't have x2apic, then it is wrong to disable the LAPIC mode? What about older hardware?

I guess I wasn't very clear in my description. In older hardware without x2apic, it is correct to simply twiddle the ENABLE bit in the APICBASE MSR. However, with x2apic mode enabled, setting the ENABLE bit from 1 to 0 while leaving the EXTD bit set will result in a protection fault which will propagate to a general protection fault the same codepath will be called in the fault handler. As a result, the current code in disable_local_APIC will result in a GPF if the BIOS boots with LAPICs disabled (fine as per the spec for compatibility) and xen decided to take advantage of x2apic mode.

Quoted 2 lines

Quoted 1 line

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 <[email protected]>
M:      Haren Myneni <[email protected]>
L:      [email protected]
W:      http://lse.sourceforge.net/kdump/
S:      Maintained
F:      Documentation/kdump/

Quoted 6 lines · Andrew Cooper

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.

You mean some CPUs have APIC totally disabled?

Impossible: you need the APIC
enabled to be able to INIT-SIPI bootstrap the secondary processors.

And Xen
itself has no code to enable disabled APICs (except some very legacy code that executes only in the uniprocessor case, which I'm pretty sure you're not touching).

You call your new function so late in secondary CPU bringup that our own APIC setup has already run (via smp_callin->{x2apic_ap_setup,setup_local_APIC}). Which would indicate that the boot state of the secondary CPUs is not of importance to you, since you I'm sure tested your patch as working okay in this current form, and that would imply that you don't need to record per-cpu boot state.

The singleshot BUG_ON in record_boot_APIC_mode is broken, since secondary CPUs can be started up multiple times (we can offline/online them). You need to bail on repeated invocations. Or not be recording boot state for secondary cpus since apparently it isn't needed...

We're going to be going through these patches very slowly and painfully, because frankly the arguments and assertions seem to be plenty full of holes.

-- Keir

Quoted 124 lines

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.

Signed-off-by Andrew Cooper<[email protected]>

diff -r f531ed84b066 -r 62a8ce6595ad xen/arch/x86/apic.c

xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/apic.c Tue May 17 17:32:19 2011 +0100 +3 -0

@@ -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
+ */

xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/apic.c Tue May 17 17:32:19 2011 +0100 +13 -0

+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 */

xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/apic.c Tue May 17 17:32:19 2011 +0100 +19 -0

+    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?
+             */
+            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());
+}
+
  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

xen/arch/x86/genapic/probe.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/genapic/probe.c Tue May 17 17:32:19 2011 +0100 +2 -0

@@ -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 f531ed84b066 -r 62a8ce6595ad xen/arch/x86/smpboot.c

xen/arch/x86/smpboot.c Wed May 18 19:00:13 2011 +0100 renamed from xen/arch/x86/smpboot.c Tue May 17 17:32:19 2011 +0100 +3 -0

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

xen/include/asm-x86/apic.h Wed May 18 19:00:13 2011 +0100 renamed from xen/include/asm-x86/apic.h Tue May 17 17:32:19 2011 +0100 +3 -0

@@ -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 */

xen/include/asm-x86/apic.h Wed May 18 19:00:13 2011 +0100 renamed from xen/include/asm-x86/apic.h Tue May 17 17:32:19 2011 +0100 +6 -0

+};
+
+/* PerCPU local APIC boot mode - so we can taredown to bios state */
+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
[email protected]
http://lists.xensource.com/xen-devel

evidence shows however that linux 2.6.32 kdump (and presumably

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

What about AMD VI IOMMUs? Does it work when that IOMMU is used?

It worked on the AMD box I tested the code on. Like the comment says - as far as I can tell, it is architecture independent code.

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     */
+    iommu_disable_x2apic_IR();

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

Can't that function be done in the suspend code of the IOMMU?

There is a comment in iommu suspend stating that it cant and isn't done, but rather is left for the local/ioapic_suspend functions which dont properly work in the kexec path.

OK, how about just moving it out of driver/passthrought/vtd then?

Because that code is fragile enough without me poking about in it. I would prefer someone with more knowledge about IOMMU to make that call.

Quoted 1 line

evidence shows however that linux 2.6.32 kdump (and presumably

I would suggest you use the latest linux kernel (2.6.39) to look at the kdump implementation, much fresher.

Quoted 11 lines · Konrad Rzeszutek Wilk

Quoted 2 lines

Quoted 1 line

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 <[email protected]>
M:      Haren Myneni <[email protected]>
L:      [email protected]
W:      http://lse.sourceforge.net/kdump/
S:      Maintained
F:      Documentation/kdump/

That's the Linux MAINTAINERS file, which is why Andrew probably didn't look there since this is a Xen side patch. The question didn't really seem like a kdump maintainer one in any case, more of a "person who knows about APICs one", perhaps those are sometimes the same person ;-)

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

What about AMD VI IOMMUs? Does it work when that IOMMU is used?

It worked on the AMD box I tested the code on. Like the comment says - as far as I can tell, it is architecture independent code.

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     */
+    iommu_disable_x2apic_IR();

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

Can't that function be done in the suspend code of the IOMMU?

There is a comment in iommu suspend stating that it cant and isn't done, but rather is left for the local/ioapic_suspend functions which dont properly work in the kexec path.

OK, how about just moving it out of driver/passthrought/vtd then?

Because that code is fragile enough without me poking about in it. I would prefer someone with more knowledge about IOMMU to make that call.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

What about AMD VI IOMMUs? Does it work when that IOMMU is used?

It worked on the AMD box I tested the code on. Like the comment says - as far as I can tell, it is architecture independent code.

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     */
+    iommu_disable_x2apic_IR();

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

Can't that function be done in the suspend code of the IOMMU?

There is a comment in iommu suspend stating that it cant and isn't done, but rather is left for the local/ioapic_suspend functions which dont properly work in the kexec path.

OK, how about just moving it out of driver/passthrought/vtd then?

Because that code is fragile enough without me poking about in it. I would prefer someone with more knowledge about IOMMU to make that call.