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:
> > > > > > +       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?

Nope, not with the kernel's build flags.

> 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;
> +}

...

> Is this sufficient?

It's certainly better, but unless a sea of helpers is orders of magnitude worse,
I would prefer to make it even harder to put hole in our foot.

E.g. unless we're hyper diligent about constifying everything, it's not _that_
hard to imagine a chain of events where we end up with a "live" pointer to the
cache.

  1. A helper like __nested_vmcb_check_controls() isn't const, so we cast to strip
     the const.

  2. Someone "improves" the code by grabbing the non-const variable to pass it
     into other helpers.

  3. The non-const variable is used to update the cache for whatever reason, and
     it works 99.9% of the time, until it doesn't.

Now, I don't think that's at all likely to happen, but as the years pile on and
developers come and go, the probability of introducing a goof goes up, bit by bit.

> > > > > 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), 

Yep.

> or what do you mean by "add the flag"?