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

[Sean Christopherson <seanjc@google.com>]

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.

> > > > +       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.

> > > 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.