recalc_intercepts() currently uses c, h, g as local variables for the
control area of the current VMCB, vmcb01, and (cached) vmcb12.
The current VMCB should always be vmcb02 when recalc_intercepts() is
executed in guest mode. Use vmcb01/vmcb02 local variables instead to
make it clear the function is updating intercepts in vmcb02 based on the
intercepts in vmcb01 and (cached) vmcb12.
Add a WARNING() if the current VMCB is not in fact vmcb02.
No functional change intended.
Signed-off-by: Yosry Ahmed <yosry.ahmed@linux.dev>
---
arch/x86/kvm/svm/nested.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index f295a41ec659..2dda52221fd8 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -125,8 +125,7 @@ static bool nested_vmcb_needs_vls_intercept(struct vcpu_svm *svm)
void recalc_intercepts(struct vcpu_svm *svm)
{
- struct vmcb_control_area *c, *h;
- struct vmcb_ctrl_area_cached *g;
+ struct vmcb *vmcb01, *vmcb02;
unsigned int i;
vmcb_mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
@@ -134,14 +133,14 @@ void recalc_intercepts(struct vcpu_svm *svm)
if (!is_guest_mode(&svm->vcpu))
return;
- c = &svm->vmcb->control;
- h = &svm->vmcb01.ptr->control;
- g = &svm->nested.ctl;
+ vmcb01 = svm->vmcb01.ptr;
+ vmcb02 = svm->nested.vmcb02.ptr;
+ WARN_ON_ONCE(svm->vmcb != vmcb02);
for (i = 0; i < MAX_INTERCEPT; i++)
- c->intercepts[i] = h->intercepts[i];
+ vmcb02->control.intercepts[i] = vmcb01->control.intercepts[i];
- if (g->int_ctl & V_INTR_MASKING_MASK) {
+ if (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) {
/*
* If L2 is active and V_INTR_MASKING is enabled in vmcb12,
* disable intercept of CR8 writes as L2's CR8 does not affect
@@ -152,9 +151,9 @@ void recalc_intercepts(struct vcpu_svm *svm)
* the effective RFLAGS.IF for L1 interrupts will never be set
* while L2 is running (L2's RFLAGS.IF doesn't affect L1 IRQs).
*/
- vmcb_clr_intercept(c, INTERCEPT_CR8_WRITE);
- if (!(svm->vmcb01.ptr->save.rflags & X86_EFLAGS_IF))
- vmcb_clr_intercept(c, INTERCEPT_VINTR);
+ vmcb_clr_intercept(&vmcb02->control, INTERCEPT_CR8_WRITE);
+ if (!(vmcb01->save.rflags & X86_EFLAGS_IF))
+ vmcb_clr_intercept(&vmcb02->control, INTERCEPT_VINTR);
}
/*
@@ -162,14 +161,14 @@ void recalc_intercepts(struct vcpu_svm *svm)
* flush feature is enabled.
*/
if (!nested_svm_l2_tlb_flush_enabled(&svm->vcpu))
- vmcb_clr_intercept(c, INTERCEPT_VMMCALL);
+ vmcb_clr_intercept(&vmcb02->control, INTERCEPT_VMMCALL);
for (i = 0; i < MAX_INTERCEPT; i++)
- c->intercepts[i] |= g->intercepts[i];
+ vmcb02->control.intercepts[i] |= svm->nested.ctl.intercepts[i];
/* If SMI is not intercepted, ignore guest SMI intercept as well */
if (!intercept_smi)
- vmcb_clr_intercept(c, INTERCEPT_SMI);
+ vmcb_clr_intercept(&vmcb02->control, INTERCEPT_SMI);
if (nested_vmcb_needs_vls_intercept(svm)) {
/*
@@ -177,10 +176,10 @@ void recalc_intercepts(struct vcpu_svm *svm)
* we must intercept these instructions to correctly
* emulate them in case L1 doesn't intercept them.
*/
- vmcb_set_intercept(c, INTERCEPT_VMLOAD);
- vmcb_set_intercept(c, INTERCEPT_VMSAVE);
+ vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMLOAD);
+ vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMSAVE);
} else {
- WARN_ON(!(c->virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
+ WARN_ON(!(vmcb02->control.virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
}
}
--
2.52.0.457.g6b5491de43-googOn Mon, Jan 12, 2026, Yosry Ahmed wrote:
> recalc_intercepts() currently uses c, h, g as local variables for the
> control area of the current VMCB, vmcb01, and (cached) vmcb12.
>
> The current VMCB should always be vmcb02 when recalc_intercepts() is
> executed in guest mode. Use vmcb01/vmcb02 local variables instead to
> make it clear the function is updating intercepts in vmcb02 based on the
> intercepts in vmcb01 and (cached) vmcb12.
>
> Add a WARNING() if the current VMCB is not in fact vmcb02.
This belongs in a separate patch.
> No functional change intended.
>
> Signed-off-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> ---
> arch/x86/kvm/svm/nested.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index f295a41ec659..2dda52221fd8 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -125,8 +125,7 @@ static bool nested_vmcb_needs_vls_intercept(struct vcpu_svm *svm)
>
> void recalc_intercepts(struct vcpu_svm *svm)
> {
> - struct vmcb_control_area *c, *h;
> - struct vmcb_ctrl_area_cached *g;
> + struct vmcb *vmcb01, *vmcb02;
> unsigned int i;
>
> vmcb_mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> @@ -134,14 +133,14 @@ void recalc_intercepts(struct vcpu_svm *svm)
> if (!is_guest_mode(&svm->vcpu))
> return;
>
> - c = &svm->vmcb->control;
> - h = &svm->vmcb01.ptr->control;
> - g = &svm->nested.ctl;
> + vmcb01 = svm->vmcb01.ptr;
> + vmcb02 = svm->nested.vmcb02.ptr;
> + WARN_ON_ONCE(svm->vmcb != vmcb02);
If we're going to bother with a WARN, then this code should definitely bail,
because configuring vmcb01 using the nested logic is all but guaranteed to break
L1 in weird ways.
> for (i = 0; i < MAX_INTERCEPT; i++)
> - c->intercepts[i] = h->intercepts[i];
> + vmcb02->control.intercepts[i] = vmcb01->control.intercepts[i];
>
> - if (g->int_ctl & V_INTR_MASKING_MASK) {
> + if (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) {
I vote to keep a pointer to the cached control as vmcb12_ctrl. Coming from a
nVMX-focused background, I can never remember what svm->nested.ctl holds. For
me, this is waaaay more intuivite:
if (vmcb12_ctrl->int_ctl & V_INTR_MASKING_MASK) {
> for (i = 0; i < MAX_INTERCEPT; i++)
> - c->intercepts[i] |= g->intercepts[i];
> + vmcb02->control.intercepts[i] |= svm->nested.ctl.intercepts[i];
And even more so here:
for (i = 0; i < MAX_INTERCEPT; i++)
vmcb02->control.intercepts[i] |= vmcb12_ctrl->intercepts[i];
>
> /* If SMI is not intercepted, ignore guest SMI intercept as well */
> if (!intercept_smi)
> - vmcb_clr_intercept(c, INTERCEPT_SMI);
> + vmcb_clr_intercept(&vmcb02->control, INTERCEPT_SMI);
>
> if (nested_vmcb_needs_vls_intercept(svm)) {
> /*
> @@ -177,10 +176,10 @@ void recalc_intercepts(struct vcpu_svm *svm)
> * we must intercept these instructions to correctly
> * emulate them in case L1 doesn't intercept them.
> */
> - vmcb_set_intercept(c, INTERCEPT_VMLOAD);
> - vmcb_set_intercept(c, INTERCEPT_VMSAVE);
> + vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMLOAD);
> + vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMSAVE);
> } else {
> - WARN_ON(!(c->virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
> + WARN_ON(!(vmcb02->control.virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
Opportunistically switch this to WARN_ON_ONCE. Any "unguarded" WARN in KVM
(outside of e.g. __init code) is just asking for a self-DoS.
> }
> }
>
> --
> 2.52.0.457.g6b5491de43-goog
>
On Wed, Feb 04, 2026 at 09:29:36AM -0800, Sean Christopherson wrote:
> On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > recalc_intercepts() currently uses c, h, g as local variables for the
> > control area of the current VMCB, vmcb01, and (cached) vmcb12.
> >
> > The current VMCB should always be vmcb02 when recalc_intercepts() is
> > executed in guest mode. Use vmcb01/vmcb02 local variables instead to
> > make it clear the function is updating intercepts in vmcb02 based on the
> > intercepts in vmcb01 and (cached) vmcb12.
> >
> > Add a WARNING() if the current VMCB is not in fact vmcb02.
>
> This belongs in a separate patch.
>
> > No functional change intended.
> >
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> > ---
> > arch/x86/kvm/svm/nested.c | 31 +++++++++++++++----------------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index f295a41ec659..2dda52221fd8 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -125,8 +125,7 @@ static bool nested_vmcb_needs_vls_intercept(struct vcpu_svm *svm)
> >
> > void recalc_intercepts(struct vcpu_svm *svm)
> > {
> > - struct vmcb_control_area *c, *h;
> > - struct vmcb_ctrl_area_cached *g;
> > + struct vmcb *vmcb01, *vmcb02;
> > unsigned int i;
> >
> > vmcb_mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> > @@ -134,14 +133,14 @@ void recalc_intercepts(struct vcpu_svm *svm)
> > if (!is_guest_mode(&svm->vcpu))
> > return;
> >
> > - c = &svm->vmcb->control;
> > - h = &svm->vmcb01.ptr->control;
> > - g = &svm->nested.ctl;
> > + vmcb01 = svm->vmcb01.ptr;
> > + vmcb02 = svm->nested.vmcb02.ptr;
> > + WARN_ON_ONCE(svm->vmcb != vmcb02);
>
> If we're going to bother with a WARN, then this code should definitely bail,
> because configuring vmcb01 using the nested logic is all but guaranteed to break
> L1 in weird ways.
I can put the WARN + bail in a separate patch.
>
> > for (i = 0; i < MAX_INTERCEPT; i++)
> > - c->intercepts[i] = h->intercepts[i];
> > + vmcb02->control.intercepts[i] = vmcb01->control.intercepts[i];
> >
> > - if (g->int_ctl & V_INTR_MASKING_MASK) {
> > + if (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) {
>
> I vote to keep a pointer to the cached control as vmcb12_ctrl. Coming from a
> nVMX-focused background, I can never remember what svm->nested.ctl holds. For
> me, this is waaaay more intuivite:
I agree it reads better, but honestly all of nSVM code uses
svm->nested.ctl, and changing its name here just makes things
inconsistent imo.
>
> if (vmcb12_ctrl->int_ctl & V_INTR_MASKING_MASK) {
>
> > for (i = 0; i < MAX_INTERCEPT; i++)
> > - c->intercepts[i] |= g->intercepts[i];
> > + vmcb02->control.intercepts[i] |= svm->nested.ctl.intercepts[i];
>
> And even more so here:
>
> for (i = 0; i < MAX_INTERCEPT; i++)
> vmcb02->control.intercepts[i] |= vmcb12_ctrl->intercepts[i];
>
> >
> > /* If SMI is not intercepted, ignore guest SMI intercept as well */
> > if (!intercept_smi)
> > - vmcb_clr_intercept(c, INTERCEPT_SMI);
> > + vmcb_clr_intercept(&vmcb02->control, INTERCEPT_SMI);
> >
> > if (nested_vmcb_needs_vls_intercept(svm)) {
> > /*
> > @@ -177,10 +176,10 @@ void recalc_intercepts(struct vcpu_svm *svm)
> > * we must intercept these instructions to correctly
> > * emulate them in case L1 doesn't intercept them.
> > */
> > - vmcb_set_intercept(c, INTERCEPT_VMLOAD);
> > - vmcb_set_intercept(c, INTERCEPT_VMSAVE);
> > + vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMLOAD);
> > + vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMSAVE);
> > } else {
> > - WARN_ON(!(c->virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
> > + WARN_ON(!(vmcb02->control.virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
>
> Opportunistically switch this to WARN_ON_ONCE. Any "unguarded" WARN in KVM
> (outside of e.g. __init code) is just asking for a self-DoS.
Will do.
>
> > }
> > }
> >
> > --
> > 2.52.0.457.g6b5491de43-goog
> >
On Wed, Feb 04, 2026, Yosry Ahmed wrote:
> On Wed, Feb 04, 2026 at 09:29:36AM -0800, Sean Christopherson wrote:
> >
> > > for (i = 0; i < MAX_INTERCEPT; i++)
> > > - c->intercepts[i] = h->intercepts[i];
> > > + vmcb02->control.intercepts[i] = vmcb01->control.intercepts[i];
> > >
> > > - if (g->int_ctl & V_INTR_MASKING_MASK) {
> > > + if (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) {
> >
> > I vote to keep a pointer to the cached control as vmcb12_ctrl. Coming from a
> > nVMX-focused background, I can never remember what svm->nested.ctl holds. For
> > me, this is waaaay more intuivite:
>
> I agree it reads better, but honestly all of nSVM code uses svm->nested.ctl,
> and changing its name here just makes things inconsistent imo.
Gotta start somewhere :-) In all seriousness, if we didn't allow chipping away
to at historical oddities in KVM, the code base would be a disaster. I'm all for
prioritizing consistency, but I draw the line at "everything else sucks, so this
needs to suck too".
I'm not saying we need to do a wholesale rename, but giving at least
nested_vmcb02_prepare_control() the same treatment will be a huge improvement.
Actually, I'm going to go do that right now...
On Wed, Feb 04, 2026 at 09:55:36AM -0800, Sean Christopherson wrote:
> On Wed, Feb 04, 2026, Yosry Ahmed wrote:
> > On Wed, Feb 04, 2026 at 09:29:36AM -0800, Sean Christopherson wrote:
> > >
> > > > for (i = 0; i < MAX_INTERCEPT; i++)
> > > > - c->intercepts[i] = h->intercepts[i];
> > > > + vmcb02->control.intercepts[i] = vmcb01->control.intercepts[i];
> > > >
> > > > - if (g->int_ctl & V_INTR_MASKING_MASK) {
> > > > + if (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) {
> > >
> > > I vote to keep a pointer to the cached control as vmcb12_ctrl. Coming from a
> > > nVMX-focused background, I can never remember what svm->nested.ctl holds. For
> > > me, this is waaaay more intuivite:
> >
> > I agree it reads better, but honestly all of nSVM code uses svm->nested.ctl,
> > and changing its name here just makes things inconsistent imo.
>
> Gotta start somewhere :-) In all seriousness, if we didn't allow chipping away
> to at historical oddities in KVM, the code base would be a disaster. I'm all for
> prioritizing consistency, but I draw the line at "everything else sucks, so this
> needs to suck too".
>
> I'm not saying we need to do a wholesale rename, but giving at least
> nested_vmcb02_prepare_control() the same treatment will be a huge improvement.
> Actually, I'm going to go do that right now...
For what it's worth, at some point I was going to send a patch to put
svm->nested.ctl and svm->nested.save in an anonymous struct, to end up
with svm->nested.cached_vmcb12.ctl and svm->nested.cached_vmcb12.save,
but the names are too long :)
© 2016 - 2026 Red Hat, Inc.