On Fri, Mar 29, 2019 at 10:51:19AM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
>
> > Disclaimer: this is very much an RFC. All patches are compile tested
> > only, the changelogs are non-existent and the order of the patches is
> > completely backwards in a lot of ways, e.g. the fix should really be
> > squeezed in much earlier. I'd like to get feedback on the idea before
> > actually spending the effort to get it working. Without further ado...
> >
>
> Thanks for working on this!
>
> > RSM emulation is currently broken on VMX when the interrupted guest has
> > CR4.VMXE=1. There are a handful of ideas to fix the issue, but they are
> > all undesirable in some way, e.g. hacky, ugly, fragile, etc...
> >
> > Rather than dance around the issue of HF_SMM_MASK being set when loading
> > SMSTATE into architectural state, rework RSM emulation itself to clear
> > HF_SMM_MASK prior to loading architectural state. The basic concept is
> > to move the guts of em_rsm() out of the emulator and into x86.c as
> > leave_smm() so that leave_smm() can modify HF_SMM_MASK in a more granular
> > way, i.e. clear HF_SMM_MASK without triggerring kvm_smm_changed().
> >
> > AFAICT, the only motivation for having HF_SMM_MASK set throughout is so
> > that the memory access from GET_SMSTATE() are tagged with role.smm (though
> > arguably even that is unnecessary). This can be avoided by taking the
> > enter_smm() approach of reading all of SMSTATE into a buffer and then
> > loading state from the buffer. Note, it's entirely possible this will
> > break horribly, commit 660a5d517aaa ("KVM: x86: save/load state on SMM
> > switch") does not provide any insight as to why enter_smm() buffers
> > memory while em_rsm() reads it piecemeal.
>
> I got interested, googled for mailing list discussions but still can't
> see the reasoning so we'll have to explicitly summon Paolo and Radim :-)
>
> >
> > Almost all of the patches are unrelated cleanup to remove usage of
> > emulator function as much as possible, and to make leave_smm() mirror
> > enter_smm().
>
> One may wonder why we're getting away from using emulator abstractions
> for RSM moving it to x86.c; we may argue that it would be too cumbersome
> to abstract the missing pieces and SMM is a very special case
That was pretty much my thought process as well. A non-trivial number
of x86_emulate_ops are used only for RSM, and the whole duplicate HFLAGS
#defines is quite ugly.
My other cheeky argument would be that RSM is just a really short
instruction that triggers the ucode flow for leaving SMM ;-)
>, or we may
> think that the abstraction layer is too thin anyways and it adds more
> complexity than benefits, not only for SMM... That one is not going to
> be me :-)
>
> >
> > Lastly, even if this approach works, it likely makes sense to take
> > Vitaly's patch (to toggle HF_SMM_MASK when setting CR4 during RSM)
> > so that the actual fix less risky and can be easily backported.
>
> Thank you for suggesting this, if maintainers agree I would also want to
> have a simple-and-backportable crutch commited first.
>
> >
> > Sean Christopherson (18):
> > KVM: x86: Move emulation of RSM, i.e. leave_smm, out of emulator
> > KVM: x86: Drop emulator_has_longmode()
> > KVM: x86: Drop emulator_pre_leave_smm()
> > KVM: x86: Drop emulator_set_hflags()
> > KVM: x86: Add emulator_is_smm()
> > KVM: x86: Add emulator_is_guest_mode()
> > KVM: x86: Drop emulator_get_hflags()
> > KVM: x86: Call set_nmi_mask() directly when leaving SMM
> > KVM: x86: Call kvm_{get,set}_cr*() directly when leaving SMM
> > KVM: x86: Call __kvm_set_dr() directly when leaving SMM
> > KVM: x86: Call kvm_x86_ops.set_*dt() directly when leaving SMM
> > KVM: x86: call emulator_set_msr() directly when leaving SMM
> > KVM: x86: Drop x86_emulate_ops.read_phys()
> > KVM: x86: Use kvm_set_segment() directly when leaving SMM
> > KVM: x86: Invert passing of vcpu and ctxt when leaving SMM
> > KVM: x86: Open code kvm_set_hflags
> > KVM: x86: Load SMRAM in a single shot when leaving SMM
> > KVM: x86: clear SMM flags before loading state while leaving SMM
> >
> > arch/x86/include/asm/kvm_emulate.h | 23 +-
> > arch/x86/include/asm/kvm_host.h | 5 +-
> > arch/x86/kvm/emulate.c | 310 +-----------------------
> > arch/x86/kvm/svm.c | 30 +--
> > arch/x86/kvm/vmx/vmx.c | 4 +-
> > arch/x86/kvm/x86.c | 377 ++++++++++++++++++++++++-----
> > 6 files changed, 348 insertions(+), 401 deletions(-)
>
> --
> Vitaly
