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

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> On Fri, Jul 10, 2026, Ackerley Tng wrote:
>> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
>>
>> > On Thu, Jul 09, 2026, Ackerley Tng wrote:
>> >> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
>> >>
>> >> > Pass a GPA instead of a GFN to kvm_arch_gmem_prepare() so that the GFN/GPA
>> >> > can be made optional in the future, i.e. so that guest_memfd can pass
>> >> > INVALID_GPA.  This will allow reworking the hook into a general .convert()
>> >>
>> >> How about defining a INVALID_GFN instead of using gpa just so
>> >> INVALID_GPA can be passed? The SNP side takes a gfn, so it seems like
>> >> the 2 PAGE_SHIFTs are extra work.
>> >
>> > They are, but I am very hesitant to define INVALID_GFN as I'm worried about the
>> > potential for subtle bugs if both INVALID_GPA and INVALID_GFN exist.  E.g. I
>> > almost made a goof in this exact series where I was going to do:
>> >
>> > 	gfn_t gfn = gpa_to_gfn(svm->sev_es.snp_guest_vmsa_gpa);
>> >
>> > 	...
>> >
>> > 	if (gfn == INVALID_GFN)
>>
>> When you did this check INVALID_GFN was defined as (~0) too I suppose...
>
> Ya.  I didn't actually get that far, because INVALID_GFN didn't exist, but that
> was my plan.
>
>> > 		return;
>> >
>> > Where snp_guest_vmsa_gpa is set to INVALID_GPA.
>> >
>> > I suppose we could define INVALID_GFN to gpa_to_gfn(INVALID_GPA), but that feels
>> > weird, and I feel more comfortable having magic invalid values in only one domain.
>>
>> 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 :)

We could also go all the way where gfn_t is always retrieved with a
getter, and the getter always checks for the sentinel value, and then
maybe the compiler will optimize out the check sometimes, but this is
probably just C's way, where when invalid values are not possible
they're not checked (pre-optimized out).

No strong opinions, I do prefer passing gfn over gpa though.

>> Isn't the relationship between gpa and gfn always supposed to be always a
>> fixed shift apart?
>
> For valid GPAs, yes.  But if a value is completely bogus, is there really any
> relationship?
>
>> When a GPA is passed to the arch function, it almost feels like now the
>> arch should be validating PAGE_ALIGNED(gpa).
>
> Not sure I follow this.
>

If sev_gmem_convert() was handed a gpa, it seems to imply that the
caller could have passed in an unaligned gpa, which means
sev_gmem_convert() should do

  if (WARN_ON_ONCE(!PAGE_ALIGNED(gpa)))
  	return -EIO;

which is even more extra work in addition to the conversion from gfn to
gpa in the caller and back within sev_gmem_convert().

>> Mentioned this off-list, another thing that seems incongruent is that
>> one parameter is in the address domain (gpa) and the other is in the
>> page frame domain (pfn).
>
> Yeah, I definitely don't love this either.  Hrm.
>
> Aha!  Idea!  What if we just kinda sorta punt on the INVALID_GFN concept for the
> moment?  This series doesn't actually _need_ to detect an INVALID_GFN, the only
> "usage" is in a sanity check:
>
>        if (to_private) {
>                if (WARN_ON_ONCE(!kvm || gpa == INVALID_GPA))
>                        return -EIO;
>
>                return sev_gmem_make_private(kvm, gpa, pfn, nr_pages, order);
>        }
>
> and that can very easily be this, which is even better from a safety perspective,
> and is actually somewhat sensible given that it also ensures KVM isn't trying to
> assign a gfn with the C-bit set.  (Untested, i.e. I may botched the math, but the
> idea is sound).
>
> 	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?

+ Is the purpose of checking for the bit being set to set up a contract?
  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?

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)