Re: [PATCH v4 13/18] KVM: guest_memfd: Pass GPA, not GFN, to prepare() hook

[Sean Christopherson]

On Mon, Jul 13, 2026, Ackerley Tng wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> >> Maybe INVALID_GPA and INVALID_GFN should be defined such that both
> >> gpa_to_gfn and gfn_to_gpa are correct for both.
> >
> > Oh, _that_ was actually what I almost broke.  I was going to track the gfn instead
> > of the gpa, and then do:
> >
> > 	svm->sev_es.snp_guest_vmsa_gfn = INVALID_GFN;
> >
> > 	...
> >
> > 	gpa = gfn_to_gpa(svm->sev_es.snp_guest_vmsa_gfn);
> >
> > 	if (gpa == INVALID_GPA)
> > 		return;
> >
> > That would have failed miserably, because even if if INVALID_GFN is -1ull, the
> > lower bits would be zero after the shift, i.e. wouldn't match INVALID_GPA.
> >
> > I _think_ make that work, by doing something like:
> >
> >   #define INVALID_GFN	((~(gfn_t)0) >> PAGE_SHIFT)
> >   #define INVALID_GPA	((gpa_t)INVALID_GFN << PAGE_SHIFT)
> >
> > Which I actually don't mind conceptually, but I'm still not sure it's a good
> > idea in practice.  For whatever reason, converting between INVALID_GFN and
> > INVALID_GPA just feels wrong.
> 
> The above isn't a conversion, it's picking/defining sentinel values that
> are dependent on each other :)

My point is that the entire reason the sential values need to be dependent on
each other is that if we add INVALID_GFN, then KVM will inevitably convert
INVALID_GFN to a gpa and check the result against INVALID_GPA.

> > 	if (to_private) {
> > 		if (WARN_ON_ONCE(!kvm || gfn >> (kvm_host.maxphyaddr - PAGE_SIZE)))
> > 			return -EIO;
> >
> > 		return sev_gmem_make_private(kvm, gfn, pfn, nr_pages, order);
> > 	}
> >
> > Then in kvm_gmem_free_folio(), open code -1ull for the gfn:
> >
> > 	WARN_ON_ONCE(kvm_arch_gmem_convert(NULL, -1ull,
> > 					   folio_file_pfn(folio, 0),
> > 					   folio_nr_pages(folio),
> > 					   folio_order(folio), false));
> >
> > Which is all a little gross?  But I think it's less gross than passing a gpa, and
> > we don't risk delaying this and the in-place conversion series trying to find the
> > perfect answer.
> 
> Yes thanks, we should go ahead with the GFN version, we can definitely
> update this later, if at all.
> 
> I have some more questions, though not blocking:
> 
> + I didn't notice that NULL was passed here. Is this kind of leaking SNP
>   details into guest_memfd, that SNP doesn't need to know which kvm it
>   is to convert a page to shared? Or is it that in principle converting
>   to shared means "removing from ANY VM" and hence kvm = NULL?

Neither?  It's saying "there's no VM at this point", because there's not.  And
I wouldn't consider that an SNP detail, because any CoCo implementation has to
work that way in practice.  Requiring an active VM to reclaim memory for host
usage would be unusable.

> + Is the purpose of checking for the bit being set to set up a contract?

No, it's to harden against against screwing things up in the futures.  And the
check isn't against the C-bit, it's against the max legal gfn KVM supports, which
just so happens to have implications with respect to the C-bit.

>   It's better than passing gpa, but TDX has the opposite definition for
>   the bit, with 1 == shared. TDX doesn't use this callback so that's
>   fine, but it's hard even for SNP to check since IIUC with VTOM 1 ==
>   shared? What if we just not check and depend only on kvm == NULL?

Because the to_private path requires a valid gfn

> How about this: as a lifecycle callback, guest_memfd calling this
> function should be saying "guest_memfd is doing a conversion of nr_pages
> beginning at this pfn to shared/private, and by the way here's the kvm
> and gfn since I have the information." (all arguments presented if
> available) and SNP says "I'll take action, for to_shared I don't need
> kvm or gfn" (just ignore kvm and gfn for to_shared conversions)

No?  That's more or less what I proposed, except I guess for "by the way, here's
the kvm and gfn" for to_shared.  Because I do NOT think we should walk all bindings
to find the kvm+gfn pairs, and because I think we should firmly establish that
to_private requires a kvm+gfn pair, and to_shared does not.  As above, in pracitce,
that needs to hold true for any CoCo VM.