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

[Yosry Ahmed]

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

I would argue that casting to strip the const is a red flag and this
scenario should have stopped here :P

That being said, as you said, it depends on how many helpers we'll
actually need.

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

I don't think it matters that much when we start doing this. In all
cases:

1. KVM will need to be backward-compatible.

2. Any new features that depend on save+restore of those fields will be
a in a new KVM that does the 'full' save+restore (assuming we don't let
people add per-field flags).

The only scenario that I can think of is if a feature can be enabled at
runtime, and we want to be able to enable it for a running VM after
migrating from an old KVM to a new KVM. Not sure how likely this is.