[PATCHv3 0/8] Virtual NMI feature

[PATCHv3 0/8] Virtual NMI feature

[Santosh Shukla]

Change History:
v3 (rebased on eb555cb5b794f):
03 - added clear/set_vnmi_mask API for SMM case
04 - added vnmi_pending check so to detect the pending VNMI scenario
05 - removed WARN_ON
06 - handle one of nested case where L1 using vnmi and L2 doesn't
07 - Emulate the VMEXIT(#INVALID) case for nested case
08 - No change.

v2:
https://lore.kernel.org/lkml/20220709134230.2397-7-santosh.shukla@amd.com/T/#m4bf8a131748688fed00ab0fefdcac209a169e202
01, 02 - added maxim reviwed-by.
03 - Added get_vnmi_vmcb API to return vmcb for l1 and l2.
04 - Moved vnmi check after is_guest_mode() in func svm_nmi_blocked().
05 - Added WARN_ON check for vnmi pending.
06 - Save the V_NMI_PENDING/MASK state in vmcb12 on vmexit.
07 - No change.

v1:
https://lore.kernel.org/all/20220602142620.3196-1-santosh.shukla@amd.com/

Description:
Currently, NMI is delivered to the guest using the Event Injection
mechanism [1]. The Event Injection mechanism does not block the delivery
of subsequent NMIs. So the Hypervisor needs to track the NMI delivery
and its completion(by intercepting IRET) before sending a new NMI.

Virtual NMI (VNMI) allows the hypervisor to inject the NMI into the guest
w/o using Event Injection mechanism meaning not required to track the
guest NMI and intercepting the IRET. To achieve that,
VNMI feature provides virtualized NMI and NMI_MASK capability bits in
VMCB intr_control -
V_NMI(11) - Indicates whether a virtual NMI is pending in the guest.
V_NMI_MASK(12) - Indicates whether virtual NMI is masked in the guest.
V_NMI_ENABLE(26) - Enables the NMI virtualization feature for the guest.

When Hypervisor wants to inject NMI, it will set V_NMI bit, Processor will
clear the V_NMI bit and Set the V_NMI_MASK which means the Guest is
handling NMI, After the guest handled the NMI, The processor will clear
the V_NMI_MASK on the successful completion of IRET instruction
Or if VMEXIT occurs while delivering the virtual NMI.

If NMI virtualization enabled and NMI_INTERCEPT bit is unset
then HW will exit with #INVALID exit reason.

To enable the VNMI capability, Hypervisor need to program
V_NMI_ENABLE bit 1.

The presence of this feature is indicated via the CPUID function
0x8000000A_EDX[25].

Testing -
* Used qemu's `inject_nmi` for testing.
* tested with and w/o AVIC case.
* tested with kvm-unit-test
* tested with vGIF enable and disable.
* tested nested env:
  - L1+L2 using vnmi
  - L1 using vnmi and L2 not 


Thanks,
Santosh
[1] https://www.amd.com/system/files/TechDocs/40332.pdf - APM Vol2,
ch-15.20 - "Event Injection".

Santosh Shukla (8):
  x86/cpu: Add CPUID feature bit for VNMI
  KVM: SVM: Add VNMI bit definition
  KVM: SVM: Add VNMI support in get/set_nmi_mask
  KVM: SVM: Report NMI not allowed when Guest busy handling VNMI
  KVM: SVM: Add VNMI support in inject_nmi
  KVM: nSVM: implement nested VNMI
  KVM: nSVM: emulate VMEXIT_INVALID case for nested VNMI
  KVM: SVM: Enable VNMI feature

 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/svm.h         |  7 +++
 arch/x86/kvm/svm/nested.c          | 32 ++++++++++++++
 arch/x86/kvm/svm/svm.c             | 44 ++++++++++++++++++-
 arch/x86/kvm/svm/svm.h             | 68 ++++++++++++++++++++++++++++++
 5 files changed, 151 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCHv3 1/8] x86/cpu: Add CPUID feature bit for VNMI

[Santosh Shukla]

VNMI feature allows the hypervisor to inject NMI into the guest w/o
using Event injection mechanism, The benefit of using VNMI over the
event Injection that does not require tracking the Guest's NMI state and
intercepting the IRET for the NMI completion. VNMI achieves that by
exposing 3 capability bits in VMCB intr_cntrl which helps with
virtualizing NMI injection and NMI_Masking.

The presence of this feature is indicated via the CPUID function
0x8000000A_EDX[25].

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 14ed039dff55..3d984fd9d41b 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -355,6 +355,7 @@
 #define X86_FEATURE_VGIF		(15*32+16) /* Virtual GIF */
 #define X86_FEATURE_X2AVIC		(15*32+18) /* Virtual x2apic */
 #define X86_FEATURE_V_SPEC_CTRL		(15*32+20) /* Virtual SPEC_CTRL */
+#define X86_FEATURE_V_NMI		(15*32+25) /* Virtual NMI */
 #define X86_FEATURE_SVME_ADDR_CHK	(15*32+28) /* "" SVME addr check */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
-- 
2.25.1

[PATCHv3 2/8] KVM: SVM: Add VNMI bit definition

[Santosh Shukla]

VNMI exposes 3 capability bits (V_NMI, V_NMI_MASK, and V_NMI_ENABLE) to
virtualize NMI and NMI_MASK, Those capability bits are part of
VMCB::intr_ctrl -
V_NMI(11) - Indicates whether a virtual NMI is pending in the guest.
V_NMI_MASK(12) - Indicates whether virtual NMI is masked in the guest.
V_NMI_ENABLE(26) - Enables the NMI virtualization feature for the guest.

When Hypervisor wants to inject NMI, it will set V_NMI bit, Processor
will clear the V_NMI bit and Set the V_NMI_MASK which means the Guest is
handling NMI, After the guest handled the NMI, The processor will clear
the V_NMI_MASK on the successful completion of IRET instruction Or if
VMEXIT occurs while delivering the virtual NMI.

To enable the VNMI capability, Hypervisor need to program
V_NMI_ENABLE bit 1.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 arch/x86/include/asm/svm.h | 7 +++++++
 arch/x86/kvm/svm/svm.c     | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 0361626841bc..73bf97e04fe3 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -198,6 +198,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 #define X2APIC_MODE_SHIFT 30
 #define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT)
 
+#define V_NMI_PENDING_SHIFT 11
+#define V_NMI_PENDING (1 << V_NMI_PENDING_SHIFT)
+#define V_NMI_MASK_SHIFT 12
+#define V_NMI_MASK (1 << V_NMI_MASK_SHIFT)
+#define V_NMI_ENABLE_SHIFT 26
+#define V_NMI_ENABLE (1 << V_NMI_ENABLE_SHIFT)
+
 #define LBR_CTL_ENABLE_MASK BIT_ULL(0)
 #define VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK BIT_ULL(1)
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 38f873cb6f2c..0259b909ed16 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -229,6 +229,8 @@ module_param(dump_invalid_vmcb, bool, 0644);
 bool intercept_smi = true;
 module_param(intercept_smi, bool, 0444);
 
+bool vnmi = true;
+module_param(vnmi, bool, 0444);
 
 static bool svm_gp_erratum_intercept = true;
 
@@ -5060,6 +5062,10 @@ static __init int svm_hardware_setup(void)
 		svm_x86_ops.vcpu_get_apicv_inhibit_reasons = NULL;
 	}
 
+	vnmi = vnmi && boot_cpu_has(X86_FEATURE_V_NMI);
+	if (vnmi)
+		pr_info("V_NMI enabled\n");
+
 	if (vls) {
 		if (!npt_enabled ||
 		    !boot_cpu_has(X86_FEATURE_V_VMSAVE_VMLOAD) ||
-- 
2.25.1

[PATCHv3 3/8] KVM: SVM: Add VNMI support in get/set_nmi_mask

[Santosh Shukla]

VMCB intr_ctrl bit12 (V_NMI_MASK) is set by the processor when handling
NMI in guest and is cleared after the NMI is handled. Treat V_NMI_MASK
as read-only in the hypervisor except for the SMM case where hypervisor
before entring and after leaving SMM mode requires to set and unset
V_NMI_MASK.

Adding API(get_vnmi_vmcb) in order to return the correct vmcb for L1 or
L2, and also API(clear/set_vnmi_mask) to clear and set mask.

Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
v3:
* Handle SMM case
* Added set/clear_vnmi_mask() API.

v2:
- Added get_vnmi_vmcb API to return vmcb for l1 and l2.
- Use get_vnmi_vmcb to get correct vmcb in func -
  is_vnmi_enabled/_mask_set()
- removed vnmi check from is_vnmi_enabled() func.

 arch/x86/kvm/svm/svm.c | 17 +++++++++++++-
 arch/x86/kvm/svm/svm.h | 52 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 0259b909ed16..f0ac197fd965 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3621,13 +3621,28 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu, bool for_injection)
 
 static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
 {
-	return !!(vcpu->arch.hflags & HF_NMI_MASK);
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (is_vnmi_enabled(svm))
+		return is_vnmi_mask_set(svm);
+	else
+		return !!(vcpu->arch.hflags & HF_NMI_MASK);
 }
 
 static void svm_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (is_vnmi_enabled(svm)) {
+		if (is_smm(vcpu)) {
+			if (masked)
+				set_vnmi_mask(svm);
+			else
+				clear_vnmi_mask(svm);
+		}
+		return;
+	}
+
 	if (masked) {
 		vcpu->arch.hflags |= HF_NMI_MASK;
 		if (!sev_es_guest(vcpu->kvm))
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 6a7686bf6900..cc98ec7bd119 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -35,6 +35,7 @@ extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
 extern bool npt_enabled;
 extern int vgif;
 extern bool intercept_smi;
+extern bool vnmi;
 
 enum avic_modes {
 	AVIC_MODE_NONE = 0,
@@ -532,6 +533,57 @@ static inline bool is_x2apic_msrpm_offset(u32 offset)
 	       (msr < (APIC_BASE_MSR + 0x100));
 }
 
+static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
+{
+	if (!vnmi)
+		return NULL;
+
+	if (is_guest_mode(&svm->vcpu))
+		return svm->nested.vmcb02.ptr;
+	else
+		return svm->vmcb01.ptr;
+}
+
+static inline bool is_vnmi_enabled(struct vcpu_svm *svm)
+{
+	struct vmcb *vmcb = get_vnmi_vmcb(svm);
+
+	if (vmcb)
+		return !!(vmcb->control.int_ctl & V_NMI_ENABLE);
+	else
+		return false;
+}
+
+static inline bool is_vnmi_mask_set(struct vcpu_svm *svm)
+{
+	struct vmcb *vmcb = get_vnmi_vmcb(svm);
+
+	if (vmcb)
+		return !!(vmcb->control.int_ctl & V_NMI_MASK);
+	else
+		return false;
+}
+
+static inline void set_vnmi_mask(struct vcpu_svm *svm)
+{
+	struct vmcb *vmcb = get_vnmi_vmcb(svm);
+
+	if (vmcb)
+		vmcb->control.int_ctl |= V_NMI_MASK;
+	else
+		svm->vcpu.arch.hflags |= HF_GIF_MASK;
+}
+
+static inline void clear_vnmi_mask(struct vcpu_svm *svm)
+{
+	struct vmcb *vmcb = get_vnmi_vmcb(svm);
+
+	if (vmcb)
+		vmcb->control.int_ctl &= ~V_NMI_MASK;
+	else
+		svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
+}
+
 /* svm.c */
 #define MSR_INVALID				0xffffffffU
 
-- 
2.25.1

[PATCHv3 4/8] KVM: SVM: Report NMI not allowed when Guest busy handling VNMI

[Santosh Shukla]

In the VNMI case, Report NMI is not allowed when V_NMI_PENDING is set
which mean virtual NMI already pended for Guest to process while
the Guest is busy handling the current virtual NMI. The Guest
will first finish handling the current virtual NMI and then it will
take the pended event w/o vmexit.

Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
v3:
- Added is_vnmi_pending_set API so to check the vnmi pending state.
- Replaced is_vnmi_mask_set check with is_vnmi_pending_set.

v2:
- Moved vnmi check after is_guest_mode() in func _nmi_blocked().
- Removed is_vnmi_mask_set check from _enable_nmi_window().
as it was a redundent check.

 arch/x86/kvm/svm/svm.c |  6 ++++++
 arch/x86/kvm/svm/svm.h | 10 ++++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index f0ac197fd965..e260e8cb0c81 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3598,6 +3598,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
 	if (is_guest_mode(vcpu) && nested_exit_on_nmi(svm))
 		return false;
 
+	if (is_vnmi_enabled(svm) && is_vnmi_pending_set(svm))
+		return true;
+
 	ret = (vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) ||
 	      (vcpu->arch.hflags & HF_NMI_MASK);
 
@@ -3734,6 +3737,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (is_vnmi_enabled(svm) && is_vnmi_pending_set(svm))
+		return;
+
 	if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
 		return; /* IRET will cause a vm exit */
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index cc98ec7bd119..7857a89d0ec8 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -584,6 +584,16 @@ static inline void clear_vnmi_mask(struct vcpu_svm *svm)
 		svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
 }
 
+static inline bool is_vnmi_pending_set(struct vcpu_svm *svm)
+{
+	struct vmcb *vmcb = get_vnmi_vmcb(svm);
+
+	if (vmcb)
+		return !!(vmcb->control.int_ctl & V_NMI_PENDING);
+	else
+		return false;
+}
+
 /* svm.c */
 #define MSR_INVALID				0xffffffffU
 
-- 
2.25.1

[PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Santosh Shukla]

Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
will clear V_NMI to acknowledge processing has started and will keep the
V_NMI_MASK set until the processor is done with processing the NMI event.

Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
v3: 
- Removed WARN_ON check.

v2:
- Added WARN_ON check for vnmi pending.
- use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.

 arch/x86/kvm/svm/svm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e260e8cb0c81..8c4098b8a63e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
 static void svm_inject_nmi(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
+	struct vmcb *vmcb = NULL;
 
+	if (is_vnmi_enabled(svm)) {
+		vmcb = get_vnmi_vmcb(svm);
+		vmcb->control.int_ctl |= V_NMI_PENDING;
+		++vcpu->stat.nmi_injections;
+		return;
+	}
 	svm->vmcb->control.event_inj = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_NMI;
 
 	if (svm->nmi_l1_to_l2)
-- 
2.25.1

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Maciej S. Szmigiero]

On 10.08.2022 08:12, Santosh Shukla wrote:
> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
> will clear V_NMI to acknowledge processing has started and will keep the
> V_NMI_MASK set until the processor is done with processing the NMI event.
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> ---
> v3:
> - Removed WARN_ON check.
> 
> v2:
> - Added WARN_ON check for vnmi pending.
> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
> 
>   arch/x86/kvm/svm/svm.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e260e8cb0c81..8c4098b8a63e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>   static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>   {
>   	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct vmcb *vmcb = NULL;
>   
> +	if (is_vnmi_enabled(svm)) {

I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
since if nmi_l1_to_l2 is true then the NMI to be injected originally
comes from L1's VMCB12 EVENTINJ field.

As you said in the cover letter, this field has different semantics
than vNMI - specifically, it should allow injecting even if L2 is in
NMI blocking state (it's then up to L1 to keep track of NMI injectability
for its L2 guest - so L0 should be transparently injecting it when L1
wants so).

> +		vmcb = get_vnmi_vmcb(svm);
> +		vmcb->control.int_ctl |= V_NMI_PENDING;
> +		++vcpu->stat.nmi_injections;
> +		return;
> +	}
>   	svm->vmcb->control.event_inj = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_NMI;
>   
>   	if (svm->nmi_l1_to_l2)

Thanks,
Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Shukla, Santosh]

Hi Maciej,

On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
> On 10.08.2022 08:12, Santosh Shukla wrote:
>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>> will clear V_NMI to acknowledge processing has started and will keep the
>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>> ---
>> v3:
>> - Removed WARN_ON check.
>>
>> v2:
>> - Added WARN_ON check for vnmi pending.
>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>
>>   arch/x86/kvm/svm/svm.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index e260e8cb0c81..8c4098b8a63e 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>   static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>   {
>>       struct vcpu_svm *svm = to_svm(vcpu);
>> +    struct vmcb *vmcb = NULL;
>>   +    if (is_vnmi_enabled(svm)) {
> 
> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
> since if nmi_l1_to_l2 is true then the NMI to be injected originally
> comes from L1's VMCB12 EVENTINJ field.
> 

Not sure if I understood the case fully.. so trying to sketch scenario here -
if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
be one of following case -
1) L0 (vnmi enabled) and L1 (vnmi disabled)
2) L0 & L1 both vnmi disabled.

In both cases the vnmi check will fail for L1 and execution path
will fall back to default - right?

Thanks,
Santosh

> As you said in the cover letter, this field has different semantics
> than vNMI - specifically, it should allow injecting even if L2 is in
> NMI blocking state (it's then up to L1 to keep track of NMI injectability
> for its L2 guest - so L0 should be transparently injecting it when L1
> wants so).
> 
>> +        vmcb = get_vnmi_vmcb(svm);
>> +        vmcb->control.int_ctl |= V_NMI_PENDING;
>> +        ++vcpu->stat.nmi_injections;
>> +        return;
>> +    }
>>       svm->vmcb->control.event_inj = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_NMI;
>>         if (svm->nmi_l1_to_l2)
> 
> Thanks,
> Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Maciej S. Szmigiero]

On 24.08.2022 14:13, Shukla, Santosh wrote:
> Hi Maciej,
> 
> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>> will clear V_NMI to acknowledge processing has started and will keep the
>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>
>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>> ---
>>> v3:
>>> - Removed WARN_ON check.
>>>
>>> v2:
>>> - Added WARN_ON check for vnmi pending.
>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>
>>>    arch/x86/kvm/svm/svm.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index e260e8cb0c81..8c4098b8a63e 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>    static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>    {
>>>        struct vcpu_svm *svm = to_svm(vcpu);
>>> +    struct vmcb *vmcb = NULL;
>>>    +    if (is_vnmi_enabled(svm)) {
>>
>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>> comes from L1's VMCB12 EVENTINJ field.
>>
> 
> Not sure if I understood the case fully.. so trying to sketch scenario here -
> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
> be one of following case -
> 1) L0 (vnmi enabled) and L1 (vnmi disabled)

As far as I can see in this case:
is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.

This field in VMCB02 comes from nested_vmcb02_prepare_control() which
in the !nested_vnmi_enabled() case (L1 is not using vNMI) copies these bits
from VMCB01:
> int_ctl_vmcb01_bits |= (V_NMI_PENDING | V_NMI_ENABLE | V_NMI_MASK);

So in this case (L0 uses vNMI) V_NMI_ENABLE will be set in VMCB01, right?

This bit will then be copied to VMCB02 so re-injection will attempt to use
vNMI instead of EVTINJ.

> 2) L0 & L1 both vnmi disabled.

This case is ok.

> 
> In both cases the vnmi check will fail for L1 and execution path
> will fall back to default - right?
> 
> Thanks,
> Santosh

Thanks,
Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Shukla, Santosh]

On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
> On 24.08.2022 14:13, Shukla, Santosh wrote:
>> Hi Maciej,
>>
>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>
>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>> ---
>>>> v3:
>>>> - Removed WARN_ON check.
>>>>
>>>> v2:
>>>> - Added WARN_ON check for vnmi pending.
>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>
>>>>    arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>> --- a/arch/x86/kvm/svm/svm.c
>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>    static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>    {
>>>>        struct vcpu_svm *svm = to_svm(vcpu);
>>>> +    struct vmcb *vmcb = NULL;
>>>>    +    if (is_vnmi_enabled(svm)) {
>>>
>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>> comes from L1's VMCB12 EVENTINJ field.
>>>
>>
>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>> be one of following case -
>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
> 
> As far as I can see in this case:
> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
> 

For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
execution path will opt EVTINJ model for re-injection.

Thanks,
Santosh

> This field in VMCB02 comes from nested_vmcb02_prepare_control() which
> in the !nested_vnmi_enabled() case (L1 is not using vNMI) copies these bits
> from VMCB01:
>> int_ctl_vmcb01_bits |= (V_NMI_PENDING | V_NMI_ENABLE | V_NMI_MASK);
> 
> So in this case (L0 uses vNMI) V_NMI_ENABLE will be set in VMCB01, right?
> 
> This bit will then be copied to VMCB02 so re-injection will attempt to use
> vNMI instead of EVTINJ.
> 
>> 2) L0 & L1 both vnmi disabled.
> 
> This case is ok.
> 
>>
>> In both cases the vnmi check will fail for L1 and execution path
>> will fall back to default - right?
>>
>> Thanks,
>> Santosh
> 
> Thanks,
> Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Maciej S. Szmigiero]

On 25.08.2022 12:56, Shukla, Santosh wrote:
> On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
>> On 24.08.2022 14:13, Shukla, Santosh wrote:
>>> Hi Maciej,
>>>
>>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>>
>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>> ---
>>>>> v3:
>>>>> - Removed WARN_ON check.
>>>>>
>>>>> v2:
>>>>> - Added WARN_ON check for vnmi pending.
>>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>>
>>>>>     arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>>     1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>>     static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>>     {
>>>>>         struct vcpu_svm *svm = to_svm(vcpu);
>>>>> +    struct vmcb *vmcb = NULL;
>>>>>     +    if (is_vnmi_enabled(svm)) {
>>>>
>>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>>> comes from L1's VMCB12 EVENTINJ field.
>>>>
>>>
>>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>>> be one of following case -
>>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
>>
>> As far as I can see in this case:
>> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
>>
> 
> For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
> execution path will opt EVTINJ model for re-injection.

I guess by "get_vnmi_vmcb() will return false" you mean it will return NULL,
since this function returns a pointer, not a bool.

I can't see however, how this will happen:
>static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
>{
>	if (!vnmi)
>		return NULL;
         ^ "vnmi" variable controls whether L0 uses vNMI,
	   so this variable is true in our case

>
>	if (is_guest_mode(&svm->vcpu))
>		return svm->nested.vmcb02.ptr;
		^ this should be always non-NULL.

So get_vnmi_vmcb() will return VMCB02 pointer in our case, not NULL...

> 
> Thanks,
> Santosh
> 
>> This field in VMCB02 comes from nested_vmcb02_prepare_control() which
>> in the !nested_vnmi_enabled() case (L1 is not using vNMI) copies these bits
>> from VMCB01:
>>> int_ctl_vmcb01_bits |= (V_NMI_PENDING | V_NMI_ENABLE | V_NMI_MASK);
>>
>> So in this case (L0 uses vNMI) V_NMI_ENABLE will be set in VMCB01, right?
>>
>> This bit will then be copied to VMCB02 

... and due to the above is_vnmi_enabled() will return true, so
re-injection will attempt to use vNMI instead of EVTINJ (wrong).

>>> 2) L0 & L1 both vnmi disabled.
>>
>> This case is ok.
>>
>>>
>>> In both cases the vnmi check will fail for L1 and execution path
>>> will fall back to default - right?
>>>
>>> Thanks,
>>> Santosh
>>

Thanks,
Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Shukla, Santosh]

On 8/25/2022 6:15 PM, Maciej S. Szmigiero wrote:
> On 25.08.2022 12:56, Shukla, Santosh wrote:
>> On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
>>> On 24.08.2022 14:13, Shukla, Santosh wrote:
>>>> Hi Maciej,
>>>>
>>>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>>>
>>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>>> ---
>>>>>> v3:
>>>>>> - Removed WARN_ON check.
>>>>>>
>>>>>> v2:
>>>>>> - Added WARN_ON check for vnmi pending.
>>>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>>>
>>>>>>     arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>>>     static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>>>     {
>>>>>>         struct vcpu_svm *svm = to_svm(vcpu);
>>>>>> +    struct vmcb *vmcb = NULL;
>>>>>>     +    if (is_vnmi_enabled(svm)) {
>>>>>
>>>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>>>> comes from L1's VMCB12 EVENTINJ field.
>>>>>
>>>>
>>>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>>>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>>>> be one of following case -
>>>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
>>>
>>> As far as I can see in this case:
>>> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
>>>
>>
>> For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
>> execution path will opt EVTINJ model for re-injection.
> 
> I guess by "get_vnmi_vmcb() will return false" you mean it will return NULL,
> since this function returns a pointer, not a bool.
> 

Yes, I meant is_vnmi_enabled() will return false if vnmi param is unset.

> I can't see however, how this will happen:
>> static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
>> {
>>     if (!vnmi)
>>         return NULL;
>         ^ "vnmi" variable controls whether L0 uses vNMI,
>        so this variable is true in our case
> 

No.

In L1 case (vnmi disabled) - vnmi param will be false.
In L0 case (vnmi enabled) - vnmi param will be true.

So in L1 case, is_vnmi_enabled() will return false and
in L0 case will return true.

Thanks,
Santosh
>>
>>     if (is_guest_mode(&svm->vcpu))
>>         return svm->nested.vmcb02.ptr;
>         ^ this should be always non-NULL.
> 
> So get_vnmi_vmcb() will return VMCB02 pointer in our case, not NULL...
> 
>>
>> Thanks,
>> Santosh
>>
>>> This field in VMCB02 comes from nested_vmcb02_prepare_control() which
>>> in the !nested_vnmi_enabled() case (L1 is not using vNMI) copies these bits
>>> from VMCB01:
>>>> int_ctl_vmcb01_bits |= (V_NMI_PENDING | V_NMI_ENABLE | V_NMI_MASK);
>>>
>>> So in this case (L0 uses vNMI) V_NMI_ENABLE will be set in VMCB01, right?
>>>
>>> This bit will then be copied to VMCB02 
> 
> ... and due to the above is_vnmi_enabled() will return true, so
> re-injection will attempt to use vNMI instead of EVTINJ (wrong).
> 
>>>> 2) L0 & L1 both vnmi disabled.
>>>
>>> This case is ok.
>>>
>>>>
>>>> In both cases the vnmi check will fail for L1 and execution path
>>>> will fall back to default - right?
>>>>
>>>> Thanks,
>>>> Santosh
>>>
> 
> Thanks,
> Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Maciej S. Szmigiero]

On 25.08.2022 16:05, Shukla, Santosh wrote:
> On 8/25/2022 6:15 PM, Maciej S. Szmigiero wrote:
>> On 25.08.2022 12:56, Shukla, Santosh wrote:
>>> On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
>>>> On 24.08.2022 14:13, Shukla, Santosh wrote:
>>>>> Hi Maciej,
>>>>>
>>>>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>>>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>>>>
>>>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>>>> ---
>>>>>>> v3:
>>>>>>> - Removed WARN_ON check.
>>>>>>>
>>>>>>> v2:
>>>>>>> - Added WARN_ON check for vnmi pending.
>>>>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>>>>
>>>>>>>      arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>>>>      1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>>>>      static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>>>>      {
>>>>>>>          struct vcpu_svm *svm = to_svm(vcpu);
>>>>>>> +    struct vmcb *vmcb = NULL;
>>>>>>>      +    if (is_vnmi_enabled(svm)) {
>>>>>>
>>>>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>>>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>>>>> comes from L1's VMCB12 EVENTINJ field.
>>>>>>
>>>>>
>>>>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>>>>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>>>>> be one of following case -
>>>>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
>>>>
>>>> As far as I can see in this case:
>>>> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
>>>>
>>>
>>> For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
>>> execution path will opt EVTINJ model for re-injection.
>>
>> I guess by "get_vnmi_vmcb() will return false" you mean it will return NULL,
>> since this function returns a pointer, not a bool.
>>
> 
> Yes, I meant is_vnmi_enabled() will return false if vnmi param is unset.
> 
>> I can't see however, how this will happen:
>>> static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
>>> {
>>>      if (!vnmi)
>>>          return NULL;
>>          ^ "vnmi" variable controls whether L0 uses vNMI,
>>         so this variable is true in our case
>>
> 
> No.
> 
> In L1 case (vnmi disabled) - vnmi param will be false.

Perhaps there was a misunderstanding here - the case here
isn't the code under discussion running as L1, but as L0
where L1 not using vNMI - L1 here can be an old version of KVM,
or Hyper-V, or any other hypervisor.

In this case L0 is re-injecting an EVENTINJ NMI into L2 on
the behalf of L1.
That's when "nmi_l1_to_l2" is true.

Since the code is physically running on L0 (which makes use of vNMI)
it has the "vnmi" param set.

So is_vnmi_enabled() will return true and vNMI will be used
for the re-injection instead of the required EVENTINJ.

> In L0 case (vnmi enabled) - vnmi param will be true.
> 
> So in L1 case, is_vnmi_enabled() will return false and
> in L0 case will return true.
> 
> Thanks,
> Santosh

Thanks,
Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Shukla, Santosh]

On 8/25/2022 7:46 PM, Maciej S. Szmigiero wrote:
> On 25.08.2022 16:05, Shukla, Santosh wrote:
>> On 8/25/2022 6:15 PM, Maciej S. Szmigiero wrote:
>>> On 25.08.2022 12:56, Shukla, Santosh wrote:
>>>> On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
>>>>> On 24.08.2022 14:13, Shukla, Santosh wrote:
>>>>>> Hi Maciej,
>>>>>>
>>>>>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>>>>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>>>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>>>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>>>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>>>>>
>>>>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>>>>> ---
>>>>>>>> v3:
>>>>>>>> - Removed WARN_ON check.
>>>>>>>>
>>>>>>>> v2:
>>>>>>>> - Added WARN_ON check for vnmi pending.
>>>>>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>>>>>
>>>>>>>>      arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>>>>>      1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>>>>>      static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>>>>>      {
>>>>>>>>          struct vcpu_svm *svm = to_svm(vcpu);
>>>>>>>> +    struct vmcb *vmcb = NULL;
>>>>>>>>      +    if (is_vnmi_enabled(svm)) {
>>>>>>>
>>>>>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>>>>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>>>>>> comes from L1's VMCB12 EVENTINJ field.
>>>>>>>
>>>>>>
>>>>>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>>>>>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>>>>>> be one of following case -
>>>>>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
>>>>>
>>>>> As far as I can see in this case:
>>>>> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
>>>>>
>>>>
>>>> For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
>>>> execution path will opt EVTINJ model for re-injection.
>>>
>>> I guess by "get_vnmi_vmcb() will return false" you mean it will return NULL,
>>> since this function returns a pointer, not a bool.
>>>
>>
>> Yes, I meant is_vnmi_enabled() will return false if vnmi param is unset.
>>
>>> I can't see however, how this will happen:
>>>> static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
>>>> {
>>>>      if (!vnmi)
>>>>          return NULL;
>>>          ^ "vnmi" variable controls whether L0 uses vNMI,
>>>         so this variable is true in our case
>>>
>>
>> No.
>>
>> In L1 case (vnmi disabled) - vnmi param will be false.
> 
> Perhaps there was a misunderstanding here - the case here
> isn't the code under discussion running as L1, but as L0
> where L1 not using vNMI - L1 here can be an old version of KVM,
> or Hyper-V, or any other hypervisor.
> 

Ok.

> In this case L0 is re-injecting an EVENTINJ NMI into L2 on
> the behalf of L1.
> That's when "nmi_l1_to_l2" is true.
 
hmm,. trying to understand the event re-injection flow -
First L1 (non-vnmi) injecting event to L2 guest, in-turn
intercepted by L0, L0 sees event injection through EVTINJ
so sets the 'nmi_l1_to_l2' var and then L0 calls svm_inject_nmi()
to re-inject event in L2 - is that correct (nmi_l1_to_l2) flow?

Thanks,.
Santosh
> Since the code is physically running on L0 (which makes use of vNMI)
> it has the "vnmi" param set.
> 
> So is_vnmi_enabled() will return true and vNMI will be used
> for the re-injection instead of the required EVENTINJ.
> 
>> In L0 case (vnmi enabled) - vnmi param will be true.
>>
>> So in L1 case, is_vnmi_enabled() will return false and
>> in L0 case will return true.
>>
>> Thanks,
>> Santosh
> 
> Thanks,
> Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Maciej S. Szmigiero]

On 26.08.2022 11:35, Shukla, Santosh wrote:
> On 8/25/2022 7:46 PM, Maciej S. Szmigiero wrote:
>> On 25.08.2022 16:05, Shukla, Santosh wrote:
>>> On 8/25/2022 6:15 PM, Maciej S. Szmigiero wrote:
>>>> On 25.08.2022 12:56, Shukla, Santosh wrote:
>>>>> On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
>>>>>> On 24.08.2022 14:13, Shukla, Santosh wrote:
>>>>>>> Hi Maciej,
>>>>>>>
>>>>>>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>>>>>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>>>>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>>>>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>>>>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>>>>>> ---
>>>>>>>>> v3:
>>>>>>>>> - Removed WARN_ON check.
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>> - Added WARN_ON check for vnmi pending.
>>>>>>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>>>>>>
>>>>>>>>>       arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>>>>>>       1 file changed, 7 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>>>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>>>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>>>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>>>>>>       static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>>>>>>       {
>>>>>>>>>           struct vcpu_svm *svm = to_svm(vcpu);
>>>>>>>>> +    struct vmcb *vmcb = NULL;
>>>>>>>>>       +    if (is_vnmi_enabled(svm)) {
>>>>>>>>
>>>>>>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>>>>>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>>>>>>> comes from L1's VMCB12 EVENTINJ field.
>>>>>>>>
>>>>>>>
>>>>>>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>>>>>>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>>>>>>> be one of following case -
>>>>>>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
>>>>>>
>>>>>> As far as I can see in this case:
>>>>>> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
>>>>>>
>>>>>
>>>>> For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
>>>>> execution path will opt EVTINJ model for re-injection.
>>>>
>>>> I guess by "get_vnmi_vmcb() will return false" you mean it will return NULL,
>>>> since this function returns a pointer, not a bool.
>>>>
>>>
>>> Yes, I meant is_vnmi_enabled() will return false if vnmi param is unset.
>>>
>>>> I can't see however, how this will happen:
>>>>> static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
>>>>> {
>>>>>       if (!vnmi)
>>>>>           return NULL;
>>>>           ^ "vnmi" variable controls whether L0 uses vNMI,
>>>>          so this variable is true in our case
>>>>
>>>
>>> No.
>>>
>>> In L1 case (vnmi disabled) - vnmi param will be false.
>>
>> Perhaps there was a misunderstanding here - the case here
>> isn't the code under discussion running as L1, but as L0
>> where L1 not using vNMI - L1 here can be an old version of KVM,
>> or Hyper-V, or any other hypervisor.
>>
> 
> Ok.
> 
>> In this case L0 is re-injecting an EVENTINJ NMI into L2 on
>> the behalf of L1.
>> That's when "nmi_l1_to_l2" is true.
>   
> hmm,. trying to understand the event re-injection flow -
> First L1 (non-vnmi) injecting event to L2 guest, in-turn
> intercepted by L0, 

That's right, the L1's VMRUN of L2 gets intercepted by L0.

> L0 sees event injection through EVTINJ

It sees that L1 wants to inject an NMI into L2 via VMCB12 EVTINJ.

> so sets the 'nmi_l1_to_l2' var 

That's right, L0 needs to keep track of this fact.

> and then L0 calls svm_inject_nmi()

Not yet - at this point svm_inject_nmi() is NOT called
(rather than, VMCB12 EVTINJ is directly copied into VMCB02 EVTINJ).

Now L0 does the actual VMRUN of L2.

Let's say that there is an intervening VMExit during delivery of
that NMI to L2, of type which is handled by L0 (perhaps a NPF on
L2 IDT or so).

In this case the NMI will be returned in VMCB02 EXITINTINFO and
needs to be re-injected into L2 on the next VMRUN,
again via EVTINJ.

That's when svm_inject_nmi() will get called to re-inject
that NMI.

> to re-inject event in L2 - is that correct (nmi_l1_to_l2) flow?
Hope the flow is clear now.

> 
> Thanks,.
> Santosh

Thanks,
Maciej

Re: [PATCHv3 5/8] KVM: SVM: Add VNMI support in inject_nmi

[Shukla, Santosh]

On 8/26/2022 5:50 PM, Maciej S. Szmigiero wrote:
> On 26.08.2022 11:35, Shukla, Santosh wrote:
>> On 8/25/2022 7:46 PM, Maciej S. Szmigiero wrote:
>>> On 25.08.2022 16:05, Shukla, Santosh wrote:
>>>> On 8/25/2022 6:15 PM, Maciej S. Szmigiero wrote:
>>>>> On 25.08.2022 12:56, Shukla, Santosh wrote:
>>>>>> On 8/24/2022 6:26 PM, Maciej S. Szmigiero wrote:
>>>>>>> On 24.08.2022 14:13, Shukla, Santosh wrote:
>>>>>>>> Hi Maciej,
>>>>>>>>
>>>>>>>> On 8/11/2022 2:54 AM, Maciej S. Szmigiero wrote:
>>>>>>>>> On 10.08.2022 08:12, Santosh Shukla wrote:
>>>>>>>>>> Inject the NMI by setting V_NMI in the VMCB interrupt control. processor
>>>>>>>>>> will clear V_NMI to acknowledge processing has started and will keep the
>>>>>>>>>> V_NMI_MASK set until the processor is done with processing the NMI event.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>>>>>>> ---
>>>>>>>>>> v3:
>>>>>>>>>> - Removed WARN_ON check.
>>>>>>>>>>
>>>>>>>>>> v2:
>>>>>>>>>> - Added WARN_ON check for vnmi pending.
>>>>>>>>>> - use `get_vnmi_vmcb` to get correct vmcb so to inject vnmi.
>>>>>>>>>>
>>>>>>>>>>       arch/x86/kvm/svm/svm.c | 7 +++++++
>>>>>>>>>>       1 file changed, 7 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>>>>>>> index e260e8cb0c81..8c4098b8a63e 100644
>>>>>>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>>>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>>>>>>> @@ -3479,7 +3479,14 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>>>>>>>>>>       static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>>>>>>>>>       {
>>>>>>>>>>           struct vcpu_svm *svm = to_svm(vcpu);
>>>>>>>>>> +    struct vmcb *vmcb = NULL;
>>>>>>>>>>       +    if (is_vnmi_enabled(svm)) {
>>>>>>>>>
>>>>>>>>> I guess this should be "is_vnmi_enabled(svm) && !svm->nmi_l1_to_l2"
>>>>>>>>> since if nmi_l1_to_l2 is true then the NMI to be injected originally
>>>>>>>>> comes from L1's VMCB12 EVENTINJ field.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not sure if I understood the case fully.. so trying to sketch scenario here -
>>>>>>>> if nmi_l1_to_l2 is true then event is coming from EVTINJ. .which could
>>>>>>>> be one of following case -
>>>>>>>> 1) L0 (vnmi enabled) and L1 (vnmi disabled)
>>>>>>>
>>>>>>> As far as I can see in this case:
>>>>>>> is_vnmi_enabled() returns whether VMCB02's int_ctl has V_NMI_ENABLE bit set.
>>>>>>>
>>>>>>
>>>>>> For L1 with vnmi disabled case - is_vnmi_enabled()->get_vnmi_vmcb() will return false so the
>>>>>> execution path will opt EVTINJ model for re-injection.
>>>>>
>>>>> I guess by "get_vnmi_vmcb() will return false" you mean it will return NULL,
>>>>> since this function returns a pointer, not a bool.
>>>>>
>>>>
>>>> Yes, I meant is_vnmi_enabled() will return false if vnmi param is unset.
>>>>
>>>>> I can't see however, how this will happen:
>>>>>> static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
>>>>>> {
>>>>>>       if (!vnmi)
>>>>>>           return NULL;
>>>>>           ^ "vnmi" variable controls whether L0 uses vNMI,
>>>>>          so this variable is true in our case
>>>>>
>>>>
>>>> No.
>>>>
>>>> In L1 case (vnmi disabled) - vnmi param will be false.
>>>
>>> Perhaps there was a misunderstanding here - the case here
>>> isn't the code under discussion running as L1, but as L0
>>> where L1 not using vNMI - L1 here can be an old version of KVM,
>>> or Hyper-V, or any other hypervisor.
>>>
>>
>> Ok.
>>
>>> In this case L0 is re-injecting an EVENTINJ NMI into L2 on
>>> the behalf of L1.
>>> That's when "nmi_l1_to_l2" is true.
>>   hmm,. trying to understand the event re-injection flow -
>> First L1 (non-vnmi) injecting event to L2 guest, in-turn
>> intercepted by L0, 
> 
> That's right, the L1's VMRUN of L2 gets intercepted by L0.
> 
>> L0 sees event injection through EVTINJ
> 
> It sees that L1 wants to inject an NMI into L2 via VMCB12 EVTINJ.
> 
>> so sets the 'nmi_l1_to_l2' var 
> 
> That's right, L0 needs to keep track of this fact.
> 
>> and then L0 calls svm_inject_nmi()
> 
> Not yet - at this point svm_inject_nmi() is NOT called
> (rather than, VMCB12 EVTINJ is directly copied into VMCB02 EVTINJ).
> 
> Now L0 does the actual VMRUN of L2.
> 
> Let's say that there is an intervening VMExit during delivery of
> that NMI to L2, of type which is handled by L0 (perhaps a NPF on
> L2 IDT or so).
> 
> In this case the NMI will be returned in VMCB02 EXITINTINFO and
> needs to be re-injected into L2 on the next VMRUN,
> again via EVTINJ.
> 
> That's when svm_inject_nmi() will get called to re-inject
> that NMI.
> 
>> to re-inject event in L2 - is that correct (nmi_l1_to_l2) flow?
> Hope the flow is clear now.
> 

Yes, Thank-you for the clarification :).

Santosh.

>>
>> Thanks,.
>> Santosh
> 
> Thanks,
> Maciej

[PATCHv3 6/8] KVM: nSVM: implement nested VNMI

[Santosh Shukla]

In order to support nested VNMI requires saving and restoring the VNMI
bits during nested entry and exit.
In case of L1 and L2 both using VNMI- Copy VNMI bits from vmcb12 to
vmcb02 during entry and vice-versa during exit.
And in case of L1 uses VNMI and L2 doesn't- Copy VNMI bits from vmcb01 to
vmcb02 during entry and vice-versa during exit.

Tested with the KVM-unit-test and Nested Guest scenario.

Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
v3:
- Added code to save and restore VNMI bit for L1 using vnmi and L2
  doesn't for the entry and exit case.

v2:
- Save the V_NMI_PENDING/MASK state in vmcb12 on vmexit.
- Tested nested environment - L1 injecting vnmi to L2.
- Tested vnmi in kvm-unit-test.

 arch/x86/kvm/svm/nested.c | 27 +++++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c    |  5 +++++
 arch/x86/kvm/svm/svm.h    |  6 ++++++
 3 files changed, 38 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 76dcc8a3e849..3d986ec83147 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -662,6 +662,11 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
 	else
 		int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
 
+	if (nested_vnmi_enabled(svm))
+		int_ctl_vmcb12_bits |= (V_NMI_PENDING | V_NMI_ENABLE | V_NMI_MASK);
+	else
+		int_ctl_vmcb01_bits |= (V_NMI_PENDING | V_NMI_ENABLE | V_NMI_MASK);
+
 	/* Copied from vmcb01.  msrpm_base can be overwritten later.  */
 	vmcb02->control.nested_ctl = vmcb01->control.nested_ctl;
 	vmcb02->control.iopm_base_pa = vmcb01->control.iopm_base_pa;
@@ -1010,6 +1015,28 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	vmcb12->control.event_inj         = svm->nested.ctl.event_inj;
 	vmcb12->control.event_inj_err     = svm->nested.ctl.event_inj_err;
 
+	if (nested_vnmi_enabled(svm)) {
+		if (vmcb02->control.int_ctl & V_NMI_MASK)
+			vmcb12->control.int_ctl |= V_NMI_MASK;
+		else
+			vmcb12->control.int_ctl &= ~V_NMI_MASK;
+
+		if (vmcb02->control.int_ctl & V_NMI_PENDING)
+			vmcb12->control.int_ctl |= V_NMI_PENDING;
+		else
+			vmcb12->control.int_ctl &= ~V_NMI_PENDING;
+	} else {
+		if (vmcb02->control.int_ctl & V_NMI_MASK)
+			vmcb01->control.int_ctl |= V_NMI_MASK;
+		else
+			vmcb01->control.int_ctl &= ~V_NMI_MASK;
+
+		if (vmcb02->control.int_ctl & V_NMI_PENDING)
+			vmcb01->control.int_ctl |= V_NMI_PENDING;
+		else
+			vmcb01->control.int_ctl &= ~V_NMI_PENDING;
+	}
+
 	if (!kvm_pause_in_guest(vcpu->kvm)) {
 		vmcb01->control.pause_filter_count = vmcb02->control.pause_filter_count;
 		vmcb_mark_dirty(vmcb01, VMCB_INTERCEPTS);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8c4098b8a63e..c9636e033782 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4217,6 +4217,8 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 
 	svm->vgif_enabled = vgif && guest_cpuid_has(vcpu, X86_FEATURE_VGIF);
 
+	svm->vnmi_enabled = vnmi && guest_cpuid_has(vcpu, X86_FEATURE_V_NMI);
+
 	svm_recalc_instruction_intercepts(vcpu, svm);
 
 	/* For sev guests, the memory encryption bit is not reserved in CR3.  */
@@ -4967,6 +4969,9 @@ static __init void svm_set_cpu_caps(void)
 		if (vgif)
 			kvm_cpu_cap_set(X86_FEATURE_VGIF);
 
+		if (vnmi)
+			kvm_cpu_cap_set(X86_FEATURE_V_NMI);
+
 		/* Nested VM can receive #VMEXIT instead of triggering #GP */
 		kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
 	}
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 7857a89d0ec8..4d6245ef5b6c 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -253,6 +253,7 @@ struct vcpu_svm {
 	bool pause_filter_enabled         : 1;
 	bool pause_threshold_enabled      : 1;
 	bool vgif_enabled                 : 1;
+	bool vnmi_enabled                 : 1;
 
 	u32 ldr_reg;
 	u32 dfr_reg;
@@ -533,6 +534,11 @@ static inline bool is_x2apic_msrpm_offset(u32 offset)
 	       (msr < (APIC_BASE_MSR + 0x100));
 }
 
+static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
+{
+	return svm->vnmi_enabled && (svm->nested.ctl.int_ctl & V_NMI_ENABLE);
+}
+
 static inline struct vmcb *get_vnmi_vmcb(struct vcpu_svm *svm)
 {
 	if (!vnmi)
-- 
2.25.1

[PATCHv3 7/8] KVM: nSVM: emulate VMEXIT_INVALID case for nested VNMI

[Santosh Shukla]

If NMI virtualization enabled and NMI_INTERCEPT is unset then next vm
entry will exit with #INVALID exit reason.

In order to emulate above (VMEXIT(#INVALID)) scenario for nested
environment, extending check for V_NMI_ENABLE, NMI_INTERCEPT bit in func
__nested_vmcb_check_controls.

Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 arch/x86/kvm/svm/nested.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 3d986ec83147..9d031fadcd67 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -296,6 +296,11 @@ static bool __nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
 	if (CC(!nested_svm_check_tlb_ctl(vcpu, control->tlb_ctl)))
 		return false;
 
+	if (CC((control->int_ctl & V_NMI_ENABLE) &&
+		!vmcb12_is_intercept(control, INTERCEPT_NMI))) {
+		return false;
+	}
+
 	return true;
 }
 
-- 
2.25.1

[PATCHv3 8/8] KVM: SVM: Enable VNMI feature

[Santosh Shukla]

Enable the NMI virtualization (V_NMI_ENABLE) in the VMCB interrupt
control when the vnmi module parameter is set.

Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 arch/x86/kvm/svm/svm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c9636e033782..eefb4d5428c4 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1309,6 +1309,9 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
 	if (kvm_vcpu_apicv_active(vcpu))
 		avic_init_vmcb(svm, vmcb);
 
+	if (vnmi)
+		svm->vmcb->control.int_ctl |= V_NMI_ENABLE;
+
 	if (vgif) {
 		svm_clr_intercept(svm, INTERCEPT_STGI);
 		svm_clr_intercept(svm, INTERCEPT_CLGI);
-- 
2.25.1