Re: [PATCH 3 of 3] kexec: disable iommu jumping into the kdump kernel

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

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@citrix.com>
> >>
> >>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(&current_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?