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

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

On Mon, Jul 22, 2013 at 01:15:17PM +0100, David Vrabel wrote:
> 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?

There were no commits. Let me rephrase it.
> 
> > 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.

Hm, perhaps add a #ifdef CONFIG for that then.

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

I will snip it a bit.
> 
> > --- 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.

Duh. Let me add a comment saying that for PV domU guests we need the 'local APIC'
functionality on to be able to send NMIs.

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

What version of them would that be ? Anything prior to 3.4 cannot
boot with the Linux pvops.

> 
> 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.

It is used in 'kgdb_roundup_cpus' and if you look in __prepare_ICR
the NMI_VECTOR translates that to APIC_DM_NMI.

Hm, it looks like a bug in the kgdb_roundup_cpus to actually use
that - it should use NMI_VECTOR I think.


> 
> > +		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.

I guess? I was thinking it would be helpfull to know if it does not work.

> 
> > --- 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.

Nah, lets keep it change by change. It makes it easier to figure out
what is actually supported.
> 
> David