1G page support for guest_memfd

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +5 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 
+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+
 See KVM_SET_USER_MEMORY_REGION2 for additional details.
 
 4.143 KVM_PRE_FAULT_MEMORY

include/uapi/linux/kvm.h +2 -0

index 4cc824a3a7c9..d7df312479aa 100644
@@ -1567,7 +1567,9 @@ struct kvm_memory_attributes {
 #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
 
 #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
+
 #define GUEST_MEMFD_FLAG_SUPPORT_SHARED	(1UL << 0)
+#define GUEST_MEMFD_FLAG_INIT_PRIVATE	(1UL << 1)
 
 struct kvm_create_guest_memfd {
 	__u64 size;

virt/kvm/guest_memfd.c +34 -0

index 239d0f13dcc1..590932499eba 100644
@@ -4,6 +4,7 @@
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/kvm_host.h>
+#include <linux/maple_tree.h>
 #include <linux/pseudo_fs.h>
 #include <linux/pagemap.h>
 

@@ -17,6 +18,24 @@ struct kvm_gmem {
 	struct list_head entry;
 };
 
+struct kvm_gmem_inode_private {
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	struct maple_tree shareability;
+#endif
+};
+
+enum shareability {
+	SHAREABILITY_GUEST = 1,	/* Only the guest can map (fault) folios in this range. */
+	SHAREABILITY_ALL = 2,	/* Both guest and host can fault folios in this range. */
+};
+
+static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index);
+
+static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
+{
+	return inode->i_mapping->i_private_data;
+}
+
 /**
  * folio_file_pfn - like folio_file_page, but return a pfn.
  * @folio: The folio which contains this index.

@@ -29,6 +48,58 @@ static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index)
 	return folio_pfn(folio) + (index & (folio_nr_pages(folio) - 1));
 }
 
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+
+static int kvm_gmem_shareability_setup(struct kvm_gmem_inode_private *private,
+				      loff_t size, u64 flags)
+{
+	enum shareability m;
+	pgoff_t last;
+
+	last = (size >> PAGE_SHIFT) - 1;
+	m = flags & GUEST_MEMFD_FLAG_INIT_PRIVATE ? SHAREABILITY_GUEST :
+						    SHAREABILITY_ALL;
+	return mtree_store_range(&private->shareability, 0, last, xa_mk_value(m),
+				 GFP_KERNEL);
+}
+

virt/kvm/guest_memfd.c +54 -1

+	return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+		return ERR_PTR(-EACCES);
+
+	return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+	return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	WARN_ONCE("Unexpected call to get shared folio.")
+	return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
 				    pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
 	filemap_invalidate_lock_shared(inode->i_mapping);
 
-	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+	folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
 	if (IS_ERR(folio)) {
 		int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
 	.fallocate	= kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+	kfree(private);
+
+	free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	/*
+	 * mtree_destroy() can't be used within rcu callback, hence can't be
+	 * done in ->free_inode().
+	 */
+	if (private)
+		mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
 	.statfs		= simple_statfs,
+	.destroy_inode	= kvm_gmem_destroy_inode,
+	.free_inode	= kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 						      loff_t size, u64 flags)
 {
+	struct kvm_gmem_inode_private *private;
 	struct inode *inode;
+	int err;
 

virt/kvm/guest_memfd.c +17 -1

+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
 	inode->i_private = (void *)(unsigned long)flags;
 	inode->i_op = &kvm_gmem_iops;
 	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
 	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
 	if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
 		valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+	if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+		valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
 	if (flags & ~valid_flags)
 		return -EINVAL;
 

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 	if (!file)
 		return -EFAULT;
 
+	filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+
 	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
 	if (IS_ERR(folio)) {
 		r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 		*page = folio_file_page(folio, index);
 	else
 		folio_put(folio);
-
 out:
+	filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
 	fput(file);
 	return r;
 }

Signature · 1 line

2.49.0.1045.g170613ef41-goog

Quoted 118 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +5 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 
+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+
 See KVM_SET_USER_MEMORY_REGION2 for additional details.
 
 4.143 KVM_PRE_FAULT_MEMORY

include/uapi/linux/kvm.h +2 -0

index 4cc824a3a7c9..d7df312479aa 100644
@@ -1567,7 +1567,9 @@ struct kvm_memory_attributes {
 #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
 
 #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
+
 #define GUEST_MEMFD_FLAG_SUPPORT_SHARED	(1UL << 0)
+#define GUEST_MEMFD_FLAG_INIT_PRIVATE	(1UL << 1)
 
 struct kvm_create_guest_memfd {
 	__u64 size;

virt/kvm/guest_memfd.c +34 -0

index 239d0f13dcc1..590932499eba 100644
@@ -4,6 +4,7 @@
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/kvm_host.h>
+#include <linux/maple_tree.h>
 #include <linux/pseudo_fs.h>
 #include <linux/pagemap.h>
 

@@ -17,6 +18,24 @@ struct kvm_gmem {
 	struct list_head entry;
 };
 
+struct kvm_gmem_inode_private {
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	struct maple_tree shareability;
+#endif
+};
+
+enum shareability {
+	SHAREABILITY_GUEST = 1,	/* Only the guest can map (fault) folios in this range. */
+	SHAREABILITY_ALL = 2,	/* Both guest and host can fault folios in this range. */
+};
+
+static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index);
+
+static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
+{
+	return inode->i_mapping->i_private_data;
+}
+
 /**
  * folio_file_pfn - like folio_file_page, but return a pfn.
  * @folio: The folio which contains this index.

@@ -29,6 +48,58 @@ static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index)
 	return folio_pfn(folio) + (index & (folio_nr_pages(folio) - 1));
 }
 
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+
+static int kvm_gmem_shareability_setup(struct kvm_gmem_inode_private *private,
+				      loff_t size, u64 flags)
+{
+	enum shareability m;
+	pgoff_t last;
+
+	last = (size >> PAGE_SHIFT) - 1;
+	m = flags & GUEST_MEMFD_FLAG_INIT_PRIVATE ? SHAREABILITY_GUEST :
+						    SHAREABILITY_ALL;
+	return mtree_store_range(&private->shareability, 0, last, xa_mk_value(m),
+				 GFP_KERNEL);
+}
+

virt/kvm/guest_memfd.c static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index) · line 75

Quoted 10 lines

virt/kvm/guest_memfd.c +10 -0

+static enum shareability kvm_gmem_shareability_get(struct inode *inode,
+						 pgoff_t index)
+{
+	struct maple_tree *mt;
+	void *entry;
+
+	mt = &kvm_gmem_private(inode)->shareability;
+	entry = mtree_load(mt, index);
+	WARN(!entry,
+	     "Shareability should always be defined for all indices in inode.");

I noticed that in [1], the kvm_gmem_mmap() does not check the range. So, the WARN() here can be hit when userspace mmap() an area larger than the inode size and accesses the out of band HVA.

Maybe limit the mmap() range?

(quoted diff) +3 -0

@@ -1609,6 +1620,10 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
        if (!kvm_gmem_supports_shared(file_inode(file)))
                return -ENODEV;
 
+       if (vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT) > i_size_read(file_inode(file)))
+               return -EINVAL;
+
        if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) !=
            (VM_SHARED | VM_MAYSHARE)) {
                return -EINVAL;

[1] https://lore.kernel.org/all/[email protected]/

Quoted 84 lines

virt/kvm/guest_memfd.c +54 -1

+	return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+		return ERR_PTR(-EACCES);
+
+	return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+	return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	WARN_ONCE("Unexpected call to get shared folio.")
+	return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
 				    pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
 	filemap_invalidate_lock_shared(inode->i_mapping);
 
-	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+	folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
 	if (IS_ERR(folio)) {
 		int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
 	.fallocate	= kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+	kfree(private);
+
+	free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	/*
+	 * mtree_destroy() can't be used within rcu callback, hence can't be
+	 * done in ->free_inode().
+	 */
+	if (private)
+		mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
 	.statfs		= simple_statfs,
+	.destroy_inode	= kvm_gmem_destroy_inode,
+	.free_inode	= kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 						      loff_t size, u64 flags)
 {
+	struct kvm_gmem_inode_private *private;
 	struct inode *inode;
+	int err;
 
 	inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
 	if (IS_ERR(inode))
 		return inode;
 

virt/kvm/guest_memfd.c static const struct inode_operations kvm_gmem_iops = { · line 661

Quoted 6 lines

virt/kvm/guest_memfd.c +6 -0

+	err = -ENOMEM;
+	private = kzalloc(sizeof(*private), GFP_KERNEL);
+	if (!private)
+		goto out;
+
+	mt_init(&private->shareability);

Wrap the mt_init() inside "#ifdef CONFIG_KVM_GMEM_SHARED_MEM" ?

Quoted 54 lines

virt/kvm/guest_memfd.c +17 -1

+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
 	inode->i_private = (void *)(unsigned long)flags;
 	inode->i_op = &kvm_gmem_iops;
 	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
 	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
 	if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
 		valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+	if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+		valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
 	if (flags & ~valid_flags)
 		return -EINVAL;
 

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 	if (!file)
 		return -EFAULT;
 
+	filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+
 	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
 	if (IS_ERR(folio)) {
 		r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 		*page = folio_file_page(folio, index);
 	else
 		folio_put(folio);
-
 out:
+	filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
 	fput(file);
 	return r;
 }

Signature · 1 line

2.49.0.1045.g170613ef41-goog

I noticed that in [1], the kvm_gmem_mmap() does not check the range. So, the WARN() here can be hit when userspace mmap() an area larger than the inode size and accesses the out of band HVA.

Maybe limit the mmap() range?

(quoted diff) +3 -0

@@ -1609,6 +1620,10 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
        if (!kvm_gmem_supports_shared(file_inode(file)))
                return -ENODEV;
 
+       if (vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT) > i_size_read(file_inode(file)))
+               return -EINVAL;
+
        if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) !=
            (VM_SHARED | VM_MAYSHARE)) {
                return -EINVAL;

[1] https://lore.kernel.org/all/[email protected]/

Quoted 80 lines

virt/kvm/guest_memfd.c +54 -1

+     return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+     if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+             return ERR_PTR(-EACCES);
+
+     return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+     return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+     WARN_ONCE("Unexpected call to get shared folio.")
+     return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
                                  pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
      filemap_invalidate_lock_shared(inode->i_mapping);
 
-     folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+     folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
      if (IS_ERR(folio)) {
              int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
      .fallocate      = kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+     struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+     kfree(private);
+
+     free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+     struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+     /*
+      * mtree_destroy() can't be used within rcu callback, hence can't be
+      * done in ->free_inode().
+      */
+     if (private)
+             mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
      .statfs         = simple_statfs,
+     .destroy_inode  = kvm_gmem_destroy_inode,
+     .free_inode     = kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
                                                    loff_t size, u64 flags)
 {
+     struct kvm_gmem_inode_private *private;
      struct inode *inode;
+     int err;
 

virt/kvm/guest_memfd.c static const struct inode_operations kvm_gmem_iops = { · line 661

Quoted 10 lines

virt/kvm/guest_memfd.c +6 -0

      inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
      if (IS_ERR(inode))
              return inode;
 
+     err = -ENOMEM;
+     private = kzalloc(sizeof(*private), GFP_KERNEL);
+     if (!private)
+             goto out;
+
+     mt_init(&private->shareability);

Wrap the mt_init() inside "#ifdef CONFIG_KVM_GMEM_SHARED_MEM" ?

Quoted 54 lines

virt/kvm/guest_memfd.c +17 -2

+     inode->i_mapping->i_private_data = private;
+
+     err = kvm_gmem_shareability_setup(private, size, flags);
+     if (err)
+             goto out;
+
      inode->i_private = (void *)(unsigned long)flags;
      inode->i_op = &kvm_gmem_iops;
      inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
      WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
      return inode;
+
+out:
+     iput(inode);
+
+     return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
      if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
              valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+     if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+             valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
      if (flags & ~valid_flags)
              return -EINVAL;
 

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
      if (!file)
              return -EFAULT;
 
+     filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+
      folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
      if (IS_ERR(folio)) {
              r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
              *page = folio_file_page(folio, index);
      else
              folio_put(folio);
-
 out:
+     filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
      fput(file);
      return r;
 }
--

2.49.0.1045.g170613ef41-goog

Hi,

.. snip..

Quoted 17 lines

I noticed that in [1], the kvm_gmem_mmap() does not check the range. So, the WARN() here can be hit when userspace mmap() an area larger than the inode size and accesses the out of band HVA.

Maybe limit the mmap() range?

(quoted diff) +3 -0

@@ -1609,6 +1620,10 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
        if (!kvm_gmem_supports_shared(file_inode(file)))
                return -ENODEV;
 
+       if (vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT) > i_size_read(file_inode(file)))
+               return -EINVAL;
+
        if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) !=
            (VM_SHARED | VM_MAYSHARE)) {
                return -EINVAL;

Quoted 1 line

[1] https://lore.kernel.org/all/[email protected]/

I don't think we want to do that for a couple of reasons. We catch such invalid accesses on faulting, and, by analogy, afaikt, neither secretmem nor memfd perform a similar check on mmap (nor do memory-mapped files in general).

There are also valid reasons why a user would want to deliberately mmap more memory than the backing store, knowing that it's only going to fault what it's going to use, e.g., alignment.

Cheers,
/fuad

Quoted 146 lines

Quoted 80 lines

virt/kvm/guest_memfd.c +54 -1

+     return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+     if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+             return ERR_PTR(-EACCES);
+
+     return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+     return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+     WARN_ONCE("Unexpected call to get shared folio.")
+     return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
                                  pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
      filemap_invalidate_lock_shared(inode->i_mapping);
 
-     folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+     folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
      if (IS_ERR(folio)) {
              int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
      .fallocate      = kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+     struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+     kfree(private);
+
+     free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+     struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+     /*
+      * mtree_destroy() can't be used within rcu callback, hence can't be
+      * done in ->free_inode().
+      */
+     if (private)
+             mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
      .statfs         = simple_statfs,
+     .destroy_inode  = kvm_gmem_destroy_inode,
+     .free_inode     = kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
                                                    loff_t size, u64 flags)
 {
+     struct kvm_gmem_inode_private *private;
      struct inode *inode;
+     int err;
 

virt/kvm/guest_memfd.c static const struct inode_operations kvm_gmem_iops = { · line 661

Quoted 10 lines

virt/kvm/guest_memfd.c +6 -0

      inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
      if (IS_ERR(inode))
              return inode;
 
+     err = -ENOMEM;
+     private = kzalloc(sizeof(*private), GFP_KERNEL);
+     if (!private)
+             goto out;
+
+     mt_init(&private->shareability);

Wrap the mt_init() inside "#ifdef CONFIG_KVM_GMEM_SHARED_MEM" ?

Quoted 54 lines

virt/kvm/guest_memfd.c +17 -2

+     inode->i_mapping->i_private_data = private;
+
+     err = kvm_gmem_shareability_setup(private, size, flags);
+     if (err)
+             goto out;
+
      inode->i_private = (void *)(unsigned long)flags;
      inode->i_op = &kvm_gmem_iops;
      inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
      WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
      return inode;
+
+out:
+     iput(inode);
+
+     return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
      if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
              valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+     if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+             valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
      if (flags & ~valid_flags)
              return -EINVAL;
 

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
      if (!file)
              return -EFAULT;
 
+     filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+
      folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
      if (IS_ERR(folio)) {
              r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
              *page = folio_file_page(folio, index);
      else
              folio_put(folio);
-
 out:
+     filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
      fput(file);
      return r;
 }
--

2.49.0.1045.g170613ef41-goog

Quoted 28 lines · Fuad Tabba

Hi,

.. snip..

Quoted 17 lines

I noticed that in [1], the kvm_gmem_mmap() does not check the range. So, the WARN() here can be hit when userspace mmap() an area larger than the inode size and accesses the out of band HVA.

Maybe limit the mmap() range?

(quoted diff) +3 -0

@@ -1609,6 +1620,10 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
        if (!kvm_gmem_supports_shared(file_inode(file)))
                return -ENODEV;
 
+       if (vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT) > i_size_read(file_inode(file)))
+               return -EINVAL;
+
        if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) !=
            (VM_SHARED | VM_MAYSHARE)) {
                return -EINVAL;

Quoted 1 line

[1] https://lore.kernel.org/all/[email protected]/

I don't think we want to do that for a couple of reasons. We catch such invalid accesses on faulting, and, by analogy, afaikt, neither secretmem nor memfd perform a similar check on mmap (nor do memory-mapped files in general).

Quoted 4 lines

There are also valid reasons why a user would want to deliberately mmap more memory than the backing store, knowing that it's only going to fault what it's going to use, e.g., alignment.

This is a good point.

I think there's no check against the inode size on faulting now though? v10's [1] kvm_gmem_fault_shared() calls kvm_gmem_get_folio() straightaway.

We should add a check like [2] to kvm_gmem_fault_shared().

[1] https://lore.kernel.org/all/[email protected]/
[2] https://github.com/torvalds/linux/blob/8477ab143069c6b05d6da4a8184ded8b969240f5/mm/filemap.c#L3373

Quoted 150 lines

Cheers,
/fuad

Quoted 146 lines

Quoted 80 lines

virt/kvm/guest_memfd.c +54 -1

+     return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+     if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+             return ERR_PTR(-EACCES);
+
+     return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+     return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+     WARN_ONCE("Unexpected call to get shared folio.")
+     return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
                                  pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
      filemap_invalidate_lock_shared(inode->i_mapping);
 
-     folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+     folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
      if (IS_ERR(folio)) {
              int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
      .fallocate      = kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+     struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+     kfree(private);
+
+     free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+     struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+     /*
+      * mtree_destroy() can't be used within rcu callback, hence can't be
+      * done in ->free_inode().
+      */
+     if (private)
+             mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
      .statfs         = simple_statfs,
+     .destroy_inode  = kvm_gmem_destroy_inode,
+     .free_inode     = kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
                                                    loff_t size, u64 flags)
 {
+     struct kvm_gmem_inode_private *private;
      struct inode *inode;
+     int err;
 

virt/kvm/guest_memfd.c static const struct inode_operations kvm_gmem_iops = { · line 661

Quoted 10 lines

virt/kvm/guest_memfd.c +6 -0

      inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
      if (IS_ERR(inode))
              return inode;
 
+     err = -ENOMEM;
+     private = kzalloc(sizeof(*private), GFP_KERNEL);
+     if (!private)
+             goto out;
+
+     mt_init(&private->shareability);

Wrap the mt_init() inside "#ifdef CONFIG_KVM_GMEM_SHARED_MEM" ?

Quoted 54 lines

virt/kvm/guest_memfd.c +17 -2

+     inode->i_mapping->i_private_data = private;
+
+     err = kvm_gmem_shareability_setup(private, size, flags);
+     if (err)
+             goto out;
+
      inode->i_private = (void *)(unsigned long)flags;
      inode->i_op = &kvm_gmem_iops;
      inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
      WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
      return inode;
+
+out:
+     iput(inode);
+
+     return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
      if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
              valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+     if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+             valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
      if (flags & ~valid_flags)
              return -EINVAL;
 

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
      if (!file)
              return -EFAULT;
 
+     filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+
      folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
      if (IS_ERR(folio)) {
              r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
              *page = folio_file_page(folio, index);
      else
              folio_put(folio);
-
 out:
+     filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
      fput(file);
      return r;
 }
--

2.49.0.1045.g170613ef41-goog

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
  Documentation/virt/kvm/api.rst |   5 ++
  include/uapi/linux/kvm.h       |   2 +
  virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
  3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644

Setting GUEST_MEMFD_FLAG_INIT_PRIVATE

Documentation/virt/kvm/api.rst +3 -0

+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

static int kvm_gmem_init_fs_context(struct fs_context *fc)

Documentation/virt/kvm/api.rst +2 -0

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
  static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
  						      loff_t size, u64 flags)
  {
+	struct kvm_gmem_inode_private *private;
  	struct inode *inode;
+	int err;
 

Documentation/virt/kvm/api.rst +11 -0

+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
  	inode->i_private = (void *)(unsigned long)flags;
  	inode->i_op = &kvm_gmem_iops;
  	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
  	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
  	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
  }
 
 

Quoted 32 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
  Documentation/virt/kvm/api.rst |   5 ++ include/uapi/linux/kvm.h       |   2 + virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-

  3 files changed, 129 insertions(+), 2 deletions(-)

Quoted 9 lines

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.

  The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs. This is validated when the guest_memfd instance is bound to the VM. +If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field +supports GUEST_MEMFD_FLAG_INIT_PRIVATE.

It seems that the sentence is stale?
Didn't find the definition of KVM_CAP_GMEM_CONVERSIONS.

Quoted 5 lines

Setting GUEST_MEMFD_FLAG_INIT_PRIVATE

Documentation/virt/kvm/api.rst +3 -0

+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

[...]

Quoted 7 lines

    static int kvm_gmem_init_fs_context(struct fs_context *fc)

Documentation/virt/kvm/api.rst +1 -0

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
   static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
                                 loff_t size, u64 flags)
   {
+    struct kvm_gmem_inode_private *private;
       struct inode *inode;

Documentation/virt/kvm/api.rst static const struct inode_operations kvm_gmem_iops = { · line 659

Quoted 10 lines

Documentation/virt/kvm/api.rst +6 -0

+    int err;
         inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
       if (IS_ERR(inode))
           return inode;
   +    err = -ENOMEM;
+    private = kzalloc(sizeof(*private), GFP_KERNEL);
+    if (!private)
+        goto out;
+
+    mt_init(&private->shareability);

shareability is defined only when CONFIG_KVM_GMEM_SHARED_MEM enabled, should be done within CONFIG_KVM_GMEM_SHARED_MEM .

Quoted 19 lines

Documentation/virt/kvm/api.rst +11 -0

+ inode->i_mapping->i_private_data = private;
+
+    err = kvm_gmem_shareability_setup(private, size, flags);
+    if (err)
+        goto out;
+
       inode->i_private = (void *)(unsigned long)flags;
       inode->i_op = &kvm_gmem_iops;
       inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
       WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
         return inode;
+
+out:
+    iput(inode);
+
+    return ERR_PTR(err);
   }
 

[...]

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
  Documentation/virt/kvm/api.rst |   5 ++
  include/uapi/linux/kvm.h       |   2 +
  virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
  3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
  The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
  This is validated when the guest_memfd instance is bound to the VM.
 

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6412

Quoted 2 lines

Documentation/virt/kvm/api.rst +2 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.

It seems that the sentence is stale?
Didn't find the definition of KVM_CAP_GMEM_CONVERSIONS.

Quoted 4 lines

Setting GUEST_MEMFD_FLAG_INIT_PRIVATE

Documentation/virt/kvm/api.rst +3 -0

+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

[...]

Quoted 14 lines

static int kvm_gmem_init_fs_context(struct fs_context *fc)

Documentation/virt/kvm/api.rst +2 -0

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
  static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
  						      loff_t size, u64 flags)
  {
+	struct kvm_gmem_inode_private *private;
  	struct inode *inode;
+	int err;
 
  	inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
  	if (IS_ERR(inode))
  		return inode;
 

Documentation/virt/kvm/api.rst static const struct inode_operations kvm_gmem_iops = { · line 661

Quoted 6 lines

Documentation/virt/kvm/api.rst +6 -0

+	err = -ENOMEM;
+	private = kzalloc(sizeof(*private), GFP_KERNEL);
+	if (!private)
+		goto out;
+
+	mt_init(&private->shareability);

shareability is defined only when CONFIG_KVM_GMEM_SHARED_MEM enabled, should be done within CONFIG_KVM_GMEM_SHARED_MEM .

Quoted 21 lines

Documentation/virt/kvm/api.rst +11 -0

+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
  	inode->i_private = (void *)(unsigned long)flags;
  	inode->i_op = &kvm_gmem_iops;
  	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
  	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
  	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
  }
 
 

[...]

Quoted 41 lines · Binbin Wu

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
  Documentation/virt/kvm/api.rst |   5 ++
  include/uapi/linux/kvm.h       |   2 +
  virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
  3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
  The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
  This is validated when the guest_memfd instance is bound to the VM.
 

Quoted 6 lines

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6412

Quoted 2 lines

Documentation/virt/kvm/api.rst +2 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.

It seems that the sentence is stale?
Didn't find the definition of KVM_CAP_GMEM_CONVERSIONS.

Thanks. This should read

If the capability KVM_CAP_GMEM_SHARED_MEM is supported, and GUEST_MEMFD_FLAG_SUPPORT_SHARED is specified, then the 'flags' field supports GUEST_MEMFD_FLAG_INIT_PRIVATE.

Quoted 19 lines

Quoted 4 lines

Setting GUEST_MEMFD_FLAG_INIT_PRIVATE

Documentation/virt/kvm/api.rst +3 -0

+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

[...]

Quoted 14 lines

static int kvm_gmem_init_fs_context(struct fs_context *fc)

Documentation/virt/kvm/api.rst +2 -0

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
  static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
  						      loff_t size, u64 flags)
  {
+	struct kvm_gmem_inode_private *private;
  	struct inode *inode;
+	int err;
 
  	inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
  	if (IS_ERR(inode))
  		return inode;
 

Quoted 10 lines

Documentation/virt/kvm/api.rst static const struct inode_operations kvm_gmem_iops = { · line 661

Quoted 6 lines

Documentation/virt/kvm/api.rst +6 -0

+	err = -ENOMEM;
+	private = kzalloc(sizeof(*private), GFP_KERNEL);
+	if (!private)
+		goto out;
+
+	mt_init(&private->shareability);

shareability is defined only when CONFIG_KVM_GMEM_SHARED_MEM enabled, should be done within CONFIG_KVM_GMEM_SHARED_MEM .

Yes, thank you! Will also update this to only initialize shareability if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED).

Quoted 22 lines

Quoted 21 lines

Documentation/virt/kvm/api.rst +11 -0

+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
  	inode->i_private = (void *)(unsigned long)flags;
  	inode->i_op = &kvm_gmem_iops;
  	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
  	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
  	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
  }
 
 

[...]

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644

Documentation/virt/kvm/api.rst +0 -0

 See KVM_SET_USER_MEMORY_REGION2 for additional details.
 
 4.143 KVM_PRE_FAULT_MEMORY

include/uapi/linux/kvm.h +2 -0

index 4cc824a3a7c9..d7df312479aa 100644
@@ -1567,7 +1567,9 @@ struct kvm_memory_attributes {
 #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
 
 #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
+
 #define GUEST_MEMFD_FLAG_SUPPORT_SHARED	(1UL << 0)
+#define GUEST_MEMFD_FLAG_INIT_PRIVATE	(1UL << 1)
 
 struct kvm_create_guest_memfd {
 	__u64 size;

virt/kvm/guest_memfd.c +22 -0

index 239d0f13dcc1..590932499eba 100644
@@ -4,6 +4,7 @@
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/kvm_host.h>
+#include <linux/maple_tree.h>
 #include <linux/pseudo_fs.h>
 #include <linux/pagemap.h>
 

@@ -17,6 +18,24 @@ struct kvm_gmem {
 	struct list_head entry;
 };
 
+struct kvm_gmem_inode_private {
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	struct maple_tree shareability;
+#endif
+};
+
+enum shareability {
+	SHAREABILITY_GUEST = 1,	/* Only the guest can map (fault) folios in this range. */
+	SHAREABILITY_ALL = 2,	/* Both guest and host can fault folios in this range. */
+};
+
+static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index);
+
+static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
+{
+	return inode->i_mapping->i_private_data;
+}
+
 /**
  * folio_file_pfn - like folio_file_page, but return a pfn.
  * @folio: The folio which contains this index.

@@ -29,6 +48,58 @@ static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index)
 	return folio_pfn(folio) + (index & (folio_nr_pages(folio) - 1));
 }
 
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+
+static int kvm_gmem_shareability_setup(struct kvm_gmem_inode_private *private,

virt/kvm/guest_memfd.c +87 -1

+}
+
+static enum shareability kvm_gmem_shareability_get(struct inode *inode,
+						 pgoff_t index)
+{
+	struct maple_tree *mt;
+	void *entry;
+
+	mt = &kvm_gmem_private(inode)->shareability;
+	entry = mtree_load(mt, index);
+	WARN(!entry,
+	     "Shareability should always be defined for all indices in inode.");
+
+	return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+		return ERR_PTR(-EACCES);
+
+	return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+	return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	WARN_ONCE("Unexpected call to get shared folio.")
+	return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
 				    pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
 	filemap_invalidate_lock_shared(inode->i_mapping);
 
-	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+	folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
 	if (IS_ERR(folio)) {
 		int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
 	.fallocate	= kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+	kfree(private);
+
+	free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	/*
+	 * mtree_destroy() can't be used within rcu callback, hence can't be
+	 * done in ->free_inode().
+	 */
+	if (private)
+		mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
 	.statfs		= simple_statfs,
+	.destroy_inode	= kvm_gmem_destroy_inode,
+	.free_inode	= kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 						      loff_t size, u64 flags)
 {
+	struct kvm_gmem_inode_private *private;
 	struct inode *inode;
+	int err;
 
 	inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
 	if (IS_ERR(inode))
 		return inode;
 
+	err = -ENOMEM;
+	private = kzalloc(sizeof(*private), GFP_KERNEL);
+	if (!private)
+		goto out;
+
+	mt_init(&private->shareability);
+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
 	inode->i_private = (void *)(unsigned long)flags;
 	inode->i_op = &kvm_gmem_iops;
 	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
 	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
 	if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
 		valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+	if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+		valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
 	if (flags & ~valid_flags)
 		return -EINVAL;
 

virt/kvm/guest_memfd.c +1 -1

 	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
 	if (IS_ERR(folio)) {
 		r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 		*page = folio_file_page(folio, index);
 	else
 		folio_put(folio);
-
 out:
+	filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
 	fput(file);
 	return r;
 }

Signature · 1 line

2.49.0.1045.g170613ef41-goog

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6415

Quoted 5 lines

Documentation/virt/kvm/api.rst +5 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

KVM_CAP_GMEM_CONVERSION doesn't get introduced until later, so it seems like this flag should be deferred until that patch is in place. Is it really needed at that point though? Userspace would be able to set the initial state via KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls.

The mtree contents seems to get stored in the same manner in either case so performance-wise only the overhead of a few userspace<->kernel switches would be saved. Are there any other reasons?

Otherwise, maybe just settle on SHARED as a documented default (since at least non-CoCo VMs would be able to reliably benefit) and let CoCo/GUEST_MEMFD_FLAG_SUPPORT_SHARED VMs set PRIVATE at whatever granularity makes sense for the architecture/guest configuration.

Quoted 61 lines

Documentation/virt/kvm/api.rst +0 -0

 See KVM_SET_USER_MEMORY_REGION2 for additional details.
 
 4.143 KVM_PRE_FAULT_MEMORY

include/uapi/linux/kvm.h +2 -0

index 4cc824a3a7c9..d7df312479aa 100644
@@ -1567,7 +1567,9 @@ struct kvm_memory_attributes {
 #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
 
 #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
+
 #define GUEST_MEMFD_FLAG_SUPPORT_SHARED	(1UL << 0)
+#define GUEST_MEMFD_FLAG_INIT_PRIVATE	(1UL << 1)
 
 struct kvm_create_guest_memfd {
 	__u64 size;

virt/kvm/guest_memfd.c +22 -0

index 239d0f13dcc1..590932499eba 100644
@@ -4,6 +4,7 @@
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/kvm_host.h>
+#include <linux/maple_tree.h>
 #include <linux/pseudo_fs.h>
 #include <linux/pagemap.h>
 

@@ -17,6 +18,24 @@ struct kvm_gmem {
 	struct list_head entry;
 };
 
+struct kvm_gmem_inode_private {
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	struct maple_tree shareability;
+#endif
+};
+
+enum shareability {
+	SHAREABILITY_GUEST = 1,	/* Only the guest can map (fault) folios in this range. */
+	SHAREABILITY_ALL = 2,	/* Both guest and host can fault folios in this range. */
+};
+
+static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index);
+
+static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
+{
+	return inode->i_mapping->i_private_data;
+}
+
 /**
  * folio_file_pfn - like folio_file_page, but return a pfn.
  * @folio: The folio which contains this index.

@@ -29,6 +48,58 @@ static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index)
 	return folio_pfn(folio) + (index & (folio_nr_pages(folio) - 1));
 }
 
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+
+static int kvm_gmem_shareability_setup(struct kvm_gmem_inode_private *private,

virt/kvm/guest_memfd.c static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index) · line 63

Quoted 10 lines

virt/kvm/guest_memfd.c +10 -0

+				      loff_t size, u64 flags)
+{
+	enum shareability m;
+	pgoff_t last;
+
+	last = (size >> PAGE_SHIFT) - 1;
+	m = flags & GUEST_MEMFD_FLAG_INIT_PRIVATE ? SHAREABILITY_GUEST :
+						    SHAREABILITY_ALL;
+	return mtree_store_range(&private->shareability, 0, last, xa_mk_value(m),
+				 GFP_KERNEL);

One really nice thing about using a maple tree is that it should get rid of a fairly significant startup delay for SNP/TDX when the entire xarray gets initialized with private attribute entries via KVM_SET_MEMORY_ATTRIBUTES (which is the current QEMU default behavior).

I'd originally advocated for sticking with the xarray implementation Fuad was using until we'd determined we really need it for HugeTLB support, but I'm sort of thinking it's already justified just based on the above.

Maybe it would make sense for KVM memory attributes too?

Quoted 135 lines

virt/kvm/guest_memfd.c +87 -1

+}
+
+static enum shareability kvm_gmem_shareability_get(struct inode *inode,
+						 pgoff_t index)
+{
+	struct maple_tree *mt;
+	void *entry;
+
+	mt = &kvm_gmem_private(inode)->shareability;
+	entry = mtree_load(mt, index);
+	WARN(!entry,
+	     "Shareability should always be defined for all indices in inode.");
+
+	return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+		return ERR_PTR(-EACCES);
+
+	return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+	return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	WARN_ONCE("Unexpected call to get shared folio.")
+	return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
 				    pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
 	filemap_invalidate_lock_shared(inode->i_mapping);
 
-	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+	folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
 	if (IS_ERR(folio)) {
 		int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
 	.fallocate	= kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+	kfree(private);
+
+	free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	/*
+	 * mtree_destroy() can't be used within rcu callback, hence can't be
+	 * done in ->free_inode().
+	 */
+	if (private)
+		mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
 	.statfs		= simple_statfs,
+	.destroy_inode	= kvm_gmem_destroy_inode,
+	.free_inode	= kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 						      loff_t size, u64 flags)
 {
+	struct kvm_gmem_inode_private *private;
 	struct inode *inode;
+	int err;
 
 	inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
 	if (IS_ERR(inode))
 		return inode;
 
+	err = -ENOMEM;
+	private = kzalloc(sizeof(*private), GFP_KERNEL);
+	if (!private)
+		goto out;
+
+	mt_init(&private->shareability);
+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
 	inode->i_private = (void *)(unsigned long)flags;
 	inode->i_op = &kvm_gmem_iops;
 	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
 	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
 	if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
 		valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+	if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+		valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
 	if (flags & ~valid_flags)
 		return -EINVAL;
 

virt/kvm/guest_memfd.c int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, · line 964

Quoted 5 lines

virt/kvm/guest_memfd.c +2 -0

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 	if (!file)
 		return -EFAULT;
 
+	filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+

I like the idea of using a write-lock/read-lock to protect write/read access to shareability state (though maybe not necessarily re-using filemap's invalidate lock), it's simple and still allows concurrent faulting in of gmem pages. One issue on the SNP side (which also came up in one of the gmem calls) is if we introduce support for tracking preparedness as discussed (e.g. via a new SHAREABILITY_GUEST_PREPARED state) the SHAREABILITY_GUEST->SHAREABILITY_GUEST_PREPARED transition would occur at fault-time, and so would need to take the write-lock and no longer allow for concurrent fault-handling.

I was originally planning on introducing a new rw_semaphore with similar semantics to the rw_lock that Fuad previously had in his restricted mmap series[1] (and simiar semantics to filemap invalidate lock here). The main difference, to handle setting SHAREABILITY_GUEST_PREPARED within fault paths, was that in the case of a folio being present for an index, the folio lock would also need to be held in order to update the shareability state. Because of that, fault paths (which will always either have or allocate folio basically) can rely on the folio lock to guard shareability state in a more granular way and so can avoid a global write lock.

They would still need to hold the read lock to access the tree however. Or more specifically, any paths that could allocate a folio need to take a read lock so there isn't a TOCTOU situation where shareability is being updated for an index for which a folio hasn't been allocated, but then just afterward the folio gets faulted in/allocated while the shareability state is already being updated which the understand that there was no folio around that needed locking.

I had a branch with in-place conversion support for SNP[2] that added this lock reworking on top of Fuad's series along with preparation tracking, but I'm now planning to rebase that on top of the patches from this series that Sean mentioned[3] earlier:

KVM: guest_memfd: Add CAP KVM_CAP_GMEM_CONVERSION
KVM: Query guest_memfd for private/shared status
KVM: guest_memfd: Skip LRU for guest_memfd folios
KVM: guest_memfd: Introduce KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls
KVM: guest_memfd: Introduce and use shareability to guard faulting
KVM: guest_memfd: Make guest mem use guest mem inodes instead of anonymous inodes

but figured I'd mention it here in case there are other things to consider on the locking front.

Definitely agree with Sean though that it would be nice to start identifying a common base of patches for the in-place conversion enablement for SNP, TDX, and pKVM so the APIs/interfaces for hugepages can be handled separately.

-Mike

[1] https://lore.kernel.org/kvm/[email protected]/
[2] https://github.com/mdroth/linux/commits/mmap-swprot-v10-snp0-wip2/
[3] https://lore.kernel.org/kvm/[email protected]/

Quoted 16 lines

virt/kvm/guest_memfd.c +1 -1

 	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
 	if (IS_ERR(folio)) {
 		r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 		*page = folio_file_page(folio, index);
 	else
 		folio_put(folio);
-
 out:
+	filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
 	fput(file);
 	return r;
 }

Signature · 1 line

2.49.0.1045.g170613ef41-goog

Quoted 41 lines · Michael Roth

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 

Quoted 11 lines

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6415

Quoted 5 lines

Documentation/virt/kvm/api.rst +5 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

KVM_CAP_GMEM_CONVERSION doesn't get introduced until later, so it seems like this flag should be deferred until that patch is in place. Is it really needed at that point though? Userspace would be able to set the initial state via KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls.

I can move this change to the later patch. Thanks! Will fix in the next revision.

Quoted 9 lines

The mtree contents seems to get stored in the same manner in either case so performance-wise only the overhead of a few userspace<->kernel switches would be saved. Are there any other reasons?

Otherwise, maybe just settle on SHARED as a documented default (since at least non-CoCo VMs would be able to reliably benefit) and let CoCo/GUEST_MEMFD_FLAG_SUPPORT_SHARED VMs set PRIVATE at whatever granularity makes sense for the architecture/guest configuration.

Because shared pages are split once any memory is allocated, having a way to INIT_PRIVATE could avoid the split and then merge on conversion. I feel that is enough value to have this config flag, what do you think?

I guess we could also have userspace be careful not to do any allocation before converting.

Quoted 294 lines

Quoted 62 lines

Documentation/virt/kvm/api.rst +0 -0

 See KVM_SET_USER_MEMORY_REGION2 for additional details.
 
 4.143 KVM_PRE_FAULT_MEMORY

include/uapi/linux/kvm.h +2 -0

index 4cc824a3a7c9..d7df312479aa 100644
@@ -1567,7 +1567,9 @@ struct kvm_memory_attributes {
 #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
 
 #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
+
 #define GUEST_MEMFD_FLAG_SUPPORT_SHARED	(1UL << 0)
+#define GUEST_MEMFD_FLAG_INIT_PRIVATE	(1UL << 1)
 
 struct kvm_create_guest_memfd {
 	__u64 size;

virt/kvm/guest_memfd.c +22 -0

index 239d0f13dcc1..590932499eba 100644
@@ -4,6 +4,7 @@
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/kvm_host.h>
+#include <linux/maple_tree.h>
 #include <linux/pseudo_fs.h>
 #include <linux/pagemap.h>
 

@@ -17,6 +18,24 @@ struct kvm_gmem {
 	struct list_head entry;
 };
 
+struct kvm_gmem_inode_private {
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	struct maple_tree shareability;
+#endif
+};
+
+enum shareability {
+	SHAREABILITY_GUEST = 1,	/* Only the guest can map (fault) folios in this range. */
+	SHAREABILITY_ALL = 2,	/* Both guest and host can fault folios in this range. */
+};
+
+static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index);
+
+static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
+{
+	return inode->i_mapping->i_private_data;
+}
+
 /**
  * folio_file_pfn - like folio_file_page, but return a pfn.
  * @folio: The folio which contains this index.

@@ -29,6 +48,58 @@ static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index)
 	return folio_pfn(folio) + (index & (folio_nr_pages(folio) - 1));
 }
 
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+
+static int kvm_gmem_shareability_setup(struct kvm_gmem_inode_private *private,

virt/kvm/guest_memfd.c static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index) · line 63

Quoted 10 lines

virt/kvm/guest_memfd.c +10 -0

+				      loff_t size, u64 flags)
+{
+	enum shareability m;
+	pgoff_t last;
+
+	last = (size >> PAGE_SHIFT) - 1;
+	m = flags & GUEST_MEMFD_FLAG_INIT_PRIVATE ? SHAREABILITY_GUEST :
+						    SHAREABILITY_ALL;
+	return mtree_store_range(&private->shareability, 0, last, xa_mk_value(m),
+				 GFP_KERNEL);

One really nice thing about using a maple tree is that it should get rid of a fairly significant startup delay for SNP/TDX when the entire xarray gets initialized with private attribute entries via KVM_SET_MEMORY_ATTRIBUTES (which is the current QEMU default behavior).

I'd originally advocated for sticking with the xarray implementation Fuad was using until we'd determined we really need it for HugeTLB support, but I'm sort of thinking it's already justified just based on the above.

Maybe it would make sense for KVM memory attributes too?

Quoted 135 lines

virt/kvm/guest_memfd.c +87 -1

+}
+
+static enum shareability kvm_gmem_shareability_get(struct inode *inode,
+						 pgoff_t index)
+{
+	struct maple_tree *mt;
+	void *entry;
+
+	mt = &kvm_gmem_private(inode)->shareability;
+	entry = mtree_load(mt, index);
+	WARN(!entry,
+	     "Shareability should always be defined for all indices in inode.");
+
+	return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+		return ERR_PTR(-EACCES);
+
+	return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+	return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+	WARN_ONCE("Unexpected call to get shared folio.")
+	return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
 				    pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
 	filemap_invalidate_lock_shared(inode->i_mapping);
 
-	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+	folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
 	if (IS_ERR(folio)) {
 		int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
 	.fallocate	= kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+	kfree(private);
+
+	free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+	struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+	/*
+	 * mtree_destroy() can't be used within rcu callback, hence can't be
+	 * done in ->free_inode().
+	 */
+	if (private)
+		mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
 	.statfs		= simple_statfs,
+	.destroy_inode	= kvm_gmem_destroy_inode,
+	.free_inode	= kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 						      loff_t size, u64 flags)
 {
+	struct kvm_gmem_inode_private *private;
 	struct inode *inode;
+	int err;
 
 	inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
 	if (IS_ERR(inode))
 		return inode;
 
+	err = -ENOMEM;
+	private = kzalloc(sizeof(*private), GFP_KERNEL);
+	if (!private)
+		goto out;
+
+	mt_init(&private->shareability);
+	inode->i_mapping->i_private_data = private;
+
+	err = kvm_gmem_shareability_setup(private, size, flags);
+	if (err)
+		goto out;
+
 	inode->i_private = (void *)(unsigned long)flags;
 	inode->i_op = &kvm_gmem_iops;
 	inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
 	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
 	return inode;
+
+out:
+	iput(inode);
+
+	return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
 	if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
 		valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+	if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+		valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
 	if (flags & ~valid_flags)
 		return -EINVAL;
 

virt/kvm/guest_memfd.c int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, · line 964

Quoted 5 lines

virt/kvm/guest_memfd.c +2 -0

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 	if (!file)
 		return -EFAULT;
 
+	filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+

I like the idea of using a write-lock/read-lock to protect write/read access to shareability state (though maybe not necessarily re-using filemap's invalidate lock), it's simple and still allows concurrent faulting in of gmem pages. One issue on the SNP side (which also came up in one of the gmem calls) is if we introduce support for tracking preparedness as discussed (e.g. via a new SHAREABILITY_GUEST_PREPARED state) the SHAREABILITY_GUEST->SHAREABILITY_GUEST_PREPARED transition would occur at fault-time, and so would need to take the write-lock and no longer allow for concurrent fault-handling.

I was originally planning on introducing a new rw_semaphore with similar semantics to the rw_lock that Fuad previously had in his restricted mmap series[1] (and simiar semantics to filemap invalidate lock here). The main difference, to handle setting SHAREABILITY_GUEST_PREPARED within fault paths, was that in the case of a folio being present for an index, the folio lock would also need to be held in order to update the shareability state. Because of that, fault paths (which will always either have or allocate folio basically) can rely on the folio lock to guard shareability state in a more granular way and so can avoid a global write lock.

They would still need to hold the read lock to access the tree however. Or more specifically, any paths that could allocate a folio need to take a read lock so there isn't a TOCTOU situation where shareability is being updated for an index for which a folio hasn't been allocated, but then just afterward the folio gets faulted in/allocated while the shareability state is already being updated which the understand that there was no folio around that needed locking.

I had a branch with in-place conversion support for SNP[2] that added this lock reworking on top of Fuad's series along with preparation tracking, but I'm now planning to rebase that on top of the patches from this series that Sean mentioned[3] earlier:

KVM: guest_memfd: Add CAP KVM_CAP_GMEM_CONVERSION
KVM: Query guest_memfd for private/shared status
KVM: guest_memfd: Skip LRU for guest_memfd folios
KVM: guest_memfd: Introduce KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls
KVM: guest_memfd: Introduce and use shareability to guard faulting
KVM: guest_memfd: Make guest mem use guest mem inodes instead of anonymous inodes

but figured I'd mention it here in case there are other things to consider on the locking front.

Definitely agree with Sean though that it would be nice to start identifying a common base of patches for the in-place conversion enablement for SNP, TDX, and pKVM so the APIs/interfaces for hugepages can be handled separately.

-Mike

[1] https://lore.kernel.org/kvm/[email protected]/
[2] https://github.com/mdroth/linux/commits/mmap-swprot-v10-snp0-wip2/
[3] https://lore.kernel.org/kvm/[email protected]/

Quoted 16 lines

virt/kvm/guest_memfd.c +1 -1

 	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
 	if (IS_ERR(folio)) {
 		r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 		*page = folio_file_page(folio, index);
 	else
 		folio_put(folio);
-
 out:
+	filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
 	fput(file);
 	return r;
 }

Signature · 1 line

2.49.0.1045.g170613ef41-goog

Quoted 68 lines · Ackerley Tng

Quoted 41 lines · Michael Roth

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 

Quoted 11 lines

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6415

Quoted 5 lines

Documentation/virt/kvm/api.rst +5 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

KVM_CAP_GMEM_CONVERSION doesn't get introduced until later, so it seems like this flag should be deferred until that patch is in place. Is it really needed at that point though? Userspace would be able to set the initial state via KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls.

I can move this change to the later patch. Thanks! Will fix in the next revision.

Quoted 9 lines

The mtree contents seems to get stored in the same manner in either case so performance-wise only the overhead of a few userspace<->kernel switches would be saved. Are there any other reasons?

Otherwise, maybe just settle on SHARED as a documented default (since at least non-CoCo VMs would be able to reliably benefit) and let CoCo/GUEST_MEMFD_FLAG_SUPPORT_SHARED VMs set PRIVATE at whatever granularity makes sense for the architecture/guest configuration.

Quoted 7 lines

Because shared pages are split once any memory is allocated, having a way to INIT_PRIVATE could avoid the split and then merge on conversion. I feel that is enough value to have this config flag, what do you think?

I guess we could also have userspace be careful not to do any allocation before converting.

I assume we do want to support things like preallocating guest memory so not sure this approach is feasible to avoid splits.

But I feel like we might be working around a deeper issue here, which is that we are pre-emptively splitting anything that *could* be mapped into userspace (i.e. allocated+shared/mixed), rather than splitting when necessary.

I know that was the plan laid out in the guest_memfd calls, but I've run into a couple instances that have me thinking we should revisit this.

1) Some of the recent guest_memfd seems to be gravitating towards having userspace populate/initialize guest memory payload prior to boot via mmap()'ing the shared guest_memfd pages so things work the same as they would for initialized normal VM memory payload (rather than relying on back-channels in the kernel to user data into guest_memfd pages).

When you do this though, for an SNP guest at least, that memory acceptance is done in chunks of 4MB (with accept_memory=lazy), and because that will put each 1GB page into an allocated+mixed state,

we end up splitting every 1GB to 4K and the guest can't even accept/PVALIDATE it 2MB at that point even if userspace doesn't touch anything in the range. As some point the guest will convert/accept the entire range, at which point we could merge, but for SNP we'd need guest cooperation to actually use a higher-granularity in stage2 page tables at that point since RMP entries are effectively all split to 4K.

I understand the intent is to default to private where this wouldn't be an issue, and we could punt to userspace to deal with it, but it feels like an artificial restriction to place on userspace. And if we do want to allow/expect guest_memfd contents to be initialized pre-boot just like normal memory, then userspace would need to jump through some hoops:

- if defaulting to private: add hooks to convert each range that's being modified to a shared state prior to writing to it

- if defaulting to shared: initialize memory in-place, then covert everything else to private to avoid unecessarily splitting folios at run-time

It feels like implementations details are bleeding out into the API to some degree here (e.g. we'd probably at least need to document this so users know how to take proper advantage of hugepage support).

2) There are some use-cases for HugeTLB + CoCo that have come to my attention recently that put a lot of weight on still being able to maximize mapping/hugepage size when accessing shared mem from userspace, e.g. for certain DPDK workloads that accessed shared guest buffers from host userspace. We don't really have a story for this, and I wouldn't expect us to at this stage, but I think it ties into #1 so might be worth considering in that context.

I'm still fine with the current approach as a starting point, but I'm wondering if improving both #1/#2 might not be so bad and maybe even give us some more flexibility (for instance, Sean had mentioned leaving open the option of tracking more than just shareability/mappability, and if there is split/merge logic associated with those transitions then re-scanning each of these attributes for a 1G range seems like it could benefit from some sort of intermediate data structure to help determine things like what mapping granularity is available for guest/userspace for a particular range.

One approach I was thinking of was that we introduce a data structure similar to KVM's memslot->arch.lpage_info() where we store information about what 1G/2M ranges are shared/private/mixed, and then instead of splitting ahead of time we just record that state into this data structure (using the same write lock as with the shareability/mappability state), and then at *fault* time we split the folio if our lpage_info-like data structure says the range is mixed.

Then, if guest converts a 2M/4M range to private while lazilly-accepting (for instance), we can still keep the folio intact as 1GB, but mark the 1G range in the lpage_info-like data structure as mixed so that we still inform KVM/etc. they need to map it as 2MB or lower in stage2 page tables. In that case, even at guest fault-time, we can leave the folio unsplit until userspace tries to touch it (though in most cases it never will and we can keep most of the guest's 1G intact for the duration of its lifetime).

On the userspace side, another nice thing there is if we see 1G is in a mixed state, but 2M is all-shared, then we can still leave the folio as 2M, and I think the refcount'ing logic would still work for the most part, which makes #2 a bit easier to implement as well.

And of course, we wouldn't need the INIT_PRIVATE then since we are only splitting when necessary.

But I guess this all comes down to how much extra pain there is in tracking a 1G folio that's been split into a mixed of 2MB/4K regions, but I think we'd get a lot more mileage out of getting that working and just completely stripping out all of the merging logic for initial implementation (other than at cleanup time), so maybe complexity-wise it balances out a bit?

Thanks,

Mike

Quoted 295 lines

Quoted 294 lines

Quoted 62 lines

Documentation/virt/kvm/api.rst +0 -0

 See KVM_SET_USER_MEMORY_REGION2 for additional details.
 
 4.143 KVM_PRE_FAULT_MEMORY

include/uapi/linux/kvm.h +2 -0

index 4cc824a3a7c9..d7df312479aa 100644
@@ -1567,7 +1567,9 @@ struct kvm_memory_attributes {
 #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
 
 #define KVM_CREATE_GUEST_MEMFD    _IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
+
 #define GUEST_MEMFD_FLAG_SUPPORT_SHARED   (1UL << 0)
+#define GUEST_MEMFD_FLAG_INIT_PRIVATE     (1UL << 1)
 
 struct kvm_create_guest_memfd {
   __u64 size;

virt/kvm/guest_memfd.c +22 -0

index 239d0f13dcc1..590932499eba 100644
@@ -4,6 +4,7 @@
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/kvm_host.h>
+#include <linux/maple_tree.h>
 #include <linux/pseudo_fs.h>
 #include <linux/pagemap.h>
 

@@ -17,6 +18,24 @@ struct kvm_gmem {
   struct list_head entry;
 };
 
+struct kvm_gmem_inode_private {
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+  struct maple_tree shareability;
+#endif
+};
+
+enum shareability {
+  SHAREABILITY_GUEST = 1, /* Only the guest can map (fault) folios in this range. */
+  SHAREABILITY_ALL = 2,   /* Both guest and host can fault folios in this range. */
+};
+
+static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index);
+
+static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
+{
+  return inode->i_mapping->i_private_data;
+}
+
 /**
  * folio_file_pfn - like folio_file_page, but return a pfn.
  * @folio: The folio which contains this index.

@@ -29,6 +48,58 @@ static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index)
   return folio_pfn(folio) + (index & (folio_nr_pages(folio) - 1));
 }
 
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+
+static int kvm_gmem_shareability_setup(struct kvm_gmem_inode_private *private,

virt/kvm/guest_memfd.c static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index) · line 63

Quoted 10 lines

virt/kvm/guest_memfd.c +10 -0

+                                loff_t size, u64 flags)
+{
+  enum shareability m;
+  pgoff_t last;
+
+  last = (size >> PAGE_SHIFT) - 1;
+  m = flags & GUEST_MEMFD_FLAG_INIT_PRIVATE ? SHAREABILITY_GUEST :
+                                              SHAREABILITY_ALL;
+  return mtree_store_range(&private->shareability, 0, last, xa_mk_value(m),
+                           GFP_KERNEL);

One really nice thing about using a maple tree is that it should get rid of a fairly significant startup delay for SNP/TDX when the entire xarray gets initialized with private attribute entries via KVM_SET_MEMORY_ATTRIBUTES (which is the current QEMU default behavior).

I'd originally advocated for sticking with the xarray implementation Fuad was using until we'd determined we really need it for HugeTLB support, but I'm sort of thinking it's already justified just based on the above.

Maybe it would make sense for KVM memory attributes too?

Quoted 135 lines

virt/kvm/guest_memfd.c +87 -1

+}
+
+static enum shareability kvm_gmem_shareability_get(struct inode *inode,
+                                           pgoff_t index)
+{
+  struct maple_tree *mt;
+  void *entry;
+
+  mt = &kvm_gmem_private(inode)->shareability;
+  entry = mtree_load(mt, index);
+  WARN(!entry,
+       "Shareability should always be defined for all indices in inode.");
+
+  return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+  if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+          return ERR_PTR(-EACCES);
+
+  return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+  return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+  WARN_ONCE("Unexpected call to get shared folio.")
+  return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
                               pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
   filemap_invalidate_lock_shared(inode->i_mapping);
 
-  folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+  folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
   if (IS_ERR(folio)) {
           int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
   .fallocate      = kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+  struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+  kfree(private);
+
+  free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+  struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+  /*
+   * mtree_destroy() can't be used within rcu callback, hence can't be
+   * done in ->free_inode().
+   */
+  if (private)
+          mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
   .statfs         = simple_statfs,
+  .destroy_inode  = kvm_gmem_destroy_inode,
+  .free_inode     = kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
                                                 loff_t size, u64 flags)
 {
+  struct kvm_gmem_inode_private *private;
   struct inode *inode;
+  int err;
 
   inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
   if (IS_ERR(inode))
           return inode;
 
+  err = -ENOMEM;
+  private = kzalloc(sizeof(*private), GFP_KERNEL);
+  if (!private)
+          goto out;
+
+  mt_init(&private->shareability);
+  inode->i_mapping->i_private_data = private;
+
+  err = kvm_gmem_shareability_setup(private, size, flags);
+  if (err)
+          goto out;
+
   inode->i_private = (void *)(unsigned long)flags;
   inode->i_op = &kvm_gmem_iops;
   inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
   WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
   return inode;
+
+out:
+  iput(inode);
+
+  return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
   if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
           valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+  if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+          valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
   if (flags & ~valid_flags)
           return -EINVAL;
 

virt/kvm/guest_memfd.c int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, · line 964

Quoted 5 lines

virt/kvm/guest_memfd.c +2 -0

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
   if (!file)
           return -EFAULT;
 
+  filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+

I like the idea of using a write-lock/read-lock to protect write/read access to shareability state (though maybe not necessarily re-using filemap's invalidate lock), it's simple and still allows concurrent faulting in of gmem pages. One issue on the SNP side (which also came up in one of the gmem calls) is if we introduce support for tracking preparedness as discussed (e.g. via a new SHAREABILITY_GUEST_PREPARED state) the SHAREABILITY_GUEST->SHAREABILITY_GUEST_PREPARED transition would occur at fault-time, and so would need to take the write-lock and no longer allow for concurrent fault-handling.

I was originally planning on introducing a new rw_semaphore with similar semantics to the rw_lock that Fuad previously had in his restricted mmap series[1] (and simiar semantics to filemap invalidate lock here). The main difference, to handle setting SHAREABILITY_GUEST_PREPARED within fault paths, was that in the case of a folio being present for an index, the folio lock would also need to be held in order to update the shareability state. Because of that, fault paths (which will always either have or allocate folio basically) can rely on the folio lock to guard shareability state in a more granular way and so can avoid a global write lock.

They would still need to hold the read lock to access the tree however. Or more specifically, any paths that could allocate a folio need to take a read lock so there isn't a TOCTOU situation where shareability is being updated for an index for which a folio hasn't been allocated, but then just afterward the folio gets faulted in/allocated while the shareability state is already being updated which the understand that there was no folio around that needed locking.

I had a branch with in-place conversion support for SNP[2] that added this lock reworking on top of Fuad's series along with preparation tracking, but I'm now planning to rebase that on top of the patches from this series that Sean mentioned[3] earlier:

KVM: guest_memfd: Add CAP KVM_CAP_GMEM_CONVERSION
KVM: Query guest_memfd for private/shared status
KVM: guest_memfd: Skip LRU for guest_memfd folios
KVM: guest_memfd: Introduce KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls
KVM: guest_memfd: Introduce and use shareability to guard faulting
KVM: guest_memfd: Make guest mem use guest mem inodes instead of anonymous inodes

but figured I'd mention it here in case there are other things to consider on the locking front.

Definitely agree with Sean though that it would be nice to start identifying a common base of patches for the in-place conversion enablement for SNP, TDX, and pKVM so the APIs/interfaces for hugepages can be handled separately.

-Mike

[1] https://lore.kernel.org/kvm/[email protected]/
[2] https://github.com/mdroth/linux/commits/mmap-swprot-v10-snp0-wip2/
[3] https://lore.kernel.org/kvm/[email protected]/

Quoted 16 lines

virt/kvm/guest_memfd.c +1 -2

   folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
   if (IS_ERR(folio)) {
           r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
           *page = folio_file_page(folio, index);
   else
           folio_put(folio);
-
 out:
+  filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
   fput(file);
   return r;
 }
--

2.49.0.1045.g170613ef41-goog

...

Quoted 8 lines

2) There are some use-cases for HugeTLB + CoCo that have come to my attention recently that put a lot of weight on still being able to maximize mapping/hugepage size when accessing shared mem from userspace, e.g. for certain DPDK workloads that accessed shared guest buffers from host userspace. We don't really have a story for this, and I wouldn't expect us to at this stage, but I think it ties into #1 so might be worth considering in that context.

Major problem I see here is that if anything in the kernel does a GUP on shared memory ranges (which is very likely to happen), it would be difficult to get them to let go of the whole hugepage before it can be split safely.

Quoted 70 lines · Michael Roth

Quoted 68 lines · Ackerley Tng

Quoted 41 lines · Michael Roth

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 

Quoted 11 lines

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6415

Quoted 5 lines

Documentation/virt/kvm/api.rst +5 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

KVM_CAP_GMEM_CONVERSION doesn't get introduced until later, so it seems like this flag should be deferred until that patch is in place. Is it really needed at that point though? Userspace would be able to set the initial state via KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls.

I can move this change to the later patch. Thanks! Will fix in the next revision.

Quoted 9 lines

The mtree contents seems to get stored in the same manner in either case so performance-wise only the overhead of a few userspace<->kernel switches would be saved. Are there any other reasons?

Otherwise, maybe just settle on SHARED as a documented default (since at least non-CoCo VMs would be able to reliably benefit) and let CoCo/GUEST_MEMFD_FLAG_SUPPORT_SHARED VMs set PRIVATE at whatever granularity makes sense for the architecture/guest configuration.

Quoted 7 lines

Quoted 7 lines

Because shared pages are split once any memory is allocated, having a way to INIT_PRIVATE could avoid the split and then merge on conversion. I feel that is enough value to have this config flag, what do you think?

I guess we could also have userspace be careful not to do any allocation before converting.

Quoted 19 lines

I assume we do want to support things like preallocating guest memory so not sure this approach is feasible to avoid splits.

But I feel like we might be working around a deeper issue here, which is that we are pre-emptively splitting anything that *could* be mapped into userspace (i.e. allocated+shared/mixed), rather than splitting when necessary.

I know that was the plan laid out in the guest_memfd calls, but I've run into a couple instances that have me thinking we should revisit this.

1) Some of the recent guest_memfd seems to be gravitating towards having userspace populate/initialize guest memory payload prior to boot via mmap()'ing the shared guest_memfd pages so things work the same as they would for initialized normal VM memory payload (rather than relying on back-channels in the kernel to user data into guest_memfd pages).

Quoted 3 lines

When you do this though, for an SNP guest at least, that memory acceptance is done in chunks of 4MB (with accept_memory=lazy), and because that will put each 1GB page into an allocated+mixed state,

I would like your help in understanding why we need to start guest_memfd ranges as shared for SNP guests. guest_memfd ranges being private simply should mean that certain ranges are not faultable by the userspace.

Will following work?
1) Userspace starts all guest_memfd ranges as private.
2) During early guest boot it starts issuing PSC requests for converting memory from shared to private

    -> KVM forwards this request to userspace
    -> Userspace checks that the pages are already private and simply

does nothing.
3) Pvalidate from guest on that memory will result in guest_memfd offset query which will cause the RMP table entries to actually get populated.

Quoted 16 lines

we end up splitting every 1GB to 4K and the guest can't even accept/PVALIDATE it 2MB at that point even if userspace doesn't touch anything in the range. As some point the guest will convert/accept the entire range, at which point we could merge, but for SNP we'd need guest cooperation to actually use a higher-granularity in stage2 page tables at that point since RMP entries are effectively all split to 4K.

I understand the intent is to default to private where this wouldn't be an issue, and we could punt to userspace to deal with it, but it feels like an artificial restriction to place on userspace. And if we do want to allow/expect guest_memfd contents to be initialized pre-boot just like normal memory, then userspace would need to jump through some hoops:

Quoted 2 lines

- if defaulting to private: add hooks to convert each range that's being modified to a shared state prior to writing to it

Why is that a problem?

Quoted 8 lines

- if defaulting to shared: initialize memory in-place, then covert everything else to private to avoid unecessarily splitting folios at run-time

It feels like implementations details are bleeding out into the API to some degree here (e.g. we'd probably at least need to document this so users know how to take proper advantage of hugepage support).

Does it make sense to keep the default behavior as INIT_PRIVATE for SNP VMs always even without using hugepages?

Quoted 9 lines

2) There are some use-cases for HugeTLB + CoCo that have come to my attention recently that put a lot of weight on still being able to maximize mapping/hugepage size when accessing shared mem from userspace, e.g. for certain DPDK workloads that accessed shared guest buffers from host userspace. We don't really have a story for this, and I wouldn't expect us to at this stage, but I think it ties into #1 so might be worth considering in that context.

Major problem I see here is that if anything in the kernel does a GUP on shared memory ranges (which is very likely to happen), it would be difficult to get them to let go of the whole hugepage before it can be split safely.

Another problem is guest_memfd today doesn't support management of large user space page table mappings, this can turnout to be significant work to do referring to hugetlb pagetable management logic.

Quoted 343 lines

I'm still fine with the current approach as a starting point, but I'm wondering if improving both #1/#2 might not be so bad and maybe even give us some more flexibility (for instance, Sean had mentioned leaving open the option of tracking more than just shareability/mappability, and if there is split/merge logic associated with those transitions then re-scanning each of these attributes for a 1G range seems like it could benefit from some sort of intermediate data structure to help determine things like what mapping granularity is available for guest/userspace for a particular range.

One approach I was thinking of was that we introduce a data structure similar to KVM's memslot->arch.lpage_info() where we store information about what 1G/2M ranges are shared/private/mixed, and then instead of splitting ahead of time we just record that state into this data structure (using the same write lock as with the shareability/mappability state), and then at *fault* time we split the folio if our lpage_info-like data structure says the range is mixed.

Then, if guest converts a 2M/4M range to private while lazilly-accepting (for instance), we can still keep the folio intact as 1GB, but mark the 1G range in the lpage_info-like data structure as mixed so that we still inform KVM/etc. they need to map it as 2MB or lower in stage2 page tables. In that case, even at guest fault-time, we can leave the folio unsplit until userspace tries to touch it (though in most cases it never will and we can keep most of the guest's 1G intact for the duration of its lifetime).

On the userspace side, another nice thing there is if we see 1G is in a mixed state, but 2M is all-shared, then we can still leave the folio as 2M, and I think the refcount'ing logic would still work for the most part, which makes #2 a bit easier to implement as well.

And of course, we wouldn't need the INIT_PRIVATE then since we are only splitting when necessary.

But I guess this all comes down to how much extra pain there is in tracking a 1G folio that's been split into a mixed of 2MB/4K regions, but I think we'd get a lot more mileage out of getting that working and just completely stripping out all of the merging logic for initial implementation (other than at cleanup time), so maybe complexity-wise it balances out a bit?

Thanks,

Mike

Quoted 295 lines

Quoted 294 lines

Quoted 62 lines

Documentation/virt/kvm/api.rst +0 -0

 See KVM_SET_USER_MEMORY_REGION2 for additional details.
 
 4.143 KVM_PRE_FAULT_MEMORY

include/uapi/linux/kvm.h +2 -0

index 4cc824a3a7c9..d7df312479aa 100644
@@ -1567,7 +1567,9 @@ struct kvm_memory_attributes {
 #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
 
 #define KVM_CREATE_GUEST_MEMFD    _IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
+
 #define GUEST_MEMFD_FLAG_SUPPORT_SHARED   (1UL << 0)
+#define GUEST_MEMFD_FLAG_INIT_PRIVATE     (1UL << 1)
 
 struct kvm_create_guest_memfd {
   __u64 size;

virt/kvm/guest_memfd.c +22 -0

index 239d0f13dcc1..590932499eba 100644
@@ -4,6 +4,7 @@
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/kvm_host.h>
+#include <linux/maple_tree.h>
 #include <linux/pseudo_fs.h>
 #include <linux/pagemap.h>
 

@@ -17,6 +18,24 @@ struct kvm_gmem {
   struct list_head entry;
 };
 
+struct kvm_gmem_inode_private {
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+  struct maple_tree shareability;
+#endif
+};
+
+enum shareability {
+  SHAREABILITY_GUEST = 1, /* Only the guest can map (fault) folios in this range. */
+  SHAREABILITY_ALL = 2,   /* Both guest and host can fault folios in this range. */
+};
+
+static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index);
+
+static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
+{
+  return inode->i_mapping->i_private_data;
+}
+
 /**
  * folio_file_pfn - like folio_file_page, but return a pfn.
  * @folio: The folio which contains this index.

@@ -29,6 +48,58 @@ static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index)
   return folio_pfn(folio) + (index & (folio_nr_pages(folio) - 1));
 }
 
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+
+static int kvm_gmem_shareability_setup(struct kvm_gmem_inode_private *private,

virt/kvm/guest_memfd.c static inline kvm_pfn_t folio_file_pfn(struct folio *folio, pgoff_t index) · line 63

Quoted 10 lines

virt/kvm/guest_memfd.c +10 -0

+                                loff_t size, u64 flags)
+{
+  enum shareability m;
+  pgoff_t last;
+
+  last = (size >> PAGE_SHIFT) - 1;
+  m = flags & GUEST_MEMFD_FLAG_INIT_PRIVATE ? SHAREABILITY_GUEST :
+                                              SHAREABILITY_ALL;
+  return mtree_store_range(&private->shareability, 0, last, xa_mk_value(m),
+                           GFP_KERNEL);

One really nice thing about using a maple tree is that it should get rid of a fairly significant startup delay for SNP/TDX when the entire xarray gets initialized with private attribute entries via KVM_SET_MEMORY_ATTRIBUTES (which is the current QEMU default behavior).

I'd originally advocated for sticking with the xarray implementation Fuad was using until we'd determined we really need it for HugeTLB support, but I'm sort of thinking it's already justified just based on the above.

Maybe it would make sense for KVM memory attributes too?

Quoted 135 lines

virt/kvm/guest_memfd.c +87 -1

+}
+
+static enum shareability kvm_gmem_shareability_get(struct inode *inode,
+                                           pgoff_t index)
+{
+  struct maple_tree *mt;
+  void *entry;
+
+  mt = &kvm_gmem_private(inode)->shareability;
+  entry = mtree_load(mt, index);
+  WARN(!entry,
+       "Shareability should always be defined for all indices in inode.");
+
+  return xa_to_value(entry);
+}
+
+static struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+  if (kvm_gmem_shareability_get(inode, index) != SHAREABILITY_ALL)
+          return ERR_PTR(-EACCES);
+
+  return kvm_gmem_get_folio(inode, index);
+}
+
+#else
+
+static int kvm_gmem_shareability_setup(struct maple_tree *mt, loff_t size, u64 flags)
+{
+  return 0;
+}
+
+static inline struct folio *kvm_gmem_get_shared_folio(struct inode *inode, pgoff_t index)
+{
+  WARN_ONCE("Unexpected call to get shared folio.")
+  return NULL;
+}
+
+#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
+
 static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
                               pgoff_t index, struct folio *folio)
 {

@@ -333,7 +404,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
 
   filemap_invalidate_lock_shared(inode->i_mapping);
 
-  folio = kvm_gmem_get_folio(inode, vmf->pgoff);
+  folio = kvm_gmem_get_shared_folio(inode, vmf->pgoff);
   if (IS_ERR(folio)) {
           int err = PTR_ERR(folio);
 

@@ -420,8 +491,33 @@ static struct file_operations kvm_gmem_fops = {
   .fallocate      = kvm_gmem_fallocate,
 };
 
+static void kvm_gmem_free_inode(struct inode *inode)
+{
+  struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+  kfree(private);
+
+  free_inode_nonrcu(inode);
+}
+
+static void kvm_gmem_destroy_inode(struct inode *inode)
+{
+  struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
+
+#ifdef CONFIG_KVM_GMEM_SHARED_MEM
+  /*
+   * mtree_destroy() can't be used within rcu callback, hence can't be
+   * done in ->free_inode().
+   */
+  if (private)
+          mtree_destroy(&private->shareability);
+#endif
+}
+
 static const struct super_operations kvm_gmem_super_operations = {
   .statfs         = simple_statfs,
+  .destroy_inode  = kvm_gmem_destroy_inode,
+  .free_inode     = kvm_gmem_free_inode,
 };
 
 static int kvm_gmem_init_fs_context(struct fs_context *fc)

@@ -549,12 +645,26 @@ static const struct inode_operations kvm_gmem_iops = {
 static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
                                                 loff_t size, u64 flags)
 {
+  struct kvm_gmem_inode_private *private;
   struct inode *inode;
+  int err;
 
   inode = alloc_anon_secure_inode(kvm_gmem_mnt->mnt_sb, name);
   if (IS_ERR(inode))
           return inode;
 
+  err = -ENOMEM;
+  private = kzalloc(sizeof(*private), GFP_KERNEL);
+  if (!private)
+          goto out;
+
+  mt_init(&private->shareability);
+  inode->i_mapping->i_private_data = private;
+
+  err = kvm_gmem_shareability_setup(private, size, flags);
+  if (err)
+          goto out;
+
   inode->i_private = (void *)(unsigned long)flags;
   inode->i_op = &kvm_gmem_iops;
   inode->i_mapping->a_ops = &kvm_gmem_aops;

@@ -566,6 +676,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
   WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
 
   return inode;
+
+out:
+  iput(inode);
+
+  return ERR_PTR(err);
 }
 
 static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

@@ -654,6 +769,9 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
   if (kvm_arch_vm_supports_gmem_shared_mem(kvm))
           valid_flags |= GUEST_MEMFD_FLAG_SUPPORT_SHARED;
 
+  if (flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED)
+          valid_flags |= GUEST_MEMFD_FLAG_INIT_PRIVATE;
+
   if (flags & ~valid_flags)
           return -EINVAL;
 

virt/kvm/guest_memfd.c int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, · line 964

Quoted 5 lines

virt/kvm/guest_memfd.c +2 -0

@@ -842,6 +960,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
   if (!file)
           return -EFAULT;
 
+  filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
+

I like the idea of using a write-lock/read-lock to protect write/read access to shareability state (though maybe not necessarily re-using filemap's invalidate lock), it's simple and still allows concurrent faulting in of gmem pages. One issue on the SNP side (which also came up in one of the gmem calls) is if we introduce support for tracking preparedness as discussed (e.g. via a new SHAREABILITY_GUEST_PREPARED state) the SHAREABILITY_GUEST->SHAREABILITY_GUEST_PREPARED transition would occur at fault-time, and so would need to take the write-lock and no longer allow for concurrent fault-handling.

I was originally planning on introducing a new rw_semaphore with similar semantics to the rw_lock that Fuad previously had in his restricted mmap series[1] (and simiar semantics to filemap invalidate lock here). The main difference, to handle setting SHAREABILITY_GUEST_PREPARED within fault paths, was that in the case of a folio being present for an index, the folio lock would also need to be held in order to update the shareability state. Because of that, fault paths (which will always either have or allocate folio basically) can rely on the folio lock to guard shareability state in a more granular way and so can avoid a global write lock.

They would still need to hold the read lock to access the tree however. Or more specifically, any paths that could allocate a folio need to take a read lock so there isn't a TOCTOU situation where shareability is being updated for an index for which a folio hasn't been allocated, but then just afterward the folio gets faulted in/allocated while the shareability state is already being updated which the understand that there was no folio around that needed locking.

I had a branch with in-place conversion support for SNP[2] that added this lock reworking on top of Fuad's series along with preparation tracking, but I'm now planning to rebase that on top of the patches from this series that Sean mentioned[3] earlier:

KVM: guest_memfd: Add CAP KVM_CAP_GMEM_CONVERSION
KVM: Query guest_memfd for private/shared status
KVM: guest_memfd: Skip LRU for guest_memfd folios
KVM: guest_memfd: Introduce KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls
KVM: guest_memfd: Introduce and use shareability to guard faulting
KVM: guest_memfd: Make guest mem use guest mem inodes instead of anonymous inodes

but figured I'd mention it here in case there are other things to consider on the locking front.

Definitely agree with Sean though that it would be nice to start identifying a common base of patches for the in-place conversion enablement for SNP, TDX, and pKVM so the APIs/interfaces for hugepages can be handled separately.

-Mike

[1] https://lore.kernel.org/kvm/[email protected]/
[2] https://github.com/mdroth/linux/commits/mmap-swprot-v10-snp0-wip2/
[3] https://lore.kernel.org/kvm/[email protected]/

Quoted 16 lines

virt/kvm/guest_memfd.c +1 -2

   folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
   if (IS_ERR(folio)) {
           r = PTR_ERR(folio);

@@ -857,8 +977,8 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
           *page = folio_file_page(folio, index);
   else
           folio_put(folio);
-
 out:
+  filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
   fput(file);
   return r;
 }
--

2.49.0.1045.g170613ef41-goog

Quoted 76 lines · Vishal Annapurve

Quoted 75 lines · Michael Roth

Quoted 73 lines · Ackerley Tng

Quoted 41 lines · Michael Roth

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 

Quoted 11 lines

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6415

Quoted 5 lines

Documentation/virt/kvm/api.rst +5 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

KVM_CAP_GMEM_CONVERSION doesn't get introduced until later, so it seems like this flag should be deferred until that patch is in place. Is it really needed at that point though? Userspace would be able to set the initial state via KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls.

I can move this change to the later patch. Thanks! Will fix in the next revision.

Quoted 9 lines

The mtree contents seems to get stored in the same manner in either case so performance-wise only the overhead of a few userspace<->kernel switches would be saved. Are there any other reasons?

Otherwise, maybe just settle on SHARED as a documented default (since at least non-CoCo VMs would be able to reliably benefit) and let CoCo/GUEST_MEMFD_FLAG_SUPPORT_SHARED VMs set PRIVATE at whatever granularity makes sense for the architecture/guest configuration.

Because shared pages are split once any memory is allocated, having a way to INIT_PRIVATE could avoid the split and then merge on conversion. I feel that is enough value to have this config flag, what do you think?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I guess we could also have userspace be careful not to do any allocation before converting.

(Re-visiting this with the assumption that we *don't* intend to use mmap() to populate memory (in which case you can pretty much ignore my previous response))

I'm still not sure where the INIT_PRIVATE flag comes into play. For SNP, userspace already defaults to marking everything private pretty close to guest_memfd creation time, so the potential for allocations to occur in-between seems small, but worth confirming.

But I know in the past there was a desire to ensure TDX/SNP could support pre-allocating guest_memfd memory (and even pre-faulting via KVM_PRE_FAULT_MEMORY), but I think that could still work right? The fallocate() handling could still avoid the split if the whole hugepage is private, though there is a bit more potential for that fallocate() to happen before userspace does the "manually" shared->private conversion. I'll double-check on that aspect, but otherwise, is there still any other need for it?

Quoted 20 lines

Quoted 20 lines

I assume we do want to support things like preallocating guest memory so not sure this approach is feasible to avoid splits.

But I feel like we might be working around a deeper issue here, which is that we are pre-emptively splitting anything that *could* be mapped into userspace (i.e. allocated+shared/mixed), rather than splitting when necessary.

I know that was the plan laid out in the guest_memfd calls, but I've run into a couple instances that have me thinking we should revisit this.

1) Some of the recent guest_memfd seems to be gravitating towards having userspace populate/initialize guest memory payload prior to boot via mmap()'ing the shared guest_memfd pages so things work the same as they would for initialized normal VM memory payload (rather than relying on back-channels in the kernel to user data into guest_memfd pages).

Quoted 8 lines

Quoted 3 lines

When you do this though, for an SNP guest at least, that memory acceptance is done in chunks of 4MB (with accept_memory=lazy), and because that will put each 1GB page into an allocated+mixed state,

I would like your help in understanding why we need to start guest_memfd ranges as shared for SNP guests. guest_memfd ranges being private simply should mean that certain ranges are not faultable by the userspace.

It's seeming like I probably misremembered, but I thought there was a discussion on guest_memfd call a month (or so?) ago about whether to continue to use backchannels to populate guest_memfd pages prior to launch. It was in the context of whether to keep using kvm_gmem_populate() for populating guest_memfd pages by copying them in from separate userspace buffer vs. simply populating them directly from userspace. I thought we were leaning on the latter since it was simpler all-around, which is great for SNP since that is already how it populates memory: by writing to it from userspace, which kvm_gmem_populate() then copies into guest_memfd pages. With shared gmem support, we just skip the latter now in the kernel rather needing changes to how userspace handles things in that regard. But maybe that was just wishful thinking :)

But you raise some very compelling points on why this might not be a good idea even if that was how that discussion went.

Quoted 11 lines

Will following work?
1) Userspace starts all guest_memfd ranges as private.
2) During early guest boot it starts issuing PSC requests for converting memory from shared to private

    -> KVM forwards this request to userspace
    -> Userspace checks that the pages are already private and simply

does nothing.
3) Pvalidate from guest on that memory will result in guest_memfd offset query which will cause the RMP table entries to actually get populated.

That would work, but there will need to be changes on userspace to deal with how SNP populates memory pre-boot just like normal VMs do. We will instead need to copy that data into separate buffers, and pass those in as the buffer hva instead of the shared hva corresponding to that GPA.

But that seems reasonable if it avoids so many other problems.

Quoted 19 lines

Quoted 18 lines

we end up splitting every 1GB to 4K and the guest can't even accept/PVALIDATE it 2MB at that point even if userspace doesn't touch anything in the range. As some point the guest will convert/accept the entire range, at which point we could merge, but for SNP we'd need guest cooperation to actually use a higher-granularity in stage2 page tables at that point since RMP entries are effectively all split to 4K.

I understand the intent is to default to private where this wouldn't be an issue, and we could punt to userspace to deal with it, but it feels like an artificial restriction to place on userspace. And if we do want to allow/expect guest_memfd contents to be initialized pre-boot just like normal memory, then userspace would need to jump through some hoops:

- if defaulting to private: add hooks to convert each range that's being modified to a shared state prior to writing to it

Quoted 1 line

Why is that a problem?

These were only problems if we went the above-mentioned way of populating memory pre-boot via mmap() instead of other backchannels. If we don't do that, then both these things cease to be problems. Sounds goods to me. :)

Quoted 78 lines · Michael Roth

Quoted 76 lines · Vishal Annapurve

Quoted 75 lines · Michael Roth

Quoted 73 lines · Ackerley Tng

Quoted 41 lines · Michael Roth

Quoted 40 lines · Ackerley Tng

Track guest_memfd memory's shareability status within the inode as opposed to the file, since it is property of the guest_memfd's memory contents.

Shareability is a property of the memory and is indexed using the page's index in the inode. Because shareability is the memory's property, it is stored within guest_memfd instead of within KVM, like in kvm->mem_attr_array.

KVM_MEMORY_ATTRIBUTE_PRIVATE in kvm->mem_attr_array must still be retained to allow VMs to only use guest_memfd for private memory and some other memory for shared memory.

Not all use cases require guest_memfd() to be shared with the host when first created. Add a new flag, GUEST_MEMFD_FLAG_INIT_PRIVATE, which when set on KVM_CREATE_GUEST_MEMFD, initializes the memory as private to the guest, and therefore not mappable by the host. Otherwise, memory is shared until explicitly converted to private.

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Vishal Annapurve <[email protected]>

Signed-off-by Vishal Annapurve <[email protected]>

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

Change-Id If03609cbab3ad1564685c85bdba6dcbb6b240c0f

---
 Documentation/virt/kvm/api.rst |   5 ++
 include/uapi/linux/kvm.h       |   2 +
 virt/kvm/guest_memfd.c         | 124 ++++++++++++++++++++++++++++++++-
 3 files changed, 129 insertions(+), 2 deletions(-)

Documentation/virt/kvm/api.rst +0 -0

index 86f74ce7f12a..f609337ae1c2 100644
@@ -6408,6 +6408,11 @@ belonging to the slot via its userspace_addr.
 The use of GUEST_MEMFD_FLAG_SUPPORT_SHARED will not be allowed for CoCo VMs.
 This is validated when the guest_memfd instance is bound to the VM.
 

Quoted 11 lines

Documentation/virt/kvm/api.rst belonging to the slot via its userspace_addr. · line 6415

Quoted 5 lines

Documentation/virt/kvm/api.rst +5 -0

+If the capability KVM_CAP_GMEM_CONVERSIONS is supported, then the 'flags' field
+supports GUEST_MEMFD_FLAG_INIT_PRIVATE.  Setting GUEST_MEMFD_FLAG_INIT_PRIVATE
+will initialize the memory for the guest_memfd as guest-only and not faultable
+by the host.
+

KVM_CAP_GMEM_CONVERSION doesn't get introduced until later, so it seems like this flag should be deferred until that patch is in place. Is it really needed at that point though? Userspace would be able to set the initial state via KVM_GMEM_CONVERT_SHARED/PRIVATE ioctls.

I can move this change to the later patch. Thanks! Will fix in the next revision.

Quoted 9 lines

The mtree contents seems to get stored in the same manner in either case so performance-wise only the overhead of a few userspace<->kernel switches would be saved. Are there any other reasons?

Otherwise, maybe just settle on SHARED as a documented default (since at least non-CoCo VMs would be able to reliably benefit) and let CoCo/GUEST_MEMFD_FLAG_SUPPORT_SHARED VMs set PRIVATE at whatever granularity makes sense for the architecture/guest configuration.

Because shared pages are split once any memory is allocated, having a way to INIT_PRIVATE could avoid the split and then merge on conversion. I feel that is enough value to have this config flag, what do you think?

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I guess we could also have userspace be careful not to do any allocation before converting.

(Re-visiting this with the assumption that we *don't* intend to use mmap() to populate memory (in which case you can pretty much ignore my previous response))

I am assuming in-place conversion with huge page backing for the discussion below.

Looks like there are three scenarios/usecases we are discussing here:
1) Pre-allocating guest_memfd file offsets
- Userspace can use fallocate to do this for hugepages by keeping
the file ranges marked private.
2) Prefaulting guest EPT/NPT entries
3) Populating initial guest payload into guest_memfd memory
- Userspace can mark certain ranges as shared, populate the
contents and convert the ranges back to private. So mmap will come in handy here.

Quoted 5 lines

I'm still not sure where the INIT_PRIVATE flag comes into play. For SNP, userspace already defaults to marking everything private pretty close to guest_memfd creation time, so the potential for allocations to occur in-between seems small, but worth confirming.

Ok, I am not much worried about whether the INIT_PRIVATE flag gets supported or not, but more about the default setting that different CVMs start with. To me, it looks like all CVMs should start as everything private by default and if there is a way to bake that configuration during guest_memfd creation time that would be good to have instead of doing "create and convert" operations and there is a fairly low cost to support this flag.

Quoted 9 lines

But I know in the past there was a desire to ensure TDX/SNP could support pre-allocating guest_memfd memory (and even pre-faulting via KVM_PRE_FAULT_MEMORY), but I think that could still work right? The fallocate() handling could still avoid the split if the whole hugepage is private, though there is a bit more potential for that fallocate() to happen before userspace does the "manually" shared->private conversion. I'll double-check on that aspect, but otherwise, is there still any other need for it?