Re: [PATCH 1/3] KVM: x86: Load SMRAM in a single shot when leaving SMM

Re: [PATCH 1/3] KVM: x86: Load SMRAM in a single shot when leaving SMM

[Paolo Bonzini]

On 02/04/19 17:03, Sean Christopherson wrote:
> Ostensibly, the only motivation for having HF_SMM_MASK set throughout
> the loading of state from the SMRAM save state area is so that the
> memory accesses from GET_SMSTATE() are tagged with role.smm (though
> arguably even that is unnecessary). 

Why is that unnecessary?  If they do not have role.smm they would access
video RAM or TSEG, not the state save area.

Paolo

Re: [PATCH 1/3] KVM: x86: Load SMRAM in a single shot when leaving SMM

[Sean Christopherson]

On Wed, Apr 10, 2019 at 11:19:19AM +0200, Paolo Bonzini wrote:
> On 02/04/19 17:03, Sean Christopherson wrote:
> > Ostensibly, the only motivation for having HF_SMM_MASK set throughout
> > the loading of state from the SMRAM save state area is so that the
> > memory accesses from GET_SMSTATE() are tagged with role.smm (though
> > arguably even that is unnecessary). 
> 
> Why is that unnecessary?  If they do not have role.smm they would access
> video RAM or TSEG, not the state save area.

You're right, feel free to strike that line from the record.  For some
reason I had it in my mind that enter_smm() was writing SMRAM before
setting HF_SMM_MASK.

Re: [PATCH 0/3] KVM: x86: clear HF_SMM_MASK before loading state

[Paolo Bonzini]

On 02/04/19 17:03, Sean Christopherson wrote:
> RSM emulation is currently broken on VMX when the interrupted guest has
> CR4.VMXE=1.  Similar breakage has also occurred in the past due to
> HF_SMM_MASK being cleared only after RSM completes, i.e. loading non-SMM
> state while is_smm() returns true is unsurprisingly problematic.
> 
> 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.   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).  Sidestep that particular issue by taking the
> enter_smm() approach of reading all of SMSTATE into a buffer and then
> loading state from the buffer.
> 
> The actual fix is the same concept as an earlier RFC, but without first
> moving em_rsm() to x86.c, i.e. doesn't add a big pile of dependent patches
> before fixing the bug.  I'm still planning on sending a series to move
> the bulk of em_rsm() to x86.c, but it'll be a true cleanup.
> 
> [1] https://patchwork.kernel.org/cover/10875623/
> 
> Sean Christopherson (3):
>   KVM: x86: Load SMRAM in a single shot when leaving SMM
>   KVM: x86: Open code kvm_set_hflags
>   KVM: x86: clear SMM flags before loading state while leaving SMM
> 
>  arch/x86/include/asm/kvm_emulate.h |   4 +-
>  arch/x86/include/asm/kvm_host.h    |   5 +-
>  arch/x86/kvm/emulate.c             | 160 +++++++++++++++--------------
>  arch/x86/kvm/svm.c                 |  30 ++----
>  arch/x86/kvm/vmx/vmx.c             |   4 +-
>  arch/x86/kvm/x86.c                 |  38 ++++---
>  6 files changed, 118 insertions(+), 123 deletions(-)
> 

Queued, thanks.  I only changed the name of the emulator callback from
smm_changed to post_leave_smm, since it is invoked only on RSM.

Paolo