Re: [PATCH v5 6/7] KVM: x86: Combine .gmem_prepare()+.gmem_invalidate() into .gmem_convert()

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> Smush x86's prepare() and invalidate() hooks into a common convert() flow,
> as they are effectively two sides of the same coin for SNP: they're invoked
> when private/shared memory is about to made accessible/visible to the guest
> or host.  I.e. prepare() is really "make private", and invalidate() is
> really "make shared".
>
> Using a single hook will yield more intuitive code when in-place conversion
> comes along.
>
> For all intents and purposes, no functional change intended.
>
>
> [...snip...]
>

I just tried this out with in-place conversion and I'm stuck trying to
make a call to kvm_arch_gmem_convert() in the conversions ioctl.

I think we wanted to have a single .gmem_convert callback rather than
.gmem_prepare and .gmem_reclaim, but it turns out that it's not quite
easy to just call convert, because I need to pass both gfn and pfn to
.gmem_convert().

The conversion is called on the inode:

   __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
 			     size_t nr_pages, uint64_t attrs,
			     pgoff_t *err_index)

Then guest_memfd would call

   kvm_gmem_convert(inode, start, end, to_private);

It is easy to get the kvm pointer for .gmem_convert(), but then to
always pass something for gfn and pfn, I have to make sure that

+ the folio was allocated (to get a valid pfn)
+ some binding exists for the index (to get a valid gfn)

So for each folio that exists, I would check if there is a valid
binding. If there is a valid binding, I compute the gfn, and if there's
no valid binding I pass -1ull for gfn.

I think this is a little complicated and not actually all that
intuitive...


Looking at KVM_AMD_SEV's Kconfig, it selects 4 gmem lifecycle callbacks:

    config KVM_AMD_SEV
	bool "AMD Secure Encrypted Virtualization (SEV) support"
	default y
	depends on KVM_AMD && X86_64
	depends on CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
	select ARCH_HAS_CC_PLATFORM
	select KVM_VM_MEMORY_ATTRIBUTES
	select HAVE_KVM_ARCH_GMEM_CONVERT
	select HAVE_KVM_ARCH_GMEM_RECLAIM
	select HAVE_KVM_ARCH_GMEM_INVALIDATE
	select HAVE_KVM_ARCH_GMEM_POPULATE
	help
	  Provides support for launching encrypted VMs which use Secure
	  Encrypted Virtualization (SEV), Secure Encrypted Virtualization with
	  Encrypted State (SEV-ES), and Secure Encrypted Virtualization with
	  Secure Nested Paging (SEV-SNP) technologies on AMD processors.

So actually smushing .gmem_prepare and .gmem_reclaim doesn't really save
us CONFIG flags, and results in a kvm_gmem_convert() that needs to find
the intersection of folio and binding existing just so the .gmem_convert
call is valid for conversions in both directions.

If we set kvm_gmem_convert() up to do handle make_private() and
make_shared() separately, then I don't think there's much value in
collapsing .gmem_prepare() and .gmem_reclaim() just so we mux on the
caller side and demux on the SNP side.


How about this:

1. Retain .gmem_reclaim() for "host reclaim" aka "return to host" aka
   "make shared" for x86.
     + Rename the make shared function to sev_gmem_reclaim
         + .gmem_reclaim = sev_gmem_reclaim
     + pKVM will defined kvm_arch_gmem_reclaim() to do what it needs
     + TDX and ARM CCA don't use this callback.
     + From __kvm_gmem_set_attributes(),

         if (!to_private)
             kvm_arch_gmem_reclaim(pfn, nr_pages)

     + From .free_folio()
         kvm_arch_gmem_reclaim(folio_pfn(folio), folio_nr_pages(folio))

2. Rename .gmem_prepare() to .gmem_grant(), or .gmem_assign(), something
   that is the opposite of "reclaim", that indicates the host
   granting/assigning/linking (other names welcome) the gfn <-> pfn
     + Only SNP will define this
     + .gmem_assign = sev_gmem_assign(kvm, gfn, pfn, nr_pages, order);
     + From kvm_gmem_get_pfn()

         if (kvm_gmem_is_private_mem(inode, index))
             kvm_arch_gmem_assign(kvm, gfn, pfn, nr_pages, order);

I think it is fair that "assign" or "grant" is only called for private
pages, since only private pages need to be "given" away.

On the reclaim side, it's also fair that only shared pages need to be
reclaimed.

And with this, we still have 4 CONFIGs just like above, except that it's
now

	select HAVE_KVM_ARCH_GMEM_ASSIGN
	select HAVE_KVM_ARCH_GMEM_RECLAIM
	select HAVE_KVM_ARCH_GMEM_INVALIDATE
	select HAVE_KVM_ARCH_GMEM_POPULATE

And I guess it's more straightforward that one CONFIG selects one
callback, avoiding having either of the CONFIGs select .gmem_convert():

> +#if defined(CONFIG_HAVE_KVM_ARCH_GMEM_PREPARE) || defined(CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM)
> +	int (*gmem_convert)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> +			    kvm_pfn_t nr_pages, int max_order, bool to_private);
>  #endif
>
> [...snip...]
>