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

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 16, 2026, Ackerley Tng wrote:
> Sean Christopherson <seanjc@google.com> 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

As I stated somewhere else, conversion to SHARED does NOT and *should* NOT require
a valid GFN.  Conceptually, a SHARED page can't be strictly associated with a
single GFN.

> + 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.

And I'm saying don't do that for the to_shared case.  If passing a NULL @kvm
and -1ull for @gfn from guest_memfd is too ugly, I'd be a-ok with providing
separate kvm_arch_gmem_make_{private,shared}() under GMEM_CONVERT.

I guess at that point I don't have a strong preference betwee having a single
kvm_x86_ops hook versus also having kvm_x86_ops.gmem_make_{private,shared}().

I.e. Option A:

	#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT
	int kvm_arch_gmem_make_private(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
				kvm_pfn_t nr_pages, int max_order)
	{
		return kvm_x86_call(gmem_convert)(kvm, gfn, pfn, nr_pages, max_order, true);
	}
	int kvm_arch_gmem_make_shared(kvm_pfn_t pfn, kvm_pfn_t nr_pages, int max_order,
				bool to_private)
	{
		return kvm_x86_call(gmem_convert)(NULL, -1ull, pfn, nr_pages, max_order, false);
	}
	#endif

	#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM
	void kvm_arch_gmem_reclaim(kvm_pfn_t pfn, kvm_pfn_t nr_pages, int max_order)
	{
		WARN_ON_ONCE(kvm_x86_call(gmem_convert)(NULL, -1ull, pfn, nr_pages, max_order, false));
	}
	#endif


Or Option B:

	#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT
	int kvm_arch_gmem_make_private(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
				kvm_pfn_t nr_pages, int max_order)
	{
		return kvm_x86_call(gmem_make_private)(kvm, gfn, pfn, nr_pages, max_order);
	}
	int kvm_arch_gmem_make_shared(kvm_pfn_t pfn, kvm_pfn_t nr_pages, int max_order,
				bool to_private)
	{
		kvm_x86_call(gmem_make_shared)(pfn, nr_pages, max_order);
		return 0;
	}
	#endif

	#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM
	void kvm_arch_gmem_reclaim(kvm_pfn_t pfn, kvm_pfn_t nr_pages, int max_order)
	{
		kvm_x86_call(gmem_make_shared)(pfn, nr_pages, max_order);
	}
	#endif

Typing it out, option B does look prettier...

> 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)

NAK, pKVM doesn't want to "reclaim" on conversions to SHARED.  reclaim() and
convert() are two similar but distinct operations.  Irrespective of pKVM, IMO
it's also very useful to differentiate between "this page is being shared with
the host, but KVM still owns the page" and "this page is being freed back to the
host".  E.g. reclaim() *must* succeed or take evasive action, whereas convert()
can gracefully return an error.

>      + 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...]
> >