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

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> On Thu, Jul 16, 2026, Ackerley Tng wrote:
>> 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
>
> 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,

It is ugly, but I thought the point of providing a single .gmem_convert
callback was so the conversion path could just call convert with kvm and
gfn arguments that work for both directions.

I like Option B below better, which should also resolve my issue.

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

Makes sense to distinguish reclaim and make_shared. Thanks!

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