[PATCH v2 0/2] xen/x86: implement NMI continuation as softirq

[PATCH v2 0/2] xen/x86: implement NMI continuation as softirq

[Juergen Gross]

Move sending of a virq event for oprofile to the local vcpu from NMI
to softirq context.

This has been tested with a small test patch using the continuation
framework of patch 1 for all NMIs and doing a print to console in
the continuation handler.

Version 1 of this small series was sent to the security list before.

Juergen Gross (2):
  xen/x86: add nmi continuation framework
  xen/oprofile: use set_nmi_continuation() for sending virq to guest

 xen/arch/x86/oprofile/nmi_int.c |  9 +++++++-
 xen/arch/x86/traps.c            | 37 +++++++++++++++++++++++++++++++++
 xen/include/asm-x86/nmi.h       |  8 ++++++-
 xen/include/xen/softirq.h       |  5 ++++-
 4 files changed, 56 insertions(+), 3 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] xen/x86: add nmi continuation framework

[Juergen Gross]

Actions in NMI context are rather limited as e.g. locking is rather
fragile.

Add a generic framework to continue processing in softirq context after
leaving NMI processing. This is working for NMIs happening in guest
context as NMI exit handling will issue an IPI to itself in case a
softirq is pending, resulting in the continuation running before the
guest gets control again.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/traps.c      | 37 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/nmi.h |  8 +++++++-
 xen/include/xen/softirq.h |  5 ++++-
 3 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index bc5b8f8ea3..f433fe5acb 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1799,6 +1799,42 @@ void unset_nmi_callback(void)
     nmi_callback = dummy_nmi_callback;
 }
 
+static DEFINE_PER_CPU(void (*)(void *), nmi_cont_func);
+static DEFINE_PER_CPU(void *, nmi_cont_par);
+
+static void nmi_cont_softirq(void)
+{
+    unsigned int cpu = smp_processor_id();
+    void (*func)(void *par) = per_cpu(nmi_cont_func, cpu);
+    void *par = per_cpu(nmi_cont_par, cpu);
+
+    /* Reads must be done before following write (local cpu ordering only). */
+    barrier();
+
+    per_cpu(nmi_cont_func, cpu) = NULL;
+
+    if ( func )
+        func(par);
+}
+
+int set_nmi_continuation(void (*func)(void *par), void *par)
+{
+    unsigned int cpu = smp_processor_id();
+
+    if ( per_cpu(nmi_cont_func, cpu) )
+    {
+        printk("Trying to set NMI continuation while still one active!\n");
+        return -EBUSY;
+    }
+
+    per_cpu(nmi_cont_func, cpu) = func;
+    per_cpu(nmi_cont_par, cpu) = par;
+
+    raise_softirq(NMI_CONT_SOFTIRQ);
+
+    return 0;
+}
+
 void do_device_not_available(struct cpu_user_regs *regs)
 {
 #ifdef CONFIG_PV
@@ -2132,6 +2168,7 @@ void __init trap_init(void)
 
     cpu_init();
 
+    open_softirq(NMI_CONT_SOFTIRQ, nmi_cont_softirq);
     open_softirq(PCI_SERR_SOFTIRQ, pci_serr_softirq);
 }
 
diff --git a/xen/include/asm-x86/nmi.h b/xen/include/asm-x86/nmi.h
index a288f02a50..da40fb6599 100644
--- a/xen/include/asm-x86/nmi.h
+++ b/xen/include/asm-x86/nmi.h
@@ -33,5 +33,11 @@ nmi_callback_t *set_nmi_callback(nmi_callback_t *callback);
 void unset_nmi_callback(void);
 
 DECLARE_PER_CPU(unsigned int, nmi_count);
- 
+
+/**
+ * set_nmi_continuation
+ *
+ * Schedule a function to be started in softirq context after NMI handling.
+ */
+int set_nmi_continuation(void (*func)(void *par), void *par);
 #endif /* ASM_NMI_H */
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 1f6c4783da..14c744bbf7 100644
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -3,7 +3,10 @@
 
 /* Low-latency softirqs come first in the following list. */
 enum {
-    TIMER_SOFTIRQ = 0,
+#ifdef CONFIG_X86
+    NMI_CONT_SOFTIRQ,
+#endif
+    TIMER_SOFTIRQ,
     RCU_SOFTIRQ,
     SCHED_SLAVE_SOFTIRQ,
     SCHEDULE_SOFTIRQ,
-- 
2.26.2

[PATCH v2 2/2] xen/oprofile: use set_nmi_continuation() for sending virq to guest

[Juergen Gross]

Instead of calling send_guest_vcpu_virq() from NMI context use the
NMI continuation framework for that purpose.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/oprofile/nmi_int.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/oprofile/nmi_int.c b/xen/arch/x86/oprofile/nmi_int.c
index 0f103d80a6..659e31fe19 100644
--- a/xen/arch/x86/oprofile/nmi_int.c
+++ b/xen/arch/x86/oprofile/nmi_int.c
@@ -83,6 +83,13 @@ void passive_domain_destroy(struct vcpu *v)
 		model->free_msr(v);
 }
 
+static void nmi_oprofile_send_virq(void *par)
+{
+	struct vcpu *v = par;
+
+	send_guest_vcpu_virq(v, VIRQ_XENOPROF);
+}
+
 static int nmi_callback(const struct cpu_user_regs *regs, int cpu)
 {
 	int xen_mode, ovf;
@@ -90,7 +97,7 @@ static int nmi_callback(const struct cpu_user_regs *regs, int cpu)
 	ovf = model->check_ctrs(cpu, &cpu_msrs[cpu], regs);
 	xen_mode = ring_0(regs);
 	if ( ovf && is_active(current->domain) && !xen_mode )
-		send_guest_vcpu_virq(current, VIRQ_XENOPROF);
+		set_nmi_continuation(nmi_oprofile_send_virq, current);
 
 	if ( ovf == 2 )
 		current->arch.nmi_pending = true;
-- 
2.26.2

Re: [PATCH v2 1/2] xen/x86: add nmi continuation framework

[Roger Pau Monné]

On Wed, Oct 07, 2020 at 03:30:10PM +0200, Juergen Gross wrote:
> Actions in NMI context are rather limited as e.g. locking is rather
> fragile.
> 
> Add a generic framework to continue processing in softirq context after
> leaving NMI processing. This is working for NMIs happening in guest
> context as NMI exit handling will issue an IPI to itself in case a
> softirq is pending, resulting in the continuation running before the
> guest gets control again.

There's already kind of a nmi callback framework using nmi_callback.
I assume that moving this existing callback code to be executed in
softirq context is not suitable because that would be past the
execution of an iret instruction?

Might be worth mentioning in the commit message.

> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/arch/x86/traps.c      | 37 +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/nmi.h |  8 +++++++-
>  xen/include/xen/softirq.h |  5 ++++-
>  3 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index bc5b8f8ea3..f433fe5acb 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1799,6 +1799,42 @@ void unset_nmi_callback(void)
>      nmi_callback = dummy_nmi_callback;
>  }
>  
> +static DEFINE_PER_CPU(void (*)(void *), nmi_cont_func);
> +static DEFINE_PER_CPU(void *, nmi_cont_par);
> +
> +static void nmi_cont_softirq(void)
> +{
> +    unsigned int cpu = smp_processor_id();
> +    void (*func)(void *par) = per_cpu(nmi_cont_func, cpu);
> +    void *par = per_cpu(nmi_cont_par, cpu);

I think you can use this_cpu here and below, and avoid having to worry
about the processor id at all? Also less likely to mess up and assign
a NMI callback to a wrong CPU.

> +
> +    /* Reads must be done before following write (local cpu ordering only). */
> +    barrier();

Is this added because of the usage of RELOC_HIDE by per_cpu?

> +
> +    per_cpu(nmi_cont_func, cpu) = NULL;

Since we are using RELOC_HIDE, doesn't it imply that the compiler
cannot reorder this, since it has no idea what variable we are
accessing?

> +
> +    if ( func )
> +        func(par);
> +}
> +
> +int set_nmi_continuation(void (*func)(void *par), void *par)
> +{
> +    unsigned int cpu = smp_processor_id();
> +
> +    if ( per_cpu(nmi_cont_func, cpu) )
> +    {
> +        printk("Trying to set NMI continuation while still one active!\n");
> +        return -EBUSY;
> +    }
> +
> +    per_cpu(nmi_cont_func, cpu) = func;
> +    per_cpu(nmi_cont_par, cpu) = par;
> +
> +    raise_softirq(NMI_CONT_SOFTIRQ);
> +
> +    return 0;
> +}
> +
>  void do_device_not_available(struct cpu_user_regs *regs)
>  {
>  #ifdef CONFIG_PV
> @@ -2132,6 +2168,7 @@ void __init trap_init(void)
>  
>      cpu_init();
>  
> +    open_softirq(NMI_CONT_SOFTIRQ, nmi_cont_softirq);
>      open_softirq(PCI_SERR_SOFTIRQ, pci_serr_softirq);
>  }
>  
> diff --git a/xen/include/asm-x86/nmi.h b/xen/include/asm-x86/nmi.h
> index a288f02a50..da40fb6599 100644
> --- a/xen/include/asm-x86/nmi.h
> +++ b/xen/include/asm-x86/nmi.h
> @@ -33,5 +33,11 @@ nmi_callback_t *set_nmi_callback(nmi_callback_t *callback);
>  void unset_nmi_callback(void);
>  
>  DECLARE_PER_CPU(unsigned int, nmi_count);
> - 
> +
> +/**
> + * set_nmi_continuation
> + *
> + * Schedule a function to be started in softirq context after NMI handling.
> + */
> +int set_nmi_continuation(void (*func)(void *par), void *par);

I would introduce a type for the nmi callback, as I think it's easier
than having to retype the prototype everywhere:

typedef void nmi_continuation_t(void *);

Thanks, Roger.

Re: [PATCH v2 1/2] xen/x86: add nmi continuation framework

[Jürgen Groß]

On 08.10.20 10:43, Roger Pau Monné wrote:
> On Wed, Oct 07, 2020 at 03:30:10PM +0200, Juergen Gross wrote:
>> Actions in NMI context are rather limited as e.g. locking is rather
>> fragile.
>>
>> Add a generic framework to continue processing in softirq context after
>> leaving NMI processing. This is working for NMIs happening in guest
>> context as NMI exit handling will issue an IPI to itself in case a
>> softirq is pending, resulting in the continuation running before the
>> guest gets control again.
> 
> There's already kind of a nmi callback framework using nmi_callback.
> I assume that moving this existing callback code to be executed in
> softirq context is not suitable because that would be past the
> execution of an iret instruction?
> 
> Might be worth mentioning in the commit message.
> 
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/arch/x86/traps.c      | 37 +++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-x86/nmi.h |  8 +++++++-
>>   xen/include/xen/softirq.h |  5 ++++-
>>   3 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index bc5b8f8ea3..f433fe5acb 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1799,6 +1799,42 @@ void unset_nmi_callback(void)
>>       nmi_callback = dummy_nmi_callback;
>>   }
>>   
>> +static DEFINE_PER_CPU(void (*)(void *), nmi_cont_func);
>> +static DEFINE_PER_CPU(void *, nmi_cont_par);
>> +
>> +static void nmi_cont_softirq(void)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +    void (*func)(void *par) = per_cpu(nmi_cont_func, cpu);
>> +    void *par = per_cpu(nmi_cont_par, cpu);
> 
> I think you can use this_cpu here and below, and avoid having to worry
> about the processor id at all? Also less likely to mess up and assign
> a NMI callback to a wrong CPU.

this_cpu() and smp_processor_id() are both based on get_cpu_info().
So multiple uses of this_cpu() are less efficient than per_cpu() with
cpu in a local variable (regarding code-size and speed, at least I think
so).

> 
>> +
>> +    /* Reads must be done before following write (local cpu ordering only). */
>> +    barrier();
> 
> Is this added because of the usage of RELOC_HIDE by per_cpu?

This is added because reordering of loads by the compiler must be
avoided as a NMI using those fields again might happen anytime.

> 
>> +
>> +    per_cpu(nmi_cont_func, cpu) = NULL;
> 
> Since we are using RELOC_HIDE, doesn't it imply that the compiler
> cannot reorder this, since it has no idea what variable we are
> accessing?

I'd rather be safe here than relying on per_cpu() internals.

> 
>> +
>> +    if ( func )
>> +        func(par);
>> +}
>> +
>> +int set_nmi_continuation(void (*func)(void *par), void *par)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +
>> +    if ( per_cpu(nmi_cont_func, cpu) )
>> +    {
>> +        printk("Trying to set NMI continuation while still one active!\n");
>> +        return -EBUSY;
>> +    }
>> +
>> +    per_cpu(nmi_cont_func, cpu) = func;
>> +    per_cpu(nmi_cont_par, cpu) = par;
>> +
>> +    raise_softirq(NMI_CONT_SOFTIRQ);
>> +
>> +    return 0;
>> +}
>> +
>>   void do_device_not_available(struct cpu_user_regs *regs)
>>   {
>>   #ifdef CONFIG_PV
>> @@ -2132,6 +2168,7 @@ void __init trap_init(void)
>>   
>>       cpu_init();
>>   
>> +    open_softirq(NMI_CONT_SOFTIRQ, nmi_cont_softirq);
>>       open_softirq(PCI_SERR_SOFTIRQ, pci_serr_softirq);
>>   }
>>   
>> diff --git a/xen/include/asm-x86/nmi.h b/xen/include/asm-x86/nmi.h
>> index a288f02a50..da40fb6599 100644
>> --- a/xen/include/asm-x86/nmi.h
>> +++ b/xen/include/asm-x86/nmi.h
>> @@ -33,5 +33,11 @@ nmi_callback_t *set_nmi_callback(nmi_callback_t *callback);
>>   void unset_nmi_callback(void);
>>   
>>   DECLARE_PER_CPU(unsigned int, nmi_count);
>> -
>> +
>> +/**
>> + * set_nmi_continuation
>> + *
>> + * Schedule a function to be started in softirq context after NMI handling.
>> + */
>> +int set_nmi_continuation(void (*func)(void *par), void *par);
> 
> I would introduce a type for the nmi callback, as I think it's easier
> than having to retype the prototype everywhere:
> 
> typedef void nmi_continuation_t(void *);

Fine with me.


Juergen

Re: [PATCH v2 0/2] xen/x86: implement NMI continuation as softirq

[Roger Pau Monné]

On Wed, Oct 07, 2020 at 03:30:09PM +0200, Juergen Gross wrote:
> Move sending of a virq event for oprofile to the local vcpu from NMI
> to softirq context.
> 
> This has been tested with a small test patch using the continuation
> framework of patch 1 for all NMIs and doing a print to console in
> the continuation handler.
> 
> Version 1 of this small series was sent to the security list before.
> 
> Juergen Gross (2):
>   xen/x86: add nmi continuation framework
>   xen/oprofile: use set_nmi_continuation() for sending virq to guest

Apart from the comments in patch 1, I think this is a fine approach if
it allows us to restore to the previous state of the event lock.

I think we should be expecting a v3 with the nmi callback prototype?

Thanks, Roger.

Re: [PATCH v2 0/2] xen/x86: implement NMI continuation as softirq

[Jürgen Groß]

On 15.10.20 12:49, Roger Pau Monné wrote:
> On Wed, Oct 07, 2020 at 03:30:09PM +0200, Juergen Gross wrote:
>> Move sending of a virq event for oprofile to the local vcpu from NMI
>> to softirq context.
>>
>> This has been tested with a small test patch using the continuation
>> framework of patch 1 for all NMIs and doing a print to console in
>> the continuation handler.
>>
>> Version 1 of this small series was sent to the security list before.
>>
>> Juergen Gross (2):
>>    xen/x86: add nmi continuation framework
>>    xen/oprofile: use set_nmi_continuation() for sending virq to guest
> 
> Apart from the comments in patch 1, I think this is a fine approach if
> it allows us to restore to the previous state of the event lock.

This will not be enough to do that, but it is clearly removing a
potential deadlock.

> I think we should be expecting a v3 with the nmi callback prototype?

And using an IPI instead of a softirq, yes.


Juergen