Re: [PATCH v2] KVM: SVM: use vmsave/vmload for saving/restoring additional host state

[Sean Christopherson <seanjc@google.com>]

On Mon, Dec 14, 2020, Michael Roth wrote:
> On Mon, Dec 14, 2020 at 11:38:23AM -0800, Sean Christopherson wrote:
> > > +	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.

That can, and should, be optimized away.  Sorry I didn't make that clear.  The
below will yield high level symmetry with VMX, which I like.

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 523df10fb979..057661723a5c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1399,6 +1399,7 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)

        avic_vcpu_put(vcpu);

+       svm->host_state_saved = false;
        ++vcpu->stat.host_state_reload;
        if (sev_es_guest(svm->vcpu.kvm)) {
                sev_es_vcpu_put(svm);
@@ -3522,6 +3523,12 @@ static void svm_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t gva)

 static void svm_prepare_guest_switch(struct kvm_vcpu *vcpu)
 {
+       struct vcpu_svm *svm = to_svm(vcpu);
+
+       if (!svm->host_state_saved) {
+               svm->need_host_state_save = true;
+               vmsave();
+       }
 }


> 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.
>
> It looks like the SEV-ES patches might land before this one, and those
> introduce similar handling of VMSAVE in svm_vcpu_load(), so I think it
> might also create some churn there if we take this approach and want to
> keep the SEV-ES and non-SEV-ES handling similar.

Hmm, I'll make sure to pay attention to that when I review the SEV-ES patches,
which I was hoping to get to today, but that's looking unlikely at this point.