[PATCH v2 0/2] KVM: SVM: GIF and EFER.SVME are independent

[PATCH v2 0/2] KVM: SVM: GIF and EFER.SVME are independent

[Jim Mattson]

Clearing EFER.SVME is not architected to set GIF, so GIF may be clear
even when EFER.SVME is clear.

This is covered in the discussion at
https://lore.kernel.org/all/5b8787b8-16e9-13dc-7fca-0dc441d673f9@citrix.com/.

v2:
  - Allow KVM_SET_NESTED_STATE to clear GIF when SVME==0

v1: https://lore.kernel.org/kvm/20251007224405.1914008-1-jmattson@google.com/

Jim Mattson (2):
  KVM: SVM: Allow KVM_SET_NESTED_STATE to clear GIF when SVME==0
  KVM: SVM: Don't set GIF when clearing EFER.SVME

 arch/x86/kvm/svm/nested.c | 4 ++--
 arch/x86/kvm/svm/svm.c    | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

-- 
2.51.0.740.g6adb054d12-goog

[PATCH v2 1/2] KVM: SVM: Allow KVM_SET_NESTED_STATE to clear GIF when SVME==0

[Jim Mattson]

GIF==0 together with EFER.SVME==0 is a valid architectural
state. Don't return -EINVAL for KVM_SET_NESTED_STATE when this
combination is specified.

Fixes: cc440cdad5b7 ("KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm/nested.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index a6443feab252..db0d4f2b128c 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1798,8 +1798,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	 * EFER.SVME, but EFER.SVME still has to be 1 for VMRUN to succeed.
 	 */
 	if (!(vcpu->arch.efer & EFER_SVME)) {
-		/* GIF=1 and no guest mode are required if SVME=0.  */
-		if (kvm_state->flags != KVM_STATE_NESTED_GIF_SET)
+		/* GUEST_MODE must be clear when SVME==0 */
+		if (kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)
 			return -EINVAL;
 	}
 
-- 
2.51.0.740.g6adb054d12-goog

Re: [PATCH v2 1/2] KVM: SVM: Allow KVM_SET_NESTED_STATE to clear GIF when SVME==0

[Yosry Ahmed]

On Thu, Oct 09, 2025 at 03:31:33PM -0700, Jim Mattson wrote:
> GIF==0 together with EFER.SVME==0 is a valid architectural
> state. Don't return -EINVAL for KVM_SET_NESTED_STATE when this
> combination is specified.
> 
> Fixes: cc440cdad5b7 ("KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE")
> Signed-off-by: Jim Mattson <jmattson@google.com>

Reviewed-by: Yosry Ahmed <yosry.ahmed@linux.dev>

> ---
>  arch/x86/kvm/svm/nested.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index a6443feab252..db0d4f2b128c 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1798,8 +1798,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	 * EFER.SVME, but EFER.SVME still has to be 1 for VMRUN to succeed.
>  	 */
>  	if (!(vcpu->arch.efer & EFER_SVME)) {
> -		/* GIF=1 and no guest mode are required if SVME=0.  */
> -		if (kvm_state->flags != KVM_STATE_NESTED_GIF_SET)
> +		/* GUEST_MODE must be clear when SVME==0 */
> +		if (kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)
>  			return -EINVAL;
>  	}
>  
> -- 
> 2.51.0.740.g6adb054d12-goog
> 

[PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Jim Mattson]

Clearing EFER.SVME is not architected to set GIF. Don't set GIF when
emulating a change to EFER that clears EFER.SVME.

Fixes: c513f484c558 ("KVM: nSVM: leave guest mode when clearing EFER.SVME")
Reviewed-by: Yosry Ahmed <yosry.ahmed@linux.dev>
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm/svm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 153c12dbf3eb..96177758d778 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -215,7 +215,6 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
 		if (!(efer & EFER_SVME)) {
 			svm_leave_nested(vcpu);
-			svm_set_gif(svm, true);
 			/* #GP intercept is still needed for vmware backdoor */
 			if (!enable_vmware_backdoor)
 				clr_exception_intercept(svm, GP_VECTOR);
-- 
2.51.0.740.g6adb054d12-goog

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Sean Christopherson]

On Thu, Oct 09, 2025, Jim Mattson wrote:
> Clearing EFER.SVME is not architected to set GIF.

But it's also not architected to leave GIF set when the guest is running, which
was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
vCPU into SMM mode, which might actually be invalid.

I think what we actually want is to to set GIF when force-leaving nested.  The
only path where it's not obvious that's "safe" is toggling SMM in 
kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
correct/desirable, or irrelevant because the caller immediately and unconditionally
sets/clears GIF.

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index a6443feab252..3392c7e22cae 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
                nested_svm_uninit_mmu_context(vcpu);
                vmcb_mark_all_dirty(svm->vmcb);
 
+               svm_set_gif(svm, true);
+
                if (kvm_apicv_activated(vcpu->kvm))
                        kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
        }


> Don't set GIF when emulating a change to EFER that clears EFER.SVME.
> 
> Fixes: c513f484c558 ("KVM: nSVM: leave guest mode when clearing EFER.SVME")
> Reviewed-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 153c12dbf3eb..96177758d778 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -215,7 +215,6 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
>  		if (!(efer & EFER_SVME)) {
>  			svm_leave_nested(vcpu);
> -			svm_set_gif(svm, true);
>  			/* #GP intercept is still needed for vmware backdoor */
>  			if (!enable_vmware_backdoor)
>  				clr_exception_intercept(svm, GP_VECTOR);
> -- 
> 2.51.0.740.g6adb054d12-goog
> 

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Jim Mattson]

On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Oct 09, 2025, Jim Mattson wrote:
> > Clearing EFER.SVME is not architected to set GIF.
>
> But it's also not architected to leave GIF set when the guest is running, which
> was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> vCPU into SMM mode, which might actually be invalid.
>
> I think what we actually want is to to set GIF when force-leaving nested.  The
> only path where it's not obvious that's "safe" is toggling SMM in
> kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> correct/desirable, or irrelevant because the caller immediately and unconditionally
> sets/clears GIF.
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index a6443feab252..3392c7e22cae 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
>                 nested_svm_uninit_mmu_context(vcpu);
>                 vmcb_mark_all_dirty(svm->vmcb);
>
> +               svm_set_gif(svm, true);
> +
>                 if (kvm_apicv_activated(vcpu->kvm))
>                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
>         }
>

This seems dangerously close to KVM making up "hardware" behavior, but
I'm okay with that if you are.

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Sean Christopherson]

On Tue, Oct 14, 2025, Jim Mattson wrote:
> On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Oct 09, 2025, Jim Mattson wrote:
> > > Clearing EFER.SVME is not architected to set GIF.
> >
> > But it's also not architected to leave GIF set when the guest is running, which
> > was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> > intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> > vCPU into SMM mode, which might actually be invalid.
> >
> > I think what we actually want is to to set GIF when force-leaving nested.  The
> > only path where it's not obvious that's "safe" is toggling SMM in
> > kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> > correct/desirable, or irrelevant because the caller immediately and unconditionally
> > sets/clears GIF.
> >
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index a6443feab252..3392c7e22cae 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
> >                 nested_svm_uninit_mmu_context(vcpu);
> >                 vmcb_mark_all_dirty(svm->vmcb);
> >
> > +               svm_set_gif(svm, true);
> > +
> >                 if (kvm_apicv_activated(vcpu->kvm))
> >                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> >         }
> >
> 
> This seems dangerously close to KVM making up "hardware" behavior, but
> I'm okay with that if you are.

Regardless of what KVM does, we're defining hardware behavior, i.e. keeping GIF
unchanged defines behavior just as much as setting GIF.  The only way to truly
avoid defining behavior would be to terminate the VM and completely prevent
userspace from accessing its state.

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Jim Mattson]

On Tue, Oct 14, 2025 at 2:18 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Oct 14, 2025, Jim Mattson wrote:
> > On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Thu, Oct 09, 2025, Jim Mattson wrote:
> > > > Clearing EFER.SVME is not architected to set GIF.
> > >
> > > But it's also not architected to leave GIF set when the guest is running, which
> > > was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> > > intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> > > vCPU into SMM mode, which might actually be invalid.
> > >
> > > I think what we actually want is to to set GIF when force-leaving nested.  The
> > > only path where it's not obvious that's "safe" is toggling SMM in
> > > kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> > > correct/desirable, or irrelevant because the caller immediately and unconditionally
> > > sets/clears GIF.
> > >
> > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > index a6443feab252..3392c7e22cae 100644
> > > --- a/arch/x86/kvm/svm/nested.c
> > > +++ b/arch/x86/kvm/svm/nested.c
> > > @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
> > >                 nested_svm_uninit_mmu_context(vcpu);
> > >                 vmcb_mark_all_dirty(svm->vmcb);
> > >
> > > +               svm_set_gif(svm, true);
> > > +
> > >                 if (kvm_apicv_activated(vcpu->kvm))
> > >                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> > >         }
> > >
> >
> > This seems dangerously close to KVM making up "hardware" behavior, but
> > I'm okay with that if you are.
>
> Regardless of what KVM does, we're defining hardware behavior, i.e. keeping GIF
> unchanged defines behavior just as much as setting GIF.  The only way to truly
> avoid defining behavior would be to terminate the VM and completely prevent
> userspace from accessing its state.

This can't be the only instance of "undefined behavior" that KVM deals
with. What about, say, misaligned accesses to xAPIC memory?

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Sean Christopherson]

On Tue, Oct 14, 2025, Jim Mattson wrote:
> On Tue, Oct 14, 2025 at 2:18 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Thu, Oct 09, 2025, Jim Mattson wrote:
> > > > > Clearing EFER.SVME is not architected to set GIF.
> > > >
> > > > But it's also not architected to leave GIF set when the guest is running, which
> > > > was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> > > > intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> > > > vCPU into SMM mode, which might actually be invalid.
> > > >
> > > > I think what we actually want is to to set GIF when force-leaving nested.  The
> > > > only path where it's not obvious that's "safe" is toggling SMM in
> > > > kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> > > > correct/desirable, or irrelevant because the caller immediately and unconditionally
> > > > sets/clears GIF.
> > > >
> > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > index a6443feab252..3392c7e22cae 100644
> > > > --- a/arch/x86/kvm/svm/nested.c
> > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
> > > >                 nested_svm_uninit_mmu_context(vcpu);
> > > >                 vmcb_mark_all_dirty(svm->vmcb);
> > > >
> > > > +               svm_set_gif(svm, true);
> > > > +
> > > >                 if (kvm_apicv_activated(vcpu->kvm))
> > > >                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> > > >         }
> > > >
> > >
> > > This seems dangerously close to KVM making up "hardware" behavior, but
> > > I'm okay with that if you are.
> >
> > Regardless of what KVM does, we're defining hardware behavior, i.e. keeping GIF
> > unchanged defines behavior just as much as setting GIF.  The only way to truly
> > avoid defining behavior would be to terminate the VM and completely prevent
> > userspace from accessing its state.
> 
> This can't be the only instance of "undefined behavior" that KVM deals
> with.

Oh, for sure.  But unsurprisingly, people only care about cases that actually
matter in practice.  E.g. the other one that comes to mind is SHUTDOWN on AMD:

	/*
	 * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
	 * the VMCB in a known good state.  Unfortuately, KVM doesn't have
	 * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
	 * userspace.  At a platform view, INIT is acceptable behavior as
	 * there exist bare metal platforms that automatically INIT the CPU
	 * in response to shutdown.
	 *

> What about, say, misaligned accesses to xAPIC memory?

Drops all accesses (doesn't even set the destination on reads).

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Jim Mattson]

On Tue, Oct 14, 2025 at 3:07 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Oct 14, 2025, Jim Mattson wrote:
> > On Tue, Oct 14, 2025 at 2:18 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > > On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > >
> > > > > On Thu, Oct 09, 2025, Jim Mattson wrote:
> > > > > > Clearing EFER.SVME is not architected to set GIF.
> > > > >
> > > > > But it's also not architected to leave GIF set when the guest is running, which
> > > > > was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> > > > > intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> > > > > vCPU into SMM mode, which might actually be invalid.
> > > > >
> > > > > I think what we actually want is to to set GIF when force-leaving nested.  The
> > > > > only path where it's not obvious that's "safe" is toggling SMM in
> > > > > kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> > > > > correct/desirable, or irrelevant because the caller immediately and unconditionally
> > > > > sets/clears GIF.
> > > > >
> > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > > index a6443feab252..3392c7e22cae 100644
> > > > > --- a/arch/x86/kvm/svm/nested.c
> > > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > > @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
> > > > >                 nested_svm_uninit_mmu_context(vcpu);
> > > > >                 vmcb_mark_all_dirty(svm->vmcb);
> > > > >
> > > > > +               svm_set_gif(svm, true);
> > > > > +
> > > > >                 if (kvm_apicv_activated(vcpu->kvm))
> > > > >                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> > > > >         }
> > > > >
> > > >
> > > > This seems dangerously close to KVM making up "hardware" behavior, but
> > > > I'm okay with that if you are.
> > >
> > > Regardless of what KVM does, we're defining hardware behavior, i.e. keeping GIF
> > > unchanged defines behavior just as much as setting GIF.  The only way to truly
> > > avoid defining behavior would be to terminate the VM and completely prevent
> > > userspace from accessing its state.
> >
> > This can't be the only instance of "undefined behavior" that KVM deals
> > with.
>
> Oh, for sure.  But unsurprisingly, people only care about cases that actually
> matter in practice.  E.g. the other one that comes to mind is SHUTDOWN on AMD:
>
>         /*
>          * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
>          * the VMCB in a known good state.  Unfortuately, KVM doesn't have
>          * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
>          * userspace.  At a platform view, INIT is acceptable behavior as
>          * there exist bare metal platforms that automatically INIT the CPU
>          * in response to shutdown.
>          *

The behavior of SHUTDOWN while GIF==0 is clearly architected:

"If the processor enters the shutdown state (due to a triple fault for
instance) while GIF is clear, it can only be restarted by means of a
RESET."

Doesn't setting GIF in svm_leave_nested() violate this specification?

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Sean Christopherson]

On Tue, Oct 14, 2025, Jim Mattson wrote:
> On Tue, Oct 14, 2025 at 3:07 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > On Tue, Oct 14, 2025 at 2:18 PM Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > > > On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 09, 2025, Jim Mattson wrote:
> > > > > > > Clearing EFER.SVME is not architected to set GIF.
> > > > > >
> > > > > > But it's also not architected to leave GIF set when the guest is running, which
> > > > > > was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> > > > > > intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> > > > > > vCPU into SMM mode, which might actually be invalid.
> > > > > >
> > > > > > I think what we actually want is to to set GIF when force-leaving nested.  The
> > > > > > only path where it's not obvious that's "safe" is toggling SMM in
> > > > > > kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> > > > > > correct/desirable, or irrelevant because the caller immediately and unconditionally
> > > > > > sets/clears GIF.
> > > > > >
> > > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > > > index a6443feab252..3392c7e22cae 100644
> > > > > > --- a/arch/x86/kvm/svm/nested.c
> > > > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > > > @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
> > > > > >                 nested_svm_uninit_mmu_context(vcpu);
> > > > > >                 vmcb_mark_all_dirty(svm->vmcb);
> > > > > >
> > > > > > +               svm_set_gif(svm, true);
> > > > > > +
> > > > > >                 if (kvm_apicv_activated(vcpu->kvm))
> > > > > >                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> > > > > >         }
> > > > > >
> > > > >
> > > > > This seems dangerously close to KVM making up "hardware" behavior, but
> > > > > I'm okay with that if you are.
> > > >
> > > > Regardless of what KVM does, we're defining hardware behavior, i.e. keeping GIF
> > > > unchanged defines behavior just as much as setting GIF.  The only way to truly
> > > > avoid defining behavior would be to terminate the VM and completely prevent
> > > > userspace from accessing its state.
> > >
> > > This can't be the only instance of "undefined behavior" that KVM deals
> > > with.
> >
> > Oh, for sure.  But unsurprisingly, people only care about cases that actually
> > matter in practice.  E.g. the other one that comes to mind is SHUTDOWN on AMD:
> >
> >         /*
> >          * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
> >          * the VMCB in a known good state.  Unfortuately, KVM doesn't have
> >          * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
> >          * userspace.  At a platform view, INIT is acceptable behavior as
> >          * there exist bare metal platforms that automatically INIT the CPU
> >          * in response to shutdown.
> >          *
> 
> The behavior of SHUTDOWN while GIF==0 is clearly architected:
> 
> "If the processor enters the shutdown state (due to a triple fault for
> instance) while GIF is clear, it can only be restarted by means of a
> RESET."
> 
> Doesn't setting GIF in svm_leave_nested() violate this specification?

Probably?  But SHUTDOWN also makes the VMCB undefined, so KVM is caught between
a rock and a hard place.  And when using vGIF, I don't see how KVM can do the
right thing, because the state of GIF at the time of SHUTDOWN is unknown.

And FWIW, if userspace does RESET the guest (which KVM can't detect with 100%
accuracy), GIF=1 on RESET, so it's kinda sorta right :-)

Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

[Yosry Ahmed]

On Fri, Oct 17, 2025 at 02:56:58PM -0700, Sean Christopherson wrote:
> On Tue, Oct 14, 2025, Jim Mattson wrote:
> > On Tue, Oct 14, 2025 at 3:07 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > > On Tue, Oct 14, 2025 at 2:18 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > >
> > > > > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > > > > On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 09, 2025, Jim Mattson wrote:
> > > > > > > > Clearing EFER.SVME is not architected to set GIF.
> > > > > > >
> > > > > > > But it's also not architected to leave GIF set when the guest is running, which
> > > > > > > was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> > > > > > > intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> > > > > > > vCPU into SMM mode, which might actually be invalid.
> > > > > > >
> > > > > > > I think what we actually want is to to set GIF when force-leaving nested.  The
> > > > > > > only path where it's not obvious that's "safe" is toggling SMM in
> > > > > > > kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> > > > > > > correct/desirable, or irrelevant because the caller immediately and unconditionally
> > > > > > > sets/clears GIF.
> > > > > > >
> > > > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > > > > index a6443feab252..3392c7e22cae 100644
> > > > > > > --- a/arch/x86/kvm/svm/nested.c
> > > > > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > > > > @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
> > > > > > >                 nested_svm_uninit_mmu_context(vcpu);
> > > > > > >                 vmcb_mark_all_dirty(svm->vmcb);
> > > > > > >
> > > > > > > +               svm_set_gif(svm, true);
> > > > > > > +
> > > > > > >                 if (kvm_apicv_activated(vcpu->kvm))
> > > > > > >                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> > > > > > >         }
> > > > > > >
> > > > > >
> > > > > > This seems dangerously close to KVM making up "hardware" behavior, but
> > > > > > I'm okay with that if you are.
> > > > >
> > > > > Regardless of what KVM does, we're defining hardware behavior, i.e. keeping GIF
> > > > > unchanged defines behavior just as much as setting GIF.  The only way to truly
> > > > > avoid defining behavior would be to terminate the VM and completely prevent
> > > > > userspace from accessing its state.
> > > >
> > > > This can't be the only instance of "undefined behavior" that KVM deals
> > > > with.
> > >
> > > Oh, for sure.  But unsurprisingly, people only care about cases that actually
> > > matter in practice.  E.g. the other one that comes to mind is SHUTDOWN on AMD:
> > >
> > >         /*
> > >          * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
> > >          * the VMCB in a known good state.  Unfortuately, KVM doesn't have
> > >          * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
> > >          * userspace.  At a platform view, INIT is acceptable behavior as
> > >          * there exist bare metal platforms that automatically INIT the CPU
> > >          * in response to shutdown.
> > >          *
> > 
> > The behavior of SHUTDOWN while GIF==0 is clearly architected:
> > 
> > "If the processor enters the shutdown state (due to a triple fault for
> > instance) while GIF is clear, it can only be restarted by means of a
> > RESET."
> > 
> > Doesn't setting GIF in svm_leave_nested() violate this specification?
> 
> Probably?  But SHUTDOWN also makes the VMCB undefined, so KVM is caught between
> a rock and a hard place.  And when using vGIF, I don't see how KVM can do the
> right thing, because the state of GIF at the time of SHUTDOWN is unknown.
> 
> And FWIW, if userspace does RESET the guest (which KVM can't detect with 100%
> accuracy), GIF=1 on RESET, so it's kinda sorta right :-)

This thread kinda ended on a cliffhanger.

Sean, is the plan to apply Jim's patch + your diff to svm_set_gif() in
svm_leave_nested()? Or are you waiting for Jim to send a v2?