WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] kexec: disable iommu and x2apic before jumping

To: Jan Beulich <JBeulich@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] kexec: disable iommu and x2apic before jumping into the kdump kernel
From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Date: Fri, 13 May 2011 15:32:17 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 13 May 2011 07:33:27 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4DCD5AD30200007800041514@xxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <efa733ebd00d1648f04a.1305290877@andrewcoop> <4DCD5AD30200007800041514@xxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110424 Lightning/1.0b2 Thunderbird/3.1.10


On 13/05/11 15:22, Jan Beulich wrote:
On 13.05.11 at 14:47, Andrew Cooper<andrew.cooper3@xxxxxxxxxx>  wrote:
This is a massive hack but is better than the code currently in place.
Some functions have been duped into a crash_* version to separate them
from the regular shutdown path, where disabling these subsystems is
irrelevant.
Hmm, I'd rather not see this go in (in this form). A few comment below,
but even with them addressed I'm not going to be in favor of it.

Also did you compare all of this with the way Linux deals with the same
situation?
Linux 2.6.32 which I was comparing against successfully reverts the lapic to boot state, but has no clue about iommu so will never enable it in the first place. There is currently not enough information in apic.c to remember the boot state - the hack for xapic boot state is poor to say the least. I will rejig this to properly record boot state in all cases for local apics.
Signed-off-by: Andrew Cooper<andrew.cooper3@xxxxxxxxxx>

diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/apic.c
--- a/xen/arch/x86/apic.c       Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/apic.c       Fri May 13 13:45:05 2011 +0100
@@ -344,6 +344,42 @@
      }
  }

+/* Customised version of disable_local_APIC which is only called on the kexec 
crash path.
+ * It is a brutal hack and only here untill the APIC code is overhauled.  We 
NEED to swtich
+ * out of x2apic mode whatever the case as the kdump kernel cant cope due to the 
MMIO<->  MSR
+ * swap invovled with x2apic mode.
Unless the system booted in x2apic mode. Otherwise you'll confuse
the secondary kernel even more.

+ */
+void crash_disable_local_APIC(void)
+{
+    uint64_t msr_content;
+    clear_local_APIC();
+
+    apic_write_around(APIC_SPIV,
+        apic_read(APIC_SPIV)&  ~APIC_SPIV_APIC_ENABLED);
+
+    rdmsrl(MSR_IA32_APICBASE, msr_content);
+
+    /* This is the only way to exit x2apic mode.  Trying to disable x2apic 
mode while staying
+     * xapic mode will result in a protection fault which leads to a general 
protection fault
+     * as this is on the fault handler codepath.
+     */
+    msr_content&= ~(MSR_IA32_APICBASE_ENABLE|MSR_IA32_APICBASE_EXTD);
+    wrmsrl(MSR_IA32_APICBASE, msr_content);
+
+    /* In most cases, we should leave the lapic in xapic mode as this is the 
default state on
+     * CPU reset.  However, some bioses specifically disable xapic mode for 
hilarity sake so
+     * we should follow suit so the kdump kernel does not get confused about 
mismatching ACPI
+     * tables.
+     */
+    if ( ! enabled_via_apicbase) {
+           /* If we did not manually enable xapic mode, I.E. the bios left it 
enabled,
+            * lets follow suit and leave it enabled for the kdump kernel.
+            */
+        wrmsrl(MSR_IA32_APICBASE, msr_content | MSR_IA32_APICBASE_ENABLE);
+    }
+}
+
+
  /*
   * This is to verify that we're looking at a real local APIC.
   * Check these against your board if the CPUs aren't getting
diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/crash.c
--- a/xen/arch/x86/crash.c      Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/crash.c      Fri May 13 13:45:05 2011 +0100
@@ -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,12 @@

      kexec_crash_save_cpu();

-    __stop_this_cpu();
+    crash_disable_local_APIC();
+    hvm_cpu_down();
+    /* Clear FPU, zapping any pending exceptions. Needed for warm reset with
+     * some BIOSes.*/
+    clts();
+    asm volatile ( "fninit" );

      atomic_dec(&waiting_for_crash_ipi);

@@ -77,7 +84,15 @@
          msecs--;
      }

-    __stop_this_cpu();
+    crash_disable_local_APIC();
+    hvm_cpu_down();
+    /* Clear FPU, zapping any pending exceptions. Needed for warm reset with
+     * some BIOSes.*/
+    clts();
+    asm volatile ( "fninit" );
+
+    x2apic_enabled = 0;
Why? And if indeed needed here, why not above? (Your patch wasn't
generated with -p, so it's a little difficult to understand what functions
you modify here.)

+
      disable_IO_APIC();

      local_irq_enable();
@@ -86,9 +101,24 @@
  void machine_crash_shutdown(void)
  {
      crash_xen_info_t *info;
+    const struct iommu_ops * ops;

      nmi_shootdown_cpus();

+    local_irq_disable();
local_irq_save()
Fair enough in the general case but in this case it really doesnt matter. Having said that, there are other areas on the crash/reboot path which need updating with this logic
+
+    /* 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();
+
+    /* 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
+     */
+    iommu_disable_x2apic_IR();
+    local_irq_enable();
local_irq_restore() - you absolutely want to avoid turning on
interrupts if they weren't on entry to the function.

+
      info = kexec_crash_save_info();
      info->xen_phys_start = xen_phys_start;
      info->dom0_pfn_to_mfn_frame_list_list =
diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/hpet.c
--- a/xen/arch/x86/hpet.c       Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/hpet.c       Fri May 13 13:45:05 2011 +0100
@@ -670,6 +670,34 @@
      smp_send_event_check_mask(&cpu_online_map);
  }

+void crash_hpet_disable_legacy_broadcast(void)
+{
+    u32 cfg;
+
+    if ( !hpet_events || !(hpet_events->flags&  HPET_EVT_LEGACY) )
+        return;
+
+    /* By this point in the crash handler, we are the only CPU still going,
+     * so the lock is irrelevant
+     */
+    /* spin_lock_irqsave(&hpet_events->lock, flags); */
+
+    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);
+
+    /* spin_unlock_irqrestore(&hpet_events->lock, flags); */
+}
This looks to be the same except for the skipping of the locks.
Calls for adding a parameter to the original function.

Jan
This function skips the IPI to all other processors, because at this point in the code, all other processors have been shot down. I will comment that detail
+
+
  void hpet_broadcast_enter(void)
  {
      unsigned int cpu = smp_processor_id();
diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/machine_kexec.c
--- a/xen/arch/x86/machine_kexec.c      Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/machine_kexec.c      Fri May 13 13:45:05 2011 +0100
@@ -97,7 +97,7 @@
      };

      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 0c446850d85e -r efa733ebd00d xen/include/asm-x86/apic.h
--- a/xen/include/asm-x86/apic.h        Wed May 11 12:58:04 2011 +0100
+++ b/xen/include/asm-x86/apic.h        Fri May 13 13:45:05 2011 +0100
@@ -182,6 +182,7 @@
  extern void connect_bsp_APIC (void);
  extern void disconnect_bsp_APIC (int virt_wire_setup);
  extern void disable_local_APIC (void);
+extern void crash_disable_local_APIC (void);
  extern int verify_local_APIC (void);
  extern void cache_APIC_registers (void);
  extern void sync_Arb_IDs (void);
diff -r 0c446850d85e -r efa733ebd00d xen/include/asm-x86/hpet.h
--- a/xen/include/asm-x86/hpet.h        Wed May 11 12:58:04 2011 +0100
+++ b/xen/include/asm-x86/hpet.h        Fri May 13 13:45:05 2011 +0100
@@ -73,5 +73,6 @@
  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
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>