Re: [PATCH v3 07/11] KVM: VMX: Handle NMI Source report in VM exit

From: Xin Li
Date: Mon Jul 01 2024 - 13:04:58 EST


On 7/1/2024 8:45 AM, Jacob Pan wrote:

On Fri, 28 Jun 2024 21:07:04 -0700, Xin Li <xin@xxxxxxxxx> wrote:

On 6/28/2024 1:18 PM, Jacob Pan wrote:
From: Zeng Guang <guang.zeng@xxxxxxxxx>

If the "NMI exiting" VM-execution control is 1, the value of the 16-bit
NMI source vector is saved in the exit-qualification field in the VMCS
when VM exits occur on CPUs that support NMI source.

KVM that is aware of NMI-source reporting will push the bitmask of NMI
source vectors as the exceptoin event data field on the stack for then
entry of FRED exception. Subsequently, the host NMI exception handler
is invoked which will process NMI source information in the event data.
This operation is independent of vCPU FRED enabling status.

Signed-off-by: Zeng Guang <guang.zeng@xxxxxxxxx>
Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
---
arch/x86/entry/entry_64_fred.S | 2 +-
arch/x86/kvm/vmx/vmx.c | 11 ++++++++---
2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/entry_64_fred.S
b/arch/x86/entry/entry_64_fred.S index a02bc6f3d2e6..0d934a3fcaf8 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -92,7 +92,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
* +--------+-----------------+
*/
push $0 /* Reserved, must be 0
*/
- push $0 /* Event data, 0 for
IRQ/NMI */
+ push %rsi /* Event data for IRQ/NMI */
push %rdi /* fred_ss handed in by the
caller */ push %rbp
pushf

This belongs to the previous patch, it is part of the host changes.
right, will do.


diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4e7b36081b76..6719c598fa5f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7331,10 +7331,15 @@ static noinstr void vmx_vcpu_enter_exit(struct
kvm_vcpu *vcpu, if ((u16)vmx->exit_reason.basic ==
EXIT_REASON_EXCEPTION_NMI && is_nmi(vmx_get_intr_info(vcpu))) {
kvm_before_interrupt(vcpu, KVM_HANDLING_NMI);
- if (cpu_feature_enabled(X86_FEATURE_FRED))
- fred_entry_from_kvm(EVENT_TYPE_NMI,
NMI_VECTOR, 0);
- else
+ if (cpu_feature_enabled(X86_FEATURE_FRED)) {
+ unsigned long edata = 0;
+
+ if
(cpu_feature_enabled(X86_FEATURE_NMI_SOURCE))
+ edata = vmx_get_exit_qual(vcpu);
+ fred_entry_from_kvm(EVENT_TYPE_NMI,
NMI_VECTOR, edata);

Simply do fred_entry_from_kvm(EVENT_TYPE_NMI, NMI_VECTOR,
vmx_get_exit_qual(vcpu))?
I don't have strong preference but having a local variable improves
readability IMHO.

My point was, do we actually need this check:
(cpu_feature_enabled(X86_FEATURE_NMI_SOURCE)?

I don't have a strong preference either.


+ } else {
vmx_do_nmi_irqoff();
+ }
kvm_after_interrupt(vcpu);
}



Thanks,

Jacob