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

[David Vrabel <david.vrabel@citrix.com>]

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