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

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

> +     */
> +    iommu_disable_x2apic_IR();

Can't that function be done in the suspend code of the IOMMU?
> +
> +
>      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
> --- a/xen/arch/x86/hpet.c	Wed May 18 19:00:13 2011 +0100
> +++ b/xen/arch/x86/hpet.c	Wed May 18 19:00:13 2011 +0100
> @@ -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
> --- a/xen/arch/x86/machine_kexec.c	Wed May 18 19:00:13 2011 +0100
> +++ b/xen/arch/x86/machine_kexec.c	Wed May 18 19:00:13 2011 +0100
> @@ -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
> --- a/xen/include/asm-x86/hpet.h	Wed May 18 19:00:13 2011 +0100
> +++ b/xen/include/asm-x86/hpet.h	Wed May 18 19:00:13 2011 +0100
> @@ -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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel