* [PATCH v5 0/3] xen/x86: implement NMI continuation
@ 2020-11-12 13:14 Juergen Gross
2020-11-12 13:14 ` [PATCH v5 1/3] xen/x86: add nmi continuation framework Juergen Gross
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Juergen Gross @ 2020-11-12 13:14 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
Changes in V4:
- use less generic approach
Changes in V5:
- addressed comments
Juergen Gross (3):
xen/x86: add nmi continuation framework
xen/oprofile: use 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/genapic/x2apic.c | 1 +
xen/arch/x86/oprofile/nmi_int.c | 19 ++++++++++++--
xen/arch/x86/smp.c | 1 +
xen/arch/x86/traps.c | 46 ++++++++++++++++++++++++++++-----
xen/include/asm-x86/nmi.h | 11 +++++++-
xen/include/asm-x86/softirq.h | 5 ++--
xen/include/asm-x86/xenoprof.h | 7 +++++
8 files changed, 88 insertions(+), 15 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v5 1/3] xen/x86: add nmi continuation framework
2020-11-12 13:14 [PATCH v5 0/3] xen/x86: implement NMI continuation Juergen Gross
@ 2020-11-12 13:14 ` Juergen Gross
2020-11-12 13:41 ` Jan Beulich
2020-11-12 13:14 ` [PATCH v5 2/3] xen/oprofile: use NMI continuation for sending virq to guest Juergen Gross
2020-11-12 13:14 ` [PATCH v5 3/3] xen/x86: issue pci_serr error message via NMI continuation Juergen Gross
2 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2020-11-12 13:14 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 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
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 <jgross@suse.com>
---
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(-)
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 */
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v5 2/3] xen/oprofile: use NMI continuation for sending virq to guest
2020-11-12 13:14 [PATCH v5 0/3] xen/x86: implement NMI continuation Juergen Gross
2020-11-12 13:14 ` [PATCH v5 1/3] xen/x86: add nmi continuation framework Juergen Gross
@ 2020-11-12 13:14 ` Juergen Gross
2020-11-12 13:43 ` Jan Beulich
2020-11-12 13:14 ` [PATCH v5 3/3] xen/x86: issue pci_serr error message via NMI continuation Juergen Gross
2 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2020-11-12 13:14 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>
---
V5:
- use Linux coding style (Jan Beulich)
- assume races could happen (Jan Beulich)
V4:
- rework to less generic approach
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/oprofile/nmi_int.c | 19 +++++++++++++++++--
xen/arch/x86/traps.c | 4 ++++
xen/include/asm-x86/xenoprof.h | 7 +++++++
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/oprofile/nmi_int.c b/xen/arch/x86/oprofile/nmi_int.c
index 0f103d80a6..a13bd82915 100644
--- a/xen/arch/x86/oprofile/nmi_int.c
+++ b/xen/arch/x86/oprofile/nmi_int.c
@@ -38,6 +38,8 @@ static unsigned long saved_lvtpc[NR_CPUS];
static char *cpu_type;
+static DEFINE_PER_CPU(struct vcpu *, nmi_cont_vcpu);
+
static int passive_domain_msr_op_checks(unsigned int msr, int *typep, int *indexp)
{
struct vpmu_struct *vpmu = vcpu_vpmu(current);
@@ -83,14 +85,27 @@ void passive_domain_destroy(struct vcpu *v)
model->free_msr(v);
}
+bool nmi_oprofile_send_virq(void)
+{
+ struct vcpu *v = xchg(&this_cpu(nmi_cont_vcpu), NULL);
+
+ if (v)
+ send_guest_vcpu_virq(v, VIRQ_XENOPROF);
+
+ return v;
+}
+
static int nmi_callback(const struct cpu_user_regs *regs, int cpu)
{
int xen_mode, ovf;
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);
+ if (ovf && is_active(current->domain) && !xen_mode &&
+ !this_cpu(nmi_cont_vcpu)) {
+ this_cpu(nmi_cont_vcpu) = current;
+ trigger_nmi_continuation();
+ }
if ( ovf == 2 )
current->arch.nmi_pending = true;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 5cbaa49031..240fd1b089 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -65,6 +65,7 @@
#include <asm/debugger.h>
#include <asm/msr.h>
#include <asm/nmi.h>
+#include <asm/xenoprof.h>
#include <asm/shared.h>
#include <asm/x86_emulate.h>
#include <asm/traps.h>
@@ -1805,6 +1806,9 @@ bool nmi_check_continuation(void)
{
bool ret = false;
+ if ( nmi_oprofile_send_virq() )
+ ret = true;
+
return ret;
}
diff --git a/xen/include/asm-x86/xenoprof.h b/xen/include/asm-x86/xenoprof.h
index 1026ba2e1f..cf6af8c5df 100644
--- a/xen/include/asm-x86/xenoprof.h
+++ b/xen/include/asm-x86/xenoprof.h
@@ -69,6 +69,8 @@ int passive_domain_do_rdmsr(unsigned int msr, uint64_t *msr_content);
int passive_domain_do_wrmsr(unsigned int msr, uint64_t msr_content);
void passive_domain_destroy(struct vcpu *v);
+bool nmi_oprofile_send_virq(void);
+
#else
static inline int passive_domain_do_rdmsr(unsigned int msr,
@@ -85,6 +87,11 @@ static inline int passive_domain_do_wrmsr(unsigned int msr,
static inline void passive_domain_destroy(struct vcpu *v) {}
+static inline bool nmi_oprofile_send_virq(void)
+{
+ return false;
+}
+
#endif /* CONFIG_XENOPROF */
#endif /* __ASM_X86_XENOPROF_H__ */
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v5 3/3] xen/x86: issue pci_serr error message via NMI continuation
2020-11-12 13:14 [PATCH v5 0/3] xen/x86: implement NMI continuation Juergen Gross
2020-11-12 13:14 ` [PATCH v5 1/3] xen/x86: add nmi continuation framework Juergen Gross
2020-11-12 13:14 ` [PATCH v5 2/3] xen/oprofile: use NMI continuation for sending virq to guest Juergen Gross
@ 2020-11-12 13:14 ` Juergen Gross
2 siblings, 0 replies; 7+ messages in thread
From: Juergen Gross @ 2020-11-12 13:14 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V5:
- make PCISERR higher prior than oprofile (Jan Beulich)
V4:
- rework to less generic approach
---
xen/arch/x86/traps.c | 21 +++++++++++++++------
xen/include/asm-x86/softirq.h | 5 ++---
2 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 240fd1b089..0459cee9fb 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1661,10 +1661,18 @@ 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 bool pci_serr_cont;
+
+static bool pci_serr_nmicont(void)
{
+ if ( !pci_serr_cont )
+ return false;
+
+ pci_serr_cont = false;
printk("\n\nNMI - PCI system error (SERR)\n");
outb(inb(0x61) & 0x0b, 0x61); /* re-enable the PCI SERR error line. */
+
+ return true;
}
static void nmi_hwdom_report(unsigned int reason_idx)
@@ -1689,9 +1697,9 @@ 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. */
+ pci_serr_cont = true;
+ trigger_nmi_continuation();
break;
default: /* 'fatal' */
console_force_unlock();
@@ -1806,6 +1814,9 @@ bool nmi_check_continuation(void)
{
bool ret = false;
+ if ( pci_serr_nmicont() )
+ ret = true;
+
if ( nmi_oprofile_send_virq() )
ret = true;
@@ -2157,8 +2168,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 related [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/3] xen/x86: add nmi continuation framework
2020-11-12 13:14 ` [PATCH v5 1/3] xen/x86: add nmi continuation framework Juergen Gross
@ 2020-11-12 13:41 ` Jan Beulich
2020-11-12 14:50 ` Jürgen Groß
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2020-11-12 13:41 UTC (permalink / raw)
To: Juergen Gross; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 2/3] xen/oprofile: use NMI continuation for sending virq to guest
2020-11-12 13:14 ` [PATCH v5 2/3] xen/oprofile: use NMI continuation for sending virq to guest Juergen Gross
@ 2020-11-12 13:43 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2020-11-12 13:43 UTC (permalink / raw)
To: Juergen Gross; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
On 12.11.2020 14:14, Juergen Gross wrote:
> 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/3] xen/x86: add nmi continuation framework
2020-11-12 13:41 ` Jan Beulich
@ 2020-11-12 14:50 ` Jürgen Groß
0 siblings, 0 replies; 7+ messages in thread
From: Jürgen Groß @ 2020-11-12 14:50 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 1097 bytes --]
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
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-12 15:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 13:14 [PATCH v5 0/3] xen/x86: implement NMI continuation Juergen Gross
2020-11-12 13:14 ` [PATCH v5 1/3] xen/x86: add nmi continuation framework Juergen Gross
2020-11-12 13:41 ` Jan Beulich
2020-11-12 14:50 ` Jürgen Groß
2020-11-12 13:14 ` [PATCH v5 2/3] xen/oprofile: use NMI continuation for sending virq to guest Juergen Gross
2020-11-12 13:43 ` Jan Beulich
2020-11-12 13:14 ` [PATCH v5 3/3] xen/x86: issue pci_serr error message via NMI continuation Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).