From: Nikita Kalyazin <kalyazin@amazon.com>

write syscall populates guest_memfd with user-supplied data in a generic
way, ie no vendor-specific preparation is performed.  This is supposed
to be used in non-CoCo setups where guest memory is not
hardware-encrypted.

The following behaviour is implemented:
 - only page-aligned count and offset are allowed
 - if the memory is already allocated, the call will successfully
   populate it
 - if the memory is not allocated, the call will both allocate and
   populate
 - if the memory is already populated, the call will not repopulate it

Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
---
 virt/kvm/guest_memfd.c | 64 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 08a6bc7d25b6..a2e86ec13e4b 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -379,7 +379,9 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
 }
 
 static struct file_operations kvm_gmem_fops = {
-	.mmap		= kvm_gmem_mmap,
+	.mmap           = kvm_gmem_mmap,
+	.llseek         = default_llseek,
+	.write_iter     = generic_perform_write,
 	.open		= generic_file_open,
 	.release	= kvm_gmem_release,
 	.fallocate	= kvm_gmem_fallocate,
@@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module)
 	kvm_gmem_fops.owner = module;
 }
 
+static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,
+				     struct address_space *mapping,
+				     loff_t pos, unsigned int len,
+				     struct folio **foliop,
+				     void **fsdata)
+{
+	struct file *file = kiocb->ki_filp;
+	pgoff_t index = pos >> PAGE_SHIFT;
+	struct folio *folio;
+
+	if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
+		return -EINVAL;
+
+	if (pos + len > i_size_read(file_inode(file)))
+		return -EINVAL;
+
+	folio = kvm_gmem_get_folio(file_inode(file), index);
+	if (IS_ERR(folio))
+		return -EFAULT;
+
+	if (WARN_ON_ONCE(folio_test_large(folio))) {
+		folio_unlock(folio);
+		folio_put(folio);
+		return -EFAULT;
+	}
+
+	if (folio_test_uptodate(folio)) {
+		folio_unlock(folio);
+		folio_put(folio);
+		return -ENOSPC;
+	}
+
+	*foliop = folio;
+	return 0;
+}
+
+static int kvm_kmem_gmem_write_end(const struct kiocb *kiocb,
+				   struct address_space *mapping,
+				   loff_t pos, unsigned int len,
+				   unsigned int copied,
+				   struct folio *folio, void *fsdata)
+{
+	if (copied) {
+		if (copied < len) {
+			unsigned int from = pos & (PAGE_SIZE - 1);
+
+			folio_zero_range(folio, from + copied, len - copied);
+		}
+		kvm_gmem_mark_prepared(folio);
+	}
+
+	folio_unlock(folio);
+	folio_put(folio);
+
+	return copied;
+}
+
 static int kvm_gmem_migrate_folio(struct address_space *mapping,
 				  struct folio *dst, struct folio *src,
 				  enum migrate_mode mode)
@@ -442,6 +501,8 @@ static void kvm_gmem_free_folio(struct folio *folio)
 
 static const struct address_space_operations kvm_gmem_aops = {
 	.dirty_folio = noop_dirty_folio,
+	.write_begin = kvm_kmem_gmem_write_begin,
+	.write_end = kvm_kmem_gmem_write_end,
 	.migrate_folio	= kvm_gmem_migrate_folio,
 	.error_remove_folio = kvm_gmem_error_folio,
 #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
@@ -489,6 +550,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
 	}
 
 	file->f_flags |= O_LARGEFILE;
+	file->f_mode |= FMODE_LSEEK | FMODE_PWRITE;
 
 	inode = file->f_inode;
 	WARN_ON(file->f_mapping != inode->i_mapping);
-- 
2.50.1

On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita <kalyazin@amazon.co.uk> wrote:
>
> From: Nikita Kalyazin <kalyazin@amazon.com>

Hi Nikita,

>
> write syscall populates guest_memfd with user-supplied data in a generic
> way, ie no vendor-specific preparation is performed.  This is supposed
> to be used in non-CoCo setups where guest memory is not
> hardware-encrypted.

What's meant to happen if we do use this for CoCo VMs? I would expect
write() to fail, but I don't see why it would (seems like we need/want
a check that we aren't write()ing to private memory).

> The following behaviour is implemented:
>  - only page-aligned count and offset are allowed
>  - if the memory is already allocated, the call will successfully
>    populate it
>  - if the memory is not allocated, the call will both allocate and
>    populate
>  - if the memory is already populated, the call will not repopulate it
>
> Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
> ---
>  virt/kvm/guest_memfd.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 08a6bc7d25b6..a2e86ec13e4b 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -379,7 +379,9 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
>  }
>
>  static struct file_operations kvm_gmem_fops = {
> -       .mmap           = kvm_gmem_mmap,
> +       .mmap           = kvm_gmem_mmap,
> +       .llseek         = default_llseek,
> +       .write_iter     = generic_perform_write,

You seem to have accidentally replaced some tabs with spaces here. :)
Please keep the style consistent.

>         .open           = generic_file_open,
>         .release        = kvm_gmem_release,
>         .fallocate      = kvm_gmem_fallocate,
> @@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module)
>         kvm_gmem_fops.owner = module;
>  }
>
> +static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,
> +                                    struct address_space *mapping,
> +                                    loff_t pos, unsigned int len,
> +                                    struct folio **foliop,
> +                                    void **fsdata)
> +{
> +       struct file *file = kiocb->ki_filp;
> +       pgoff_t index = pos >> PAGE_SHIFT;
> +       struct folio *folio;
> +
> +       if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
> +               return -EINVAL;

Requiring pos to be page-aligned seems like a strange restriction, and
requiring len to be exactly PAGE_SIZE just seems wrong. I don't see
any reason why the below logic can't be made to work with an
unrestricted pos and len (in other words, I don't see how guest_memfd
is special vs other filesystems in this regard).

> +
> +       if (pos + len > i_size_read(file_inode(file)))
> +               return -EINVAL;
> +
> +       folio = kvm_gmem_get_folio(file_inode(file), index);
> +       if (IS_ERR(folio))
> +               return -EFAULT;
> +
> +       if (WARN_ON_ONCE(folio_test_large(folio))) {
> +               folio_unlock(folio);
> +               folio_put(folio);
> +               return -EFAULT;
> +       }
> +
> +       if (folio_test_uptodate(folio)) {
> +               folio_unlock(folio);
> +               folio_put(folio);
> +               return -ENOSPC;

Does it actually matter for the folio not to be uptodate? It seems
unnecessarily restrictive not to be able to overwrite data if we're
saying that this is only usable for unencrypted memory anyway.

Is ENOSPC really the right errno for this? (Maybe -EFAULT?)

> +       }
> +
> +       *foliop = folio;
> +       return 0;
> +}
> +
> +static int kvm_kmem_gmem_write_end(const struct kiocb *kiocb,
> +                                  struct address_space *mapping,
> +                                  loff_t pos, unsigned int len,
> +                                  unsigned int copied,
> +                                  struct folio *folio, void *fsdata)
> +{
> +       if (copied) {
> +               if (copied < len) {
> +                       unsigned int from = pos & (PAGE_SIZE - 1);

How about:

unsigned int from  = pos & ((1UL << folio_order(*folio)) - 1)

So that we don't need to require !folio_test_large() in
kvm_kmem_gmem_write_begin().

> +
> +                       folio_zero_range(folio, from + copied, len - copied);
> +               }
> +               kvm_gmem_mark_prepared(folio);
> +       }
> +
> +       folio_unlock(folio);
> +       folio_put(folio);
> +
> +       return copied;
> +}
> +
>  static int kvm_gmem_migrate_folio(struct address_space *mapping,
>                                   struct folio *dst, struct folio *src,
>                                   enum migrate_mode mode)
> @@ -442,6 +501,8 @@ static void kvm_gmem_free_folio(struct folio *folio)
>
>  static const struct address_space_operations kvm_gmem_aops = {
>         .dirty_folio = noop_dirty_folio,
> +       .write_begin = kvm_kmem_gmem_write_begin,
> +       .write_end = kvm_kmem_gmem_write_end,
>         .migrate_folio  = kvm_gmem_migrate_folio,
>         .error_remove_folio = kvm_gmem_error_folio,
>  #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
> @@ -489,6 +550,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>         }
>
>         file->f_flags |= O_LARGEFILE;
> +       file->f_mode |= FMODE_LSEEK | FMODE_PWRITE;
>
>         inode = file->f_inode;
>         WARN_ON(file->f_mapping != inode->i_mapping);
> --
> 2.50.1
>

On 10/09/2025 22:23, James Houghton wrote:
> On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita <kalyazin@amazon.co.uk> wrote:
>>
>> From: Nikita Kalyazin <kalyazin@amazon.com>
> 
> Hi Nikita,

Hi James,

Thanks for the review!


>>
>> write syscall populates guest_memfd with user-supplied data in a generic
>> way, ie no vendor-specific preparation is performed.  This is supposed
>> to be used in non-CoCo setups where guest memory is not
>> hardware-encrypted.
> 
> What's meant to happen if we do use this for CoCo VMs? I would expect
> write() to fail, but I don't see why it would (seems like we need/want
> a check that we aren't write()ing to private memory).

I am not so sure that write() should fail even in CoCo VMs if we access 
not-yet-prepared pages.  My understanding was that the CoCoisation of 
the memory occurs during "preparation".  But I may be wrong here.


> 
>> The following behaviour is implemented:
>>   - only page-aligned count and offset are allowed
>>   - if the memory is already allocated, the call will successfully
>>     populate it
>>   - if the memory is not allocated, the call will both allocate and
>>     populate
>>   - if the memory is already populated, the call will not repopulate it
>>
>> Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
>> ---
>>   virt/kvm/guest_memfd.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index 08a6bc7d25b6..a2e86ec13e4b 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>> @@ -379,7 +379,9 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
>>   }
>>
>>   static struct file_operations kvm_gmem_fops = {
>> -       .mmap           = kvm_gmem_mmap,
>> +       .mmap           = kvm_gmem_mmap,
>> +       .llseek         = default_llseek,
>> +       .write_iter     = generic_perform_write,
> 
> You seem to have accidentally replaced some tabs with spaces here. :)
> Please keep the style consistent.

Thanks for spotting that, will fix in the next version!


> 
>>          .open           = generic_file_open,
>>          .release        = kvm_gmem_release,
>>          .fallocate      = kvm_gmem_fallocate,
>> @@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module)
>>          kvm_gmem_fops.owner = module;
>>   }
>>
>> +static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,
>> +                                    struct address_space *mapping,
>> +                                    loff_t pos, unsigned int len,
>> +                                    struct folio **foliop,
>> +                                    void **fsdata)
>> +{
>> +       struct file *file = kiocb->ki_filp;
>> +       pgoff_t index = pos >> PAGE_SHIFT;
>> +       struct folio *folio;
>> +
>> +       if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
>> +               return -EINVAL;
> 
> Requiring pos to be page-aligned seems like a strange restriction, and
> requiring len to be exactly PAGE_SIZE just seems wrong. I don't see
> any reason why the below logic can't be made to work with an
> unrestricted pos and len (in other words, I don't see how guest_memfd
> is special vs other filesystems in this regard).

I don't have a real reason to apply those restrictions.  Happy to remove 
them, thanks.


> 
>> +
>> +       if (pos + len > i_size_read(file_inode(file)))
>> +               return -EINVAL;
>> +
>> +       folio = kvm_gmem_get_folio(file_inode(file), index);
>> +       if (IS_ERR(folio))
>> +               return -EFAULT;
>> +
>> +       if (WARN_ON_ONCE(folio_test_large(folio))) {
>> +               folio_unlock(folio);
>> +               folio_put(folio);
>> +               return -EFAULT;
>> +       }
>> +
>> +       if (folio_test_uptodate(folio)) {
>> +               folio_unlock(folio);
>> +               folio_put(folio);
>> +               return -ENOSPC;
> 
> Does it actually matter for the folio not to be uptodate? It seems
> unnecessarily restrictive not to be able to overwrite data if we're
> saying that this is only usable for unencrypted memory anyway.

In the context of direct map removal [1] it does actually because when 
we mark a folio as prepared, we remove it from the direct map making it 
inaccessible to the way write() performs the copy.  It does not matter 
if direct map removal isn't enabled though.  Do you think it should be 
conditional?

[1]: https://lore.kernel.org/kvm/20250828093902.2719-1-roypat@amazon.co.uk

> 
> Is ENOSPC really the right errno for this? (Maybe -EFAULT?)

I don't have a strong opinion here.  My reasoning was if the folio is 
already "sealed" by the direct map removal, then it is no longer a part 
of the "writable space", so -ENOSPC makes sense.  Maybe this intuition 
only works for me so I'm happy to change to -EFAULT if it looks less 
confusing.


> 
>> +       }
>> +
>> +       *foliop = folio;
>> +       return 0;
>> +}
>> +
>> +static int kvm_kmem_gmem_write_end(const struct kiocb *kiocb,
>> +                                  struct address_space *mapping,
>> +                                  loff_t pos, unsigned int len,
>> +                                  unsigned int copied,
>> +                                  struct folio *folio, void *fsdata)
>> +{
>> +       if (copied) {
>> +               if (copied < len) {
>> +                       unsigned int from = pos & (PAGE_SIZE - 1);
> 
> How about:
> 
> unsigned int from  = pos & ((1UL << folio_order(*folio)) - 1)
> 
> So that we don't need to require !folio_test_large() in
> kvm_kmem_gmem_write_begin().

Thanks, will apply to the next version.


> 
>> +
>> +                       folio_zero_range(folio, from + copied, len - copied);
>> +               }
>> +               kvm_gmem_mark_prepared(folio);
>> +       }
>> +
>> +       folio_unlock(folio);
>> +       folio_put(folio);
>> +
>> +       return copied;
>> +}
>> +
>>   static int kvm_gmem_migrate_folio(struct address_space *mapping,
>>                                    struct folio *dst, struct folio *src,
>>                                    enum migrate_mode mode)
>> @@ -442,6 +501,8 @@ static void kvm_gmem_free_folio(struct folio *folio)
>>
>>   static const struct address_space_operations kvm_gmem_aops = {
>>          .dirty_folio = noop_dirty_folio,
>> +       .write_begin = kvm_kmem_gmem_write_begin,
>> +       .write_end = kvm_kmem_gmem_write_end,
>>          .migrate_folio  = kvm_gmem_migrate_folio,
>>          .error_remove_folio = kvm_gmem_error_folio,
>>   #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
>> @@ -489,6 +550,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>>          }
>>
>>          file->f_flags |= O_LARGEFILE;
>> +       file->f_mode |= FMODE_LSEEK | FMODE_PWRITE;
>>
>>          inode = file->f_inode;
>>          WARN_ON(file->f_mapping != inode->i_mapping);
>> --
>> 2.50.1
>>

On Thu, Sep 11, 2025 at 3:15 AM Nikita Kalyazin <kalyazin@amazon.com> wrote:
>
>
>
> On 10/09/2025 22:23, James Houghton wrote:
> > On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita <kalyazin@amazon.co.uk> wrote:
> >>
> >> From: Nikita Kalyazin <kalyazin@amazon.com>
> >
> > Hi Nikita,
>
> Hi James,
>
> Thanks for the review!

:) I hope it's actually helpful.

>
>
> >>
> >> write syscall populates guest_memfd with user-supplied data in a generic
> >> way, ie no vendor-specific preparation is performed.  This is supposed
> >> to be used in non-CoCo setups where guest memory is not
> >> hardware-encrypted.
> >
> > What's meant to happen if we do use this for CoCo VMs? I would expect
> > write() to fail, but I don't see why it would (seems like we need/want
> > a check that we aren't write()ing to private memory).
>
> I am not so sure that write() should fail even in CoCo VMs if we access
> not-yet-prepared pages.  My understanding was that the CoCoisation of
> the memory occurs during "preparation".  But I may be wrong here.

This sounds fine to me, but could you update the changelog with what
the behavior is for CoCo VMs and why we don't allow writing to the
same pages/regions twice? Something like:

"Although write() is only meant to be used for non-CoCo VMs, it is
valid for CoCo VMs as well: the written contents will be encrypted
(when the page is prepared). Because the contents may be encrypted, it
is nonsensical to write() again, so write()ing to prepared pages is
disallowed (even if the no memory encryption occurs). Furthermore, in
the near future, page preparation will also result in pages being
removed from the direct map, so there will be no direct map through
which to perform the second write()."

(This is all provided that it's actually okay to write() content that
will be encrypted... I don't know why that would be improper, but I'm
not exactly an expert here.)

> >> @@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module)
> >>          kvm_gmem_fops.owner = module;
> >>   }
> >>
> >> +static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,
> >> +                                    struct address_space *mapping,
> >> +                                    loff_t pos, unsigned int len,
> >> +                                    struct folio **foliop,
> >> +                                    void **fsdata)
> >> +{
> >> +       struct file *file = kiocb->ki_filp;
> >> +       pgoff_t index = pos >> PAGE_SHIFT;
> >> +       struct folio *folio;
> >> +
> >> +       if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
> >> +               return -EINVAL;
> >
> > Requiring pos to be page-aligned seems like a strange restriction, and
> > requiring len to be exactly PAGE_SIZE just seems wrong. I don't see
> > any reason why the below logic can't be made to work with an
> > unrestricted pos and len (in other words, I don't see how guest_memfd
> > is special vs other filesystems in this regard).
>
> I don't have a real reason to apply those restrictions.  Happy to remove
> them, thanks.

Thanks! Presumably you'll make it so that any unaligned segments will
be left as zeroes; please describe this in the changelog as well. :)

> >> +
> >> +       if (pos + len > i_size_read(file_inode(file)))
> >> +               return -EINVAL;
> >> +
> >> +       folio = kvm_gmem_get_folio(file_inode(file), index);
> >> +       if (IS_ERR(folio))
> >> +               return -EFAULT;
> >> +
> >> +       if (WARN_ON_ONCE(folio_test_large(folio))) {
> >> +               folio_unlock(folio);
> >> +               folio_put(folio);
> >> +               return -EFAULT;
> >> +       }
> >> +
> >> +       if (folio_test_uptodate(folio)) {
> >> +               folio_unlock(folio);
> >> +               folio_put(folio);
> >> +               return -ENOSPC;
> >
> > Does it actually matter for the folio not to be uptodate? It seems
> > unnecessarily restrictive not to be able to overwrite data if we're
> > saying that this is only usable for unencrypted memory anyway.
>
> In the context of direct map removal [1] it does actually because when
> we mark a folio as prepared, we remove it from the direct map making it
> inaccessible to the way write() performs the copy.  It does not matter
> if direct map removal isn't enabled though.  Do you think it should be
> conditional?

Oh, good point. It's simpler (both to implement and to describe) to
disallow a second write() call in all cases (no matter if the direct
map for the page has been removed or if the contents have been
encrypted), so I'm all for leaving it unconditional like you have now.
Thanks!

>
> [1]: https://lore.kernel.org/kvm/20250828093902.2719-1-roypat@amazon.co.uk
>
> >
> > Is ENOSPC really the right errno for this? (Maybe -EFAULT?)
>
> I don't have a strong opinion here.  My reasoning was if the folio is
> already "sealed" by the direct map removal, then it is no longer a part
> of the "writable space", so -ENOSPC makes sense.  Maybe this intuition
> only works for me so I'm happy to change to -EFAULT if it looks less
> confusing.

Oh actually.... how about EEXIST? That feels like the most natural.
But also no strong opinion here.

Thanks for all the clarification, Nikita. :)

On 12/09/2025 23:34, James Houghton wrote:
> On Thu, Sep 11, 2025 at 3:15 AM Nikita Kalyazin <kalyazin@amazon.com> wrote:
>>
>>
>>
>> On 10/09/2025 22:23, James Houghton wrote:
>>> On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita <kalyazin@amazon.co.uk> wrote:
>>>>
>>>> From: Nikita Kalyazin <kalyazin@amazon.com>
>>>
>>> Hi Nikita,
>>
>> Hi James,
>>
>> Thanks for the review!
> 
> :) I hope it's actually helpful.
> 
>>
>>
>>>>
>>>> write syscall populates guest_memfd with user-supplied data in a generic
>>>> way, ie no vendor-specific preparation is performed.  This is supposed
>>>> to be used in non-CoCo setups where guest memory is not
>>>> hardware-encrypted.
>>>
>>> What's meant to happen if we do use this for CoCo VMs? I would expect
>>> write() to fail, but I don't see why it would (seems like we need/want
>>> a check that we aren't write()ing to private memory).
>>
>> I am not so sure that write() should fail even in CoCo VMs if we access
>> not-yet-prepared pages.  My understanding was that the CoCoisation of
>> the memory occurs during "preparation".  But I may be wrong here.
> 
> This sounds fine to me, but could you update the changelog with what
> the behavior is for CoCo VMs and why we don't allow writing to the
> same pages/regions twice? Something like:
> 
> "Although write() is only meant to be used for non-CoCo VMs, it is
> valid for CoCo VMs as well: the written contents will be encrypted
> (when the page is prepared). Because the contents may be encrypted, it
> is nonsensical to write() again, so write()ing to prepared pages is
> disallowed (even if the no memory encryption occurs). Furthermore, in
> the near future, page preparation will also result in pages being
> removed from the direct map, so there will be no direct map through
> which to perform the second write()."
> 
> (This is all provided that it's actually okay to write() content that
> will be encrypted... I don't know why that would be improper, but I'm
> not exactly an expert here.)

 From what Vishal is saying in the other thread, it looks clearer to 
disallow write() for CoCo VM types as it looks like we gain nothing from 
supporting it now.

> 
>>>> @@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module)
>>>>           kvm_gmem_fops.owner = module;
>>>>    }
>>>>
>>>> +static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,
>>>> +                                    struct address_space *mapping,
>>>> +                                    loff_t pos, unsigned int len,
>>>> +                                    struct folio **foliop,
>>>> +                                    void **fsdata)
>>>> +{
>>>> +       struct file *file = kiocb->ki_filp;
>>>> +       pgoff_t index = pos >> PAGE_SHIFT;
>>>> +       struct folio *folio;
>>>> +
>>>> +       if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
>>>> +               return -EINVAL;
>>>
>>> Requiring pos to be page-aligned seems like a strange restriction, and
>>> requiring len to be exactly PAGE_SIZE just seems wrong. I don't see
>>> any reason why the below logic can't be made to work with an
>>> unrestricted pos and len (in other words, I don't see how guest_memfd
>>> is special vs other filesystems in this regard).
>>
>> I don't have a real reason to apply those restrictions.  Happy to remove
>> them, thanks.
> 
> Thanks! Presumably you'll make it so that any unaligned segments will
> be left as zeroes; please describe this in the changelog as well. :)

Will do!

> 
>>>> +
>>>> +       if (pos + len > i_size_read(file_inode(file)))
>>>> +               return -EINVAL;
>>>> +
>>>> +       folio = kvm_gmem_get_folio(file_inode(file), index);
>>>> +       if (IS_ERR(folio))
>>>> +               return -EFAULT;
>>>> +
>>>> +       if (WARN_ON_ONCE(folio_test_large(folio))) {
>>>> +               folio_unlock(folio);
>>>> +               folio_put(folio);
>>>> +               return -EFAULT;
>>>> +       }
>>>> +
>>>> +       if (folio_test_uptodate(folio)) {
>>>> +               folio_unlock(folio);
>>>> +               folio_put(folio);
>>>> +               return -ENOSPC;
>>>
>>> Does it actually matter for the folio not to be uptodate? It seems
>>> unnecessarily restrictive not to be able to overwrite data if we're
>>> saying that this is only usable for unencrypted memory anyway.
>>
>> In the context of direct map removal [1] it does actually because when
>> we mark a folio as prepared, we remove it from the direct map making it
>> inaccessible to the way write() performs the copy.  It does not matter
>> if direct map removal isn't enabled though.  Do you think it should be
>> conditional?
> 
> Oh, good point. It's simpler (both to implement and to describe) to
> disallow a second write() call in all cases (no matter if the direct
> map for the page has been removed or if the contents have been
> encrypted), so I'm all for leaving it unconditional like you have now.
> Thanks!
> 
>>
>> [1]: https://lore.kernel.org/kvm/20250828093902.2719-1-roypat@amazon.co.uk
>>
>>>
>>> Is ENOSPC really the right errno for this? (Maybe -EFAULT?)
>>
>> I don't have a strong opinion here.  My reasoning was if the folio is
>> already "sealed" by the direct map removal, then it is no longer a part
>> of the "writable space", so -ENOSPC makes sense.  Maybe this intuition
>> only works for me so I'm happy to change to -EFAULT if it looks less
>> confusing.
> 
> Oh actually.... how about EEXIST? That feels like the most natural.
> But also no strong opinion here.

Yes, I like EEXIST.  Will use it next time, thanks!

> 
> Thanks for all the clarification, Nikita. :)

On Fri, Sep 12, 2025 at 3:35 PM James Houghton <jthoughton@google.com> wrote:
>
> > >> +
> > >> +       if (folio_test_uptodate(folio)) {
> > >> +               folio_unlock(folio);
> > >> +               folio_put(folio);
> > >> +               return -ENOSPC;
> > >
> > > Does it actually matter for the folio not to be uptodate? It seems
> > > unnecessarily restrictive not to be able to overwrite data if we're
> > > saying that this is only usable for unencrypted memory anyway.
> >
> > In the context of direct map removal [1] it does actually because when
> > we mark a folio as prepared, we remove it from the direct map making it
> > inaccessible to the way write() performs the copy.  It does not matter
> > if direct map removal isn't enabled though.  Do you think it should be
> > conditional?
>
> Oh, good point. It's simpler (both to implement and to describe) to
> disallow a second write() call in all cases (no matter if the direct
> map for the page has been removed or if the contents have been
> encrypted), so I'm all for leaving it unconditional like you have now.
> Thanks!

Are we deviating from the way read/write semantics work for the other
filesystems? I don't think other filesystems carry this restriction of
one-time-write only. Do we strictly need the differing semantics?
Maybe it would be simpler to not overload uptodate flag and just not
allow read/write if folio is not mapped in the direct map for non-conf
VMs (assuming there could be other ways to deduce that information).
Can there be users who want to populate the file ranges multiple times
as it seems more performant?

>
> >
> > [1]: https://lore.kernel.org/kvm/20250828093902.2719-1-roypat@amazon.co.uk
> >
> > >

On 13/09/2025 01:32, Vishal Annapurve wrote:
> On Fri, Sep 12, 2025 at 3:35 PM James Houghton <jthoughton@google.com> wrote:
>>
>>>>> +
>>>>> +       if (folio_test_uptodate(folio)) {
>>>>> +               folio_unlock(folio);
>>>>> +               folio_put(folio);
>>>>> +               return -ENOSPC;
>>>>
>>>> Does it actually matter for the folio not to be uptodate? It seems
>>>> unnecessarily restrictive not to be able to overwrite data if we're
>>>> saying that this is only usable for unencrypted memory anyway.
>>>
>>> In the context of direct map removal [1] it does actually because when
>>> we mark a folio as prepared, we remove it from the direct map making it
>>> inaccessible to the way write() performs the copy.  It does not matter
>>> if direct map removal isn't enabled though.  Do you think it should be
>>> conditional?
>>
>> Oh, good point. It's simpler (both to implement and to describe) to
>> disallow a second write() call in all cases (no matter if the direct
>> map for the page has been removed or if the contents have been
>> encrypted), so I'm all for leaving it unconditional like you have now.
>> Thanks!
> 
> Are we deviating from the way read/write semantics work for the other
> filesystems? I don't think other filesystems carry this restriction of
> one-time-write only. Do we strictly need the differing semantics?

Yes, I believe we are deviating from other "regular" filesystems, but I 
don't think what we propose is too uncommon as in "special" filesystems 
such as sysfs subsequent calls to attributes like "remove" will likely 
fail as well (not due to up-to-date flag though).

> Maybe it would be simpler to not overload uptodate flag and just not
> allow read/write if folio is not mapped in the direct map for non-conf
> VMs (assuming there could be other ways to deduce that information).

The only such interface I'm aware of is kernel_page_present() so the 
check may look like:

	for (i = 0; i < folio_nr_pages(folio); i++) {
		struct page *page = folio_page(folio, i);
		if (!kernel_page_present(page)) {
			folio_unlock(folio);
			folio_put(folio);
			return -ENOSPC;
		}
	}

However, kernel_page_present() is not currently exported to modules.

Alternatively, the same effect can be achieved via checking for both 
kvm_gmem_test_no_direct_map(inode) [1] and folio_test_uptodate(folio). 
It would be the "conditional" check I mentioned earlier in the thread.

[1]: https://lore.kernel.org/kvm/20250912091708.17502-6-roypat@amazon.co.uk/

> Can there be users who want to populate the file ranges multiple times
> as it seems more performant?

Yes, you are right, there may be use cases like that.  At the same time, 
I think they are much less common because it's more typical for the 
initial population to cover larger memory ranges and be sensitive to 
performance.

> 
>>
>>>
>>> [1]: https://lore.kernel.org/kvm/20250828093902.2719-1-roypat@amazon.co.uk
>>>
>>>>

On Mon, Sep 15, 2025 at 4:01 AM Nikita Kalyazin <kalyazin@amazon.com> wrote:
>
> On 13/09/2025 01:32, Vishal Annapurve wrote:
> > On Fri, Sep 12, 2025 at 3:35 PM James Houghton <jthoughton@google.com> wrote:
> >>
> >>>>> +
> >>>>> +       if (folio_test_uptodate(folio)) {
> >>>>> +               folio_unlock(folio);
> >>>>> +               folio_put(folio);
> >>>>> +               return -ENOSPC;
> >>>>
> >>>> Does it actually matter for the folio not to be uptodate? It seems
> >>>> unnecessarily restrictive not to be able to overwrite data if we're
> >>>> saying that this is only usable for unencrypted memory anyway.
> >>>
> >>> In the context of direct map removal [1] it does actually because when
> >>> we mark a folio as prepared, we remove it from the direct map making it
> >>> inaccessible to the way write() performs the copy.  It does not matter
> >>> if direct map removal isn't enabled though.  Do you think it should be
> >>> conditional?
> >>
> >> Oh, good point. It's simpler (both to implement and to describe) to
> >> disallow a second write() call in all cases (no matter if the direct
> >> map for the page has been removed or if the contents have been
> >> encrypted), so I'm all for leaving it unconditional like you have now.
> >> Thanks!
> >
> > Are we deviating from the way read/write semantics work for the other
> > filesystems? I don't think other filesystems carry this restriction of
> > one-time-write only. Do we strictly need the differing semantics?
>
> Yes, I believe we are deviating from other "regular" filesystems, but I
> don't think what we propose is too uncommon as in "special" filesystems
> such as sysfs subsequent calls to attributes like "remove" will likely
> fail as well (not due to up-to-date flag though).
>
> > Maybe it would be simpler to not overload uptodate flag and just not
> > allow read/write if folio is not mapped in the direct map for non-conf
> > VMs (assuming there could be other ways to deduce that information).
>
> The only such interface I'm aware of is kernel_page_present() so the
> check may look like:
>
>         for (i = 0; i < folio_nr_pages(folio); i++) {
>                 struct page *page = folio_page(folio, i);
>                 if (!kernel_page_present(page)) {
>                         folio_unlock(folio);
>                         folio_put(folio);
>                         return -ENOSPC;
>                 }
>         }
>
> However, kernel_page_present() is not currently exported to modules.

I think it should be exposed if there is no cleaner way to deduce if a
folio is mapped in the direct map. That being said, we should probably
cleanly separate the series to add write population support and the
series for removal from direct map [1] and figure out the order in
which they need to be merged upstream.  I would still vote for not
overloading folio_test_uptodate() in either series.

Ackerley and Fuad are planning to post a series just for supporting
in-place conversion for 4K pages which is going to introduce a maple
tree for storing private/shared-ness of ranges. We could possibly
augment the support to track directmap removal there. RFC version [2]
is a good reference for now.

[1] https://lore.kernel.org/kvm/20250912091708.17502-1-roypat@amazon.co.uk/
[2] https://lore.kernel.org/kvm/d3832fd95a03aad562705872cbda5b3d248ca321.1747264138.git.ackerleytng@google.com/#t

On 11.09.25 12:15, Nikita Kalyazin wrote:
> 
> 
> On 10/09/2025 22:23, James Houghton wrote:
>> On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita <kalyazin@amazon.co.uk> wrote:
>>>
>>> From: Nikita Kalyazin <kalyazin@amazon.com>
>>
>> Hi Nikita,
> 
> Hi James,
> 
> Thanks for the review!
> 
> 
>>>
>>> write syscall populates guest_memfd with user-supplied data in a generic
>>> way, ie no vendor-specific preparation is performed.  This is supposed
>>> to be used in non-CoCo setups where guest memory is not
>>> hardware-encrypted.
>>
>> What's meant to happen if we do use this for CoCo VMs? I would expect
>> write() to fail, but I don't see why it would (seems like we need/want
>> a check that we aren't write()ing to private memory).
> 
> I am not so sure that write() should fail even in CoCo VMs if we access
> not-yet-prepared pages.  My understanding was that the CoCoisation of
> the memory occurs during "preparation".  But I may be wrong here.

But how do you handle that a page is actually inaccessible and should 
not be touched?

IOW, with CXL you could crash the host.

There is likely some state check missing, or it should be restricted to 
VM types.

Do we know how this would interact with the direct-map removal?

-- 
Cheers

David / dhildenb

On 12/09/2025 14:36, David Hildenbrand wrote:
> On 11.09.25 12:15, Nikita Kalyazin wrote:
>>
>>
>> On 10/09/2025 22:23, James Houghton wrote:
>>> On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita 
>>> <kalyazin@amazon.co.uk> wrote:
>>>>
>>>> From: Nikita Kalyazin <kalyazin@amazon.com>
>>>
>>> Hi Nikita,
>>
>> Hi James,
>>
>> Thanks for the review!
>>
>>
>>>>
>>>> write syscall populates guest_memfd with user-supplied data in a 
>>>> generic
>>>> way, ie no vendor-specific preparation is performed.  This is supposed
>>>> to be used in non-CoCo setups where guest memory is not
>>>> hardware-encrypted.
>>>
>>> What's meant to happen if we do use this for CoCo VMs? I would expect
>>> write() to fail, but I don't see why it would (seems like we need/want
>>> a check that we aren't write()ing to private memory).
>>
>> I am not so sure that write() should fail even in CoCo VMs if we access
>> not-yet-prepared pages.  My understanding was that the CoCoisation of
>> the memory occurs during "preparation".  But I may be wrong here.
> 
> But how do you handle that a page is actually inaccessible and should
> not be touched?
> 
> IOW, with CXL you could crash the host.
> 
> There is likely some state check missing, or it should be restricted to
> VM types.

Sorry, I'm missing the link between VM types and CXL.  How are they related?

My thinking was it is a regular (accessible) page until it is "prepared" 
by the CoCo hardware, which is currently tracked by the up-to-date flag, 
so it is safe to assume that until it is "prepared", it is accessible 
because it was allocated by filemap_grab_folio() -> 
filemap_alloc_folio() and hasn't been taken over by the CoCo hardware. 
What scenario can you see where it doesn't apply as of now?

I am aware of an attempt to remove preparation tracking from 
guest_memfd, but it is still at an RFC stage AFAIK [1].

> 
> Do we know how this would interact with the direct-map removal?

I'm using folio_test_uptodate() to determine if the page has been 
removed from the direct map as kvm_gmem_mark_prepared() is what 
currently removes the page from the direct map and marks it as 
up-to-date.  [2] is a Firecracker feature branch where the two work in 
combination.

[1]: https://lore.kernel.org/kvm/20250715225523.yzmrwghbhi56tqux@amd.com
[2]: 
https://github.com/firecracker-microvm/firecracker/tree/feature/secret-hiding

> 
> -- 
> Cheers
> 
> David / dhildenb
> 

On 12.09.25 16:48, Nikita Kalyazin wrote:
> 
> 
> On 12/09/2025 14:36, David Hildenbrand wrote:
>> On 11.09.25 12:15, Nikita Kalyazin wrote:
>>>
>>>
>>> On 10/09/2025 22:23, James Houghton wrote:
>>>> On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita
>>>> <kalyazin@amazon.co.uk> wrote:
>>>>>
>>>>> From: Nikita Kalyazin <kalyazin@amazon.com>
>>>>
>>>> Hi Nikita,
>>>
>>> Hi James,
>>>
>>> Thanks for the review!
>>>
>>>
>>>>>
>>>>> write syscall populates guest_memfd with user-supplied data in a
>>>>> generic
>>>>> way, ie no vendor-specific preparation is performed.  This is supposed
>>>>> to be used in non-CoCo setups where guest memory is not
>>>>> hardware-encrypted.
>>>>
>>>> What's meant to happen if we do use this for CoCo VMs? I would expect
>>>> write() to fail, but I don't see why it would (seems like we need/want
>>>> a check that we aren't write()ing to private memory).
>>>
>>> I am not so sure that write() should fail even in CoCo VMs if we access
>>> not-yet-prepared pages.  My understanding was that the CoCoisation of
>>> the memory occurs during "preparation".  But I may be wrong here.
>>
>> But how do you handle that a page is actually inaccessible and should
>> not be touched?
>>
>> IOW, with CXL you could crash the host.
>>
>> There is likely some state check missing, or it should be restricted to
>> VM types.
> 
> Sorry, I'm missing the link between VM types and CXL.  How are they related?

I think what you explain below clarifies it.

> 
> My thinking was it is a regular (accessible) page until it is "prepared"
> by the CoCo hardware, which is currently tracked by the up-to-date flag,
> so it is safe to assume that until it is "prepared", it is accessible
> because it was allocated by filemap_grab_folio() ->
> filemap_alloc_folio() and hasn't been taken over by the CoCo hardware.
> What scenario can you see where it doesn't apply as of now?

Thanks for clarifying, see below.

> 
> I am aware of an attempt to remove preparation tracking from
> guest_memfd, but it is still at an RFC stage AFAIK [1].
> 
>>
>> Do we know how this would interact with the direct-map removal?
> 
> I'm using folio_test_uptodate() to determine if the page has been
> removed from the direct map as kvm_gmem_mark_prepared() is what
> currently removes the page from the direct map and marks it as
> up-to-date.  [2] is a Firecracker feature branch where the two work in
> combination.

Ah, okay. Yes, I recalled [1] that we wanted to change these semantics 
to be "uptodate: was zeroed", and that preparation handling would be 
essentially handled by the arch backend.

-- 
Cheers

David / dhildenb

On Fri, Sep 12, 2025 at 8:39 AM David Hildenbrand <david@redhat.com> wrote:
>
> >>>> What's meant to happen if we do use this for CoCo VMs? I would expect
> >>>> write() to fail, but I don't see why it would (seems like we need/want
> >>>> a check that we aren't write()ing to private memory).
> >>>
> >>> I am not so sure that write() should fail even in CoCo VMs if we access
> >>> not-yet-prepared pages.  My understanding was that the CoCoisation of
> >>> the memory occurs during "preparation".  But I may be wrong here.
> >>
> >> But how do you handle that a page is actually inaccessible and should
> >> not be touched?
> >>
> >> IOW, with CXL you could crash the host.
> >>
> >> There is likely some state check missing, or it should be restricted to
> >> VM types.
> >
> > Sorry, I'm missing the link between VM types and CXL.  How are they related?
>
> I think what you explain below clarifies it.
>
> >
> > My thinking was it is a regular (accessible) page until it is "prepared"
> > by the CoCo hardware, which is currently tracked by the up-to-date flag,
> > so it is safe to assume that until it is "prepared", it is accessible
> > because it was allocated by filemap_grab_folio() ->
> > filemap_alloc_folio() and hasn't been taken over by the CoCo hardware.
> > What scenario can you see where it doesn't apply as of now?
>
> Thanks for clarifying, see below.
>
> >
> > I am aware of an attempt to remove preparation tracking from
> > guest_memfd, but it is still at an RFC stage AFAIK [1].
> >
> >>
> >> Do we know how this would interact with the direct-map removal?
> >
> > I'm using folio_test_uptodate() to determine if the page has been
> > removed from the direct map as kvm_gmem_mark_prepared() is what
> > currently removes the page from the direct map and marks it as
> > up-to-date.  [2] is a Firecracker feature branch where the two work in
> > combination.
>
> Ah, okay. Yes, I recalled [1] that we wanted to change these semantics
> to be "uptodate: was zeroed", and that preparation handling would be
> essentially handled by the arch backend.

Yes, I think we should not be overloading uptodate flag to be an
indicator of what is private for CoCo guests. Uptodate flag should
just mean zeroed/fresh folio. It's possible that future allocator
backing for huge pages already provides uptodate folios.

If there is no current use case for read/write for CoCo VMs, I think
it makes sense to disable it for now by checking the VM type before
adding further overloading of uptodate flags.

>
> --
> Cheers
>
> David / dhildenb
>
>

On 13/09/2025 01:18, Vishal Annapurve wrote:
> On Fri, Sep 12, 2025 at 8:39 AM David Hildenbrand <david@redhat.com> wrote:
>>
>>>>>> What's meant to happen if we do use this for CoCo VMs? I would expect
>>>>>> write() to fail, but I don't see why it would (seems like we need/want
>>>>>> a check that we aren't write()ing to private memory).
>>>>>
>>>>> I am not so sure that write() should fail even in CoCo VMs if we access
>>>>> not-yet-prepared pages.  My understanding was that the CoCoisation of
>>>>> the memory occurs during "preparation".  But I may be wrong here.
>>>>
>>>> But how do you handle that a page is actually inaccessible and should
>>>> not be touched?
>>>>
>>>> IOW, with CXL you could crash the host.
>>>>
>>>> There is likely some state check missing, or it should be restricted to
>>>> VM types.
>>>
>>> Sorry, I'm missing the link between VM types and CXL.  How are they related?
>>
>> I think what you explain below clarifies it.
>>
>>>
>>> My thinking was it is a regular (accessible) page until it is "prepared"
>>> by the CoCo hardware, which is currently tracked by the up-to-date flag,
>>> so it is safe to assume that until it is "prepared", it is accessible
>>> because it was allocated by filemap_grab_folio() ->
>>> filemap_alloc_folio() and hasn't been taken over by the CoCo hardware.
>>> What scenario can you see where it doesn't apply as of now?
>>
>> Thanks for clarifying, see below.
>>
>>>
>>> I am aware of an attempt to remove preparation tracking from
>>> guest_memfd, but it is still at an RFC stage AFAIK [1].
>>>
>>>>
>>>> Do we know how this would interact with the direct-map removal?
>>>
>>> I'm using folio_test_uptodate() to determine if the page has been
>>> removed from the direct map as kvm_gmem_mark_prepared() is what
>>> currently removes the page from the direct map and marks it as
>>> up-to-date.  [2] is a Firecracker feature branch where the two work in
>>> combination.
>>
>> Ah, okay. Yes, I recalled [1] that we wanted to change these semantics
>> to be "uptodate: was zeroed", and that preparation handling would be
>> essentially handled by the arch backend.
> 
> Yes, I think we should not be overloading uptodate flag to be an
> indicator of what is private for CoCo guests. Uptodate flag should
> just mean zeroed/fresh folio. It's possible that future allocator
> backing for huge pages already provides uptodate folios.

Good point, thanks for sharing.

> 
> If there is no current use case for read/write for CoCo VMs, I think
> it makes sense to disable it for now by checking the VM type before
> adding further overloading of uptodate flags.

Sounds fair.  I can add a check for the VM type and only allow it for 
KVM_X86_SW_PROTECTED_VM on x86.  When ARM CCA support [1] is added we 
should also check for KVM_VM_TYPE_ARM_NORMAL on ARM.

[1]: 
https://lore.kernel.org/kvm/20250820145606.180644-1-steven.price@arm.com

> 
>>
>> --
>> Cheers
>>
>> David / dhildenb
>>
>>