Re: [Xen-devel] [PATCH] xen: Support 64-bit PV guest receiving NMIs

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 19/07/13 16:51, Konrad Rzeszutek Wilk wrote:
> Zhenzhong Duan sent a patch that adds some of this functionality
> but this code adds the remaining pieces.

What's this actually referring to?  Which specific commits?

> The kernel has the
> logic to handle Xen-type-exceptions using the paravirt interface
> in the assembler code (see PARAVIRT_ADJUST_EXCEPTION_FRAME -
> pv_irq_ops.adjust_exception_frame and and INTERRUPT_RETURN -
> pv_cpu_ops.iret).
> 
> That means the nmi handler (and other exception handlers) use
> the hypervisor iret.
> 
> The other changes that would be neccessary for this would
> be to translate the NMI_VECTOR to one of the entries on the
> ipi_vector and make xen_send_IPI_mask_allbutself use different
> events.
> 
> Fortunately for us commit 1db01b4903639fcfaec213701a494fe3fb2c490b
> (xen: Clean up apic ipi interface) implemented this and we piggyback
> on the cleanup such that the apic IPI interface will pass the right
> vector value for NMI.
> 
> With this patch we can trigger NMIs within a PV guest (only tested
> x86_64).

I think you also need to test with 32-bit guests.  The VCPUOP_nmi
hypercall does not appear to be conditional on only 64-bit guest.

> 
> SysRq : Show backtrace of all active CPUs
> sending NMI to all CPUs:
> NMI backtrace for cpu 2

Is this clutter really necessary in a commit message?

> --- a/arch/x86/include/asm/xen/events.h
> +++ b/arch/x86/include/asm/xen/events.h
> @@ -7,6 +7,7 @@ enum ipi_vector {
>       XEN_CALL_FUNCTION_SINGLE_VECTOR,
>       XEN_SPIN_UNLOCK_VECTOR,
>       XEN_IRQ_WORK_VECTOR,
> +     XEN_NMI_VECTOR,
>  
>       XEN_NR_IPIS,
>  };
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 2fa02bc..231382a 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -427,8 +427,7 @@ static void __init xen_init_cpuid_mask(void)
>  
>       if (!xen_initial_domain())
>               cpuid_leaf1_edx_mask &=
> -                     ~((1 << X86_FEATURE_APIC) |  /* disable local APIC */
> -                       (1 << X86_FEATURE_ACPI));  /* disable ACPI */
> +                     ~((1 << X86_FEATURE_ACPI));  /* disable ACPI */
>  
>       cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_X2APIC % 32));

What's this hunk for?  It doesn't appear to be explained in the commit
message.

> @@ -735,8 +734,7 @@ static int cvt_gate_to_trap(int vector, const gate_desc 
> *val,
>               addr = (unsigned long)xen_int3;
>       else if (addr == (unsigned long)stack_segment)
>               addr = (unsigned long)xen_stack_segment;
> -     else if (addr == (unsigned long)double_fault ||
> -              addr == (unsigned long)nmi) {
> +     else if (addr == (unsigned long)double_fault) {
>               /* Don't need to handle these */
>               return 0;
>  #ifdef CONFIG_X86_MCE
> @@ -747,7 +745,12 @@ static int cvt_gate_to_trap(int vector, const gate_desc 
> *val,
>                */
>               ;
>  #endif
> -     } else {
> +     } else if (addr == (unsigned long)nmi)
> +             /*
> +              * Use the native version as well.
> +              */
> +             ;
> +     else {
>               /* Some other trap using IST? */
>               if (WARN_ON(val->ist != 0))
>                       return 0;
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index 94eac5c..f78877c 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -33,6 +33,9 @@
>  /* These are code, but not functions.  Defined in entry.S */
>  extern const char xen_hypervisor_callback[];
>  extern const char xen_failsafe_callback[];
> +#ifdef CONFIG_X86_64
> +extern const char nmi[];
> +#endif
>  extern void xen_sysenter_target(void);
>  extern void xen_syscall_target(void);
>  extern void xen_syscall32_target(void);
> @@ -525,7 +528,13 @@ void __cpuinit xen_enable_syscall(void)
>       }
>  #endif /* CONFIG_X86_64 */
>  }
> -
> +void __cpuinit xen_enable_nmi(void)
> +{
> +#ifdef CONFIG_X86_64
> +     if (register_callback(CALLBACKTYPE_nmi, nmi))
> +             BUG();
> +#endif
> +}
>  void __init xen_arch_setup(void)

Do we care about hypervisors that don't support this?

Also, missing blank line.

> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -572,6 +572,10 @@ static inline int xen_map_vector(int vector)
>       case IRQ_WORK_VECTOR:
>               xen_vector = XEN_IRQ_WORK_VECTOR;
>               break;
> +     case NMI_VECTOR:
> +     case APIC_DM_NMI:

Not clear what this APIC_DM_NMI case is for.

> +             xen_vector = XEN_NMI_VECTOR;
> +             break;
>       default:
>               xen_vector = -1;
>               printk(KERN_ERR "xen: vector 0x%x is not implemented\n",
> @@ -659,6 +663,7 @@ static irqreturn_t xen_irq_work_interrupt(int irq, void 
> *dev_id)
>       return IRQ_HANDLED;
>  }
>  
> +

Stray blank line added.

>  static const struct smp_ops xen_smp_ops __initconst = {
>       .smp_prepare_boot_cpu = xen_smp_prepare_boot_cpu,
>       .smp_prepare_cpus = xen_smp_prepare_cpus,
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index a58ac43..419cc44 100644

> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -1213,6 +1214,16 @@ EXPORT_SYMBOL_GPL(evtchn_put);
>  void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector)
>  {
>       int irq = per_cpu(ipi_to_irq, cpu)[vector];
> +
> +     /*
> +      * In which the IRQ will be -1.
> +      */
> +     if (unlikely(vector == XEN_NMI_VECTOR)) {
> +             int rc =  HYPERVISOR_vcpu_op(VCPUOP_send_nmi, cpu, NULL);
> +             if (rc < 0)
> +                     printk(KERN_WARNING "Sending nmi to CPU%d failed 
> (rc:%d)\n", cpu, rc);
> +             return;
> +     }

Move the assignment of irq after this block, then you can drop the
(unhelpful) comment.

> --- a/include/xen/interface/vcpu.h
> +++ b/include/xen/interface/vcpu.h
> @@ -170,4 +170,6 @@ struct vcpu_register_vcpu_info {
>  };
>  DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_vcpu_info);
>  
> +/* Send an NMI to the specified VCPU. @extra_arg == NULL. */
> +#define VCPUOP_send_nmi             11
>  #endif /* __XEN_PUBLIC_VCPU_H__ */

Would it be better to get this change with a full sync of the header as
a separate patch.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel