Similar to VMLOAD/VMSAVE intercept handling, move the STGI/CLGI
intercept handling to svm_recalc_instruction_intercepts().
---
arch/x86/kvm/svm/svm.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 24d59ccfa40d9..6373a25d85479 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1010,6 +1010,11 @@ static void svm_recalc_instruction_intercepts(struct kvm_vcpu *vcpu)
svm_clr_intercept(svm, INTERCEPT_VMSAVE);
svm->vmcb->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
}
+
+ if (vgif) {
+ svm_clr_intercept(svm, INTERCEPT_STGI);
+ svm_clr_intercept(svm, INTERCEPT_CLGI);
+ }
}
}
@@ -1147,11 +1152,8 @@ static void init_vmcb(struct kvm_vcpu *vcpu, bool init_event)
if (vnmi)
svm->vmcb->control.int_ctl |= V_NMI_ENABLE_MASK;
- if (vgif) {
- svm_clr_intercept(svm, INTERCEPT_STGI);
- svm_clr_intercept(svm, INTERCEPT_CLGI);
+ if (vgif)
svm->vmcb->control.int_ctl |= V_GIF_ENABLE_MASK;
- }
if (vcpu->kvm->arch.bus_lock_detection_enabled)
svm_set_intercept(svm, INTERCEPT_BUSLOCK);
--
2.52.0.457.g6b5491de43-googOn Mon, Jan 12, 2026 at 05:45:31PM +0000, Kevin Cheng wrote:
> Similar to VMLOAD/VMSAVE intercept handling, move the STGI/CLGI
> intercept handling to svm_recalc_instruction_intercepts().
> ---
> arch/x86/kvm/svm/svm.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 24d59ccfa40d9..6373a25d85479 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1010,6 +1010,11 @@ static void svm_recalc_instruction_intercepts(struct kvm_vcpu *vcpu)
> svm_clr_intercept(svm, INTERCEPT_VMSAVE);
> svm->vmcb->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
> }
> +
> + if (vgif) {
> + svm_clr_intercept(svm, INTERCEPT_STGI);
Could this cause a problem with NMI window tracking?
svm_enable_nmi_window() sets INTERCEPT_STGI to detect when NMIs are
enabled, and it's later cleared by svm_set_gif(). If we recalc
intercepts in between we will clear the intercept here and miss NMI
enablement.
We could move the logic to set/clear INTERCEPT_STGI for NMI window
tracking here as well, but then we'll need to recalc intercepts in
svm_enable_nmi_window() and svm_set_gif(), which could be expensive.
The alternative is perhaps setting a flag when INTERCEPT_STGI is set in
svm_enable_nmi_window() and avoid clearing the intercept here if the
flag is set.
Not sure what's the best way forward here.
> + svm_clr_intercept(svm, INTERCEPT_CLGI);
> + }
> }
> }
>
> @@ -1147,11 +1152,8 @@ static void init_vmcb(struct kvm_vcpu *vcpu, bool init_event)
> if (vnmi)
> svm->vmcb->control.int_ctl |= V_NMI_ENABLE_MASK;
>
> - if (vgif) {
> - svm_clr_intercept(svm, INTERCEPT_STGI);
> - svm_clr_intercept(svm, INTERCEPT_CLGI);
> + if (vgif)
> svm->vmcb->control.int_ctl |= V_GIF_ENABLE_MASK;
> - }
>
> if (vcpu->kvm->arch.bus_lock_detection_enabled)
> svm_set_intercept(svm, INTERCEPT_BUSLOCK);
> --
> 2.52.0.457.g6b5491de43-goog
>
On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> On Mon, Jan 12, 2026 at 05:45:31PM +0000, Kevin Cheng wrote:
> > Similar to VMLOAD/VMSAVE intercept handling, move the STGI/CLGI
> > intercept handling to svm_recalc_instruction_intercepts().
> > ---
> > arch/x86/kvm/svm/svm.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 24d59ccfa40d9..6373a25d85479 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1010,6 +1010,11 @@ static void svm_recalc_instruction_intercepts(struct kvm_vcpu *vcpu)
> > svm_clr_intercept(svm, INTERCEPT_VMSAVE);
> > svm->vmcb->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
> > }
> > +
> > + if (vgif) {
> > + svm_clr_intercept(svm, INTERCEPT_STGI);
>
> Could this cause a problem with NMI window tracking?
Yes.
> svm_enable_nmi_window() sets INTERCEPT_STGI to detect when NMIs are
> enabled, and it's later cleared by svm_set_gif(). If we recalc
> intercepts in between we will clear the intercept here and miss NMI
> enablement.
>
> We could move the logic to set/clear INTERCEPT_STGI for NMI window
> tracking here as well, but then we'll need to recalc intercepts in
> svm_enable_nmi_window() and svm_set_gif(), which could be expensive.
>
> The alternative is perhaps setting a flag when INTERCEPT_STGI is set in
> svm_enable_nmi_window() and avoid clearing the intercept here if the
> flag is set.
>
> Not sure what's the best way forward here.
First things first, the changelog needs to state _why_ the code is being moved.
"To be like VMLOAD/VMSAVE" doesn't suffice, because my initial answer was going
to be "well just don't move the code" (I already forgot the context of v1).
But moving the code is needed to fix the missing #UD in "Recalc instructions
intercepts when EFER.SVME is toggled".
As for how to fix this, a few ideas:
1. Set KVM_REQ_EVENT to force KVM to re-evulate all events. kvm_check_and_inject_events()
will see the pending NMI and/or SMI, that the NMI/SMI is not allowed, and
re-call enable_{nmi,smi}_window().
2. Manually check for pending+blocked NMI/SMIs.
3. Combine parts of #1 and #2. Set KVM_REQ_EVENT, but only if there's a pending
NMI or SMI.
4. Add flags to vcpu_svm to explicitly track if a vCPU has an NMI/SMI window,
similar to what we're planning on doing for IRQs[*], and use that to more
confidently do the right thing when recomputing intercepts.
I don't love any of those ideas. Ah, at least not until I poke around KVM. In
svm_set_gif() there's already this:
if (svm->vcpu.arch.smi_pending ||
svm->vcpu.arch.nmi_pending ||
kvm_cpu_has_injectable_intr(&svm->vcpu) ||
kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
So I think it makes sense to bundle that into a helper, e.g. (no idea what to
call it)
static bool svm_think_of_a_good_name(struct kvm_vcpu *vcpu)
{
if (svm->vcpu.arch.smi_pending ||
svm->vcpu.arch.nmi_pending ||
kvm_cpu_has_injectable_intr(&svm->vcpu) ||
kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
}
And then call that from svm_recalc_instruction_intercepts(). That implements #3
in a fairly maintainable way (we'll hopefully notice sooner than later if we break
svm_set_gif()).
https://lore.kernel.org/all/26732815475bf1c5ba672bc3b1785265f1a994e6.1752819570.git.naveen@kernel.org
> > + svm_clr_intercept(svm, INTERCEPT_CLGI);
> > + }
> > }
> > }
> >
> > @@ -1147,11 +1152,8 @@ static void init_vmcb(struct kvm_vcpu *vcpu, bool init_event)
> > if (vnmi)
> > svm->vmcb->control.int_ctl |= V_NMI_ENABLE_MASK;
> >
> > - if (vgif) {
> > - svm_clr_intercept(svm, INTERCEPT_STGI);
> > - svm_clr_intercept(svm, INTERCEPT_CLGI);
> > + if (vgif)
> > svm->vmcb->control.int_ctl |= V_GIF_ENABLE_MASK;
> > - }
> >
> > if (vcpu->kvm->arch.bus_lock_detection_enabled)
> > svm_set_intercept(svm, INTERCEPT_BUSLOCK);
> > --
> > 2.52.0.457.g6b5491de43-goog
> >
On Wed, Jan 14, 2026 at 05:39:13PM -0800, Sean Christopherson wrote:
> On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > On Mon, Jan 12, 2026 at 05:45:31PM +0000, Kevin Cheng wrote:
> > > Similar to VMLOAD/VMSAVE intercept handling, move the STGI/CLGI
> > > intercept handling to svm_recalc_instruction_intercepts().
> > > ---
> > > arch/x86/kvm/svm/svm.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index 24d59ccfa40d9..6373a25d85479 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -1010,6 +1010,11 @@ static void svm_recalc_instruction_intercepts(struct kvm_vcpu *vcpu)
> > > svm_clr_intercept(svm, INTERCEPT_VMSAVE);
> > > svm->vmcb->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
> > > }
> > > +
> > > + if (vgif) {
> > > + svm_clr_intercept(svm, INTERCEPT_STGI);
> >
> > Could this cause a problem with NMI window tracking?
>
> Yes.
>
> > svm_enable_nmi_window() sets INTERCEPT_STGI to detect when NMIs are
> > enabled, and it's later cleared by svm_set_gif(). If we recalc
> > intercepts in between we will clear the intercept here and miss NMI
> > enablement.
> >
> > We could move the logic to set/clear INTERCEPT_STGI for NMI window
> > tracking here as well, but then we'll need to recalc intercepts in
> > svm_enable_nmi_window() and svm_set_gif(), which could be expensive.
> >
> > The alternative is perhaps setting a flag when INTERCEPT_STGI is set in
> > svm_enable_nmi_window() and avoid clearing the intercept here if the
> > flag is set.
> >
> > Not sure what's the best way forward here.
>
> First things first, the changelog needs to state _why_ the code is being moved.
> "To be like VMLOAD/VMSAVE" doesn't suffice, because my initial answer was going
> to be "well just don't move the code" (I already forgot the context of v1).
>
> But moving the code is needed to fix the missing #UD in "Recalc instructions
> intercepts when EFER.SVME is toggled".
>
> As for how to fix this, a few ideas:
>
> 1. Set KVM_REQ_EVENT to force KVM to re-evulate all events. kvm_check_and_inject_events()
> will see the pending NMI and/or SMI, that the NMI/SMI is not allowed, and
> re-call enable_{nmi,smi}_window().
>
> 2. Manually check for pending+blocked NMI/SMIs.
>
> 3. Combine parts of #1 and #2. Set KVM_REQ_EVENT, but only if there's a pending
> NMI or SMI.
>
> 4. Add flags to vcpu_svm to explicitly track if a vCPU has an NMI/SMI window,
> similar to what we're planning on doing for IRQs[*], and use that to more
> confidently do the right thing when recomputing intercepts.
>
> I don't love any of those ideas. Ah, at least not until I poke around KVM. In
> svm_set_gif() there's already this:
>
> if (svm->vcpu.arch.smi_pending ||
> svm->vcpu.arch.nmi_pending ||
> kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
>
> So I think it makes sense to bundle that into a helper, e.g. (no idea what to
> call it)
>
> static bool svm_think_of_a_good_name(struct kvm_vcpu *vcpu)
> {
> if (svm->vcpu.arch.smi_pending ||
> svm->vcpu.arch.nmi_pending ||
> kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> }
Maybe svm_check_gif_events() or svm_check_gif_interrupts()?
Or maybe it's clearer if we just put the checks in a helper like
svm_waiting_for_gif() or svm_pending_gif_interrupt().
Then in svm_recalc_instruction_intercepts() we do:
/*
* If there is a pending interrupt controlled by GIF, set
* KVM_REQ_EVENT to re-evaluate if the intercept needs to be set
* again to track when GIF is re-enabled (e.g. for NMI
* injection).
*/
svm_clr_intercept(svm, INTERCEPT_STGI);
if (svm_pending_gif_interrupt())
kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
and in svm_set_gif() it reads well semantically:
enable_gif(svm);
if (svm_pending_gif_interrupt())
kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
>
> And then call that from svm_recalc_instruction_intercepts(). That implements #3
> in a fairly maintainable way (we'll hopefully notice sooner than later if we break
> svm_set_gif()).
>
> https://lore.kernel.org/all/26732815475bf1c5ba672bc3b1785265f1a994e6.1752819570.git.naveen@kernel.org
>
On Thu, Jan 15, 2026, Yosry Ahmed wrote:
> On Wed, Jan 14, 2026 at 05:39:13PM -0800, Sean Christopherson wrote:
> > On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > As for how to fix this, a few ideas:
> >
> > 1. Set KVM_REQ_EVENT to force KVM to re-evulate all events. kvm_check_and_inject_events()
> > will see the pending NMI and/or SMI, that the NMI/SMI is not allowed, and
> > re-call enable_{nmi,smi}_window().
> >
> > 2. Manually check for pending+blocked NMI/SMIs.
> >
> > 3. Combine parts of #1 and #2. Set KVM_REQ_EVENT, but only if there's a pending
> > NMI or SMI.
> >
> > 4. Add flags to vcpu_svm to explicitly track if a vCPU has an NMI/SMI window,
> > similar to what we're planning on doing for IRQs[*], and use that to more
> > confidently do the right thing when recomputing intercepts.
> >
> > I don't love any of those ideas. Ah, at least not until I poke around KVM. In
> > svm_set_gif() there's already this:
> >
> > if (svm->vcpu.arch.smi_pending ||
> > svm->vcpu.arch.nmi_pending ||
> > kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> > kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> > kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> >
> > So I think it makes sense to bundle that into a helper, e.g. (no idea what to
> > call it)
> >
> > static bool svm_think_of_a_good_name(struct kvm_vcpu *vcpu)
> > {
> > if (svm->vcpu.arch.smi_pending ||
> > svm->vcpu.arch.nmi_pending ||
> > kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> > kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> > kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> > }
>
> Maybe svm_check_gif_events() or svm_check_gif_interrupts()?
>
> Or maybe it's clearer if we just put the checks in a helper like
> svm_waiting_for_gif() or svm_pending_gif_interrupt().
This was my first idea as well, though I would name it svm_has_pending_gif_event()
to better align with kvm_vcpu_has_events().
I suggested a single helper because I don't love that how to react to the pending
event is duplicated. But I definitely don't object to open coding the request if
the consensus is that it's more readable overall.
> Then in svm_recalc_instruction_intercepts() we do:
>
> /*
> * If there is a pending interrupt controlled by GIF, set
> * KVM_REQ_EVENT to re-evaluate if the intercept needs to be set
> * again to track when GIF is re-enabled (e.g. for NMI
> * injection).
> */
> svm_clr_intercept(svm, INTERCEPT_STGI);
> if (svm_pending_gif_interrupt())
> kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
>
> and in svm_set_gif() it reads well semantically:
>
> enable_gif(svm);
> if (svm_pending_gif_interrupt())
> kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
On Thu, Jan 15, 2026 at 09:00:07AM -0800, Sean Christopherson wrote:
> On Thu, Jan 15, 2026, Yosry Ahmed wrote:
> > On Wed, Jan 14, 2026 at 05:39:13PM -0800, Sean Christopherson wrote:
> > > On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > > As for how to fix this, a few ideas:
> > >
> > > 1. Set KVM_REQ_EVENT to force KVM to re-evulate all events. kvm_check_and_inject_events()
> > > will see the pending NMI and/or SMI, that the NMI/SMI is not allowed, and
> > > re-call enable_{nmi,smi}_window().
> > >
> > > 2. Manually check for pending+blocked NMI/SMIs.
> > >
> > > 3. Combine parts of #1 and #2. Set KVM_REQ_EVENT, but only if there's a pending
> > > NMI or SMI.
> > >
> > > 4. Add flags to vcpu_svm to explicitly track if a vCPU has an NMI/SMI window,
> > > similar to what we're planning on doing for IRQs[*], and use that to more
> > > confidently do the right thing when recomputing intercepts.
> > >
> > > I don't love any of those ideas. Ah, at least not until I poke around KVM. In
> > > svm_set_gif() there's already this:
> > >
> > > if (svm->vcpu.arch.smi_pending ||
> > > svm->vcpu.arch.nmi_pending ||
> > > kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> > > kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> > > kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> > >
> > > So I think it makes sense to bundle that into a helper, e.g. (no idea what to
> > > call it)
> > >
> > > static bool svm_think_of_a_good_name(struct kvm_vcpu *vcpu)
> > > {
> > > if (svm->vcpu.arch.smi_pending ||
> > > svm->vcpu.arch.nmi_pending ||
> > > kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> > > kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> > > kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> > > }
> >
> > Maybe svm_check_gif_events() or svm_check_gif_interrupts()?
> >
> > Or maybe it's clearer if we just put the checks in a helper like
> > svm_waiting_for_gif() or svm_pending_gif_interrupt().
>
> This was my first idea as well, though I would name it svm_has_pending_gif_event()
> to better align with kvm_vcpu_has_events().
svm_has_pending_gif_event() sounds good.
>
> I suggested a single helper because I don't love that how to react to the pending
> event is duplicated. But I definitely don't object to open coding the request if
> the consensus is that it's more readable overall.
A single helper is nice, but I can't think of a name that would read
well. My first instinct is svm_check_pending_gif_event(), but we are not
really checking the event as much as requesting for it to be checked.
We can do svm_request_gif_event(), perhaps? Not sure if that's better or
worse than svm_has_pending_gif_event().
>
> > Then in svm_recalc_instruction_intercepts() we do:
> >
> > /*
> > * If there is a pending interrupt controlled by GIF, set
> > * KVM_REQ_EVENT to re-evaluate if the intercept needs to be set
> > * again to track when GIF is re-enabled (e.g. for NMI
> > * injection).
> > */
> > svm_clr_intercept(svm, INTERCEPT_STGI);
> > if (svm_pending_gif_interrupt())
> > kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> >
> > and in svm_set_gif() it reads well semantically:
> >
> > enable_gif(svm);
> > if (svm_pending_gif_interrupt())
> > kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
On Thu, Jan 15, 2026, Yosry Ahmed wrote: > On Thu, Jan 15, 2026 at 09:00:07AM -0800, Sean Christopherson wrote: > > On Thu, Jan 15, 2026, Yosry Ahmed wrote: > > > Or maybe it's clearer if we just put the checks in a helper like > > > svm_waiting_for_gif() or svm_pending_gif_interrupt(). > > > > This was my first idea as well, though I would name it svm_has_pending_gif_event() > > to better align with kvm_vcpu_has_events(). > > svm_has_pending_gif_event() sounds good. > > > > > I suggested a single helper because I don't love that how to react to the pending > > event is duplicated. But I definitely don't object to open coding the request if > > the consensus is that it's more readable overall. > > A single helper is nice, but I can't think of a name that would read > well. My first instinct is svm_check_pending_gif_event(), but we are not > really checking the event as much as requesting for it to be checked. Ya, that's the same problem I'm having. I can't even come up with an absurdly verbose name to describe the behavior. > We can do svm_request_gif_event(), perhaps? Not sure if that's better or > worse than svm_has_pending_gif_event(). Definitely worse in my opinion. My entire motivation for a single helper would be to avoid bleeding implementation details (use of KVM_REQ_EVENT) to trigger the potential re-evaluation STGI/CLGI intercepts. And then there's the fact that in most cases, there probably isn't a pending event, i.e. not request will be made. Let's just go with svm_has_pending_gif_event().
On Thu, Jan 15, 2026 at 12:29 PM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Jan 15, 2026, Yosry Ahmed wrote: > > On Thu, Jan 15, 2026 at 09:00:07AM -0800, Sean Christopherson wrote: > > > On Thu, Jan 15, 2026, Yosry Ahmed wrote: > > > > Or maybe it's clearer if we just put the checks in a helper like > > > > svm_waiting_for_gif() or svm_pending_gif_interrupt(). > > > > > > This was my first idea as well, though I would name it svm_has_pending_gif_event() > > > to better align with kvm_vcpu_has_events(). > > > > svm_has_pending_gif_event() sounds good. > > > > > > > > I suggested a single helper because I don't love that how to react to the pending > > > event is duplicated. But I definitely don't object to open coding the request if > > > the consensus is that it's more readable overall. > > > > A single helper is nice, but I can't think of a name that would read > > well. My first instinct is svm_check_pending_gif_event(), but we are not > > really checking the event as much as requesting for it to be checked. > > Ya, that's the same problem I'm having. I can't even come up with an absurdly > verbose name to describe the behavior. > > > We can do svm_request_gif_event(), perhaps? Not sure if that's better or > > worse than svm_has_pending_gif_event(). > > Definitely worse in my opinion. My entire motivation for a single helper would > be to avoid bleeding implementation details (use of KVM_REQ_EVENT) to trigger > the potential re-evaluation STGI/CLGI intercepts. And then there's the fact that > in most cases, there probably isn't a pending event, i.e. not request will be > made. > > Let's just go with svm_has_pending_gif_event(). Sounds good. Thanks for the suggestions Yosry and Sean :) And thanks for catching this!
© 2016 - 2026 Red Hat, Inc.