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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Thu, Jun 2, 2022 at 7:26 AM Santosh Shukla <[email protected]> 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?
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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;
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
Maxim Levitsky
On 6/7/2022 4:31 AM, Jim Mattson wrote:
> On Thu, Jun 2, 2022 at 7:26 AM Santosh Shukla <[email protected]> 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
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 <[email protected]>
>>> ---
>>> 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
>
>
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 <[email protected]>
>> ---
>> 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;
>
>
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 <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
>>>>> ---
>>>>> 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
>
>
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 <[email protected]>
> > > > > > ---
> > > > > > 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
On Wed, Jun 8, 2022 at 1:23 AM Shukla, Santosh <[email protected]> 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.