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

From: Michael Roth <michael.roth@amd.com>
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H . Peter Anvin" <hpa@zytor.com>,
	linux-kernel@vger.kernel.org,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Andy Lutomirski <luto@kernel.org>
Subject: Re: [PATCH v2] KVM: SVM: use vmsave/vmload for saving/restoring additional host state
Date: Tue, 15 Dec 2020 12:55:41 -0600	[thread overview]
Message-ID: <20201215185541.nxm2upy76u7z2ko6@amd.com> (raw)
In-Reply-To: <X9e/L3YTAT/N+ljh@google.com>

Hi Sean,

Sorry to reply out-of-thread, our mail server is having issues with
certain email addresses at the moment so I only see your message via
the archives atm. But regarding:

>>> I think we can defer this until we're actually planning on running
>>> the guest,
>>> i.e. put this in svm_prepare_guest_switch().
>>
>> 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.

It looks like SEV-ES patches are queued now. Those patches have
undergone a lot of internal testing so I'm really hesitant to introduce
any significant change to those at this stage as a prereq for my little
patch. So for v3 I'm a little unsure how best to approach this.

The main options are:

a) go ahead and move the vmsave handling for non-sev-es case into
   prepare_guest_switch() as you suggested, but leave the sev-es where
   they are. then we can refactor those as a follow-up patch that can be
   tested/reviewed as a separate series after we've had some time to
   re-test, though that would probably just complicate the code in the
   meantime...

b) stick with the current approach for now, and consider a follow-up series
   to refactor both sev-es and non-sev-es as a whole that we can test
   separately.

c) refactor SEV-ES handling as part of this series. it's only a small change
   to the SEV-ES code but it re-orders enough things around that I'm
   concerned it might invalidate some of the internal testing we've done.
   whereas a follow-up refactoring such as the above options can be rolled
   into our internal testing so we can let our test teams re-verify

Obviously I prefer b) but I'm biased on the matter and fine with whatever
you and others think is best. I just wanted to point out my concerns with
the various options.

-Mike