Re: [PATCH 1/4] KVM: nSVM: Sync next_rip to cached vmcb12 after VMRUN of L2

[Yosry Ahmed]

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"?