diff mbox series

KVM: SVM: Don't intercept IRET when injecting NMI and vNMI is enabled

Message ID 20231009212919.221810-1-seanjc@google.com
State New
Headers
Series KVM: SVM: Don't intercept IRET when injecting NMI and vNMI is enabled |

Commit Message

Sean Christopherson Oct. 9, 2023, 9:29 p.m. UTC 
  When vNMI is enabled, rely entirely on hardware to correctly handle NMI
blocking, i.e. don't intercept IRET to detect when NMIs are no longer
blocked.  KVM already correctly ignores svm->nmi_masked when vNMI is
enabled, so the effect of the bug is essentially an unnecessary VM-Exit.

Note, per the APM, hardware sets the BLOCKING flag when software directly
directly injects an NMI:

  If Event Injection is used to inject an NMI when NMI Virtualization is
  enabled, VMRUN sets V_NMI_MASK in the guest state.

Fixes: fa4c027a7956 ("KVM: x86: Add support for SVM's Virtual NMI")
Link: https://lore.kernel.org/all/ZOdnuDZUd4mevCqe@google.como
Cc: Santosh Shukla <santosh.shukla@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---

Santosh, can you verify that I didn't break vNMI?  I don't have access to the
right hardware.  Thanks!

 arch/x86/kvm/svm/svm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)


base-commit: 86701e115030e020a052216baa942e8547e0b487

Comments

Maxim Levitsky Oct. 10, 2023, 12:03 p.m. UTC | #1 
У пн, 2023-10-09 у 14:29 -0700, Sean Christopherson пише:
> When vNMI is enabled, rely entirely on hardware to correctly handle NMI
> blocking, i.e. don't intercept IRET to detect when NMIs are no longer
> blocked.  KVM already correctly ignores svm->nmi_masked when vNMI is
> enabled, so the effect of the bug is essentially an unnecessary VM-Exit.

I would re-phrase this like that:

KVM intercepts IRET for two reasons:
- To track NMI masking to be able to know at any point of time if NMI is masked.
- To track NMI window (to inject another NMI after IRET finishes executing).

When L1 uses vNMI, both cases are fulfilled by the vNMI hardware:
- NMI masking state resides in V_NMI_BLOCKING bit of int_ctl and can be read by KVM
  at will.
- vNMI hardware injects the NMIs autonomically every time NMI is unblocked.

Thus there is no need to intercept IRET while vNMI is active.

However, even when vNMI is active in L1, the svm_inject_nmi() can still 
be called to do a direct NMI injection to support the case when KVM is 
trying to inject two NMIs simultaneously.

In this case there is no need to enable IRET interception.

Note that the effect of this bug is essentially an unnecessary VM-Exit.

Also note that even when vNMI is supported and used, running a nested guest
disables vNMI of the L1 guest, thus IRET will still be intercepted.
In this case if the nested VM exit happens before the NMI is delivered,
an unnecessary VM exit can still happen but this is even less likely.

> 
> Note, per the APM, hardware sets the BLOCKING flag when software directly
> directly injects an NMI:
> 
>   If Event Injection is used to inject an NMI when NMI Virtualization is
>   enabled, VMRUN sets V_NMI_MASK in the guest state.

I think that this comment is not needed in the commit message. It describes
a different unrelated concern and can be put somewhere in the code but
not in the commit message.

> 
> Fixes: fa4c027a7956 ("KVM: x86: Add support for SVM's Virtual NMI")
> Link: https://lore.kernel.org/all/ZOdnuDZUd4mevCqe@google.como
> Cc: Santosh Shukla <santosh.shukla@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> Santosh, can you verify that I didn't break vNMI?  I don't have access to the
> right hardware.  Thanks!
> 
>  arch/x86/kvm/svm/svm.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b7472ad183b9..4f22d12b5d60 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3569,8 +3569,15 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>  	if (svm->nmi_l1_to_l2)
>  		return;
>  
> -	svm->nmi_masked = true;
> -	svm_set_iret_intercept(svm);
> +	/*
> +	 * No need to manually track NMI masking when vNMI is enabled, hardware
> +	 * automatically sets V_NMI_BLOCKING_MASK as appropriate, including the
> +	 * case where software directly injects an NMI.
> +	 */
> +	if (!is_vnmi_enabled(svm)) {
> +		svm->nmi_masked = true;
> +		svm_set_iret_intercept(svm);
> +	}
>  	++vcpu->stat.nmi_injections;
>  }
>  
> 
> base-commit: 86701e115030e020a052216baa942e8547e0b487


Note that while nested, the 'is_vnmi_enabled()' will return false because L1's vnmi is indeed disabled
(I wonder if is_vnmi_enabled should be renamed is_l1_vnmi_enabled() to clarify this),

So when nested VM exit happens, that intercept can still continue to be true, 
which should not cause an issue but this is still something to keep in mind.

Best regards,
	Maxim Levitsky
Sean Christopherson Oct. 10, 2023, 2:46 p.m. UTC | #2 
On Tue, Oct 10, 2023, Maxim Levitsky wrote:
> У пн, 2023-10-09 у 14:29 -0700, Sean Christopherson пише:
> > Note, per the APM, hardware sets the BLOCKING flag when software directly
> > directly injects an NMI:
> > 
> >   If Event Injection is used to inject an NMI when NMI Virtualization is
> >   enabled, VMRUN sets V_NMI_MASK in the guest state.
> 
> I think that this comment is not needed in the commit message. It describes
> a different unrelated concern and can be put somewhere in the code but
> not in the commit message.

I strongly disagree, this blurb in the APM directly affects the patch.  If hardware
didn't set V_NMI_MASK, then the patch would need to be at least this:

--
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b7472ad183b9..d34ee3b8293e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3569,8 +3569,12 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
 	if (svm->nmi_l1_to_l2)
 		return;
 
-	svm->nmi_masked = true;
-	svm_set_iret_intercept(svm);
+	if (is_vnmi_enabled(svm)) {
+		svm->vmcb->control.int_ctl |= V_NMI_BLOCKING_MASK;
+	} else {
+		svm->nmi_masked = true;
+		svm_set_iret_intercept(svm);
+	}
 	++vcpu->stat.nmi_injections;
 }
 

base-commit: 86701e115030e020a052216baa942e8547e0b487
Maxim Levitsky Oct. 10, 2023, 4:06 p.m. UTC | #3 
У вт, 2023-10-10 у 07:46 -0700, Sean Christopherson пише:
> On Tue, Oct 10, 2023, Maxim Levitsky wrote:
> > У пн, 2023-10-09 у 14:29 -0700, Sean Christopherson пише:
> > > Note, per the APM, hardware sets the BLOCKING flag when software directly
> > > directly injects an NMI:
> > > 
> > >   If Event Injection is used to inject an NMI when NMI Virtualization is
> > >   enabled, VMRUN sets V_NMI_MASK in the guest state.
> > 
> > I think that this comment is not needed in the commit message. It describes
> > a different unrelated concern and can be put somewhere in the code but
> > not in the commit message.
> 
> I strongly disagree, this blurb in the APM directly affects the patch.  If hardware
> didn't set V_NMI_MASK, then the patch would need to be at least this:

I don't see how 'the blurb in the APM' relates to the removal of the 
IRET intercept, which is what this patch is about.

If the hardware was not to set the V_NMI_BLOCKING_MASK during EVENTINJ NMI injection, 
we would have had a bigger problem, a problem which would have to be addressed 
before this patch, because kvm reads back the V_NMI_BLOCKING_MASK 
(see: svm_get_nmi_mask()) to check if NMI is blocked, something that
has no relation to the IRET interception.


Best regards,
	Maxim Levtsky


> 
> --
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b7472ad183b9..d34ee3b8293e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3569,8 +3569,12 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>  	if (svm->nmi_l1_to_l2)
>  		return;
>  
> -	svm->nmi_masked = true;
> -	svm_set_iret_intercept(svm);
> +	if (is_vnmi_enabled(svm)) {
> +		svm->vmcb->control.int_ctl |= V_NMI_BLOCKING_MASK;
> +	} else {
> +		svm->nmi_masked = true;
> +		svm_set_iret_intercept(svm);
> +	}
>  	++vcpu->stat.nmi_injections;
>  }
>  
> 
> base-commit: 86701e115030e020a052216baa942e8547e0b487
Sean Christopherson Oct. 10, 2023, 5:50 p.m. UTC | #4 
On Tue, Oct 10, 2023, Maxim Levitsky wrote:
> У вт, 2023-10-10 у 07:46 -0700, Sean Christopherson пише:
> > On Tue, Oct 10, 2023, Maxim Levitsky wrote:
> > > У пн, 2023-10-09 у 14:29 -0700, Sean Christopherson пише:
> > > > Note, per the APM, hardware sets the BLOCKING flag when software directly
> > > > directly injects an NMI:
> > > > 
> > > >   If Event Injection is used to inject an NMI when NMI Virtualization is
> > > >   enabled, VMRUN sets V_NMI_MASK in the guest state.
> > > 
> > > I think that this comment is not needed in the commit message. It describes
> > > a different unrelated concern and can be put somewhere in the code but
> > > not in the commit message.
> > 
> > I strongly disagree, this blurb in the APM directly affects the patch.  If hardware
> > didn't set V_NMI_MASK, then the patch would need to be at least this:
> 
> I don't see how 'the blurb in the APM' relates to the removal of the 
> IRET intercept, which is what this patch is about.

No, it's not *just* about IRET interception.  This patch also guards:

	svm->nmi_masked = true;

If the reader doesn't already know that hardware sets V_NMI_BLOCK_MASK on direct
injection, as was the case for me when I stumbled upon this issue, it's not at
all obvious that not doing something analogous to setting nmi_masked is correct.

I mentioned only IRET interception in the shortlog because that's the only practical
impact of the change.  I can massage the shortlog if it's confusing/misleading,
but I really don't want to drop the reference to hardware setting V_NMI_BLOCK_MASK.
Santosh Shukla Oct. 14, 2023, 10:16 a.m. UTC | #5 
On 10/10/2023 8:16 PM, Sean Christopherson wrote:
> On Tue, Oct 10, 2023, Maxim Levitsky wrote:
>> У пн, 2023-10-09 у 14:29 -0700, Sean Christopherson пише:
>>> Note, per the APM, hardware sets the BLOCKING flag when software directly
>>> directly injects an NMI:
>>>
>>>   If Event Injection is used to inject an NMI when NMI Virtualization is
>>>   enabled, VMRUN sets V_NMI_MASK in the guest state.
>>
>> I think that this comment is not needed in the commit message. It describes
>> a different unrelated concern and can be put somewhere in the code but
>> not in the commit message.
> 
> I strongly disagree, this blurb in the APM directly affects the patch.  If hardware
> didn't set V_NMI_MASK, then the patch would need to be at least this:
> 
> --
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b7472ad183b9..d34ee3b8293e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3569,8 +3569,12 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>  	if (svm->nmi_l1_to_l2)
>  		return;
>  
> -	svm->nmi_masked = true;
> -	svm_set_iret_intercept(svm);
> +	if (is_vnmi_enabled(svm)) {
> +		svm->vmcb->control.int_ctl |= V_NMI_BLOCKING_MASK;
> +	} else {
> +		svm->nmi_masked = true;
> +		svm_set_iret_intercept(svm);
> +	}
>  	++vcpu->stat.nmi_injections;
>  }
>  
>

quick testing worked fine, KUT test ran fine and tested for non-nested mode so far.
Will do more nested testing and share the feedback.

Thanks,
Santosh

> base-commit: 86701e115030e020a052216baa942e8547e0b487
Santosh Shukla Oct. 14, 2023, 2:49 p.m. UTC | #6 
On 10/14/2023 3:46 PM, Santosh Shukla wrote:
> 
> 
> On 10/10/2023 8:16 PM, Sean Christopherson wrote:
>> On Tue, Oct 10, 2023, Maxim Levitsky wrote:
>>> У пн, 2023-10-09 у 14:29 -0700, Sean Christopherson пише:
>>>> Note, per the APM, hardware sets the BLOCKING flag when software directly
>>>> directly injects an NMI:
>>>>
>>>>   If Event Injection is used to inject an NMI when NMI Virtualization is
>>>>   enabled, VMRUN sets V_NMI_MASK in the guest state.
>>>
>>> I think that this comment is not needed in the commit message. It describes
>>> a different unrelated concern and can be put somewhere in the code but
>>> not in the commit message.
>>
>> I strongly disagree, this blurb in the APM directly affects the patch.  If hardware
>> didn't set V_NMI_MASK, then the patch would need to be at least this:
>>
HW sets the V_NMI_MASK.

>> --
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index b7472ad183b9..d34ee3b8293e 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3569,8 +3569,12 @@ static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>  	if (svm->nmi_l1_to_l2)
>>  		return;
>>  
>> -	svm->nmi_masked = true;
>> -	svm_set_iret_intercept(svm);
>> +	if (is_vnmi_enabled(svm)) {
>> +		svm->vmcb->control.int_ctl |= V_NMI_BLOCKING_MASK;
>> +	} else {
>> +		svm->nmi_masked = true;
>> +		svm_set_iret_intercept(svm);
>> +	}
>>  	++vcpu->stat.nmi_injections;
>>  }
>>  
>>
> 
> quick testing worked fine, KUT test ran fine and tested for non-nested mode so far.
> Will do more nested testing and share the feedback.
> 

Sean - I have tested original patch[1] for nested and KUT, worked fine.

Thanks,
Santosh 

[1] https://lore.kernel.org/r/20231009212919.221810-1-seanjc@google.com 

> Thanks,
> Santosh
> 
>> base-commit: 86701e115030e020a052216baa942e8547e0b487
>
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b7472ad183b9..4f22d12b5d60 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3569,8 +3569,15 @@  static void svm_inject_nmi(struct kvm_vcpu *vcpu)
 	if (svm->nmi_l1_to_l2)
 		return;
 
-	svm->nmi_masked = true;
-	svm_set_iret_intercept(svm);
+	/*
+	 * No need to manually track NMI masking when vNMI is enabled, hardware
+	 * automatically sets V_NMI_BLOCKING_MASK as appropriate, including the
+	 * case where software directly injects an NMI.
+	 */
+	if (!is_vnmi_enabled(svm)) {
+		svm->nmi_masked = true;
+		svm_set_iret_intercept(svm);
+	}
 	++vcpu->stat.nmi_injections;
 }