Re: [Patch 2/4] KVM:SVM: Introduce set_spte_notify support

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Sun, Aug 02, 2020 at 03:53:54PM -0500, Eric van Tassell wrote:
> 
> On 7/31/20 3:25 PM, Sean Christopherson wrote:
> >On Fri, Jul 24, 2020 at 06:54:46PM -0500, eric van tassell wrote:
> >>Improve SEV guest startup time from O(n) to a constant by deferring
> >>guest page pinning until the pages are used to satisfy nested page faults.
> >>
> >>Implement the code to do the pinning (sev_get_page) and the notifier
> >>sev_set_spte_notify().
> >>
> >>Track the pinned pages with xarray so they can be released during guest
> >>termination.
> >
> >I like that SEV is trying to be a better citizen, but this is trading one
> >hack for another.
> >
> >   - KVM goes through a lot of effort to ensure page faults don't need to
> >     allocate memory, and this throws all that effort out the window.
> >
> can you elaborate on that?

mmu_topup_memory_caches() is called from the page fault handlers before
acquiring mmu_lock to pre-allocate shadow pages, PTE list descriptors, GFN
arrays, etc... that may be needed to handle the page fault.  This allows
using standard GFP flags for the allocation and obviates the need for error
handling in the consumers.

> >>+int sev_set_spte_notify(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn,
> >>+			int level, bool mmio, u64 *spte)
> >>+{
> >>+	int rc;
> >>+
> >>+	if (!sev_guest(vcpu->kvm))
> >>+		return 0;
> >>+
> >>+	/* MMIO page contains the unencrypted data, no need to lock this page */
> >>+	if (mmio)
> >
> >Rather than make this a generic set_spte() notify hook, I think it makes
> >more sense to specifying have it be a "pin_spte" style hook.  That way the
> >caller can skip mmio PFNs as well as flows that can't possibly be relevant
> >to SEV, e.g. the sync_page() flow.
> Not sure i understand. We do ignore mmio here.

I'm saying we can have the caller, i.e. set_spte(), skip the hook for MMIO.
If the kvm_x86_ops hook is specifically designed to allow pinning pages (and
to support related features), then set_spte() can filter out MMIO PFNs.  It's
a minor detail, but it's one less thing to have to check in the vendor code.

> Can you detail a bit more what you see as problematic with the sync_page() flow?

There's no problem per se.  But, assuming TDP/NPT is required to enable SEV,
then sync_page() simply isn't relevant for pinning a host PFN as pages can't
become unsynchronized when TDP is enabled, e.g. ->sync_page() is a nop when
TDP is enabled.  If the hook is completely generic, then we need to think
about how it interacts with changing existing SPTEs via ->sync_page().  Giving
the hook more narrowly focused semantics means we can again filter out that
path and not have to worry about testing it.

The above doesn't hold true for nested TDP/NPT, but AIUI SEV doesn't yet
support nested virtualization, i.e. it's a future problem.