Re: [RFC PATCH] KVM: TDX: Decouple TDX init mem region from kvm_gmem_populate()

[Michael Roth <michael.roth@amd.com>]

On Fri, Jul 11, 2025 at 11:38:10AM -0700, Vishal Annapurve wrote:
> On Fri, Jul 11, 2025 at 9:37 AM Michael Roth <michael.roth@amd.com> wrote:
> >
> > >
> > > static long __kvm_gmem_populate(struct kvm *kvm, struct kvm_memory_slot *slot,
> > >                               struct file *file, gfn_t gfn, void __user *src,
> > >                               kvm_gmem_populate_cb post_populate, void *opaque)
> > > {
> > >       pgoff_t index = kvm_gmem_get_index(slot, gfn);
> > >       struct page *src_page = NULL;
> > >       bool is_prepared = false;
> > >       struct folio *folio;
> > >       int ret, max_order;
> > >       kvm_pfn_t pfn;
> > >
> > >       if (src) {
> > >               ret = get_user_pages((unsigned long)src, 1, 0, &src_page);
> > >               if (ret < 0)
> > >                       return ret;
> > >               if (ret != 1)
> > >                       return -ENOMEM;
> > >       }
> >
> > One tricky part here is that the uAPI currently expects the pages to
> > have the private attribute set prior to calling kvm_gmem_populate(),
> > which gets enforced below.
> >
> > For in-place conversion: the idea is that userspace will convert
> > private->shared to update in-place, then immediately convert back
> > shared->private; so that approach would remain compatible with above
> > behavior. But if we pass a 'src' parameter to kvm_gmem_populate(),
> > and do a GUP or copy_from_user() on it at any point, regardless if
> > it is is outside of filemap_invalidate_lock(), then
> > kvm_gmem_fault_shared() will return -EACCES.
> 
> I think that's a fine way to fail the initial memory population, this
> simply means userspace didn't pass the right source address. Why do we
> have to work around this error? Userspace should simply pass the
> source buffer that is accessible to the host or pass null to indicate
> that the target gfn already has the needed contents.
> 
> That is, userspace can still bring a separate source buffer even with
> in-place conversion available.

I thought there was some agreement that mmap() be the 'blessed'
approach for initializing memory with in-place conversion to help
untangle some of these paths, so it made sense to enforce that in
kvm_gmem_populate() to make it 'official', but with Sean's suggested
rework I suppose we could support both approaches.

-Mike

> 
> > The only 2 ways I see
> > around that are to either a) stop enforcing that pages that get
> > processed by kvm_gmem_populate() are private for in-place conversion
> > case, or b) enforce that 'src' is NULL for in-place conversion case.
> >