[0/3] KVM: x86: Clean up RESET "emulation"

Message

Sean Christopherson Sept. 14, 2021, 11:08 p.m. UTC

Add dedicated helpers to emulate RESET instead of having the relevant code
scattered through vcpu_create() and vcpu_reset().  Paolo, I think this is
what you meant by "have init_vmcb/svm_vcpu_reset look more like the VMX
code"[*].

Patch 01 is a bit odd; it's essentially an explicit acknowledgement that
KVM's emulation is far from complete.  It caught my eye when auditing the
"create" flows to ensure they didn't touch guest state, which should be
handled by "reset".  I waffled between deleting it outright and moving it
to the new __vmx_vcpu_reset(), and opted to delete outright to discourage
ad hoc clearing of MSRs during RESET, which isn't a maintainable approach.

[*] https://lore.kernel.org/all/[e-mail]/

Sean Christopherson (3):
  KVM: VMX: Drop explicit zeroing of MSR guest values at vCPU creation
  KVM: VMX: Move RESET emulation to vmx_vcpu_reset()
  KVM: SVM: Move RESET emulation to svm_vcpu_reset()

 arch/x86/kvm/svm/sev.c |  6 ++--
 arch/x86/kvm/svm/svm.c | 29 ++++++++++--------
 arch/x86/kvm/svm/svm.h |  2 +-
 arch/x86/kvm/vmx/vmx.c | 67 ++++++++++++++++++++----------------------
 4 files changed, 53 insertions(+), 51 deletions(-)

Comments

Paolo Bonzini Sept. 17, 2021, 4:15 p.m. UTC | #1

On Wed, Sep 15, 2021 at 1:08 AM Sean Christopherson <[e-mail]> wrote:
> Add dedicated helpers to emulate RESET instead of having the relevant code
> scattered through vcpu_create() and vcpu_reset().  Paolo, I think this is
> what you meant by "have init_vmcb/svm_vcpu_reset look more like the VMX
> code"[*].
>
> [*] https://lore.kernel.org/all/[e-mail]/

That assumes that I remember what I meant :) but I do like it so yes,
that was it. Especially the fact that init_vmcb now has a single
caller. I would further consider moving save area initialization to
*_vcpu_reset, and keeping the control fields in init_vmcb/vmcs. That
would make it easier to relate the two functions to separate parts of
the manuals.

I should go back to KVM next week. Context switching with KVM Forum
and Kangrejos this week made everything so much slower than I'd liked.

Paolo

Sean Christopherson Sept. 17, 2021, 5:34 p.m. UTC | #2

On Fri, Sep 17, 2021, Paolo Bonzini wrote:
> On Wed, Sep 15, 2021 at 1:08 AM Sean Christopherson <[e-mail]> wrote:
> > Add dedicated helpers to emulate RESET instead of having the relevant code
> > scattered through vcpu_create() and vcpu_reset().  Paolo, I think this is
> > what you meant by "have init_vmcb/svm_vcpu_reset look more like the VMX
> > code"[*].
> >
> > [*] https://lore.kernel.org/all/[e-mail]/
> 
> That assumes that I remember what I meant :)

Ha!  That's why I write changelogs with --verbose :-)

> but I do like it so yes, that was it. Especially the fact that init_vmcb now
> has a single caller. I would further consider moving save area initialization
> to *_vcpu_reset, and keeping the control fields in init_vmcb/vmcs. That would
> make it easier to relate the two functions to separate parts of the manuals.

I like the idea, but I think I'd prefer to tackle that at the same time as generic
support for handling MSRs at RESET/INIT.  E.g. instead of manually writing
vmcs.GUEST_SYSENTER_* at RESET, provide infrastruture to automagically run through
all emulated/virtualized at RESET and/or INIT as appropriate to initialize the
guest value.

Paolo Bonzini Sept. 17, 2021, 5:37 p.m. UTC | #3

On 17/09/21 19:34, Sean Christopherson wrote:
>> but I do like it so yes, that was it. Especially the fact that init_vmcb now
>> has a single caller. I would further consider moving save area initialization
>> to *_vcpu_reset, and keeping the control fields in init_vmcb/vmcs. That would
>> make it easier to relate the two functions to separate parts of the manuals.
>
> I like the idea, but I think I'd prefer to tackle that at the same time as generic
> support for handling MSRs at RESET/INIT.

No problem, just roughly sketching some ideas for the future.  But 
you're absolutely right that some MSRs have effects on the control areas 
rather than the save area (and some have effects on neither).

Thanks,

Paolo

> E.g. instead of manually writing
> vmcs.GUEST_SYSENTER_* at RESET, provide infrastruture to automagically run through
> all emulated/virtualized at RESET and/or INIT as appropriate to initialize the
> guest value.
>