Message-ID: <aQPakDuteQkg0hTu@google.com>
Date: Thu, 30 Oct 2025 14:37:20 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Nikita Kalyazin <kalyazin@...zon.com>
Cc: Nikita Kalyazin <kalyazin@...zon.co.uk>, "pbonzini@...hat.com" <pbonzini@...hat.com>, 
	"shuah@...nel.org" <shuah@...nel.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "david@...hat.com" <david@...hat.com>, 
	"jthoughton@...gle.com" <jthoughton@...gle.com>, "patrick.roy@...ux.dev" <patrick.roy@...ux.dev>, 
	Jack Thomson <jackabt@...zon.co.uk>, Derek Manwaring <derekmn@...zon.com>, 
	Marco Cali <xmarcalx@...zon.co.uk>, ackerleytng@...gle.com, 
	Vishal Annapurve <vannapurve@...gle.com>
Subject: Re: [PATCH v6 1/2] KVM: guest_memfd: add generic population via write

On Fri, Oct 24, 2025, Nikita Kalyazin wrote:
> 
> 
> On 23/10/2025 17:07, Sean Christopherson wrote:
> > On Mon, Oct 20, 2025, Nikita Kalyazin wrote:
> > > From: Nikita Kalyazin <kalyazin@...zon.com>
> 
> + Vishal and Ackerley
> 
> > > 
> > > write syscall populates guest_memfd with user-supplied data in a generic
> > > way, ie no vendor-specific preparation is performed.  If the request is
> > > not page-aligned, the remaining bytes are initialised to 0.
> > > 
> > > write is only supported for non-CoCo setups where guest memory is not
> > > hardware-encrypted.
> > 
> > Please include all of the "why".  The code mostly communicates the "what", but
> > it doesn't capture why write() support is at all interesting, nor does it explain
> > why read() isn't supported.
> 
> Hi Sean,
> 
> Thanks for the review.
> 
> Do you think including the explanation from the cover letter would be
> sufficient?

It's pretty close.  A few more details would be helpful, e.g. to explain that VMMs
may use write() to populate the initial image

> Shall I additionally say that read() isn't supported because there is no use
> case for it as of now or would it be obvious?

Hmm, I think if you want to exclude read() support, the changelog should explicitly
state why.  E.g. "there's no use case" is quite different from "deliberately
don't support read() for security reasons".

> > > Signed-off-by: Nikitia Kalyazin <kalyazin@...zon.com>
> > > ---
> > >   virt/kvm/guest_memfd.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> > 
> > There's a notable lack of uAPI and Documentation chanegs.  I.e. this needs a
> > GUEST_MEMFD_FLAG_xxx along with proper documentation.
> 
> Would the following be ok in the doc?
> 
> When the capability KVM_CAP_GUEST_MEMFD_WRITE is supported, the 'flags'

No capability is necessary, see d2042d8f96dd ("KVM: Rework KVM_CAP_GUEST_MEMFD_MMAP
into KVM_CAP_GUEST_MEMFD_FLAGS").

> field
> supports GUEST_MEMFD_FLAG_WRITE. Setting this flag on guest_memfd creation
> enables write() syscall operations to populate guest_memfd memory from host
> userspace.
> 
> When a write() operation is performed on a guest_memfd file descriptor with
> the
> GUEST_MEMFD_FLAG_WRITE set, the syscall will populate the guest memory with
> user-supplied data in a generic way, without any vendor-specific
> preparation.
> The write operation is only supported for non-CoCo (Confidential Computing)
> setups where guest memory is not hardware-encrypted. 

The restriction should be that guest memory must be SHARED, i.e. not PRIVATE.
Strictly speaking, guest memory can be encrypted, e.g. with SME and TME (I think
TME is still a thing?), but with a shared key and thus accessible from the host.

Even if that weren't the case, we want to support this for CoCo VMs.

> If the write request is not page-aligned, any remaining bytes within the page
> are initialized to zero.

Why?  (Honest question, e.g. is that standard file semantics?)

> > And while it's definitely it's a-ok to land .write() in advance of the direct map
> > changes, we do need to at least map out how we want the two to interact, e.g. so
> > that we don't end up with constraints that are impossible to satisfy.
> > 
> 
> write() shall not attempt to access a page that is not in the direct map,
> which I believe can be achieved via kvm_kmem_gmem_write_begin() consulting
> the KVM_GMEM_FOLIO_NO_DIRECT_MAP in folio->private (introduced in [1]).
> 
> Do you think we should mention it in the commit message in some way? What
> particular constraint are you cautious about?

I want to be cautious with respect to the ABI/uAPI.  Patrick's series also adds
a flag, and guest_memfd doesn't currently provide a way to toggle flags after the
file is created.  That begs the question of how GUEST_MEMFD_FLAG_NO_DIRECT_MAP
will co-exist with GUEST_MEMFD_FLAG_WRITE.  Presumably the goal is to use write()
to initialize memory, and _then_ nuke the direct map.

I want line of sight to understanding the exact semantics/flows.  E.g. will KVM
require userspace to clear GUEST_MEMFD_FLAG_WRITE before allowing
NO_DIRECT_MAP?  Or will the write() simply fail?  How will the sequencing be
achieved?

> > > +     struct inode *inode = file_inode(file);
> > > +     pgoff_t index = pos >> PAGE_SHIFT;
> > > +     struct folio *folio;
> > > +
> > > +     if (!kvm_gmem_supports_mmap(inode))
> > 
> > Checking for MMAP is neither sufficient nor strictly necessary.  MMAP doesn't
> > imply SHARED, and it's not clear to me that mmap() support should be in any way
> > tied to WRITE support.
> 
> As in my reply to the comment about doc, I plan to introduce
> KVM_CAP_GUEST_MEMFD_WRITE and GUEST_MEMFD_FLAG_WRITE.  The
> kvm_arch_supports_gmem_write() will be a weak symbol and relying on
> !kvm_arch_has_private_mem() on x86, similar to
> kvm_arch_supports_gmem_mmap().  Does it look right?

No.  As above, write() should be allowed iff memory is SHARED.  Relevant commits
that are now in Linus' tree:

  44c6cb9fe9888b371e31165b2854bd0f4e2787d4 KVM: guest_memfd: Allow mmap() on guest_memfd for x86 VMs with private memory
  9aef71c892a55e004419923ba7129abe3e58d9f1 KVM: Explicitly mark KVM_GUEST_MEMFD as depending on KVM_GENERIC_MMU_NOTIFIER
  5d3341d684be80892d8f6f9812f90f9274b81177 KVM: guest_memfd: Invalidate SHARED GPAs if gmem supports INIT_SHARED

Powered by blists - more mailing lists