Message-ID: <smsla7jgdncodh57uh7dihumnteu5sgxyzby2jc6lcp3moayzf@ixqj4ivmlgb2>
Date: Wed, 11 Feb 2026 01:02:46 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/4] KVM: nSVM: Sync next_rip to cached vmcb12 after
 VMRUN of L2

On Tue, Feb 10, 2026 at 04:39:28PM -0800, Sean Christopherson wrote:
> On Wed, Feb 11, 2026, Yosry Ahmed wrote:
> > > > We can drop it and make it a local vaiable in nested_svm_vmrun(), and
> > > > plumb it all the way down. But it could be too big for the stack.
> > > 
> > > It's 48 bytes, there's no way that's too big.
> > 
> > That's before my hardening series shoved everything in there. It's now
> > 256 bytes, which is not huge, but makes me nervous. Especially that it
> > may grow more in the future.
> > 
> > > > Allocating it every time isn't nice either.
> > > 
> > > > Do you mean to also make it opaque?
> > > 
> > > I'd prefer to drop it.
> > 
> > Me too, but I am nervous about putting it on the stack.
> 
> 256 bytes should be tolerable.  500+ is where things tend to get dicey.

In that case I think removing it completely should be fine.

> 
> > > > > +       u8 __vmcb12_ctrl[sizeof(struct vmcb_ctrl_area_cached)];
> > > > 
> > > > We have a lot of accesses to svm->nested.ctl, so we'll need a lot of
> > > > clutter to cast the field in all of these places.
> > > > 
> > > > Maybe we add a read-only accessor that returns a pointer to a constant
> > > > struct?
> > > 
> > > That's what I said :-D
> > > 
> > > 	* All reads are routed through accessors to make it all but impossible
> > > 	* for KVM to clobber its snapshot of vmcb12.
> > > 
> > > There might be a lot of helpers, but I bet it's less than nVMX has for vmcs12.
> > 
> > Oh I meant instead of having a lot of helpers, have a single helper that
> > returns it as a pointer to const struct vmcb_ctrl_area_cached? Then all
> > current users just switch to the helper instead of directly using
> > svm->nested.ctl.
> > 
> > We can even name it sth more intuitive like svm_cached_vmcb12_control().
> 
> That makes it to easy to do something like:
> 
> 
> 	u32 *int_ctl = svm_cached_vmcb12_control(xxx).
> 
> 	*int_ctl |= xxx;
> 
> Which is what I want to defend against.

Do compilers allow implicit dropping of const qualifiers?

Building with this diff fails for me:

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index de90b104a0dd..0a73dd8f9163 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1343,10 +1343,17 @@ static void nested_svm_triple_fault(struct kvm_vcpu *vcpu)
        nested_svm_simple_vmexit(to_svm(vcpu), SVM_EXIT_SHUTDOWN);
 }

+static const struct vmcb_ctrl_area_cached *svm_cached_vmcb12_control(struct vcpu_svm *svm) {
+       return &svm->nested.ctl;
+}
+
 int svm_allocate_nested(struct vcpu_svm *svm)
 {
+       struct vmcb_ctrl_area_cached *ctl = svm_cached_vmcb12_control(svm);
        struct page *vmcb02_page;

+       pr_info("%p\n", ctl);
+
        if (svm->nested.initialized)
                return 0;


I see:

arch/x86/kvm/svm/nested.c:1352:32: error: initializing 'struct vmcb_ctrl_area_cached *' with an expression of type 'const struct vmcb_ctrl_area_cached *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
 1352 |         struct vmcb_ctrl_area_cached *ctl = svm_cached_vmcb12_control(svm);
      |                                       ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

I don't explicitly see 'incompatible-pointer-types-discards-qualifiers'
anywhere, but I do see 'incompatible-pointer-types' in
scripts/Makefile.warn:

KBUILD_CFLAGS += $(call cc-option,-Werror=incompatible-pointer-types)

Is this sufficient?

> 
> > > > I think this will be annoying when new fields are added, like
> > > > insn_bytes. Perhaps at some point we move to just serializing the entire
> > > > combined vmcb02/vmcb12 control area and add a flag for that.
> > > 
> > > If we do it now, can we avoid the flag?
> > 
> > I don't think so. Fields like insn_bytes are not currently serialized at
> > all. The moment we need them, we'll probably need to add a flag, at
> > which point serializing everything under the flag would probably be the
> > sane thing to do.
> > 
> > That being said, I don't really know how a KVM that uses insn_bytes
> > should handle restoring from an older KVM that doesn't serialize it :/
> > 
> > Problem for the future, I guess :)
> 
> Oh, good point.  In that case, I think it makes sense to add the flag asap, so
> that _if_ it turns out that KVM needs to consume a field that isn't currently
> saved/restored, we'll at least have a better story for KVM's that save/restore
> everything.

Not sure I follow. Do you mean start serializing everything and setting
the flag ASAP (which IIUC would be after the rework we discussed), or
what do you mean by "add the flag"?

Powered by blists - more mailing lists