[PATCH 0/7] Virtual NMI feature

[PATCH 0/7] Virtual NMI feature

[Santosh Shukla]

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.

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

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

Santosh Shukla (7):
  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: SVM: Enable VNMI feature

 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/svm.h         |  7 +++++
 arch/x86/kvm/svm/nested.c          |  8 +++++
 arch/x86/kvm/svm/svm.c             | 47 ++++++++++++++++++++++++++++--
 arch/x86/kvm/svm/svm.h             |  1 +
 5 files changed, 62 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/7] 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].

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 393f2bbb5e3a..c8775b25856b 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -346,6 +346,7 @@
 #define X86_FEATURE_V_VMSAVE_VMLOAD	(15*32+15) /* Virtual VMSAVE VMLOAD */
 #define X86_FEATURE_VGIF		(15*32+16) /* Virtual GIF */
 #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

[PATCH 2/7] 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.

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 1b07fba11704..22d918555df0 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -195,6 +195,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 #define AVIC_ENABLE_SHIFT 31
 #define AVIC_ENABLE_MASK (1 << AVIC_ENABLE_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 200045f71df0..860f28c668bd 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -198,6 +198,8 @@ module_param(dump_invalid_vmcb, bool, 0644);
 bool intercept_smi = true;
 module_param(intercept_smi, bool, 0444);
 
+static bool vnmi;
+module_param(vnmi, bool, 0444);
 
 static bool svm_gp_erratum_intercept = true;
 
@@ -4930,6 +4932,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

[PATCH 3/7] 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 and do not populate set accessors.

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

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 860f28c668bd..d67a54517d95 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
 	return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
 }
 
+static bool is_vnmi_enabled(struct vmcb *vmcb)
+{
+	return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
+}
+
+static bool is_vnmi_mask_set(struct vmcb *vmcb)
+{
+	return !!(vmcb->control.int_ctl & V_NMI_MASK);
+}
+
 static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -3502,13 +3512,21 @@ 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->vmcb))
+		return is_vnmi_mask_set(svm->vmcb);
+	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->vmcb))
+		return;
+
 	if (masked) {
 		vcpu->arch.hflags |= HF_NMI_MASK;
 		if (!sev_es_guest(vcpu->kvm))
-- 
2.25.1

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

[Santosh Shukla]

In the VNMI case, Report NMI is not allowed when the processor set the
V_NMI_MASK to 1 which means the Guest is busy handling VNMI.

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

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d67a54517d95..a405e414cae4 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3483,6 +3483,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
 	struct vmcb *vmcb = svm->vmcb;
 	bool ret;
 
+	if (is_vnmi_enabled(vmcb) && is_vnmi_mask_set(vmcb))
+		return true;
+
 	if (!gif_set(svm))
 		return true;
 
@@ -3618,6 +3621,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (is_vnmi_enabled(svm->vmcb) && is_vnmi_mask_set(svm->vmcb))
+		return;
+
 	if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
 		return; /* IRET will cause a vm exit */
 
-- 
2.25.1

[PATCH 5/7] 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>
---
 arch/x86/kvm/svm/svm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a405e414cae4..200f979169e0 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3385,11 +3385,16 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	++vcpu->stat.nmi_injections;
+	if (is_vnmi_enabled(svm->vmcb)) {
+		svm->vmcb->control.int_ctl |= V_NMI_PENDING;
+		return;
+	}
+
 	svm->vmcb->control.event_inj = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_NMI;
 	vcpu->arch.hflags |= HF_NMI_MASK;
 	if (!sev_es_guest(vcpu->kvm))
 		svm_set_intercept(svm, INTERCEPT_IRET);
-	++vcpu->stat.nmi_injections;
 }
 
 static void svm_inject_irq(struct kvm_vcpu *vcpu)
-- 
2.25.1

[PATCH 6/7] KVM: nSVM: implement nested VNMI

[Santosh Shukla]

Currently nested_vmcb02_prepare_control func checks and programs bits
(V_TPR,_INTR, _IRQ) in nested mode, To support nested VNMI,
extending the check for VNMI bits if VNMI is enabled.

Tested with the KVM-unit-test that is developed for this purpose.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index bed5e1692cef..ce83739bae50 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -608,6 +608,11 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 	}
 }
 
+static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
+{
+	return svm->vnmi_enabled && (svm->nested.ctl.int_ctl & V_NMI_ENABLE);
+}
+
 static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
 {
 	u32 int_ctl_vmcb01_bits = V_INTR_MASKING_MASK;
@@ -627,6 +632,9 @@ 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);
+
 	/* 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;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 200f979169e0..c91af728420b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4075,6 +4075,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.  */
@@ -4831,6 +4833,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 21c5460e947a..f926c77bf857 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -240,6 +240,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;
-- 
2.25.1

[PATCH 7/7] 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 c91af728420b..69a98419203e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1205,6 +1205,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

Re: [PATCH 0/7] Virtual NMI feature

[Jim Mattson]

On Thu, Jun 2, 2022 at 7:26 AM Santosh Shukla <santosh.shukla@amd.com> wrote:
>
> 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.
>
> 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
>
> Thanks,
> Santosh
> [1] https://www.amd.com/system/files/TechDocs/40332.pdf - APM Vol2,
> ch-15.20 - "Event Injection".
>
> Santosh Shukla (7):
>   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: SVM: Enable VNMI feature
>
>  arch/x86/include/asm/cpufeatures.h |  1 +
>  arch/x86/include/asm/svm.h         |  7 +++++
>  arch/x86/kvm/svm/nested.c          |  8 +++++
>  arch/x86/kvm/svm/svm.c             | 47 ++++++++++++++++++++++++++++--
>  arch/x86/kvm/svm/svm.h             |  1 +
>  5 files changed, 62 insertions(+), 2 deletions(-)
>
> --
> 2.25.1

When will we see vNMI support in silicon? Genoa?

Where is this feature officially documented? Is there an AMD64
equivalent of the "Intel Architecture Instruction Set Extensions and
Future Features" manual?

Re: [PATCH 1/7] x86/cpu: Add CPUID feature bit for VNMI

[Maxim Levitsky]

On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> 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].
> 
> 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 393f2bbb5e3a..c8775b25856b 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -346,6 +346,7 @@
>  #define X86_FEATURE_V_VMSAVE_VMLOAD    (15*32+15) /* Virtual VMSAVE VMLOAD */
>  #define X86_FEATURE_VGIF               (15*32+16) /* Virtual GIF */
>  #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 */

I also think that AMD should publish some sort of a 'future ISA' spec like Intel does,
so that we could avoid mistakes in reviweing the code.

Other than that:

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [PATCH 2/7] KVM: SVM: Add VNMI bit definition

[Maxim Levitsky]

On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> 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.
So this is like bit in IRR

> V_NMI_MASK(12) - Indicates whether virtual NMI is masked in the guest.
And that is like bit in ISR.

Question: what are the interactions with GIF/vGIF and this feature?

> 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.
> 
> 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 1b07fba11704..22d918555df0 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -195,6 +195,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>  #define AVIC_ENABLE_SHIFT 31
>  #define AVIC_ENABLE_MASK (1 << AVIC_ENABLE_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 200045f71df0..860f28c668bd 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -198,6 +198,8 @@ module_param(dump_invalid_vmcb, bool, 0644);
>  bool intercept_smi = true;
>  module_param(intercept_smi, bool, 0444);
>  
> +static bool vnmi;
> +module_param(vnmi, bool, 0444);
>  
>  static bool svm_gp_erratum_intercept = true;
>  
> @@ -4930,6 +4932,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) ||


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi_mask

[Maxim Levitsky]

On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> 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 and do not populate set accessors.
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> ---
>  arch/x86/kvm/svm/svm.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 860f28c668bd..d67a54517d95 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
>         return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
>  }
>  
> +static bool is_vnmi_enabled(struct vmcb *vmcb)
> +{
> +       return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
> +}

Following Paolo's suggestion I recently removed vgif_enabled(),
based on the logic that vgif_enabled == vgif, because
we always enable vGIF for L1 as long as 'vgif' module param is set,
which is set unless either hardware or user cleared it.

Note that here vmcb is the current vmcb, which can be vmcb02,
and it might be wrong

> +
> +static bool is_vnmi_mask_set(struct vmcb *vmcb)
> +{
> +       return !!(vmcb->control.int_ctl & V_NMI_MASK);
> +}
> +
>  static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
>  {
>         struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3502,13 +3512,21 @@ 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->vmcb))
> +               return is_vnmi_mask_set(svm->vmcb);
> +       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->vmcb))
> +               return;

What if the KVM wants to mask NMI, shoudn't we update the 
V_NMI_MASK value in int_ctl instead of doing nothing?

Best regards,
	Maxim Levitsky


> +
>         if (masked) {
>                 vcpu->arch.hflags |= HF_NMI_MASK;
>                 if (!sev_es_guest(vcpu->kvm))

Re: [PATCH 4/7] KVM: SVM: Report NMI not allowed when Guest busy handling VNMI

[Maxim Levitsky]

On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> In the VNMI case, Report NMI is not allowed when the processor set the
> V_NMI_MASK to 1 which means the Guest is busy handling VNMI.
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> ---
>  arch/x86/kvm/svm/svm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index d67a54517d95..a405e414cae4 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3483,6 +3483,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
>         struct vmcb *vmcb = svm->vmcb;
>         bool ret;
>  
> +       if (is_vnmi_enabled(vmcb) && is_vnmi_mask_set(vmcb))
> +               return true;

How does this interact with GIF? if the guest does clgi, will the
CPU update the V_NMI_MASK on its own If vGIF is enabled?

What happens if vGIF is disabled and vNMI is enabled? KVM then intercepts
the stgi/clgi, and it should then update the V_NMI_MASK?




> +
>         if (!gif_set(svm))
>                 return true;
>  
> @@ -3618,6 +3621,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
>  {
>         struct vcpu_svm *svm = to_svm(vcpu);
>  
> +       if (is_vnmi_enabled(svm->vmcb) && is_vnmi_mask_set(svm->vmcb))
> +               return;

This might have hidden assumption that we will only enable NMI window when vNMI is masked.



> +
>         if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
>                 return; /* IRET will cause a vm exit */
>  


Best regards,
	Maxim Levitsky

Re: [PATCH 4/7] KVM: SVM: Report NMI not allowed when Guest busy handling VNMI

[Maxim Levitsky]

On Tue, 2022-06-07 at 16:10 +0300, Maxim Levitsky wrote:
> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> > In the VNMI case, Report NMI is not allowed when the processor set the
> > V_NMI_MASK to 1 which means the Guest is busy handling VNMI.
> > 
> > Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> > ---
> >  arch/x86/kvm/svm/svm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index d67a54517d95..a405e414cae4 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3483,6 +3483,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
> >         struct vmcb *vmcb = svm->vmcb;
> >         bool ret;
> >  
> > +       if (is_vnmi_enabled(vmcb) && is_vnmi_mask_set(vmcb))
> > +               return true;
> 
> How does this interact with GIF? if the guest does clgi, will the
> CPU update the V_NMI_MASK on its own If vGIF is enabled?
> 
> What happens if vGIF is disabled and vNMI is enabled? KVM then intercepts
> the stgi/clgi, and it should then update the V_NMI_MASK?
> 
> 
> 
> 
> > +
> >         if (!gif_set(svm))
> >                 return true;
> >  
> > @@ -3618,6 +3621,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
> >  {
> >         struct vcpu_svm *svm = to_svm(vcpu);
> >  
> > +       if (is_vnmi_enabled(svm->vmcb) && is_vnmi_mask_set(svm->vmcb))
> > +               return;
> 
> This might have hidden assumption that we will only enable NMI window when vNMI is masked.

Also what if vNMI is already pending?

Best regards,
	Maxim Levitsky
> 
> 
> 
> > +
> >         if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
> >                 return; /* IRET will cause a vm exit */
> >  
> 
> 
> Best regards,
>         Maxim Levitsky

Re: [PATCH 5/7] KVM: SVM: Add VNMI support in inject_nmi

[Maxim Levitsky]

On Thu, 2022-06-02 at 19:56 +0530, 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>
> ---
>  arch/x86/kvm/svm/svm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index a405e414cae4..200f979169e0 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3385,11 +3385,16 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>  {
>         struct vcpu_svm *svm = to_svm(vcpu);
>  
> +       ++vcpu->stat.nmi_injections;
> +       if (is_vnmi_enabled(svm->vmcb)) {
> +               svm->vmcb->control.int_ctl |= V_NMI_PENDING;
> +               return;
> +       }
Here I would advice to have a warning to check if vNMI is already pending.

Also we need to check what happens if we make vNMI pending and get #SMI,
while in #NMI handler, or if #NMI is blocked due to interrupt window.

Best regards,
	Maxim Levitsky


> +
>         svm->vmcb->control.event_inj = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_NMI;
>         vcpu->arch.hflags |= HF_NMI_MASK;
>         if (!sev_es_guest(vcpu->kvm))
>                 svm_set_intercept(svm, INTERCEPT_IRET);
> -       ++vcpu->stat.nmi_injections;
>  }
>  
>  static void svm_inject_irq(struct kvm_vcpu *vcpu)

Re: [PATCH 6/7] KVM: nSVM: implement nested VNMI

[Maxim Levitsky]

On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> Currently nested_vmcb02_prepare_control func checks and programs bits
> (V_TPR,_INTR, _IRQ) in nested mode, To support nested VNMI,
> extending the check for VNMI bits if VNMI is enabled.
> 
> Tested with the KVM-unit-test that is developed for this purpose.
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> ---
>  arch/x86/kvm/svm/nested.c | 8 ++++++++
>  arch/x86/kvm/svm/svm.c    | 5 +++++
>  arch/x86/kvm/svm/svm.h    | 1 +
>  3 files changed, 14 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index bed5e1692cef..ce83739bae50 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -608,6 +608,11 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>         }
>  }
>  
> +static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
> +{
> +       return svm->vnmi_enabled && (svm->nested.ctl.int_ctl & V_NMI_ENABLE);
> +}
> +
>  static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>  {
>         u32 int_ctl_vmcb01_bits = V_INTR_MASKING_MASK;
> @@ -627,6 +632,9 @@ 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);

This is for sure not enough - we also need to at least copy V_NMI_PENDING/V_NMI_MASK
back to vmc12 on vmexit, and also think about what happens with L1's VNMI while L2 is running.

E.g functions like is_vnmi_mask_set, likely should always reference vmcb01, and I *think*
that while L2 is running L1's vNMI should be sort of 'inhibited' like I did with AVIC.

For example the svm_nmi_blocked should probably first check for 'is_guest_mode(vcpu) && nested_exit_on_nmi(svm)'
and only then start checking for vNMI.

There also are interactions with vGIF and nested vGIF that should be checked as well.

Finally the patch series needs tests, several tests, including a test when a nested guest
runs and the L1 receives NMI, and check that it works both when L1 intercepts NMI and doesn't intercept NMIs,
and if vNMI is enabled L1, and both enabled and not enabled in L2.


Best regards,
	Maxim Levitsky

> +
>         /* 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;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 200f979169e0..c91af728420b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4075,6 +4075,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.  */
> @@ -4831,6 +4833,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 21c5460e947a..f926c77bf857 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -240,6 +240,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;

Re: [PATCH 0/7] Virtual NMI feature

[Shukla, Santosh]

On 6/7/2022 4:31 AM, Jim Mattson wrote:
> On Thu, Jun 2, 2022 at 7:26 AM Santosh Shukla <santosh.shukla@amd.com> wrote:
>>
>> 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.
>>
>> 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
>>
>> Thanks,
>> Santosh
>> [1] https://www.amd.com/system/files/TechDocs/40332.pdf
>> ch-15.20 - "Event Injection".
>>
>> Santosh Shukla (7):
>>   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: SVM: Enable VNMI feature
>>
>>  arch/x86/include/asm/cpufeatures.h |  1 +
>>  arch/x86/include/asm/svm.h         |  7 +++++
>>  arch/x86/kvm/svm/nested.c          |  8 +++++
>>  arch/x86/kvm/svm/svm.c             | 47 ++++++++++++++++++++++++++++--
>>  arch/x86/kvm/svm/svm.h             |  1 +
>>  5 files changed, 62 insertions(+), 2 deletions(-)
>>
>> --
>> 2.25.1
> 
> When will we see vNMI support in silicon? Genoa?
> 
> Where is this feature officially documented? Is there an AMD64
> equivalent of the "Intel Architecture Instruction Set Extensions and
> Future Features" manual?

Hi Jim,

A new revision of the Architecture programmers manual (APM) is slated
to be release soon and that is going to have all the details for
the above questions.

Thanks,
Santosh

Re: [PATCH 2/7] KVM: SVM: Add VNMI bit definition

[Shukla, Santosh]

On 6/7/2022 6:25 PM, Maxim Levitsky wrote:
> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>> 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.
> So this is like bit in IRR
> 
>> V_NMI_MASK(12) - Indicates whether virtual NMI is masked in the guest.
> And that is like bit in ISR.
> 
> Question: what are the interactions with GIF/vGIF and this feature?
> 

Hi Maxim,

The quick answer is, that V_NMI will respect the VGIF enable controls.
More info about interaction in subsequent patch reply.

Thanks,
Santosh

>> 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.
>>
>> 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 1b07fba11704..22d918555df0 100644
>> --- a/arch/x86/include/asm/svm.h
>> +++ b/arch/x86/include/asm/svm.h
>> @@ -195,6 +195,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>>  #define AVIC_ENABLE_SHIFT 31
>>  #define AVIC_ENABLE_MASK (1 << AVIC_ENABLE_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 200045f71df0..860f28c668bd 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -198,6 +198,8 @@ module_param(dump_invalid_vmcb, bool, 0644);
>>  bool intercept_smi = true;
>>  module_param(intercept_smi, bool, 0444);
>>  
>> +static bool vnmi;
>> +module_param(vnmi, bool, 0444);
>>  
>>  static bool svm_gp_erratum_intercept = true;
>>  
>> @@ -4930,6 +4932,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) ||
> 
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Best regards,
> 	Maxim Levitsky
> 

Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi_mask

[Shukla, Santosh]

On 6/7/2022 6:37 PM, Maxim Levitsky wrote:
> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>> 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 and do not populate set accessors.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>> ---
>>  arch/x86/kvm/svm/svm.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 860f28c668bd..d67a54517d95 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
>>         return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
>>  }
>>  
>> +static bool is_vnmi_enabled(struct vmcb *vmcb)
>> +{
>> +       return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
>> +}
> 
> Following Paolo's suggestion I recently removed vgif_enabled(),
> based on the logic that vgif_enabled == vgif, because
> we always enable vGIF for L1 as long as 'vgif' module param is set,
> which is set unless either hardware or user cleared it.
> 
Yes. In v2, Thanks!.

> Note that here vmcb is the current vmcb, which can be vmcb02,
> and it might be wrong
> 
>> +
>> +static bool is_vnmi_mask_set(struct vmcb *vmcb)
>> +{
>> +       return !!(vmcb->control.int_ctl & V_NMI_MASK);
>> +}
>> +
>>  static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
>>  {
>>         struct vcpu_svm *svm = to_svm(vcpu);
>> @@ -3502,13 +3512,21 @@ 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->vmcb))
>> +               return is_vnmi_mask_set(svm->vmcb);
>> +       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->vmcb))
>> +               return;
> 
> What if the KVM wants to mask NMI, shoudn't we update the 
> V_NMI_MASK value in int_ctl instead of doing nothing?
> 
> Best regards,
> 	Maxim Levitsky
> 
> 
>> +
>>         if (masked) {
>>                 vcpu->arch.hflags |= HF_NMI_MASK;
>>                 if (!sev_es_guest(vcpu->kvm))
> 
> 

Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi_mask

[Shukla, Santosh]

On 6/17/2022 8:15 PM, Shukla, Santosh wrote:
> 
> 
> On 6/7/2022 6:37 PM, Maxim Levitsky wrote:
>> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>>> 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 and do not populate set accessors.
>>>
>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>> ---
>>>  arch/x86/kvm/svm/svm.c | 20 +++++++++++++++++++-
>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index 860f28c668bd..d67a54517d95 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
>>>         return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
>>>  }
>>>  
>>> +static bool is_vnmi_enabled(struct vmcb *vmcb)
>>> +{
>>> +       return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
>>> +}
>>
>> Following Paolo's suggestion I recently removed vgif_enabled(),
>> based on the logic that vgif_enabled == vgif, because
>> we always enable vGIF for L1 as long as 'vgif' module param is set,
>> which is set unless either hardware or user cleared it.
>>
> Yes. In v2, Thanks!.
> 
>> Note that here vmcb is the current vmcb, which can be vmcb02,
>> and it might be wrong
>>
>>> +
>>> +static bool is_vnmi_mask_set(struct vmcb *vmcb)
>>> +{
>>> +       return !!(vmcb->control.int_ctl & V_NMI_MASK);
>>> +}
>>> +
>>>  static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
>>>  {
>>>         struct vcpu_svm *svm = to_svm(vcpu);
>>> @@ -3502,13 +3512,21 @@ 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->vmcb))
>>> +               return is_vnmi_mask_set(svm->vmcb);
>>> +       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->vmcb))
>>> +               return;
>>
>> What if the KVM wants to mask NMI, shoudn't we update the 
>> V_NMI_MASK value in int_ctl instead of doing nothing?
>>

V_NMI_MASK is cpu controlled meaning HW sets the mask while processing
event and clears right after processing, so in away its Read-only for hypervisor.

>> Best regards,
>> 	Maxim Levitsky
>>
>>
>>> +
>>>         if (masked) {
>>>                 vcpu->arch.hflags |= HF_NMI_MASK;
>>>                 if (!sev_es_guest(vcpu->kvm))
>>
>>

Re: [PATCH 4/7] KVM: SVM: Report NMI not allowed when Guest busy handling VNMI

[Shukla, Santosh]

On 6/7/2022 6:42 PM, Maxim Levitsky wrote:
> On Tue, 2022-06-07 at 16:10 +0300, Maxim Levitsky wrote:
>> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>>> In the VNMI case, Report NMI is not allowed when the processor set the
>>> V_NMI_MASK to 1 which means the Guest is busy handling VNMI.
>>>
>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>> ---
>>>  arch/x86/kvm/svm/svm.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index d67a54517d95..a405e414cae4 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -3483,6 +3483,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
>>>         struct vmcb *vmcb = svm->vmcb;
>>>         bool ret;
>>>  
>>> +       if (is_vnmi_enabled(vmcb) && is_vnmi_mask_set(vmcb))
>>> +               return true;
>>
>> How does this interact with GIF? if the guest does clgi, will the
>> CPU update the V_NMI_MASK on its own If vGIF is enabled?
>>
Yes.

>> What happens if vGIF is disabled and vNMI is enabled? KVM then intercepts
>> the stgi/clgi, and it should then update the V_NMI_MASK?
>>
No.

For both case - HW takes the V_NMI event at the boundary of VMRUN instruction.

>>
>>
>>
>>> +
>>>         if (!gif_set(svm))
>>>                 return true;
>>>  
>>> @@ -3618,6 +3621,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
>>>  {
>>>         struct vcpu_svm *svm = to_svm(vcpu);
>>>  
>>> +       if (is_vnmi_enabled(svm->vmcb) && is_vnmi_mask_set(svm->vmcb))
>>> +               return;
>>
>> This might have hidden assumption that we will only enable NMI window when vNMI is masked.
> 
> Also what if vNMI is already pending?
> 
If V_NMI_MASK set, that means V_NMI is pending, if so then inject another V_NMI in next VMRUN.

Thanks,
Santosh

> Best regards,
> 	Maxim Levitsky
>>
>>
>>
>>> +
>>>         if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
>>>                 return; /* IRET will cause a vm exit */
>>>  
>>
>>
>> Best regards,
>>         Maxim Levitsky
> 
> 

Re: [PATCH 5/7] KVM: SVM: Add VNMI support in inject_nmi

[Shukla, Santosh]

On 6/7/2022 6:44 PM, Maxim Levitsky wrote:
> On Thu, 2022-06-02 at 19:56 +0530, 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>
>> ---
>>  arch/x86/kvm/svm/svm.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index a405e414cae4..200f979169e0 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3385,11 +3385,16 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>  {
>>         struct vcpu_svm *svm = to_svm(vcpu);
>>  
>> +       ++vcpu->stat.nmi_injections;
>> +       if (is_vnmi_enabled(svm->vmcb)) {
>> +               svm->vmcb->control.int_ctl |= V_NMI_PENDING;
>> +               return;
>> +       }
> Here I would advice to have a warning to check if vNMI is already pending.
> 
Yes, in v2.

> Also we need to check what happens if we make vNMI pending and get #SMI,
> while in #NMI handler, or if #NMI is blocked due to interrupt window.
>

V_NMI_MASK should be saved as 1 in the save area and
hypervisor will resume the NMI handler after handling the SMI.

Thanks,
Santosh
 
> Best regards,
> 	Maxim Levitsky
> 
> 
>> +
>>         svm->vmcb->control.event_inj = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_NMI;
>>         vcpu->arch.hflags |= HF_NMI_MASK;
>>         if (!sev_es_guest(vcpu->kvm))
>>                 svm_set_intercept(svm, INTERCEPT_IRET);
>> -       ++vcpu->stat.nmi_injections;
>>  }
>>  
>>  static void svm_inject_irq(struct kvm_vcpu *vcpu)
> 
> 

Re: [PATCH 6/7] KVM: nSVM: implement nested VNMI

[Shukla, Santosh]

On 6/7/2022 6:52 PM, Maxim Levitsky wrote:
> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>> Currently nested_vmcb02_prepare_control func checks and programs bits
>> (V_TPR,_INTR, _IRQ) in nested mode, To support nested VNMI,
>> extending the check for VNMI bits if VNMI is enabled.
>>
>> Tested with the KVM-unit-test that is developed for this purpose.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>> ---
>>  arch/x86/kvm/svm/nested.c | 8 ++++++++
>>  arch/x86/kvm/svm/svm.c    | 5 +++++
>>  arch/x86/kvm/svm/svm.h    | 1 +
>>  3 files changed, 14 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index bed5e1692cef..ce83739bae50 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -608,6 +608,11 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>>         }
>>  }
>>  
>> +static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
>> +{
>> +       return svm->vnmi_enabled && (svm->nested.ctl.int_ctl & V_NMI_ENABLE);
>> +}
>> +
>>  static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>>  {
>>         u32 int_ctl_vmcb01_bits = V_INTR_MASKING_MASK;
>> @@ -627,6 +632,9 @@ 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);
> 
> This is for sure not enough - we also need to at least copy V_NMI_PENDING/V_NMI_MASK
> back to vmc12 on vmexit, and also think about what happens with L1's VNMI while L2 is running.
> 
> E.g functions like is_vnmi_mask_set, likely should always reference vmcb01, and I *think*
> that while L2 is running L1's vNMI should be sort of 'inhibited' like I did with AVIC.
> 
> For example the svm_nmi_blocked should probably first check for 'is_guest_mode(vcpu) && nested_exit_on_nmi(svm)'
> and only then start checking for vNMI.
> 
> There also are interactions with vGIF and nested vGIF that should be checked as well.
> 
> Finally the patch series needs tests, several tests, including a test when a nested guest
> runs and the L1 receives NMI, and check that it works both when L1 intercepts NMI and doesn't intercept NMIs,
> and if vNMI is enabled L1, and both enabled and not enabled in L2.
> 
> 

In V2.

Thank-you Maxim for the review comment.
Santosh.

> Best regards,
> 	Maxim Levitsky
> 
>> +
>>         /* 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;
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 200f979169e0..c91af728420b 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -4075,6 +4075,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.  */
>> @@ -4831,6 +4833,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 21c5460e947a..f926c77bf857 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -240,6 +240,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;
> 
> 

Re: [PATCH 5/7] KVM: SVM: Add VNMI support in inject_nmi

[Maxim Levitsky]

On Fri, 2022-06-17 at 20:35 +0530, Shukla, Santosh wrote:
> 
> On 6/7/2022 6:44 PM, Maxim Levitsky wrote:
> > On Thu, 2022-06-02 at 19:56 +0530, 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>
> > > ---
> > >  arch/x86/kvm/svm/svm.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index a405e414cae4..200f979169e0 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -3385,11 +3385,16 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> > >  {
> > >         struct vcpu_svm *svm = to_svm(vcpu);
> > >  
> > > +       ++vcpu->stat.nmi_injections;
> > > +       if (is_vnmi_enabled(svm->vmcb)) {
> > > +               svm->vmcb->control.int_ctl |= V_NMI_PENDING;
> > > +               return;
> > > +       }
> > Here I would advice to have a warning to check if vNMI is already pending.
> > 
> Yes, in v2.
> 
> > Also we need to check what happens if we make vNMI pending and get #SMI,
> > while in #NMI handler, or if #NMI is blocked due to interrupt window.
> > 
> 
> V_NMI_MASK should be saved as 1 in the save area and
> hypervisor will resume the NMI handler after handling the SMI.


Answering my own question, because I did some homework in this area while
working on that SMI int shadow bug:

Actually what will happen (now I checked) is that we have a special KVM host state flag
(called X86EMUL_SMM_MASK, and HF_SMM_INSIDE_NMI_MASK), and what we do is that if we
receive SMI while in NMI handler, we don't mask the NMI again, on RSM we don't unmask NMI.

Also, as I found out the hard way recently, the NMI is not blocked by the interrupt shadow.

I don't think that anything is saved to the SMRAM in this case.

Best regards,
	Maxim Levitsky

> 
> Thanks,
> Santosh
>  
> > Best regards,
> > 	Maxim Levitsky
> > 
> > 
> > > +
> > >         svm->vmcb->control.event_inj = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_NMI;
> > >         vcpu->arch.hflags |= HF_NMI_MASK;
> > >         if (!sev_es_guest(vcpu->kvm))
> > >                 svm_set_intercept(svm, INTERCEPT_IRET);
> > > -       ++vcpu->stat.nmi_injections;
> > >  }
> > >  
> > >  static void svm_inject_irq(struct kvm_vcpu *vcpu)

Re: [PATCH 4/7] KVM: SVM: Report NMI not allowed when Guest busy handling VNMI

[Maxim Levitsky]

On Fri, 2022-06-17 at 20:29 +0530, Shukla, Santosh wrote:
> 
> On 6/7/2022 6:42 PM, Maxim Levitsky wrote:
> > On Tue, 2022-06-07 at 16:10 +0300, Maxim Levitsky wrote:
> > > On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> > > > In the VNMI case, Report NMI is not allowed when the processor set the
> > > > V_NMI_MASK to 1 which means the Guest is busy handling VNMI.
> > > > 
> > > > Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> > > > ---
> > > >  arch/x86/kvm/svm/svm.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > > index d67a54517d95..a405e414cae4 100644
> > > > --- a/arch/x86/kvm/svm/svm.c
> > > > +++ b/arch/x86/kvm/svm/svm.c
> > > > @@ -3483,6 +3483,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
> > > >         struct vmcb *vmcb = svm->vmcb;
> > > >         bool ret;
> > > >  
> > > > +       if (is_vnmi_enabled(vmcb) && is_vnmi_mask_set(vmcb))
> > > > +               return true;
> > > 
> > > How does this interact with GIF? if the guest does clgi, will the
> > > CPU update the V_NMI_MASK on its own If vGIF is enabled?
> > > 
> Yes.
> 
> > > What happens if vGIF is disabled and vNMI is enabled? KVM then intercepts
> > > the stgi/clgi, and it should then update the V_NMI_MASK?
> > > 
> No.
> 
> For both case - HW takes the V_NMI event at the boundary of VMRUN instruction.

How that is possible? if vGIF is disabled in L1, then L1 can't execute STGI/CLGI - 
that means that the CPU can't update the V_NMI, as it never sees the STGI/CLGI
beeing executed.

Best regards,
	Maxim Levitsky

> 
> > > 
> > > 
> > > > +
> > > >         if (!gif_set(svm))
> > > >                 return true;
> > > >  
> > > > @@ -3618,6 +3621,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
> > > >  {
> > > >         struct vcpu_svm *svm = to_svm(vcpu);
> > > >  
> > > > +       if (is_vnmi_enabled(svm->vmcb) && is_vnmi_mask_set(svm->vmcb))
> > > > +               return;
> > > 
> > > This might have hidden assumption that we will only enable NMI window when vNMI is masked.
> > 
> > Also what if vNMI is already pending?
> > 
> If V_NMI_MASK set, that means V_NMI is pending, if so then inject another V_NMI in next VMRUN.
> 
> Thanks,
> Santosh
> 
> > Best regards,
> > 	Maxim Levitsky
> > > 
> > > 
> > > > +
> > > >         if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
> > > >                 return; /* IRET will cause a vm exit */
> > > >  
> > > 
> > > Best regards,
> > >         Maxim Levitsky

Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi_mask

[Maxim Levitsky]

On Fri, 2022-06-17 at 20:18 +0530, Shukla, Santosh wrote:
> 
> On 6/17/2022 8:15 PM, Shukla, Santosh wrote:
> > 
> > On 6/7/2022 6:37 PM, Maxim Levitsky wrote:
> > > On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> > > > 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 and do not populate set accessors.
> > > > 
> > > > Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> > > > ---
> > > >  arch/x86/kvm/svm/svm.c | 20 +++++++++++++++++++-
> > > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > > index 860f28c668bd..d67a54517d95 100644
> > > > --- a/arch/x86/kvm/svm/svm.c
> > > > +++ b/arch/x86/kvm/svm/svm.c
> > > > @@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
> > > >         return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
> > > >  }
> > > >  
> > > > +static bool is_vnmi_enabled(struct vmcb *vmcb)
> > > > +{
> > > > +       return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
> > > > +}
> > > 
> > > Following Paolo's suggestion I recently removed vgif_enabled(),
> > > based on the logic that vgif_enabled == vgif, because
> > > we always enable vGIF for L1 as long as 'vgif' module param is set,
> > > which is set unless either hardware or user cleared it.
> > > 
> > Yes. In v2, Thanks!.
> > 
> > > Note that here vmcb is the current vmcb, which can be vmcb02,
> > > and it might be wrong
> > > 
> > > > +
> > > > +static bool is_vnmi_mask_set(struct vmcb *vmcb)
> > > > +{
> > > > +       return !!(vmcb->control.int_ctl & V_NMI_MASK);
> > > > +}
> > > > +
> > > >  static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
> > > >  {
> > > >         struct vcpu_svm *svm = to_svm(vcpu);
> > > > @@ -3502,13 +3512,21 @@ 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->vmcb))
> > > > +               return is_vnmi_mask_set(svm->vmcb);
> > > > +       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->vmcb))
> > > > +               return;
> > > 
> > > What if the KVM wants to mask NMI, shoudn't we update the 
> > > V_NMI_MASK value in int_ctl instead of doing nothing?
> > > 
> 
> V_NMI_MASK is cpu controlled meaning HW sets the mask while processing
> event and clears right after processing, so in away its Read-only for hypervisor.

And yet, svm_set_nmi_mask is called when KVM wants to explicitly mask NMI
without injecting a NMI, it does this when entering (emulated) SMI.

So the KVM has to set V_NMI_MASK here, becaue no real NMI is injected,
and thus the CPU will not set this bit itself.

Best regards,
	Maxim Levitsky
> 
> > > Best regards,
> > > 	Maxim Levitsky
> > > 
> > > 
> > > > +
> > > >         if (masked) {
> > > >                 vcpu->arch.hflags |= HF_NMI_MASK;
> > > >                 if (!sev_es_guest(vcpu->kvm))

Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi_mask

[Maxim Levitsky]

On Fri, 2022-06-17 at 20:18 +0530, Shukla, Santosh wrote:
> 
> On 6/17/2022 8:15 PM, Shukla, Santosh wrote:
> > 
> > On 6/7/2022 6:37 PM, Maxim Levitsky wrote:
> > > On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> > > > 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 and do not populate set accessors.
> > > > 
> > > > Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> > > > ---
> > > >  arch/x86/kvm/svm/svm.c | 20 +++++++++++++++++++-
> > > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > > index 860f28c668bd..d67a54517d95 100644
> > > > --- a/arch/x86/kvm/svm/svm.c
> > > > +++ b/arch/x86/kvm/svm/svm.c
> > > > @@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
> > > >         return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
> > > >  }
> > > >  
> > > > +static bool is_vnmi_enabled(struct vmcb *vmcb)
> > > > +{
> > > > +       return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
> > > > +}
> > > 
> > > Following Paolo's suggestion I recently removed vgif_enabled(),
> > > based on the logic that vgif_enabled == vgif, because
> > > we always enable vGIF for L1 as long as 'vgif' module param is set,
> > > which is set unless either hardware or user cleared it.
> > > 
> > Yes. In v2, Thanks!.
> > 
> > > Note that here vmcb is the current vmcb, which can be vmcb02,
> > > and it might be wrong
> > > 
> > > > +
> > > > +static bool is_vnmi_mask_set(struct vmcb *vmcb)
> > > > +{
> > > > +       return !!(vmcb->control.int_ctl & V_NMI_MASK);
> > > > +}
> > > > +
> > > >  static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
> > > >  {
> > > >         struct vcpu_svm *svm = to_svm(vcpu);
> > > > @@ -3502,13 +3512,21 @@ 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->vmcb))
> > > > +               return is_vnmi_mask_set(svm->vmcb);
> > > > +       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->vmcb))
> > > > +               return;
> > > 
> > > What if the KVM wants to mask NMI, shoudn't we update the 
> > > V_NMI_MASK value in int_ctl instead of doing nothing?
> > > 
> 
> V_NMI_MASK is cpu controlled meaning HW sets the mask while processing
> event and clears right after processing, so in away its Read-only for hypervisor.

And yet, svm_set_nmi_mask is called when KVM wants to explicitly mask NMI
without injecting a NMI, it does this when entering (emulated) SMI.

So the KVM has to set V_NMI_MASK here, even though no real NMI was received.

Best regards,
	Maxim Levitsky
> 
> > > Best regards,
> > > 	Maxim Levitsky
> > > 
> > > 
> > > > +
> > > >         if (masked) {
> > > >                 vcpu->arch.hflags |= HF_NMI_MASK;
> > > >                 if (!sev_es_guest(vcpu->kvm))

Re: [PATCH 3/7] KVM: SVM: Add VNMI support in get/set_nmi_mask

[Shukla, Santosh]

On 7/10/2022 9:39 PM, Maxim Levitsky wrote:
> On Fri, 2022-06-17 at 20:18 +0530, Shukla, Santosh wrote:
>>
>> On 6/17/2022 8:15 PM, Shukla, Santosh wrote:
>>>
>>> On 6/7/2022 6:37 PM, Maxim Levitsky wrote:
>>>> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>>>>> 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 and do not populate set accessors.
>>>>>
>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>> ---
>>>>>  arch/x86/kvm/svm/svm.c | 20 +++++++++++++++++++-
>>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>> index 860f28c668bd..d67a54517d95 100644
>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>> @@ -323,6 +323,16 @@ static int is_external_interrupt(u32 info)
>>>>>         return info == (SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR);
>>>>>  }
>>>>>  
>>>>> +static bool is_vnmi_enabled(struct vmcb *vmcb)
>>>>> +{
>>>>> +       return vnmi && (vmcb->control.int_ctl & V_NMI_ENABLE);
>>>>> +}
>>>>
>>>> Following Paolo's suggestion I recently removed vgif_enabled(),
>>>> based on the logic that vgif_enabled == vgif, because
>>>> we always enable vGIF for L1 as long as 'vgif' module param is set,
>>>> which is set unless either hardware or user cleared it.
>>>>
>>> Yes. In v2, Thanks!.
>>>
>>>> Note that here vmcb is the current vmcb, which can be vmcb02,
>>>> and it might be wrong
>>>>
>>>>> +
>>>>> +static bool is_vnmi_mask_set(struct vmcb *vmcb)
>>>>> +{
>>>>> +       return !!(vmcb->control.int_ctl & V_NMI_MASK);
>>>>> +}
>>>>> +
>>>>>  static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
>>>>>  {
>>>>>         struct vcpu_svm *svm = to_svm(vcpu);
>>>>> @@ -3502,13 +3512,21 @@ 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->vmcb))
>>>>> +               return is_vnmi_mask_set(svm->vmcb);
>>>>> +       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->vmcb))
>>>>> +               return;
>>>>
>>>> What if the KVM wants to mask NMI, shoudn't we update the 
>>>> V_NMI_MASK value in int_ctl instead of doing nothing?
>>>>
>>
>> V_NMI_MASK is cpu controlled meaning HW sets the mask while processing
>> event and clears right after processing, so in away its Read-only for hypervisor.
> 
> And yet, svm_set_nmi_mask is called when KVM wants to explicitly mask NMI
> without injecting a NMI, it does this when entering (emulated) SMI.
> 
> So the KVM has to set V_NMI_MASK here, becaue no real NMI is injected,
> and thus the CPU will not set this bit itself.
> 

Yes, we will handle smm case in v3.

Thanks,
Santosh

> Best regards,
> 	Maxim Levitsky
>>
>>>> Best regards,
>>>> 	Maxim Levitsky
>>>>
>>>>
>>>>> +
>>>>>         if (masked) {
>>>>>                 vcpu->arch.hflags |= HF_NMI_MASK;
>>>>>                 if (!sev_es_guest(vcpu->kvm))
> 
> 

Re: [PATCH 4/7] KVM: SVM: Report NMI not allowed when Guest busy handling VNMI

[Shukla, Santosh]

On 7/10/2022 9:38 PM, Maxim Levitsky wrote:
> On Fri, 2022-06-17 at 20:29 +0530, Shukla, Santosh wrote:
>>
>> On 6/7/2022 6:42 PM, Maxim Levitsky wrote:
>>> On Tue, 2022-06-07 at 16:10 +0300, Maxim Levitsky wrote:
>>>> On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
>>>>> In the VNMI case, Report NMI is not allowed when the processor set the
>>>>> V_NMI_MASK to 1 which means the Guest is busy handling VNMI.
>>>>>
>>>>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>>>>> ---
>>>>>  arch/x86/kvm/svm/svm.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>> index d67a54517d95..a405e414cae4 100644
>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>> @@ -3483,6 +3483,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
>>>>>         struct vmcb *vmcb = svm->vmcb;
>>>>>         bool ret;
>>>>>  
>>>>> +       if (is_vnmi_enabled(vmcb) && is_vnmi_mask_set(vmcb))
>>>>> +               return true;
>>>>
>>>> How does this interact with GIF? if the guest does clgi, will the
>>>> CPU update the V_NMI_MASK on its own If vGIF is enabled?
>>>>
>> Yes.
>>
>>>> What happens if vGIF is disabled and vNMI is enabled? KVM then intercepts
>>>> the stgi/clgi, and it should then update the V_NMI_MASK?
>>>>
>> No.
>>
>> For both case - HW takes the V_NMI event at the boundary of VMRUN instruction.
> 
> How that is possible? if vGIF is disabled in L1, then L1 can't execute STGI/CLGI - 
> that means that the CPU can't update the V_NMI, as it never sees the STGI/CLGI
> beeing executed.
> 

If vGIF is disabled then HW will take the vnmi event at the boundary of vmrun instruction.

Thanks,
Santosh

> Best regards,
> 	Maxim Levitsky
> 
>>
>>>>
>>>>
>>>>> +
>>>>>         if (!gif_set(svm))
>>>>>                 return true;
>>>>>  
>>>>> @@ -3618,6 +3621,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
>>>>>  {
>>>>>         struct vcpu_svm *svm = to_svm(vcpu);
>>>>>  
>>>>> +       if (is_vnmi_enabled(svm->vmcb) && is_vnmi_mask_set(svm->vmcb))
>>>>> +               return;
>>>>
>>>> This might have hidden assumption that we will only enable NMI window when vNMI is masked.
>>>
>>> Also what if vNMI is already pending?
>>>
>> If V_NMI_MASK set, that means V_NMI is pending, if so then inject another V_NMI in next VMRUN.
>>
>> Thanks,
>> Santosh
>>
>>> Best regards,
>>> 	Maxim Levitsky
>>>>
>>>>
>>>>> +
>>>>>         if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
>>>>>                 return; /* IRET will cause a vm exit */
>>>>>  
>>>>
>>>> Best regards,
>>>>         Maxim Levitsky
> 
> 

Re: [PATCH 4/7] KVM: SVM: Report NMI not allowed when Guest busy handling VNMI

[Maxim Levitsky]

On Thu, 2022-07-21 at 15:01 +0530, Shukla, Santosh wrote:
> 
> On 7/10/2022 9:38 PM, Maxim Levitsky wrote:
> > On Fri, 2022-06-17 at 20:29 +0530, Shukla, Santosh wrote:
> > > On 6/7/2022 6:42 PM, Maxim Levitsky wrote:
> > > > On Tue, 2022-06-07 at 16:10 +0300, Maxim Levitsky wrote:
> > > > > On Thu, 2022-06-02 at 19:56 +0530, Santosh Shukla wrote:
> > > > > > In the VNMI case, Report NMI is not allowed when the processor set the
> > > > > > V_NMI_MASK to 1 which means the Guest is busy handling VNMI.
> > > > > > 
> > > > > > Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> > > > > > ---
> > > > > >  arch/x86/kvm/svm/svm.c | 6 ++++++
> > > > > >  1 file changed, 6 insertions(+)
> > > > > > 
> > > > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > > > > index d67a54517d95..a405e414cae4 100644
> > > > > > --- a/arch/x86/kvm/svm/svm.c
> > > > > > +++ b/arch/x86/kvm/svm/svm.c
> > > > > > @@ -3483,6 +3483,9 @@ bool svm_nmi_blocked(struct kvm_vcpu *vcpu)
> > > > > >         struct vmcb *vmcb = svm->vmcb;
> > > > > >         bool ret;
> > > > > >  
> > > > > > +       if (is_vnmi_enabled(vmcb) && is_vnmi_mask_set(vmcb))
> > > > > > +               return true;
> > > > > 
> > > > > How does this interact with GIF? if the guest does clgi, will the
> > > > > CPU update the V_NMI_MASK on its own If vGIF is enabled?
> > > > > 
> > > Yes.
> > > 
> > > > > What happens if vGIF is disabled and vNMI is enabled? KVM then intercepts
> > > > > the stgi/clgi, and it should then update the V_NMI_MASK?
> > > > > 
> > > No.
> > > 
> > > For both case - HW takes the V_NMI event at the boundary of VMRUN instruction.
> > 
> > How that is possible? if vGIF is disabled in L1, then L1 can't execute STGI/CLGI - 
> > that means that the CPU can't update the V_NMI, as it never sees the STGI/CLGI
> > beeing executed.
> > 
> 
> If vGIF is disabled then HW will take the vnmi event at the boundary of vmrun instruction.


I think I understand now, if vGIF is enabled, and V_NMI_MASK is set, and the guest does STGI, then nothing
new should be injected.

If V_NMI_MASK is not set, then svm_nmi_blocked will respect the HF_GIF_MASK, and on STGI interception,
the new NMI will be injected on VM entry by setting the V_NMI_PENDING.

So looks like it should work.

Thanks,
	Best regards,
		Maxim Levitsky


> 
> Thanks,
> Santosh
> 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > > > > 
> > > > > > +
> > > > > >         if (!gif_set(svm))
> > > > > >                 return true;
> > > > > >  
> > > > > > @@ -3618,6 +3621,9 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
> > > > > >  {
> > > > > >         struct vcpu_svm *svm = to_svm(vcpu);
> > > > > >  
> > > > > > +       if (is_vnmi_enabled(svm->vmcb) && is_vnmi_mask_set(svm->vmcb))
> > > > > > +               return;
> > > > > 
> > > > > This might have hidden assumption that we will only enable NMI window when vNMI is masked.
> > > > 
> > > > Also what if vNMI is already pending?
> > > > 
> > > If V_NMI_MASK set, that means V_NMI is pending, if so then inject another V_NMI in next VMRUN.
> > > 
> > > Thanks,
> > > Santosh
> > > 
> > > > Best regards,
> > > > 	Maxim Levitsky
> > > > > 
> > > > > > +
> > > > > >         if ((vcpu->arch.hflags & (HF_NMI_MASK | HF_IRET_MASK)) == HF_NMI_MASK)
> > > > > >                 return; /* IRET will cause a vm exit */
> > > > > >  
> > > > > 
> > > > > Best regards,
> > > > >         Maxim Levitsky

Re: [PATCH 0/7] Virtual NMI feature

[Jim Mattson]

On Wed, Jun 8, 2022 at 1:23 AM Shukla, Santosh <santosh.shukla@amd.com> wrote:
>
>
>
> On 6/7/2022 4:31 AM, Jim Mattson wrote:
> > When will we see vNMI support in silicon? Genoa?
> >
> > Where is this feature officially documented? Is there an AMD64
> > equivalent of the "Intel Architecture Instruction Set Extensions and
> > Future Features" manual?
>
> Hi Jim,
>
> A new revision of the Architecture programmers manual (APM) is slated
> to be release soon and that is going to have all the details for
> the above questions.

It's been about 3 months, and I haven't seen the new APM yet. Is there
an anticipated release date? It's hard to do a proper review of new
features without documentation.