[PATCH v4 0/3] xen/x86: implement NMI continuation

[PATCH v4 0/3] xen/x86: implement NMI continuation

[Juergen Gross]

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

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/oprofile/nmi_int.c | 20 +++++++++++++--
 xen/arch/x86/traps.c            | 44 ++++++++++++++++++++++++++++-----
 xen/include/asm-x86/nmi.h       | 11 ++++++++-
 xen/include/asm-x86/softirq.h   |  5 ++--
 xen/include/asm-x86/xenoprof.h  |  7 ++++++
 6 files changed, 85 insertions(+), 15 deletions(-)

-- 
2.26.2

[PATCH v4 1/3] xen/x86: add nmi continuation framework

[Juergen Gross]

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>
---
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/traps.c      | 19 +++++++++++++++++++
 xen/include/asm-x86/nmi.h | 11 ++++++++++-
 3 files changed, 39 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..5005ac6e6e 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,24 @@ 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.
+     */
+    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

[PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Juergen Gross]

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>
---
V4:
- rework to less generic approach
---
 xen/arch/x86/oprofile/nmi_int.c | 20 ++++++++++++++++++--
 xen/arch/x86/traps.c            |  4 ++++
 xen/include/asm-x86/xenoprof.h  |  7 +++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/oprofile/nmi_int.c b/xen/arch/x86/oprofile/nmi_int.c
index 0f103d80a6..5f17cba0b2 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,28 @@ void passive_domain_destroy(struct vcpu *v)
 		model->free_msr(v);
 }
 
+bool nmi_oprofile_send_virq(void)
+{
+	struct vcpu *v = this_cpu(nmi_cont_vcpu);
+
+	if ( v )
+		send_guest_vcpu_virq(v, VIRQ_XENOPROF);
+
+	this_cpu(nmi_cont_vcpu) = NULL;
+
+	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) = 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 5005ac6e6e..7cb7d7e09c 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>
@@ -1804,6 +1805,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

[PATCH v4 3/3] xen/x86: issue pci_serr error message via NMI continuation

[Juergen Gross]

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>
---
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 7cb7d7e09c..6aeccef32d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1660,10 +1660,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)
@@ -1688,9 +1696,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();
@@ -1808,6 +1816,9 @@ bool nmi_check_continuation(void)
     if ( nmi_oprofile_send_virq() )
         ret = true;
 
+    if ( pci_serr_nmicont() )
+        ret = true;
+
     return ret;
 }
 
@@ -2154,8 +2165,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

Re: [PATCH v4 1/3] xen/x86: add nmi continuation framework

[Jan Beulich]

On 09.11.2020 10:50, Juergen Gross wrote:
> 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>
with one further adjustment request:

> @@ -1799,6 +1800,24 @@ 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.
> +     */
> +    send_IPI_self(SPURIOUS_APIC_VECTOR);
> +    apic_wait_icr_idle();
> +}

This additionally relies on send_IPI_self_legacy() calling
send_IPI_shortcut(), rather than e.g. resolving the local CPU
number to a destination ID. I think this wants saying maybe
here, but more importantly in that function.

Jan

Re: [PATCH v4 1/3] xen/x86: add nmi continuation framework

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1466 bytes --]

On 11.11.20 16:37, Jan Beulich wrote:
> On 09.11.2020 10:50, Juergen Gross wrote:
>> 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>
> with one further adjustment request:
> 
>> @@ -1799,6 +1800,24 @@ 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.
>> +     */
>> +    send_IPI_self(SPURIOUS_APIC_VECTOR);
>> +    apic_wait_icr_idle();
>> +}
> 
> This additionally relies on send_IPI_self_legacy() calling
> send_IPI_shortcut(), rather than e.g. resolving the local CPU
> number to a destination ID. I think this wants saying maybe
> here, but more importantly in that function.

Okay.


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 --]

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jan Beulich]

On 09.11.2020 10:50, Juergen Gross wrote:
> @@ -83,14 +85,28 @@ void passive_domain_destroy(struct vcpu *v)
>  		model->free_msr(v);
>  }
>  
> +bool nmi_oprofile_send_virq(void)
> +{
> +	struct vcpu *v = this_cpu(nmi_cont_vcpu);
> +
> +	if ( v )
> +		send_guest_vcpu_virq(v, VIRQ_XENOPROF);
> +
> +	this_cpu(nmi_cont_vcpu) = NULL;

What if, by the time we make it here, a 2nd NMI has arrived? I
agree the next overflow interrupt shouldn't arrive this
quickly, but I also think you want to zap the per-CPU variable
first here, and ...

> +
> +	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) = current;

... avoid overwriting any non-NULL value here. That's then of
course still not closing the window, but has (imo) overall
better behavior.

Also, style-wise, going through the file it looks to be mainly
Linux style, so may I suggest your additions / changes to be
done that way, rather than extending use of this funny mixed
style?

Jan

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1776 bytes --]

On 11.11.20 16:45, Jan Beulich wrote:
> On 09.11.2020 10:50, Juergen Gross wrote:
>> @@ -83,14 +85,28 @@ void passive_domain_destroy(struct vcpu *v)
>>   		model->free_msr(v);
>>   }
>>   
>> +bool nmi_oprofile_send_virq(void)
>> +{
>> +	struct vcpu *v = this_cpu(nmi_cont_vcpu);
>> +
>> +	if ( v )
>> +		send_guest_vcpu_virq(v, VIRQ_XENOPROF);
>> +
>> +	this_cpu(nmi_cont_vcpu) = NULL;
> 
> What if, by the time we make it here, a 2nd NMI has arrived? I
> agree the next overflow interrupt shouldn't arrive this
> quickly, but I also think you want to zap the per-CPU variable
> first here, and ...

How could that happen? This function is activated only from NMI
context in case the NMI happened in guest mode. And it will be
executed with higher priority than any guest, so there is a zero
chance another NMI in guest mode can happen in between.

I can add a comment in this regard if you want.

> 
>> +
>> +	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) = current;
> 
> ... avoid overwriting any non-NULL value here. That's then of
> course still not closing the window, but has (imo) overall
> better behavior.
> 
> Also, style-wise, going through the file it looks to be mainly
> Linux style, so may I suggest your additions / changes to be
> done that way, rather than extending use of this funny mixed
> style?

Works for me.


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 --]

Re: [PATCH v4 3/3] xen/x86: issue pci_serr error message via NMI continuation

[Jan Beulich]

On 09.11.2020 10:50, Juergen Gross wrote:
> 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>
with one minor change to be considered:

> @@ -1808,6 +1816,9 @@ bool nmi_check_continuation(void)
>      if ( nmi_oprofile_send_virq() )
>          ret = true;
>  
> +    if ( pci_serr_nmicont() )
> +        ret = true;
> +
>      return ret;
>  }

As the likely more important part, wouldn't it be better to insert
this ahead of the oprofile check?

Jan

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jan Beulich]

On 11.11.2020 16:48, Jürgen Groß wrote:
> On 11.11.20 16:45, Jan Beulich wrote:
>> On 09.11.2020 10:50, Juergen Gross wrote:
>>> @@ -83,14 +85,28 @@ void passive_domain_destroy(struct vcpu *v)
>>>   		model->free_msr(v);
>>>   }
>>>   
>>> +bool nmi_oprofile_send_virq(void)
>>> +{
>>> +	struct vcpu *v = this_cpu(nmi_cont_vcpu);
>>> +
>>> +	if ( v )
>>> +		send_guest_vcpu_virq(v, VIRQ_XENOPROF);
>>> +
>>> +	this_cpu(nmi_cont_vcpu) = NULL;
>>
>> What if, by the time we make it here, a 2nd NMI has arrived? I
>> agree the next overflow interrupt shouldn't arrive this
>> quickly, but I also think you want to zap the per-CPU variable
>> first here, and ...
> 
> How could that happen? This function is activated only from NMI
> context in case the NMI happened in guest mode. And it will be
> executed with higher priority than any guest, so there is a zero
> chance another NMI in guest mode can happen in between.

While I'll admit I didn't pay attention to the bogus (as far as
HVM is concerned) xen_mode check, my understanding is that the
self-IPI will be delivered once we're back in guest mode, as
that's the first time IRQs would be on again (even event checking
gets deferred by sending a self-IPI). If another NMI was latched
by that time, it would take precedence over the IRQ and would
also be delivered on the guest mode insn that the IRET returned
to.

I agree though that this is benign, as the vCPU wouldn't have
been context switched out yet, i.e. current is still the same
and there'll then merely be two NMI instances folded into one.

However, I still think the ordering would better be changed, to
set a good precedent.

>>>   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);

Unrelated to the patch here (i.e. just as an observation), this
use of ring_0() looks bogus when the NMI occurred in HVM guest
mode.

Jan

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2438 bytes --]

On 12.11.20 11:23, Jan Beulich wrote:
> On 11.11.2020 16:48, Jürgen Groß wrote:
>> On 11.11.20 16:45, Jan Beulich wrote:
>>> On 09.11.2020 10:50, Juergen Gross wrote:
>>>> @@ -83,14 +85,28 @@ void passive_domain_destroy(struct vcpu *v)
>>>>    		model->free_msr(v);
>>>>    }
>>>>    
>>>> +bool nmi_oprofile_send_virq(void)
>>>> +{
>>>> +	struct vcpu *v = this_cpu(nmi_cont_vcpu);
>>>> +
>>>> +	if ( v )
>>>> +		send_guest_vcpu_virq(v, VIRQ_XENOPROF);
>>>> +
>>>> +	this_cpu(nmi_cont_vcpu) = NULL;
>>>
>>> What if, by the time we make it here, a 2nd NMI has arrived? I
>>> agree the next overflow interrupt shouldn't arrive this
>>> quickly, but I also think you want to zap the per-CPU variable
>>> first here, and ...
>>
>> How could that happen? This function is activated only from NMI
>> context in case the NMI happened in guest mode. And it will be
>> executed with higher priority than any guest, so there is a zero
>> chance another NMI in guest mode can happen in between.
> 
> While I'll admit I didn't pay attention to the bogus (as far as
> HVM is concerned) xen_mode check, my understanding is that the
> self-IPI will be delivered once we're back in guest mode, as
> that's the first time IRQs would be on again (even event checking
> gets deferred by sending a self-IPI). If another NMI was latched
> by that time, it would take precedence over the IRQ and would
> also be delivered on the guest mode insn that the IRET returned
> to.
> 
> I agree though that this is benign, as the vCPU wouldn't have
> been context switched out yet, i.e. current is still the same
> and there'll then merely be two NMI instances folded into one.

Correct.

> 
> However, I still think the ordering would better be changed, to
> set a good precedent.

Okay, if you want that.

> 
>>>>    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);
> 
> Unrelated to the patch here (i.e. just as an observation), this
> use of ring_0() looks bogus when the NMI occurred in HVM guest
> mode.

An NMI in an HVM guest due to oprofile would be a VMEXIT with NMI
reason, or just be handled completely inside the guest, right?

I don't see how this test should ever result in xen_mode being
false for an HVM guest.


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 --]

Re: [PATCH v4 3/3] xen/x86: issue pci_serr error message via NMI continuation

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 717 bytes --]

On 12.11.20 10:29, Jan Beulich wrote:
> On 09.11.2020 10:50, Juergen Gross wrote:
>> 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>
> with one minor change to be considered:
> 
>> @@ -1808,6 +1816,9 @@ bool nmi_check_continuation(void)
>>       if ( nmi_oprofile_send_virq() )
>>           ret = true;
>>   
>> +    if ( pci_serr_nmicont() )
>> +        ret = true;
>> +
>>       return ret;
>>   }
> 
> As the likely more important part, wouldn't it be better to insert
> this ahead of the oprofile check?

Fine with me.


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 --]

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jan Beulich]

On 12.11.2020 11:48, Jürgen Groß wrote:
> On 12.11.20 11:23, Jan Beulich wrote:
>> On 11.11.2020 16:48, Jürgen Groß wrote:
>>> On 11.11.20 16:45, Jan Beulich wrote:
>>>> On 09.11.2020 10:50, Juergen Gross wrote:
>>>>>    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);
>>
>> Unrelated to the patch here (i.e. just as an observation), this
>> use of ring_0() looks bogus when the NMI occurred in HVM guest
>> mode.
> 
> An NMI in an HVM guest due to oprofile would be a VMEXIT with NMI
> reason, or just be handled completely inside the guest, right?

Yes, and in the former case for VMX it would be handed on to do_nmi(),
with the guest register state. For SVM it would get handled on the
next STGI, i.e. would indeed never surface from HVM guest mode.

> I don't see how this test should ever result in xen_mode being
> false for an HVM guest.

I think, because of hvm_invalidate_regs_fields(), on VMX it would be
consistently true in release builds and consistently false in debug
ones.

Jan

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1437 bytes --]

On 12.11.20 12:05, Jan Beulich wrote:
> On 12.11.2020 11:48, Jürgen Groß wrote:
>> On 12.11.20 11:23, Jan Beulich wrote:
>>> On 11.11.2020 16:48, Jürgen Groß wrote:
>>>> On 11.11.20 16:45, Jan Beulich wrote:
>>>>> On 09.11.2020 10:50, Juergen Gross wrote:
>>>>>>     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);
>>>
>>> Unrelated to the patch here (i.e. just as an observation), this
>>> use of ring_0() looks bogus when the NMI occurred in HVM guest
>>> mode.
>>
>> An NMI in an HVM guest due to oprofile would be a VMEXIT with NMI
>> reason, or just be handled completely inside the guest, right?
> 
> Yes, and in the former case for VMX it would be handed on to do_nmi(),
> with the guest register state. For SVM it would get handled on the
> next STGI, i.e. would indeed never surface from HVM guest mode.
> 
>> I don't see how this test should ever result in xen_mode being
>> false for an HVM guest.
> 
> I think, because of hvm_invalidate_regs_fields(), on VMX it would be
> consistently true in release builds and consistently false in debug
> ones.

Ah, okay. I searched for do_nmi(), but the vmx code uses the exception
table instead.

So I guess this should be:

xen_mode = !guest_mode(regs);


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 --]

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jan Beulich]

On 12.11.2020 12:27, Jürgen Groß wrote:
> On 12.11.20 12:05, Jan Beulich wrote:
>> On 12.11.2020 11:48, Jürgen Groß wrote:
>>> On 12.11.20 11:23, Jan Beulich wrote:
>>>> On 11.11.2020 16:48, Jürgen Groß wrote:
>>>>> On 11.11.20 16:45, Jan Beulich wrote:
>>>>>> On 09.11.2020 10:50, Juergen Gross wrote:
>>>>>>>     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);
>>>>
>>>> Unrelated to the patch here (i.e. just as an observation), this
>>>> use of ring_0() looks bogus when the NMI occurred in HVM guest
>>>> mode.
>>>
>>> An NMI in an HVM guest due to oprofile would be a VMEXIT with NMI
>>> reason, or just be handled completely inside the guest, right?
>>
>> Yes, and in the former case for VMX it would be handed on to do_nmi(),
>> with the guest register state. For SVM it would get handled on the
>> next STGI, i.e. would indeed never surface from HVM guest mode.
>>
>>> I don't see how this test should ever result in xen_mode being
>>> false for an HVM guest.
>>
>> I think, because of hvm_invalidate_regs_fields(), on VMX it would be
>> consistently true in release builds and consistently false in debug
>> ones.
> 
> Ah, okay. I searched for do_nmi(), but the vmx code uses the exception
> table instead.
> 
> So I guess this should be:
> 
> xen_mode = !guest_mode(regs);

Yes, I think so. Just that guest_mode() also has its issues (my patch
"x86: refine guest_mode()" improving it at least some is still pending
Andrew's go / no-go / improvement suggestions), so whether it's
suitable to use here may need some careful evaluation.

Jan

Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1974 bytes --]

On 12.11.20 12:36, Jan Beulich wrote:
> On 12.11.2020 12:27, Jürgen Groß wrote:
>> On 12.11.20 12:05, Jan Beulich wrote:
>>> On 12.11.2020 11:48, Jürgen Groß wrote:
>>>> On 12.11.20 11:23, Jan Beulich wrote:
>>>>> On 11.11.2020 16:48, Jürgen Groß wrote:
>>>>>> On 11.11.20 16:45, Jan Beulich wrote:
>>>>>>> On 09.11.2020 10:50, Juergen Gross wrote:
>>>>>>>>      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);
>>>>>
>>>>> Unrelated to the patch here (i.e. just as an observation), this
>>>>> use of ring_0() looks bogus when the NMI occurred in HVM guest
>>>>> mode.
>>>>
>>>> An NMI in an HVM guest due to oprofile would be a VMEXIT with NMI
>>>> reason, or just be handled completely inside the guest, right?
>>>
>>> Yes, and in the former case for VMX it would be handed on to do_nmi(),
>>> with the guest register state. For SVM it would get handled on the
>>> next STGI, i.e. would indeed never surface from HVM guest mode.
>>>
>>>> I don't see how this test should ever result in xen_mode being
>>>> false for an HVM guest.
>>>
>>> I think, because of hvm_invalidate_regs_fields(), on VMX it would be
>>> consistently true in release builds and consistently false in debug
>>> ones.
>>
>> Ah, okay. I searched for do_nmi(), but the vmx code uses the exception
>> table instead.
>>
>> So I guess this should be:
>>
>> xen_mode = !guest_mode(regs);
> 
> Yes, I think so. Just that guest_mode() also has its issues (my patch
> "x86: refine guest_mode()" improving it at least some is still pending
> Andrew's go / no-go / improvement suggestions), so whether it's
> suitable to use here may need some careful evaluation.

I'll leave the test as is for now.

We can revisit it when your patch has been committed.


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 --]