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