On Wed, May 18, 2011 at 09:48:36PM +0100, Andrew Cooper wrote:
>
>
> On 18/05/11 19:49, Konrad Rzeszutek Wilk wrote:
> >On Wed, May 18, 2011 at 07:08:16PM +0100, Andrew Cooper wrote:
> >>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?
> 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.
>
> >>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<andrew.cooper3@xxxxxxxxxx>
> >>
> >>diff -r e80b5280fe2f -r aaf44d1a903d xen/arch/x86/crash.c
> >>--- a/xen/arch/x86/crash.c Wed May 18 19:00:13 2011 +0100
> >>+++ b/xen/arch/x86/crash.c Wed May 18 19:00:13 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,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?
> 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?
> >>
> >> 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(¤t_cpu_data, X86_FEATURE_X2APIC)
> >>+&& (msr_contents& MSR_IA32_APICBASE_EXTD) )
> >>+ 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.
Yikes. Please do explain this in the code right there were you
remove the local_irq_enable..
> >> }
> >>
> >> void machine_crash_shutdown(void)
> >> {
> >> crash_xen_info_t *info;
> >>+ const struct iommu_ops * ops;
> >>
> >> nmi_shootdown_cpus();
> >>
> >>+ /* 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.
> >>+
> >>+ /* 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.
> >>+ */
> >>+ 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-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home