Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/3] xen/x86: implement NMI continuation
@ 2020-10-16  8:53 Juergen Gross
  2020-10-16  8:53 ` [PATCH v3 1/3] xen/x86: add nmi continuation framework Juergen Gross
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Juergen Gross @ 2020-10-16  8:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu

Move sending of a virq event for oprofile to the local vcpu from NMI
to normal interrupt 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.

Changes in V3:
- switched to self-IPI instead of softirq
- added patch 3

Juergen Gross (3):
  xen/x86: add nmi continuation framework
  xen/oprofile: use set_nmi_continuation() for sending virq to guest
  xen/x86: issue pci_serr error message via NMI continuation

 xen/arch/x86/apic.c             | 13 +++++--
 xen/arch/x86/oprofile/nmi_int.c |  9 ++++-
 xen/arch/x86/traps.c            | 61 +++++++++++++++++++++++++++++----
 xen/include/asm-x86/nmi.h       | 13 ++++++-
 xen/include/asm-x86/softirq.h   |  5 ++-
 5 files changed, 87 insertions(+), 14 deletions(-)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 1/3] xen/x86: add nmi continuation framework
  2020-10-16  8:53 [PATCH v3 0/3] xen/x86: implement NMI continuation Juergen Gross
@ 2020-10-16  8:53 ` Juergen Gross
  2020-10-20 13:33   ` Jan Beulich
  2020-10-16  8:53 ` [PATCH v3 2/3] xen/oprofile: use set_nmi_continuation() for sending virq to guest Juergen Gross
  2020-10-16  8:53 ` [PATCH v3 3/3] xen/x86: issue pci_serr error message via NMI continuation Juergen Gross
  2 siblings, 1 reply; 5+ messages in thread
From: Juergen Gross @ 2020-10-16  8:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu

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

Add a generic 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 <jgross@suse.com>
---
V2:
- add prototype for continuation function (Roger Pau Monné)
- switch from softirq to explicit self-IPI (Jan Beulich)
---
 xen/arch/x86/apic.c       | 13 +++++++---
 xen/arch/x86/traps.c      | 52 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/nmi.h | 13 +++++++++-
 3 files changed, 74 insertions(+), 4 deletions(-)

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/traps.c b/xen/arch/x86/traps.c
index bc5b8f8ea3..6f4db9d549 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>
 
@@ -1799,6 +1800,57 @@ void unset_nmi_callback(void)
     nmi_callback = dummy_nmi_callback;
 }
 
+static DEFINE_PER_CPU(nmi_contfunc_t *, nmi_cont_func);
+static DEFINE_PER_CPU(void *, nmi_cont_arg);
+static DEFINE_PER_CPU(bool, nmi_cont_busy);
+
+bool nmi_check_continuation(void)
+{
+    unsigned int cpu = smp_processor_id();
+    nmi_contfunc_t *func = per_cpu(nmi_cont_func, cpu);
+    void *arg = per_cpu(nmi_cont_arg, cpu);
+
+    if ( per_cpu(nmi_cont_busy, cpu) )
+    {
+        per_cpu(nmi_cont_busy, cpu) = false;
+        printk("Trying to set NMI continuation while still one active!\n");
+    }
+
+    /* Reads must be done before following write (local cpu ordering only). */
+    barrier();
+
+    per_cpu(nmi_cont_func, cpu) = NULL;
+
+    if ( func )
+        func(arg);
+
+    return func;
+}
+
+int set_nmi_continuation(nmi_contfunc_t *func, void *arg)
+{
+    unsigned int cpu = smp_processor_id();
+
+    if ( per_cpu(nmi_cont_func, cpu) )
+    {
+        per_cpu(nmi_cont_busy, cpu) = true;
+        return -EBUSY;
+    }
+
+    per_cpu(nmi_cont_func, cpu) = func;
+    per_cpu(nmi_cont_arg, cpu) = arg;
+
+    /*
+     * 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.
+     */
+    send_IPI_self(SPURIOUS_APIC_VECTOR);
+    apic_wait_icr_idle();
+
+    return 0;
+}
+
 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..68db75b1ed 100644
--- a/xen/include/asm-x86/nmi.h
+++ b/xen/include/asm-x86/nmi.h
@@ -33,5 +33,16 @@ nmi_callback_t *set_nmi_callback(nmi_callback_t *callback);
 void unset_nmi_callback(void);
 
 DECLARE_PER_CPU(unsigned int, nmi_count);
- 
+
+typedef void nmi_contfunc_t(void *arg);
+
+/**
+ * set_nmi_continuation
+ *
+ * Schedule a function to be started in interrupt context after NMI handling.
+ */
+int set_nmi_continuation(nmi_contfunc_t *func, void *arg);
+
+/* Check for NMI continuation pending. */
+bool nmi_check_continuation(void);
 #endif /* ASM_NMI_H */
-- 
2.26.2



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 2/3] xen/oprofile: use set_nmi_continuation() for sending virq to guest
  2020-10-16  8:53 [PATCH v3 0/3] xen/x86: implement NMI continuation Juergen Gross
  2020-10-16  8:53 ` [PATCH v3 1/3] xen/x86: add nmi continuation framework Juergen Gross
@ 2020-10-16  8:53 ` Juergen Gross
  2020-10-16  8:53 ` [PATCH v3 3/3] xen/x86: issue pci_serr error message via NMI continuation Juergen Gross
  2 siblings, 0 replies; 5+ messages in thread
From: Juergen Gross @ 2020-10-16  8:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu

Instead of calling send_guest_vcpu_virq() from NMI context use the
NMI continuation framework for that purpose. This avoids taking locks
in NMI mode.

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..825f0aeef0 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 *arg)
+{
+	struct vcpu *v = arg;
+
+	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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 3/3] xen/x86: issue pci_serr error message via NMI continuation
  2020-10-16  8:53 [PATCH v3 0/3] xen/x86: implement NMI continuation Juergen Gross
  2020-10-16  8:53 ` [PATCH v3 1/3] xen/x86: add nmi continuation framework Juergen Gross
  2020-10-16  8:53 ` [PATCH v3 2/3] xen/oprofile: use set_nmi_continuation() for sending virq to guest Juergen Gross
@ 2020-10-16  8:53 ` Juergen Gross
  2 siblings, 0 replies; 5+ messages in thread
From: Juergen Gross @ 2020-10-16  8:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu

Instead of using a softirq pci_serr_error() can use NMI continuation
for issuing an error message.

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

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6f4db9d549..7a68ac40be 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1659,7 +1659,7 @@ void do_general_protection(struct cpu_user_regs *regs)
     panic("GENERAL PROTECTION FAULT\n[error_code=%04x]\n", regs->error_code);
 }
 
-static void pci_serr_softirq(void)
+static void pci_serr_nmicont(void *arg)
 {
     printk("\n\nNMI - PCI system error (SERR)\n");
     outb(inb(0x61) & 0x0b, 0x61); /* re-enable the PCI SERR error line. */
@@ -1687,9 +1687,8 @@ static void pci_serr_error(const struct cpu_user_regs *regs)
         nmi_hwdom_report(_XEN_NMIREASON_pci_serr);
         /* fallthrough */
     case 'i': /* 'ignore' */
-        /* Would like to print a diagnostic here but can't call printk()
-           from NMI context -- raise a softirq instead. */
-        raise_softirq(PCI_SERR_SOFTIRQ);
+        /* Issue error message in NMI continuation. */
+        set_nmi_continuation(pci_serr_nmicont, NULL);
         break;
     default:  /* 'fatal' */
         console_force_unlock();
@@ -2183,8 +2182,6 @@ void __init trap_init(void)
     percpu_traps_init();
 
     cpu_init();
-
-    open_softirq(PCI_SERR_SOFTIRQ, pci_serr_softirq);
 }
 
 void activate_debugregs(const struct vcpu *curr)
diff --git a/xen/include/asm-x86/softirq.h b/xen/include/asm-x86/softirq.h
index 0b7a77f11f..415ee866c7 100644
--- a/xen/include/asm-x86/softirq.h
+++ b/xen/include/asm-x86/softirq.h
@@ -6,9 +6,8 @@
 #define VCPU_KICK_SOFTIRQ      (NR_COMMON_SOFTIRQS + 2)
 
 #define MACHINE_CHECK_SOFTIRQ  (NR_COMMON_SOFTIRQS + 3)
-#define PCI_SERR_SOFTIRQ       (NR_COMMON_SOFTIRQS + 4)
-#define HVM_DPCI_SOFTIRQ       (NR_COMMON_SOFTIRQS + 5)
-#define NR_ARCH_SOFTIRQS       6
+#define HVM_DPCI_SOFTIRQ       (NR_COMMON_SOFTIRQS + 4)
+#define NR_ARCH_SOFTIRQS       5
 
 bool arch_skip_send_event_check(unsigned int cpu);
 
-- 
2.26.2



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/3] xen/x86: add nmi continuation framework
  2020-10-16  8:53 ` [PATCH v3 1/3] xen/x86: add nmi continuation framework Juergen Gross
@ 2020-10-20 13:33   ` Jan Beulich
  0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2020-10-20 13:33 UTC (permalink / raw)
  To: Juergen Gross, xen-devel; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu

On 16.10.2020 10:53, 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 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.

I'm concerned by there being just a single handler allowed, when
the series already introduces two uses. A single NMI instance
may signal multiple things in one go. At the very least we then
need a priority, such that SERR could override oprofile.

> @@ -1799,6 +1800,57 @@ void unset_nmi_callback(void)
>      nmi_callback = dummy_nmi_callback;
>  }
>  
> +static DEFINE_PER_CPU(nmi_contfunc_t *, nmi_cont_func);
> +static DEFINE_PER_CPU(void *, nmi_cont_arg);
> +static DEFINE_PER_CPU(bool, nmi_cont_busy);
> +
> +bool nmi_check_continuation(void)
> +{
> +    unsigned int cpu = smp_processor_id();
> +    nmi_contfunc_t *func = per_cpu(nmi_cont_func, cpu);
> +    void *arg = per_cpu(nmi_cont_arg, cpu);
> +
> +    if ( per_cpu(nmi_cont_busy, cpu) )
> +    {
> +        per_cpu(nmi_cont_busy, cpu) = false;
> +        printk("Trying to set NMI continuation while still one active!\n");

I guess the bool would better be a const void *, written
with __builtin_return_address(0) and logged accordingly here
(also including the CPU number).

Also (nit) maybe "... while one still active"?

> +    }
> +
> +    /* Reads must be done before following write (local cpu ordering only). */
> +    barrier();
> +
> +    per_cpu(nmi_cont_func, cpu) = NULL;

I think this still is too lax, and you really need to use
xchg() to make sure you won't lose any case of "busy" (which
may become set between the if() above and the write here).

Jan


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16  8:53 [PATCH v3 0/3] xen/x86: implement NMI continuation Juergen Gross
2020-10-16  8:53 ` [PATCH v3 1/3] xen/x86: add nmi continuation framework Juergen Gross
2020-10-20 13:33   ` Jan Beulich
2020-10-16  8:53 ` [PATCH v3 2/3] xen/oprofile: use set_nmi_continuation() for sending virq to guest Juergen Gross
2020-10-16  8:53 ` [PATCH v3 3/3] xen/x86: issue pci_serr error message via NMI continuation Juergen Gross

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git