| Message ID | 20200224104645.96381-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: fixes/improvements for scratch cpumask | expand |
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
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
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 */
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(-)