Re: [PATCH kvm-next V11 4/7] KVM: guest_memfd: Use guest mem inodes instead of anonymous inodes

On 25.09.25 13:44, Garg, Shivank wrote:

On 9/25/2025 8:20 AM, Sean Christopherson wrote:

...

My apologies for the super late feedback. None of this is critical (mechanical things that can be cleaned up after the fact), so if there's any urgency to getting this series into 6.18, just ignore it.

On Wed, Aug 27, 2025, Ackerley Tng wrote:

...

Shivank Garg shivankg@amd.com writes: @@ -463,11 +502,70 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm) return true; }

+static struct inode *kvm_gmem_inode_create(const char *name, loff_t size,

• 			   u64 flags)
  

+{

• struct inode *inode;
• inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
• if (IS_ERR(inode))

• return inode;
  

• inode->i_private = (void *)(unsigned long)flags;
• inode->i_op = &kvm_gmem_iops;
• inode->i_mapping->a_ops = &kvm_gmem_aops;
• inode->i_mode |= S_IFREG;
• inode->i_size = size;
• mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
• mapping_set_inaccessible(inode->i_mapping);
• /* Unmovable mappings are supposed to be marked unevictable as well. */
• WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
• return inode;

+}

+static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,

• 				  u64 flags)
  

+{

• static const char *name = "[kvm-gmem]";
• struct inode *inode;
• struct file *file;
• int err;
• err = -ENOENT;
• /* __fput() will take care of fops_put(). */
• if (!fops_get(&kvm_gmem_fops))

• goto err;
  

• inode = kvm_gmem_inode_create(name, size, flags);
• if (IS_ERR(inode)) {

• err = PTR_ERR(inode);
  

• goto err_fops_put;
  

• }
• file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,

• 		 &kvm_gmem_fops);
  

• if (IS_ERR(file)) {

• err = PTR_ERR(file);
  

• goto err_put_inode;
  

• }
• file->f_flags |= O_LARGEFILE;
• file->private_data = priv;
• return file;

+err_put_inode:

• iput(inode);

+err_fops_put:

• fops_put(&kvm_gmem_fops);

+err:

• return ERR_PTR(err);

+}

I don't see any reason to add two helpers. It requires quite a bit more lines of code due to adding more error paths and local variables, and IMO doesn't make the code any easier to read.

Passing in "gmem" as @priv is especially ridiculous, as it adds code and obfuscates what file->private_data is set to.

I get the sense that the code was written to be a "replacement" for common APIs, but that is nonsensical (no pun intended).

...

static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags) {

• const char *anon_name = "[kvm-gmem]"; struct kvm_gmem *gmem;
• struct inode *inode; struct file *file; int fd, err;

@@ -481,32 +579,16 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags) goto err_fd; }

• file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,

• 			 O_RDWR, NULL);
  

• file = kvm_gmem_inode_create_getfile(gmem, size, flags); if (IS_ERR(file)) { err = PTR_ERR(file); goto err_gmem; }

• file->f_flags |= O_LARGEFILE;
• inode = file->f_inode;
• WARN_ON(file->f_mapping != inode->i_mapping);
• inode->i_private = (void *)(unsigned long)flags;
• inode->i_op = &kvm_gmem_iops;
• inode->i_mapping->a_ops = &kvm_gmem_aops;
• inode->i_mode |= S_IFREG;
• inode->i_size = size;
• mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
• mapping_set_inaccessible(inode->i_mapping);
• /* Unmovable mappings are supposed to be marked unevictable as well. */
• WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
• kvm_get_kvm(kvm); gmem->kvm = kvm; xa_init(&gmem->bindings);
• list_add(&gmem->entry, &inode->i_mapping->i_private_list);

• list_add(&gmem->entry, &file_inode(file)->i_mapping->i_private_list);

I don't understand this change? Isn't file_inode(file) == inode?

Compile tested only, and again not critical, but it's -40 LoC...

Thanks. I did functional testing and it works fine.

I can queue this instead. I guess I can reuse the patch description and add Sean as author + add his SOB (if he agrees).

Let me take a look at the patch later in more detail.