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

[Sean Christopherson]

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.

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

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