[PATCH v2 5/7] HV: Use NMI-window exiting to address req missing issue


Kaige Fu
 

There is a window where we may miss the current request in the
notification period when the work flow is as the following:

CPUx + + CPUr
| |
| +--+
| | | Handle pending req
| <--+
+--+ |
| | Set req flag |
<--+ |
+------------------>---+
| Send NMI | | Handle NMI
| <--+
| |
| |
| +--> vCPU enter
| |
+ +

So, this patch enables the NMI-window exiting to trigger the next vmexit
once there is no "virtual-NMI blocking" after vCPU enter into VMX non-root
mode. Then we can process the pending request on time.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/virq.c | 17 ++++++++++++++++
hypervisor/arch/x86/guest/vmcs.c | 2 +-
hypervisor/arch/x86/guest/vmexit.c | 2 +-
hypervisor/arch/x86/irq.c | 31 +++++++++++++++++++++++++++++-
hypervisor/include/arch/x86/irq.h | 1 +
5 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/hypervisor/arch/x86/guest/virq.c b/hypervisor/arch/x86/guest/virq.c
index 174c0fb3..a513a7da 100644
--- a/hypervisor/arch/x86/guest/virq.c
+++ b/hypervisor/arch/x86/guest/virq.c
@@ -533,3 +533,20 @@ int32_t exception_vmexit_handler(struct acrn_vcpu *vcpu)

return status;
}
+
+int nmi_window_vmexit_handler(__unused struct acrn_vcpu *vcpu)
+{
+ uint32_t value32;
+
+ /*
+ * Disable NMI-window exiting here. We will process
+ * the pending request in acrn_handle_pending_request later
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 &= ~VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
+
+ vcpu_retain_rip(vcpu);
+
+ return 0;
+}
diff --git a/hypervisor/arch/x86/guest/vmcs.c b/hypervisor/arch/x86/guest/vmcs.c
index 1efa37db..7dd2d806 100644
--- a/hypervisor/arch/x86/guest/vmcs.c
+++ b/hypervisor/arch/x86/guest/vmcs.c
@@ -582,7 +582,7 @@ void switch_apicv_mode_x2apic(struct acrn_vcpu *vcpu)
* directly without vmexit. So, here we enable NMI-exiting and use NMI
* as notification signal after passthroughing the lapic to vCPU.
*/
- value32 |= VMX_PINBASED_CTLS_NMI_EXIT;
+ value32 |= VMX_PINBASED_CTLS_NMI_EXIT | VMX_PINBASED_CTLS_VIRT_NMI;
exec_vmwrite32(VMX_PIN_VM_EXEC_CONTROLS, value32);

value32 = exec_vmread32(VMX_EXIT_CONTROLS);
diff --git a/hypervisor/arch/x86/guest/vmexit.c b/hypervisor/arch/x86/guest/vmexit.c
index 43c979f8..2c1a9d4b 100644
--- a/hypervisor/arch/x86/guest/vmexit.c
+++ b/hypervisor/arch/x86/guest/vmexit.c
@@ -51,7 +51,7 @@ static const struct vm_exit_dispatch dispatch_table[NR_VMX_EXIT_REASONS] = {
[VMX_EXIT_REASON_INTERRUPT_WINDOW] = {
.handler = interrupt_window_vmexit_handler},
[VMX_EXIT_REASON_NMI_WINDOW] = {
- .handler = unhandled_vmexit_handler},
+ .handler = nmi_window_vmexit_handler},
[VMX_EXIT_REASON_TASK_SWITCH] = {
.handler = unhandled_vmexit_handler},
[VMX_EXIT_REASON_CPUID] = {
diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index 4460b44d..4a377239 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -18,6 +18,7 @@
#include <vboot.h>
#include <dump.h>
#include <logmsg.h>
+#include <vmx.h>

static spinlock_t exception_spinlock = { .head = 0U, .tail = 0U, };
static spinlock_t irq_alloc_spinlock = { .head = 0U, .tail = 0U, };
@@ -368,9 +369,37 @@ void dispatch_interrupt(const struct intr_excp_ctx *ctx)
void dispatch_exception(struct intr_excp_ctx *ctx)
{
uint16_t pcpu_id = get_pcpu_id();
+ uint32_t value32;

if (ctx->vector == IDT_NMI) {
- /* TODO: we will handle it later */
+ /*
+ * There is a window where we may miss the current request in this
+ * notification period when the work flow is as the following:
+ *
+ * CPUx + + CPUr
+ * | |
+ * | +--+
+ * | | | Handle pending req
+ * | <--+
+ * +--+ |
+ * | | Set req flag |
+ * <--+ |
+ * +------------------>---+
+ * | Send NMI | | Handle NMI
+ * | <--+
+ * | |
+ * | |
+ * | +--> vCPU enter
+ * | |
+ * + +
+ *
+ * So, here we enable the NMI-window exiting to trigger the next vmexit
+ * once there is no "virtual-NMI blocking" after vCPU enter into VMX non-root
+ * mode. Then we can process the pending request on time.
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 |= VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
} else {
/* Obtain lock to ensure exception dump doesn't get corrupted */
spinlock_obtain(&exception_spinlock);
diff --git a/hypervisor/include/arch/x86/irq.h b/hypervisor/include/arch/x86/irq.h
index 8f8f3967..a5ed3aed 100644
--- a/hypervisor/include/arch/x86/irq.h
+++ b/hypervisor/include/arch/x86/irq.h
@@ -207,6 +207,7 @@ void vcpu_make_request(struct acrn_vcpu *vcpu, uint16_t eventid);
* @pre vcpu != NULL
*/
int32_t exception_vmexit_handler(struct acrn_vcpu *vcpu);
+int32_t nmi_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t interrupt_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t external_interrupt_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t acrn_handle_pending_request(struct acrn_vcpu *vcpu);
--
2.20.0


Grandhi, Sainath
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Kaige Fu
Sent: Monday, December 09, 2019 8:48 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2 5/7] HV: Use NMI-window exiting to address req
missing issue

There is a window where we may miss the current request in the notification
period when the work flow is as the following:

CPUx + + CPUr
| |
| +--+
| | | Handle pending req
| <--+
+--+ |
| | Set req flag |
<--+ |
+------------------>---+
| Send NMI | | Handle NMI
| <--+
| |
| |
| +--> vCPU enter
| |
+ +

So, this patch enables the NMI-window exiting to trigger the next vmexit once
there is no "virtual-NMI blocking" after vCPU enter into VMX non-root mode.
Then we can process the pending request on time.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/virq.c | 17 ++++++++++++++++
hypervisor/arch/x86/guest/vmcs.c | 2 +-
hypervisor/arch/x86/guest/vmexit.c | 2 +-
hypervisor/arch/x86/irq.c | 31 +++++++++++++++++++++++++++++-
hypervisor/include/arch/x86/irq.h | 1 +
5 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/hypervisor/arch/x86/guest/virq.c b/hypervisor/arch/x86/guest/virq.c
index 174c0fb3..a513a7da 100644
--- a/hypervisor/arch/x86/guest/virq.c
+++ b/hypervisor/arch/x86/guest/virq.c
@@ -533,3 +533,20 @@ int32_t exception_vmexit_handler(struct acrn_vcpu
*vcpu)

return status;
}
+
+int nmi_window_vmexit_handler(__unused struct acrn_vcpu *vcpu) {
+ uint32_t value32;
+
+ /*
+ * Disable NMI-window exiting here. We will process
+ * the pending request in acrn_handle_pending_request later
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 &= ~VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
+
+ vcpu_retain_rip(vcpu);
+
+ return 0;
+}
diff --git a/hypervisor/arch/x86/guest/vmcs.c
b/hypervisor/arch/x86/guest/vmcs.c
index 1efa37db..7dd2d806 100644
--- a/hypervisor/arch/x86/guest/vmcs.c
+++ b/hypervisor/arch/x86/guest/vmcs.c
@@ -582,7 +582,7 @@ void switch_apicv_mode_x2apic(struct acrn_vcpu
*vcpu)
* directly without vmexit. So, here we enable NMI-exiting and
use NMI
* as notification signal after passthroughing the lapic to vCPU.
*/
- value32 |= VMX_PINBASED_CTLS_NMI_EXIT;
+ value32 |= VMX_PINBASED_CTLS_NMI_EXIT |
VMX_PINBASED_CTLS_VIRT_NMI;
exec_vmwrite32(VMX_PIN_VM_EXEC_CONTROLS, value32);

value32 = exec_vmread32(VMX_EXIT_CONTROLS); diff --git
a/hypervisor/arch/x86/guest/vmexit.c b/hypervisor/arch/x86/guest/vmexit.c
index 43c979f8..2c1a9d4b 100644
--- a/hypervisor/arch/x86/guest/vmexit.c
+++ b/hypervisor/arch/x86/guest/vmexit.c
@@ -51,7 +51,7 @@ static const struct vm_exit_dispatch
dispatch_table[NR_VMX_EXIT_REASONS] = {
[VMX_EXIT_REASON_INTERRUPT_WINDOW] = {
.handler = interrupt_window_vmexit_handler},
[VMX_EXIT_REASON_NMI_WINDOW] = {
- .handler = unhandled_vmexit_handler},
+ .handler = nmi_window_vmexit_handler},
[VMX_EXIT_REASON_TASK_SWITCH] = {
.handler = unhandled_vmexit_handler},
[VMX_EXIT_REASON_CPUID] = {
diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c index
4460b44d..4a377239 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -18,6 +18,7 @@
#include <vboot.h>
#include <dump.h>
#include <logmsg.h>
+#include <vmx.h>

static spinlock_t exception_spinlock = { .head = 0U, .tail = 0U, }; static
spinlock_t irq_alloc_spinlock = { .head = 0U, .tail = 0U, }; @@ -368,9 +369,37
@@ void dispatch_interrupt(const struct intr_excp_ctx *ctx) void
dispatch_exception(struct intr_excp_ctx *ctx) {
uint16_t pcpu_id = get_pcpu_id();
+ uint32_t value32;

if (ctx->vector == IDT_NMI) {
- /* TODO: we will handle it later */
+ /*
+ * There is a window where we may miss the current request in
this
+ * notification period when the work flow is as the following:
+ *
+ * CPUx + + CPUr
+ * | |
+ * | +--+
+ * | | | Handle pending req
+ * | <--+
+ * +--+ |
+ * | | Set req flag |
+ * <--+ |
+ * +------------------>---+
+ * | Send NMI | | Handle NMI
+ * | <--+
+ * | |
+ * | |
+ * | +--> vCPU enter
+ * | |
+ * + +
+ *
+ * So, here we enable the NMI-window exiting to trigger the
next vmexit
+ * once there is no "virtual-NMI blocking" after vCPU enter into
VMX non-root
+ * mode. Then we can process the pending request on time.
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 |= VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
Hi Kaige,
What if the reason of NMI is not due to hypervisor related notification? Can we check if there is a pending request against the vCPU running on this pCPU before opening "NMI window"?
} else {
/* Obtain lock to ensure exception dump doesn't get corrupted
*/
spinlock_obtain(&exception_spinlock);
diff --git a/hypervisor/include/arch/x86/irq.h
b/hypervisor/include/arch/x86/irq.h
index 8f8f3967..a5ed3aed 100644
--- a/hypervisor/include/arch/x86/irq.h
+++ b/hypervisor/include/arch/x86/irq.h
@@ -207,6 +207,7 @@ void vcpu_make_request(struct acrn_vcpu *vcpu,
uint16_t eventid);
* @pre vcpu != NULL
*/
int32_t exception_vmexit_handler(struct acrn_vcpu *vcpu);
+int32_t nmi_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t interrupt_window_vmexit_handler(struct acrn_vcpu *vcpu); int32_t
external_interrupt_vmexit_handler(struct acrn_vcpu *vcpu); int32_t
acrn_handle_pending_request(struct acrn_vcpu *vcpu);
--
2.20.0



Kaige Fu
 

On 12-09 Mon 14:23, Grandhi, Sainath wrote:


-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Kaige Fu
Sent: Monday, December 09, 2019 8:48 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2 5/7] HV: Use NMI-window exiting to address req
missing issue

There is a window where we may miss the current request in the notification
period when the work flow is as the following:

CPUx + + CPUr
| |
| +--+
| | | Handle pending req
| <--+
+--+ |
| | Set req flag |
<--+ |
+------------------>---+
| Send NMI | | Handle NMI
| <--+
| |
| |
| +--> vCPU enter
| |
+ +

So, this patch enables the NMI-window exiting to trigger the next vmexit once
there is no "virtual-NMI blocking" after vCPU enter into VMX non-root mode.
Then we can process the pending request on time.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/virq.c | 17 ++++++++++++++++
hypervisor/arch/x86/guest/vmcs.c | 2 +-
hypervisor/arch/x86/guest/vmexit.c | 2 +-
hypervisor/arch/x86/irq.c | 31 +++++++++++++++++++++++++++++-
hypervisor/include/arch/x86/irq.h | 1 +
5 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/hypervisor/arch/x86/guest/virq.c b/hypervisor/arch/x86/guest/virq.c
index 174c0fb3..a513a7da 100644
--- a/hypervisor/arch/x86/guest/virq.c
+++ b/hypervisor/arch/x86/guest/virq.c
@@ -533,3 +533,20 @@ int32_t exception_vmexit_handler(struct acrn_vcpu
*vcpu)

return status;
}
+
+int nmi_window_vmexit_handler(__unused struct acrn_vcpu *vcpu) {
+ uint32_t value32;
+
+ /*
+ * Disable NMI-window exiting here. We will process
+ * the pending request in acrn_handle_pending_request later
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 &= ~VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
+
+ vcpu_retain_rip(vcpu);
+
+ return 0;
+}
diff --git a/hypervisor/arch/x86/guest/vmcs.c
b/hypervisor/arch/x86/guest/vmcs.c
index 1efa37db..7dd2d806 100644
--- a/hypervisor/arch/x86/guest/vmcs.c
+++ b/hypervisor/arch/x86/guest/vmcs.c
@@ -582,7 +582,7 @@ void switch_apicv_mode_x2apic(struct acrn_vcpu
*vcpu)
* directly without vmexit. So, here we enable NMI-exiting and
use NMI
* as notification signal after passthroughing the lapic to vCPU.
*/
- value32 |= VMX_PINBASED_CTLS_NMI_EXIT;
+ value32 |= VMX_PINBASED_CTLS_NMI_EXIT |
VMX_PINBASED_CTLS_VIRT_NMI;
exec_vmwrite32(VMX_PIN_VM_EXEC_CONTROLS, value32);

value32 = exec_vmread32(VMX_EXIT_CONTROLS); diff --git
a/hypervisor/arch/x86/guest/vmexit.c b/hypervisor/arch/x86/guest/vmexit.c
index 43c979f8..2c1a9d4b 100644
--- a/hypervisor/arch/x86/guest/vmexit.c
+++ b/hypervisor/arch/x86/guest/vmexit.c
@@ -51,7 +51,7 @@ static const struct vm_exit_dispatch
dispatch_table[NR_VMX_EXIT_REASONS] = {
[VMX_EXIT_REASON_INTERRUPT_WINDOW] = {
.handler = interrupt_window_vmexit_handler},
[VMX_EXIT_REASON_NMI_WINDOW] = {
- .handler = unhandled_vmexit_handler},
+ .handler = nmi_window_vmexit_handler},
[VMX_EXIT_REASON_TASK_SWITCH] = {
.handler = unhandled_vmexit_handler},
[VMX_EXIT_REASON_CPUID] = {
diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c index
4460b44d..4a377239 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -18,6 +18,7 @@
#include <vboot.h>
#include <dump.h>
#include <logmsg.h>
+#include <vmx.h>

static spinlock_t exception_spinlock = { .head = 0U, .tail = 0U, }; static
spinlock_t irq_alloc_spinlock = { .head = 0U, .tail = 0U, }; @@ -368,9 +369,37
@@ void dispatch_interrupt(const struct intr_excp_ctx *ctx) void
dispatch_exception(struct intr_excp_ctx *ctx) {
uint16_t pcpu_id = get_pcpu_id();
+ uint32_t value32;

if (ctx->vector == IDT_NMI) {
- /* TODO: we will handle it later */
+ /*
+ * There is a window where we may miss the current request in
this
+ * notification period when the work flow is as the following:
+ *
+ * CPUx + + CPUr
+ * | |
+ * | +--+
+ * | | | Handle pending req
+ * | <--+
+ * +--+ |
+ * | | Set req flag |
+ * <--+ |
+ * +------------------>---+
+ * | Send NMI | | Handle NMI
+ * | <--+
+ * | |
+ * | |
+ * | +--> vCPU enter
+ * | |
+ * + +
+ *
+ * So, here we enable the NMI-window exiting to trigger the
next vmexit
+ * once there is no "virtual-NMI blocking" after vCPU enter into
VMX non-root
+ * mode. Then we can process the pending request on time.
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 |= VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
Hi Kaige,
What if the reason of NMI is not due to hypervisor related notification? Can we check if there is a pending request against the vCPU running on this pCPU before opening "NMI window"
If the reason of NMI is not due to hypervisor related notification. There are no request bits set in pending request.
So, we will do nothing in next nmi-window exiting. It is harmless except for extra vmexit.

Yes. We can check the pending request against the vCPU running on this pCPU before opening "NMI window". But we don't
do like this for:
- Almost all the time, the NMI is triggered by hypervisor related notification.
- If we want to check the pending request in nmi_handler, we need to find the target vCPU according pCPU. It seems
weired to me that we touch the vCPU context in exception context and will introduce more complexity just to deal with
something that rarely happens.

} else {
/* Obtain lock to ensure exception dump doesn't get corrupted
*/
spinlock_obtain(&exception_spinlock);
diff --git a/hypervisor/include/arch/x86/irq.h
b/hypervisor/include/arch/x86/irq.h
index 8f8f3967..a5ed3aed 100644
--- a/hypervisor/include/arch/x86/irq.h
+++ b/hypervisor/include/arch/x86/irq.h
@@ -207,6 +207,7 @@ void vcpu_make_request(struct acrn_vcpu *vcpu,
uint16_t eventid);
* @pre vcpu != NULL
*/
int32_t exception_vmexit_handler(struct acrn_vcpu *vcpu);
+int32_t nmi_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t interrupt_window_vmexit_handler(struct acrn_vcpu *vcpu); int32_t
external_interrupt_vmexit_handler(struct acrn_vcpu *vcpu); int32_t
acrn_handle_pending_request(struct acrn_vcpu *vcpu);
--
2.20.0





Grandhi, Sainath
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Kaige Fu
Sent: Tuesday, December 10, 2019 12:52 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v2 5/7] HV: Use NMI-window exiting to address
req missing issue

On 12-09 Mon 14:23, Grandhi, Sainath wrote:


-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Kaige Fu
Sent: Monday, December 09, 2019 8:48 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2 5/7] HV: Use NMI-window exiting to
address req missing issue

There is a window where we may miss the current request in the
notification period when the work flow is as the following:

CPUx + + CPUr
| |
| +--+
| | | Handle pending req
| <--+
+--+ |
| | Set req flag |
<--+ |
+------------------>---+
| Send NMI | | Handle NMI
| <--+
| |
| |
| +--> vCPU enter
| |
+ +

So, this patch enables the NMI-window exiting to trigger the next
vmexit once there is no "virtual-NMI blocking" after vCPU enter into VMX
non-root mode.
Then we can process the pending request on time.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/virq.c | 17 ++++++++++++++++
hypervisor/arch/x86/guest/vmcs.c | 2 +-
hypervisor/arch/x86/guest/vmexit.c | 2 +-
hypervisor/arch/x86/irq.c | 31 +++++++++++++++++++++++++++++-
hypervisor/include/arch/x86/irq.h | 1 +
5 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/hypervisor/arch/x86/guest/virq.c
b/hypervisor/arch/x86/guest/virq.c
index 174c0fb3..a513a7da 100644
--- a/hypervisor/arch/x86/guest/virq.c
+++ b/hypervisor/arch/x86/guest/virq.c
@@ -533,3 +533,20 @@ int32_t exception_vmexit_handler(struct
acrn_vcpu
*vcpu)

return status;
}
+
+int nmi_window_vmexit_handler(__unused struct acrn_vcpu *vcpu) {
+ uint32_t value32;
+
+ /*
+ * Disable NMI-window exiting here. We will process
+ * the pending request in acrn_handle_pending_request later
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 &= ~VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
+
+ vcpu_retain_rip(vcpu);
+
+ return 0;
+}
diff --git a/hypervisor/arch/x86/guest/vmcs.c
b/hypervisor/arch/x86/guest/vmcs.c
index 1efa37db..7dd2d806 100644
--- a/hypervisor/arch/x86/guest/vmcs.c
+++ b/hypervisor/arch/x86/guest/vmcs.c
@@ -582,7 +582,7 @@ void switch_apicv_mode_x2apic(struct acrn_vcpu
*vcpu)
* directly without vmexit. So, here we enable NMI-exiting and
use NMI
* as notification signal after passthroughing the lapic to vCPU.
*/
- value32 |= VMX_PINBASED_CTLS_NMI_EXIT;
+ value32 |= VMX_PINBASED_CTLS_NMI_EXIT |
VMX_PINBASED_CTLS_VIRT_NMI;
exec_vmwrite32(VMX_PIN_VM_EXEC_CONTROLS, value32);

value32 = exec_vmread32(VMX_EXIT_CONTROLS); diff --git
a/hypervisor/arch/x86/guest/vmexit.c
b/hypervisor/arch/x86/guest/vmexit.c
index 43c979f8..2c1a9d4b 100644
--- a/hypervisor/arch/x86/guest/vmexit.c
+++ b/hypervisor/arch/x86/guest/vmexit.c
@@ -51,7 +51,7 @@ static const struct vm_exit_dispatch
dispatch_table[NR_VMX_EXIT_REASONS] = {
[VMX_EXIT_REASON_INTERRUPT_WINDOW] = {
.handler = interrupt_window_vmexit_handler},
[VMX_EXIT_REASON_NMI_WINDOW] = {
- .handler = unhandled_vmexit_handler},
+ .handler = nmi_window_vmexit_handler},
[VMX_EXIT_REASON_TASK_SWITCH] = {
.handler = unhandled_vmexit_handler},
[VMX_EXIT_REASON_CPUID] = {
diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index
4460b44d..4a377239 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -18,6 +18,7 @@
#include <vboot.h>
#include <dump.h>
#include <logmsg.h>
+#include <vmx.h>

static spinlock_t exception_spinlock = { .head = 0U, .tail = 0U, };
static spinlock_t irq_alloc_spinlock = { .head = 0U, .tail = 0U, };
@@ -368,9 +369,37 @@ void dispatch_interrupt(const struct
intr_excp_ctx *ctx) void dispatch_exception(struct intr_excp_ctx *ctx) {
uint16_t pcpu_id = get_pcpu_id();
+ uint32_t value32;

if (ctx->vector == IDT_NMI) {
- /* TODO: we will handle it later */
+ /*
+ * There is a window where we may miss the current request in
this
+ * notification period when the work flow is as the following:
+ *
+ * CPUx + + CPUr
+ * | |
+ * | +--+
+ * | | | Handle pending req
+ * | <--+
+ * +--+ |
+ * | | Set req flag |
+ * <--+ |
+ * +------------------>---+
+ * | Send NMI | | Handle NMI
+ * | <--+
+ * | |
+ * | |
+ * | +--> vCPU enter
+ * | |
+ * + +
+ *
+ * So, here we enable the NMI-window exiting to trigger the
next vmexit
+ * once there is no "virtual-NMI blocking" after vCPU enter into
VMX non-root
+ * mode. Then we can process the pending request on time.
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 |= VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
Hi Kaige,
What if the reason of NMI is not due to hypervisor related notification?
Can we check if there is a pending request against the vCPU running on this
pCPU before opening "NMI window"

If the reason of NMI is not due to hypervisor related notification. There are no
request bits set in pending request.
So, we will do nothing in next nmi-window exiting. It is harmless except for extra
vmexit.
That is not the only side-effect.
What if the NMI is for some platform error? We need to inject it back to a VM which
would handle such errors.
With this approach, the NMI (for reasons of platform error) would be lost forever.

Yes. We can check the pending request against the vCPU running on this pCPU
before opening "NMI window". But we don't do like this for:
- Almost all the time, the NMI is triggered by hypervisor related notification.
- If we want to check the pending request in nmi_handler, we need to find the
target vCPU according pCPU. It seems
weired to me that we touch the vCPU context in exception context and will
introduce more complexity just to deal with
something that rarely happens.

} else {
/* Obtain lock to ensure exception dump doesn't get corrupted
*/
spinlock_obtain(&exception_spinlock);
diff --git a/hypervisor/include/arch/x86/irq.h
b/hypervisor/include/arch/x86/irq.h
index 8f8f3967..a5ed3aed 100644
--- a/hypervisor/include/arch/x86/irq.h
+++ b/hypervisor/include/arch/x86/irq.h
@@ -207,6 +207,7 @@ void vcpu_make_request(struct acrn_vcpu *vcpu,
uint16_t eventid);
* @pre vcpu != NULL
*/
int32_t exception_vmexit_handler(struct acrn_vcpu *vcpu);
+int32_t nmi_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t interrupt_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t external_interrupt_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t acrn_handle_pending_request(struct acrn_vcpu *vcpu);
--
2.20.0





Kaige Fu
 

On 12-10 Tue 05:06, Grandhi, Sainath wrote:


-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Kaige Fu
Sent: Tuesday, December 10, 2019 12:52 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v2 5/7] HV: Use NMI-window exiting to address
req missing issue

On 12-09 Mon 14:23, Grandhi, Sainath wrote:


-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Kaige Fu
Sent: Monday, December 09, 2019 8:48 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2 5/7] HV: Use NMI-window exiting to
address req missing issue

There is a window where we may miss the current request in the
notification period when the work flow is as the following:

CPUx + + CPUr
| |
| +--+
| | | Handle pending req
| <--+
+--+ |
| | Set req flag |
<--+ |
+------------------>---+
| Send NMI | | Handle NMI
| <--+
| |
| |
| +--> vCPU enter
| |
+ +

So, this patch enables the NMI-window exiting to trigger the next
vmexit once there is no "virtual-NMI blocking" after vCPU enter into VMX
non-root mode.
Then we can process the pending request on time.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/virq.c | 17 ++++++++++++++++
hypervisor/arch/x86/guest/vmcs.c | 2 +-
hypervisor/arch/x86/guest/vmexit.c | 2 +-
hypervisor/arch/x86/irq.c | 31 +++++++++++++++++++++++++++++-
hypervisor/include/arch/x86/irq.h | 1 +
5 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/hypervisor/arch/x86/guest/virq.c
b/hypervisor/arch/x86/guest/virq.c
index 174c0fb3..a513a7da 100644
--- a/hypervisor/arch/x86/guest/virq.c
+++ b/hypervisor/arch/x86/guest/virq.c
@@ -533,3 +533,20 @@ int32_t exception_vmexit_handler(struct
acrn_vcpu
*vcpu)

return status;
}
+
+int nmi_window_vmexit_handler(__unused struct acrn_vcpu *vcpu) {
+ uint32_t value32;
+
+ /*
+ * Disable NMI-window exiting here. We will process
+ * the pending request in acrn_handle_pending_request later
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 &= ~VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
+
+ vcpu_retain_rip(vcpu);
+
+ return 0;
+}
diff --git a/hypervisor/arch/x86/guest/vmcs.c
b/hypervisor/arch/x86/guest/vmcs.c
index 1efa37db..7dd2d806 100644
--- a/hypervisor/arch/x86/guest/vmcs.c
+++ b/hypervisor/arch/x86/guest/vmcs.c
@@ -582,7 +582,7 @@ void switch_apicv_mode_x2apic(struct acrn_vcpu
*vcpu)
* directly without vmexit. So, here we enable NMI-exiting and
use NMI
* as notification signal after passthroughing the lapic to vCPU.
*/
- value32 |= VMX_PINBASED_CTLS_NMI_EXIT;
+ value32 |= VMX_PINBASED_CTLS_NMI_EXIT |
VMX_PINBASED_CTLS_VIRT_NMI;
exec_vmwrite32(VMX_PIN_VM_EXEC_CONTROLS, value32);

value32 = exec_vmread32(VMX_EXIT_CONTROLS); diff --git
a/hypervisor/arch/x86/guest/vmexit.c
b/hypervisor/arch/x86/guest/vmexit.c
index 43c979f8..2c1a9d4b 100644
--- a/hypervisor/arch/x86/guest/vmexit.c
+++ b/hypervisor/arch/x86/guest/vmexit.c
@@ -51,7 +51,7 @@ static const struct vm_exit_dispatch
dispatch_table[NR_VMX_EXIT_REASONS] = {
[VMX_EXIT_REASON_INTERRUPT_WINDOW] = {
.handler = interrupt_window_vmexit_handler},
[VMX_EXIT_REASON_NMI_WINDOW] = {
- .handler = unhandled_vmexit_handler},
+ .handler = nmi_window_vmexit_handler},
[VMX_EXIT_REASON_TASK_SWITCH] = {
.handler = unhandled_vmexit_handler},
[VMX_EXIT_REASON_CPUID] = {
diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index
4460b44d..4a377239 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -18,6 +18,7 @@
#include <vboot.h>
#include <dump.h>
#include <logmsg.h>
+#include <vmx.h>

static spinlock_t exception_spinlock = { .head = 0U, .tail = 0U, };
static spinlock_t irq_alloc_spinlock = { .head = 0U, .tail = 0U, };
@@ -368,9 +369,37 @@ void dispatch_interrupt(const struct
intr_excp_ctx *ctx) void dispatch_exception(struct intr_excp_ctx *ctx) {
uint16_t pcpu_id = get_pcpu_id();
+ uint32_t value32;

if (ctx->vector == IDT_NMI) {
- /* TODO: we will handle it later */
+ /*
+ * There is a window where we may miss the current request in
this
+ * notification period when the work flow is as the following:
+ *
+ * CPUx + + CPUr
+ * | |
+ * | +--+
+ * | | | Handle pending req
+ * | <--+
+ * +--+ |
+ * | | Set req flag |
+ * <--+ |
+ * +------------------>---+
+ * | Send NMI | | Handle NMI
+ * | <--+
+ * | |
+ * | |
+ * | +--> vCPU enter
+ * | |
+ * + +
+ *
+ * So, here we enable the NMI-window exiting to trigger the
next vmexit
+ * once there is no "virtual-NMI blocking" after vCPU enter into
VMX non-root
+ * mode. Then we can process the pending request on time.
+ */
+ value32 = exec_vmread32(VMX_PROC_VM_EXEC_CONTROLS);
+ value32 |= VMX_PROCBASED_CTLS_NMI_WINEXIT;
+ exec_vmwrite32(VMX_PROC_VM_EXEC_CONTROLS, value32);
Hi Kaige,
What if the reason of NMI is not due to hypervisor related notification?
Can we check if there is a pending request against the vCPU running on this
pCPU before opening "NMI window"

If the reason of NMI is not due to hypervisor related notification. There are no
request bits set in pending request.
So, we will do nothing in next nmi-window exiting. It is harmless except for extra
vmexit.
That is not the only side-effect.
What if the NMI is for some platform error? We need to inject it back to a VM which
would handle such errors.
With this approach, the NMI (for reasons of platform error) would be lost forever.
Actually, if we use the NMI as the notification signal, acrn hypervisor will have no
way to support vNMI (inject NMI to the guest) as we can't distinguish between NMI
for notification and NMI for guest.

With this patchset, acrn hypervisor will ignore all the NMI injection request to guest.
It means that the hard rtvm will never receive NMI.


Yes. We can check the pending request against the vCPU running on this pCPU
before opening "NMI window". But we don't do like this for:
- Almost all the time, the NMI is triggered by hypervisor related notification.
- If we want to check the pending request in nmi_handler, we need to find the
target vCPU according pCPU. It seems
weired to me that we touch the vCPU context in exception context and will
introduce more complexity just to deal with
something that rarely happens.

} else {
/* Obtain lock to ensure exception dump doesn't get corrupted
*/
spinlock_obtain(&exception_spinlock);
diff --git a/hypervisor/include/arch/x86/irq.h
b/hypervisor/include/arch/x86/irq.h
index 8f8f3967..a5ed3aed 100644
--- a/hypervisor/include/arch/x86/irq.h
+++ b/hypervisor/include/arch/x86/irq.h
@@ -207,6 +207,7 @@ void vcpu_make_request(struct acrn_vcpu *vcpu,
uint16_t eventid);
* @pre vcpu != NULL
*/
int32_t exception_vmexit_handler(struct acrn_vcpu *vcpu);
+int32_t nmi_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t interrupt_window_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t external_interrupt_vmexit_handler(struct acrn_vcpu *vcpu);
int32_t acrn_handle_pending_request(struct acrn_vcpu *vcpu);
--
2.20.0