[ based on kvm/next ]
Implement guest_memfd population via the write syscall. This is useful in non-CoCo use cases where the host can access guest memory. Even though the same can also be achieved via userspace mapping and memcpying from userspace, write provides a more performant option because it does not need to set page tables and it does not cause a page fault for every page like memcpy would. Note that memcpy cannot be accelerated via MADV_POPULATE_WRITE as it is not supported by guest_memfd and relies on GUP.
Populating 512MiB of guest_memfd on a x86 machine: - via memcpy: 436 ms - via write: 202 ms (-54%)
The write syscall support is conditional on kvm_gmem_supports_mmap. When in-place shared/private conversion is supported, write should only be allowed on shared pages.
v6: - Make write support conditional on mmap support instead of relying on the up-to-date flag to decide whether writing to a page is allowed - James: Remove depenendencies on folio_test_large - James: Remove page alignment restriction - James: Formatting fixes
v5: - https://lore.kernel.org/kvm/20250902111951.58315-1-kalyazin@amazon.com/ - Replace the call to the unexported filemap_remove_folio with zeroing the bytes that could not be copied - Fix checkpatch findings
v4: - https://lore.kernel.org/kvm/20250828153049.3922-1-kalyazin@amazon.com - Switch from implementing the write callback to write_iter - Remove conditional compilation
v3: - https://lore.kernel.org/kvm/20250303130838.28812-1-kalyazin@amazon.com - David/Mike D: Only compile support for the write syscall if CONFIG_KVM_GMEM_SHARED_MEM (now gone) is enabled. v2: - https://lore.kernel.org/kvm/20241129123929.64790-1-kalyazin@amazon.com - Switch from an ioctl to the write syscall to implement population
v1: - https://lore.kernel.org/kvm/20241024095429.54052-1-kalyazin@amazon.com
Nikita Kalyazin (2): KVM: guest_memfd: add generic population via write KVM: selftests: update guest_memfd write tests
.../testing/selftests/kvm/guest_memfd_test.c | 51 ++++++++++++++++--- virt/kvm/guest_memfd.c | 49 ++++++++++++++++++ 2 files changed, 94 insertions(+), 6 deletions(-)
base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac
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. 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.
Signed-off-by: Nikita Kalyazin kalyazin@amazon.com --- virt/kvm/guest_memfd.c | 48 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c index 94bafd6c558c..f4e218049afa 100644 --- a/virt/kvm/guest_memfd.c +++ b/virt/kvm/guest_memfd.c @@ -380,6 +380,8 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
static struct file_operations kvm_gmem_fops = { .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,49 @@ 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; + struct inode *inode = file_inode(file); + pgoff_t index = pos >> PAGE_SHIFT; + struct folio *folio; + + if (!kvm_gmem_supports_mmap(inode)) + return -ENODEV; + + if (pos + len > i_size_read(inode)) + return -EINVAL; + + folio = kvm_gmem_get_folio(inode, index); + if (IS_ERR(folio)) + return -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 && copied < len) { + unsigned int from = pos & ((1UL << folio_order(folio)) - 1); + + folio_zero_range(folio, from + copied, len - copied); + } + + 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 +487,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 +536,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);
On Mon, Oct 20, 2025, Nikita Kalyazin wrote:
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. 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.
Signed-off-by: Nikita Kalyazin kalyazin@amazon.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.
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.
1 file changed, 48 insertions(+)
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c index 94bafd6c558c..f4e218049afa 100644 --- a/virt/kvm/guest_memfd.c +++ b/virt/kvm/guest_memfd.c @@ -380,6 +380,8 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma) static struct file_operations kvm_gmem_fops = { .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,49 @@ 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)
Over-aggressive wrapping, this can be
static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb, struct address_space *mapping, loff_t pos, unsigned int len, struct folio **folio, void **fsdata)
or
static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb, struct address_space *mapping, loff_t pos, unsigned int len, struct folio **folio, void **fsdata)
if we want to bundle pos+len.
+{
- struct file *file = kiocb->ki_filp;
ki_filp is already a file, and even if it were a "void *", there's no need for a local variable.
- 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.
return -ENODEV;- if (pos + len > i_size_read(inode))
return -EINVAL;- folio = kvm_gmem_get_folio(inode, index);
Eh, since "index" is only used once, my vote is to use "pos" and do the shift here, so that it's obvious that the input to kvm_gmem_get_folio() is being checked.
- if (IS_ERR(folio))
return -EFAULT;
Why EFAULT?
- *foliop = folio;
There shouldn't be any need for a local "folio". What about having the "out" param be just "folio"?
E.g.
static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb, struct address_space *mapping, loff_t pos, unsigned int len, struct folio **folio, void **fsdata) { struct inode *inode = file_inode(kiocb->ki_filp);
if (!kvm_gmem_supports_write(inode)) return -ENODEV;
if (pos + len > i_size_read(inode)) return -EINVAL;
*folio = kvm_gmem_get_folio(inode, pos >> PAGE_SHIFT); if (IS_ERR(*folio)) return PTR_ERR(*folio);
return 0; }
- 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 && copied < len) {
Why check if "copied" is non-zero? I don't see why KVM should behave differently with respect to unwritten bytes if copy_folio_from_iter_atomic() fails on the first byte or the Nth byte.
unsigned int from = pos & ((1UL << folio_order(folio)) - 1);
Uh, isn't this just offset_in_folio()?
folio_zero_range(folio, from + copied, len - copied);
I'd probably be in favor of omitting "from" entirely, e.g.
if (copied < len) folio_zero_range(folio, offset_in_folio(pos) + copied, len - copied);
- }
- folio_unlock(folio);
- folio_put(folio);
- return copied;
+}
On 23/10/2025 17:07, Sean Christopherson wrote:
On Mon, Oct 20, 2025, Nikita Kalyazin wrote:
From: Nikita Kalyazin kalyazin@amazon.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? 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?
Signed-off-by: Nikita Kalyazin kalyazin@amazon.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' 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. If the write request is not page-aligned, any remaining bytes within the page are initialized to zero.
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?
[1] https://lore.kernel.org/kvm/20250924152214.7292-2-roypat@amazon.co.uk/
1 file changed, 48 insertions(+)
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c index 94bafd6c558c..f4e218049afa 100644 --- a/virt/kvm/guest_memfd.c +++ b/virt/kvm/guest_memfd.c @@ -380,6 +380,8 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
static struct file_operations kvm_gmem_fops = { .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,49 @@ 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)Over-aggressive wrapping, this can be
static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb, struct address_space *mapping, loff_t pos, unsigned int len, struct folio **folio, void **fsdata)
or
static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb, struct address_space *mapping, loff_t pos, unsigned int len, struct folio **folio, void **fsdata)
if we want to bundle pos+len.
Ack.
+{
struct file *file = kiocb->ki_filp;ki_filp is already a file, and even if it were a "void *", there's no need for a local variable.
Ack.
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?
return -ENODEV;if (pos + len > i_size_read(inode))return -EINVAL;folio = kvm_gmem_get_folio(inode, index);Eh, since "index" is only used once, my vote is to use "pos" and do the shift here, so that it's obvious that the input to kvm_gmem_get_folio() is being checked.
Ack.
if (IS_ERR(folio))return -EFAULT;Why EFAULT?
Will propagate the error like you suggest below.
*foliop = folio;There shouldn't be any need for a local "folio". What about having the "out" param be just "folio"?
E.g.
static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb, struct address_space *mapping, loff_t pos, unsigned int len, struct folio **folio, void **fsdata) { struct inode *inode = file_inode(kiocb->ki_filp);
if (!kvm_gmem_supports_write(inode)) return -ENODEV; if (pos + len > i_size_read(inode)) return -EINVAL; *folio = kvm_gmem_get_folio(inode, pos >> PAGE_SHIFT); if (IS_ERR(*folio)) return PTR_ERR(*folio); return 0;}
Ack.
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 && copied < len) {Why check if "copied" is non-zero? I don't see why KVM should behave differently with respect to unwritten bytes if copy_folio_from_iter_atomic() fails on the first byte or the Nth byte.
No, I don't think there is a need for this check indeed. It looks like a leftover from my previous changes.
unsigned int from = pos & ((1UL << folio_order(folio)) - 1);Uh, isn't this just offset_in_folio()?
folio_zero_range(folio, from + copied, len - copied);I'd probably be in favor of omitting "from" entirely, e.g.
if (copied < len) folio_zero_range(folio, offset_in_folio(pos) + copied, len - copied);
Ack.
}folio_unlock(folio);folio_put(folio);return copied;+}
From: Nikita Kalyazin kalyazin@amazon.com
This is to reflect that the write syscall is now implemented for guest_memfd.
Signed-off-by: Nikita Kalyazin kalyazin@amazon.com --- .../testing/selftests/kvm/guest_memfd_test.c | 51 ++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c b/tools/testing/selftests/kvm/guest_memfd_test.c index b3ca6737f304..be1f78542d64 100644 --- a/tools/testing/selftests/kvm/guest_memfd_test.c +++ b/tools/testing/selftests/kvm/guest_memfd_test.c @@ -24,18 +24,55 @@ #include "test_util.h" #include "ucall_common.h"
-static void test_file_read_write(int fd) +static void test_file_read(int fd) { char buf[64];
TEST_ASSERT(read(fd, buf, sizeof(buf)) < 0, "read on a guest_mem fd should fail"); - TEST_ASSERT(write(fd, buf, sizeof(buf)) < 0, - "write on a guest_mem fd should fail"); TEST_ASSERT(pread(fd, buf, sizeof(buf), 0) < 0, "pread on a guest_mem fd should fail"); - TEST_ASSERT(pwrite(fd, buf, sizeof(buf), 0) < 0, - "pwrite on a guest_mem fd should fail"); +} + +static void test_write_supported(int fd, size_t total_size) +{ + size_t page_size = getpagesize(); + void *buf = NULL; + int ret; + + ret = posix_memalign(&buf, page_size, total_size); + TEST_ASSERT_EQ(ret, 0); + + ret = pwrite(fd, buf, page_size, total_size); + TEST_ASSERT(ret == -1, "writing past the file size on a guest_mem fd should fail"); + TEST_ASSERT_EQ(errno, EINVAL); + + ret = pwrite(fd, buf, page_size, 0); + TEST_ASSERT(ret == page_size, "write on a guest_mem fd should succeed"); + + ret = fallocate(fd, FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE, 0, page_size); + TEST_ASSERT(!ret, "fallocate(PUNCH_HOLE) should succeed"); + + free(buf); +} + +static void test_write_not_supported(int fd, size_t total_size) +{ + size_t page_size = getpagesize(); + void *buf = NULL; + int ret; + + ret = posix_memalign(&buf, page_size, total_size); + TEST_ASSERT_EQ(ret, 0); + + ret = pwrite(fd, buf, page_size, 0); + TEST_ASSERT(ret == -1, "write on guest_mem fd should fail"); + TEST_ASSERT_EQ(errno, ENODEV); + + ret = fallocate(fd, FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE, 0, page_size); + TEST_ASSERT(!ret, "fallocate(PUNCH_HOLE) should succeed"); + + free(buf); }
static void test_mmap_supported(int fd, size_t page_size, size_t total_size) @@ -281,12 +318,14 @@ static void test_guest_memfd(unsigned long vm_type)
fd = vm_create_guest_memfd(vm, total_size, flags);
- test_file_read_write(fd); + test_file_read(fd);
if (flags & GUEST_MEMFD_FLAG_MMAP) { + test_write_supported(fd, total_size); test_mmap_supported(fd, page_size, total_size); test_fault_overflow(fd, page_size, total_size); } else { + test_write_not_supported(fd, total_size); test_mmap_not_supported(fd, page_size, total_size); }
linux-kselftest-mirror@lists.linaro.org
-
Kalyazin, Nikita -
Nikita Kalyazin -
Sean Christopherson