lkml.org 
[lkml]   [2020]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] KVM: SVM: use vmsave/vmload for saving/restoring additional host state
    From
    Date


    > On Dec 14, 2020, at 2:02 PM, Michael Roth <michael.roth@amd.com> wrote:
    >
    > On Mon, Dec 14, 2020 at 11:38:23AM -0800, Sean Christopherson wrote:
    >> +Andy, who provided a lot of feedback on v1.
    >> On Mon, Dec 14, 2020, Michael Roth wrote:
    >> Cc: Andy Lutomirski <luto@kernel.org>
    >>> Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
    >>> Signed-off-by: Michael Roth <michael.roth@amd.com>
    >>> ---
    >>> v2:
    >>> * rebase on latest kvm/next
    >>> * move VMLOAD to just after vmexit so we can use it to handle all FS/GS
    >>> host state restoration and rather than relying on loadsegment() and
    >>> explicit write to MSR_GS_BASE (Andy)
    >>> * drop 'host' field from struct vcpu_svm since it is no longer needed
    >>> for storing FS/GS/LDT state (Andy)
    >>> ---
    >>> arch/x86/kvm/svm/svm.c | 44 ++++++++++++++++--------------------------
    >>> arch/x86/kvm/svm/svm.h | 14 +++-----------
    >>> 2 files changed, 20 insertions(+), 38 deletions(-)
    >>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
    >>> index 0e52fac4f5ae..fb15b7bd461f 100644
    >>> --- a/arch/x86/kvm/svm/svm.c
    >>> +++ b/arch/x86/kvm/svm/svm.c
    >>> @@ -1367,15 +1367,19 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
    >>> vmcb_mark_all_dirty(svm->vmcb);
    >>> }
    >>> -#ifdef CONFIG_X86_64
    >>> - rdmsrl(MSR_GS_BASE, to_svm(vcpu)->host.gs_base);
    >>> -#endif
    >>> - savesegment(fs, svm->host.fs);
    >>> - savesegment(gs, svm->host.gs);
    >>> - svm->host.ldt = kvm_read_ldt();
    >>> -
    >>> - for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
    >>> + for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++) {
    >>> rdmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
    >>> + }
    >
    > Hi Sean,
    >
    > Hopefully I've got my email situation sorted out now...
    >
    >> Unnecessary change that violates preferred coding style. Checkpatch explicitly
    >> complains about this.
    >> WARNING: braces {} are not necessary for single statement blocks
    >> #132: FILE: arch/x86/kvm/svm/svm.c:1370:
    >> + for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++) {
    >> rdmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
    >> +
    >
    > Sorry, that was an artifact from an earlier version of the patch that I
    > failed to notice. I'll make sure to run everything through checkpatch
    > going forward.
    >
    >>> +
    >>> + asm volatile(__ex("vmsave")
    >>> + : : "a" (page_to_pfn(sd->save_area) << PAGE_SHIFT)
    >> I'm pretty sure this can be page_to_phys().
    >>> + : "memory");
    >> I think we can defer this until we're actually planning on running the guest,
    >> i.e. put this in svm_prepare_guest_switch().
    >
    > One downside to that is that we'd need to do the VMSAVE on every
    > iteration of vcpu_run(), as opposed to just once when we enter from
    > userspace via KVM_RUN. It ends up being a similar situation to Andy's
    > earlier suggestion of moving VMLOAD just after vmexit, but in that case
    > we were able to remove an MSR write to MSR_GS_BASE, which cancelled out
    > the overhead, but in this case I think it could only cost us extra.

    If you want to micro-optimize, there is a trick you could play: use WRGSBASE if available. If X86_FEATURE_GSBASE is available, you could use WRGSBASE to restore GSBASE and defer VMLOAD to vcpu_put(). This would need benchmarking on Zen 3 to see if it’s worthwhile.


    \
     
     \ /
      Last update: 2020-12-14 23:47    [W:5.431 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site