[v3,2/5] x86: track when in #NMI context
diff mbox series

Message ID 20200224104645.96381-3-roger.pau@citrix.com
State New
Headers show
Series
  • x86: fixes/improvements for scratch cpumask
Related show

Commit Message

Roger Pau Monné Feb. 24, 2020, 10:46 a.m. UTC
Add helpers to track when running in #NMI context. This is modeled
after the in_irq helpers.

The SDM states that no #NMI can be delivered while handling a #NMI
until the processor has executed an iret instruction. It's possible
however that another fault is received while handling the #NMI (a #MC
for example), and thus the iret from that fault would allow further
#NMIs to be injected while still processing the previous one, and
hence an integer is needed in order to keep track of in service #NMIs.

While there move nmi_count() into a x86 specific header and drop the
leading underscores from __nmi_count field.

Note that there are no users of in_nmi() introduced by the change,
further users will be added by followup changes.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Use an integer instead of a boolean to keep track of in service
   #NMIs.
 - Move nmi_count into x86 specific header.
 - Drop leading underscores from __nmi_count field.
---
 xen/arch/x86/traps.c          | 6 ++++++
 xen/include/asm-x86/hardirq.h | 7 ++++++-
 xen/include/xen/irq_cpustat.h | 1 -
 3 files changed, 12 insertions(+), 2 deletions(-)

Comments

Jan Beulich Feb. 25, 2020, 4:49 p.m. UTC | #1
On 24.02.2020 11:46, Roger Pau Monne wrote:
> Add helpers to track when running in #NMI context. This is modeled
> after the in_irq helpers.
> 
> The SDM states that no #NMI can be delivered while handling a #NMI
> until the processor has executed an iret instruction. It's possible
> however that another fault is received while handling the #NMI (a #MC
> for example), and thus the iret from that fault would allow further
> #NMIs to be injected while still processing the previous one, and
> hence an integer is needed in order to keep track of in service #NMIs.

While I agree that this needs taking care of, I'm afraid in_nmi()
is becoming ambiguous because of this - you give it the meaning
"we're handling an NMI", while one could also assume it to mean
"we're in NMI context, i.e. further NMIs are not possible". IOW
I think we want to consider using another name, despite this
getting things less nicely aligned with in_irq(). Perhaps
in_nmi_handler()?

> While there move nmi_count() into a x86 specific header and drop the
> leading underscores from __nmi_count field.

I notice you re-use the fields that I suggested to be removed by
the prior patch. I think logically they should indeed be removed
there, and a new field and a new macro be introduced here.

Jan
Jan Beulich Feb. 25, 2020, 4:51 p.m. UTC | #2
On 24.02.2020 11:46, Roger Pau Monne wrote:
> @@ -18,6 +18,11 @@ typedef struct {
>  #define irq_enter()	(local_irq_count(smp_processor_id())++)
>  #define irq_exit()	(local_irq_count(smp_processor_id())--)
>  
> +#define nmi_count(cpu)	__IRQ_STAT((cpu), nmi_count)

Oh, btw (noticed only while already looking at the next
patch) - no need for the parentheses around "cpu" afaict.

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 3dbc66bb64..f4f2c13ae9 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1692,9 +1692,13 @@  void do_nmi(const struct cpu_user_regs *regs)
     bool handle_unknown = false;
 
     this_cpu(nmi_count)++;
+    nmi_enter();
 
     if ( nmi_callback(regs, cpu) )
+    {
+        nmi_exit();
         return;
+    }
 
     /*
      * Accessing port 0x61 may trap to SMM which has been actually
@@ -1720,6 +1724,8 @@  void do_nmi(const struct cpu_user_regs *regs)
         if ( !(reason & 0xc0) && handle_unknown )
             unknown_nmi_error(regs, reason);
     }
+
+    nmi_exit();
 }
 
 nmi_callback_t *set_nmi_callback(nmi_callback_t *callback)
diff --git a/xen/include/asm-x86/hardirq.h b/xen/include/asm-x86/hardirq.h
index 34e1b49260..6ccce75881 100644
--- a/xen/include/asm-x86/hardirq.h
+++ b/xen/include/asm-x86/hardirq.h
@@ -7,7 +7,7 @@ 
 typedef struct {
 	unsigned int __softirq_pending;
 	unsigned int __local_irq_count;
-	unsigned int __nmi_count;
+	unsigned int nmi_count;
 	bool_t __mwait_wakeup;
 } __cacheline_aligned irq_cpustat_t;
 
@@ -18,6 +18,11 @@  typedef struct {
 #define irq_enter()	(local_irq_count(smp_processor_id())++)
 #define irq_exit()	(local_irq_count(smp_processor_id())--)
 
+#define nmi_count(cpu)	__IRQ_STAT((cpu), nmi_count)
+#define in_nmi()	(nmi_count(smp_processor_id()) != 0)
+#define nmi_enter()	(nmi_count(smp_processor_id())++)
+#define nmi_exit()	(nmi_count(smp_processor_id())--)
+
 void ack_bad_irq(unsigned int irq);
 
 extern void apic_intr_init(void);
diff --git a/xen/include/xen/irq_cpustat.h b/xen/include/xen/irq_cpustat.h
index 73629f6ec8..b9629f25c2 100644
--- a/xen/include/xen/irq_cpustat.h
+++ b/xen/include/xen/irq_cpustat.h
@@ -24,7 +24,6 @@  extern irq_cpustat_t irq_stat[];
   /* arch independent irq_stat fields */
 #define softirq_pending(cpu)	__IRQ_STAT((cpu), __softirq_pending)
 #define local_irq_count(cpu)	__IRQ_STAT((cpu), __local_irq_count)
-#define nmi_count(cpu)		__IRQ_STAT((cpu), __nmi_count)
 #define mwait_wakeup(cpu)	__IRQ_STAT((cpu), __mwait_wakeup)
 
 #endif	/* __irq_cpustat_h */