[v5,1/3] xen/x86: add nmi continuation framework

Commit Message

Jürgen Groß Nov. 12, 2020, 1:14 p.m. UTC

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

Add a framework to continue processing in normal interrupt context
after leaving NMI processing.

This is done by a high priority interrupt vector triggered via a
self IPI from NMI context, which will then call the continuation
function specified during NMI handling.

Signed-off-by: Juergen Gross <[e-mail]>
Reviewed-by: Jan Beulich <[e-mail]>
---
V5:
- add comment (Jan Beulich)

V4:
- make the framework less generic

V2:
- add prototype for continuation function (Roger Pau Monné)
- switch from softirq to explicit self-IPI (Jan Beulich)

Signed-off-by: Juergen Gross <[e-mail]>
---
 xen/arch/x86/apic.c           | 13 ++++++++++---
 xen/arch/x86/genapic/x2apic.c |  1 +
 xen/arch/x86/smp.c            |  1 +
 xen/arch/x86/traps.c          | 21 +++++++++++++++++++++
 xen/include/asm-x86/nmi.h     | 11 ++++++++++-
 5 files changed, 43 insertions(+), 4 deletions(-)

Comments

Jan Beulich Nov. 12, 2020, 1:41 p.m. UTC | #1

On 12.11.2020 14:14, Juergen Gross wrote:
> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -89,6 +89,7 @@ static unsigned int cpu_mask_to_apicid_x2apic_cluster(const cpumask_t *cpumask)
>  
>  static void send_IPI_self_x2apic(uint8_t vector)
>  {
> +    /* NMI continuation handling relies on using a shorthand here. */
>      apic_wrmsr(APIC_SELF_IPI, vector);
>  }

I'm inclined to drop this hunk again - I did ask for ...

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -163,6 +163,7 @@ void send_IPI_self(int vector)
>  
>  void send_IPI_self_legacy(uint8_t vector)
>  {
> +    /* NMI continuation handling relies on using a shorthand here. */
>      send_IPI_shortcut(APIC_DEST_SELF, vector, APIC_DEST_PHYSICAL);
>  }

... this one only simply because x2APIC doesn't have the same restriction.

Jan

Jürgen Groß Nov. 12, 2020, 2:50 p.m. UTC | #2

On 12.11.20 14:41, Jan Beulich wrote:
> On 12.11.2020 14:14, Juergen Gross wrote:
>> --- a/xen/arch/x86/genapic/x2apic.c
>> +++ b/xen/arch/x86/genapic/x2apic.c
>> @@ -89,6 +89,7 @@ static unsigned int cpu_mask_to_apicid_x2apic_cluster(const cpumask_t *cpumask)
>>   
>>   static void send_IPI_self_x2apic(uint8_t vector)
>>   {
>> +    /* NMI continuation handling relies on using a shorthand here. */
>>       apic_wrmsr(APIC_SELF_IPI, vector);
>>   }
> 
> I'm inclined to drop this hunk again - I did ask for ...
> 
>> --- a/xen/arch/x86/smp.c
>> +++ b/xen/arch/x86/smp.c
>> @@ -163,6 +163,7 @@ void send_IPI_self(int vector)
>>   
>>   void send_IPI_self_legacy(uint8_t vector)
>>   {
>> +    /* NMI continuation handling relies on using a shorthand here. */
>>       send_IPI_shortcut(APIC_DEST_SELF, vector, APIC_DEST_PHYSICAL);
>>   }
> 
> ... this one only simply because x2APIC doesn't have the same restriction.

It would still be bad if the x2APIC variant would e.g. use
send_IPI_mask_x2apic_cluster() due to its usage of
per_cpu(scratch_mask).

Juergen

Patch

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 60627fd6e6..7497ddb5da 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -40,6 +40,7 @@ 
 #include <irq_vectors.h>
 #include <xen/kexec.h>
 #include <asm/guest.h>
+#include <asm/nmi.h>
 #include <asm/time.h>
 
 static bool __read_mostly tdt_enabled;
@@ -1376,16 +1377,22 @@  void spurious_interrupt(struct cpu_user_regs *regs)
 {
     /*
      * Check if this is a vectored interrupt (most likely, as this is probably
-     * a request to dump local CPU state). Vectored interrupts are ACKed;
-     * spurious interrupts are not.
+     * a request to dump local CPU state or to continue NMI handling).
+     * Vectored interrupts are ACKed; spurious interrupts are not.
      */
     if (apic_isr_read(SPURIOUS_APIC_VECTOR)) {
+        bool is_spurious;
+
         ack_APIC_irq();
+        is_spurious = !nmi_check_continuation();
         if (this_cpu(state_dump_pending)) {
             this_cpu(state_dump_pending) = false;
             dump_execstate(regs);
-            return;
+            is_spurious = false;
         }
+
+        if ( !is_spurious )
+            return;
     }
 
     /* see sw-dev-man vol 3, chapter 7.4.13.5 */
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index 077a576a7f..40284b70d1 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -89,6 +89,7 @@  static unsigned int cpu_mask_to_apicid_x2apic_cluster(const cpumask_t *cpumask)
 
 static void send_IPI_self_x2apic(uint8_t vector)
 {
+    /* NMI continuation handling relies on using a shorthand here. */
     apic_wrmsr(APIC_SELF_IPI, vector);
 }
 
diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index 14aa355a6b..eef0f9c6cb 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -163,6 +163,7 @@  void send_IPI_self(int vector)
 
 void send_IPI_self_legacy(uint8_t vector)
 {
+    /* NMI continuation handling relies on using a shorthand here. */
     send_IPI_shortcut(APIC_DEST_SELF, vector, APIC_DEST_PHYSICAL);
 }
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c27dd4cd43..5cbaa49031 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -79,6 +79,7 @@ 
 #include <public/hvm/params.h>
 #include <asm/cpuid.h>
 #include <xsm/xsm.h>
+#include <asm/mach-default/irq_vectors.h>
 #include <asm/pv/traps.h>
 #include <asm/pv/mm.h>
 
@@ -1800,6 +1801,26 @@  void unset_nmi_callback(void)
     nmi_callback = dummy_nmi_callback;
 }
 
+bool nmi_check_continuation(void)
+{
+    bool ret = false;
+
+    return ret;
+}
+
+void trigger_nmi_continuation(void)
+{
+    /*
+     * Issue a self-IPI. Handling is done in spurious_interrupt().
+     * NMI could have happened in IPI sequence, so wait for ICR being idle
+     * again before leaving NMI handler.
+     * This relies on self-IPI using a simple shorthand, thus avoiding any
+     * use of locking or percpu cpumasks.
+     */
+    send_IPI_self(SPURIOUS_APIC_VECTOR);
+    apic_wait_icr_idle();
+}
+
 void do_device_not_available(struct cpu_user_regs *regs)
 {
 #ifdef CONFIG_PV
diff --git a/xen/include/asm-x86/nmi.h b/xen/include/asm-x86/nmi.h
index a288f02a50..9a5da14162 100644
--- a/xen/include/asm-x86/nmi.h
+++ b/xen/include/asm-x86/nmi.h
@@ -33,5 +33,14 @@  nmi_callback_t *set_nmi_callback(nmi_callback_t *callback);
 void unset_nmi_callback(void);
 
 DECLARE_PER_CPU(unsigned int, nmi_count);
- 
+
+/**
+ * trigger_nmi_continuation
+ *
+ * Schedule continuation to be started in interrupt context after NMI handling.
+ */
+void trigger_nmi_continuation(void);
+
+/* Check for NMI continuation pending. */
+bool nmi_check_continuation(void);
 #endif /* ASM_NMI_H */