From: "Garg, Shivank" <shivankg@amd.com>
To: Sean Christopherson <seanjc@google.com>,
Marc Zyngier <maz@kernel.org>,
Oliver Upton <oliver.upton@linux.dev>,
Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
David Hildenbrand <david@redhat.com>,
Fuad Tabba <tabba@google.com>,
Ackerley Tng <ackerleytng@google.com>,
Ashish Kalra <ashish.kalra@amd.com>,
Vlastimil Babka <vbabka@suse.cz>
Subject: Re: [PATCH v12 01/12] KVM: guest_memfd: Rename "struct kvm_gmem" to "struct gmem_file"
Date: Wed, 8 Oct 2025 10:55:46 +0530 [thread overview]
Message-ID: <dd948073-0839-4f75-8cec-1f3041231ed7@amd.com> (raw)
In-Reply-To: <20251007221420.344669-2-seanjc@google.com>
On 10/8/2025 3:44 AM, Sean Christopherson wrote:
> Rename the "kvm_gmem" structure to "gmem_file" in anticipation of using
> dedicated guest_memfd inodes instead of anonyomous inodes, at which point
> the "kvm_gmem" nomenclature becomes quite misleading. In guest_memfd,
> inodes are effectively the raw underlying physical storage, and will be
> used to track properties of the physical memory, while each gmem file is
> effectively a single VM's view of that storage, and is used to track assets
> specific to its associated VM, e.g. memslots=>gmem bindings.
>
> Using "kvm_gmem" suggests that the per-VM/per-file structures are _the_
> guest_memfd instance, which almost the exact opposite of reality.
>
> Opportunistically rename local variables from "gmem" to "f", again to
> avoid confusion once guest_memfd specific inodes come along.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> virt/kvm/guest_memfd.c | 100 ++++++++++++++++++++++-------------------
> 1 file changed, 54 insertions(+), 46 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index fbca8c0972da..3c57fb42f12c 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -7,7 +7,16 @@
>
> #include "kvm_mm.h"
>
> -struct kvm_gmem {
> +/*
> + * A guest_memfd instance can be associated multiple VMs, each with its own
> + * "view" of the underlying physical memory.
> + *
> + * The gmem's inode is effectively the raw underlying physical storage, and is
> + * used to track properties of the physical memory, while each gmem file is
> + * effectively a single VM's view of that storage, and is used to track assets
> + * specific to its associated VM, e.g. memslots=>gmem bindings.
> + */
> +struct gmem_file {
> struct kvm *kvm;
> struct xarray bindings;
> struct list_head entry;
> @@ -110,16 +119,16 @@ static enum kvm_gfn_range_filter kvm_gmem_get_invalidate_filter(struct inode *in
> return KVM_FILTER_PRIVATE;
> }
>
> -static void __kvm_gmem_invalidate_begin(struct kvm_gmem *gmem, pgoff_t start,
> +static void __kvm_gmem_invalidate_begin(struct gmem_file *f, pgoff_t start,
> pgoff_t end,
> enum kvm_gfn_range_filter attr_filter)
> {
> bool flush = false, found_memslot = false;
> struct kvm_memory_slot *slot;
> - struct kvm *kvm = gmem->kvm;
> + struct kvm *kvm = f->kvm;
> unsigned long index;
>
> - xa_for_each_range(&gmem->bindings, index, slot, start, end - 1) {
> + xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
> pgoff_t pgoff = slot->gmem.pgoff;
>
> struct kvm_gfn_range gfn_range = {
> @@ -152,20 +161,20 @@ static void kvm_gmem_invalidate_begin(struct inode *inode, pgoff_t start,
> {
> struct list_head *gmem_list = &inode->i_mapping->i_private_list;
> enum kvm_gfn_range_filter attr_filter;
> - struct kvm_gmem *gmem;
> + struct gmem_file *f;
>
> attr_filter = kvm_gmem_get_invalidate_filter(inode);
>
> - list_for_each_entry(gmem, gmem_list, entry)
> - __kvm_gmem_invalidate_begin(gmem, start, end, attr_filter);
> + list_for_each_entry(f, gmem_list, entry)
> + __kvm_gmem_invalidate_begin(f, start, end, attr_filter);
> }
>
> -static void __kvm_gmem_invalidate_end(struct kvm_gmem *gmem, pgoff_t start,
> +static void __kvm_gmem_invalidate_end(struct gmem_file *f, pgoff_t start,
> pgoff_t end)
> {
> - struct kvm *kvm = gmem->kvm;
> + struct kvm *kvm = f->kvm;
>
> - if (xa_find(&gmem->bindings, &start, end - 1, XA_PRESENT)) {
> + if (xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
> KVM_MMU_LOCK(kvm);
> kvm_mmu_invalidate_end(kvm);
> KVM_MMU_UNLOCK(kvm);
> @@ -176,10 +185,10 @@ static void kvm_gmem_invalidate_end(struct inode *inode, pgoff_t start,
> pgoff_t end)
> {
> struct list_head *gmem_list = &inode->i_mapping->i_private_list;
> - struct kvm_gmem *gmem;
> + struct gmem_file *f;
>
> - list_for_each_entry(gmem, gmem_list, entry)
> - __kvm_gmem_invalidate_end(gmem, start, end);
> + list_for_each_entry(f, gmem_list, entry)
> + __kvm_gmem_invalidate_end(f, start, end);
> }
>
> static long kvm_gmem_punch_hole(struct inode *inode, loff_t offset, loff_t len)
> @@ -277,9 +286,9 @@ static long kvm_gmem_fallocate(struct file *file, int mode, loff_t offset,
>
> static int kvm_gmem_release(struct inode *inode, struct file *file)
> {
> - struct kvm_gmem *gmem = file->private_data;
> + struct gmem_file *f = file->private_data;
> struct kvm_memory_slot *slot;
> - struct kvm *kvm = gmem->kvm;
> + struct kvm *kvm = f->kvm;
> unsigned long index;
>
> /*
> @@ -299,7 +308,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
>
> filemap_invalidate_lock(inode->i_mapping);
>
> - xa_for_each(&gmem->bindings, index, slot)
> + xa_for_each(&f->bindings, index, slot)
> WRITE_ONCE(slot->gmem.file, NULL);
>
> /*
> @@ -307,18 +316,18 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
> * Zap all SPTEs pointed at by this file. Do not free the backing
> * memory, as its lifetime is associated with the inode, not the file.
> */
> - __kvm_gmem_invalidate_begin(gmem, 0, -1ul,
> + __kvm_gmem_invalidate_begin(f, 0, -1ul,
> kvm_gmem_get_invalidate_filter(inode));
> - __kvm_gmem_invalidate_end(gmem, 0, -1ul);
> + __kvm_gmem_invalidate_end(f, 0, -1ul);
>
> - list_del(&gmem->entry);
> + list_del(&f->entry);
>
> filemap_invalidate_unlock(inode->i_mapping);
>
> mutex_unlock(&kvm->slots_lock);
>
> - xa_destroy(&gmem->bindings);
> - kfree(gmem);
> + xa_destroy(&f->bindings);
> + kfree(f);
>
> kvm_put_kvm(kvm);
>
> @@ -493,7 +502,7 @@ bool __weak kvm_arch_supports_gmem_init_shared(struct kvm *kvm)
> static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> {
> const char *anon_name = "[kvm-gmem]";
> - struct kvm_gmem *gmem;
> + struct gmem_file *f;
> struct inode *inode;
> struct file *file;
> int fd, err;
> @@ -502,14 +511,13 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> if (fd < 0)
> return fd;
>
> - gmem = kzalloc(sizeof(*gmem), GFP_KERNEL);
> - if (!gmem) {
> + f = kzalloc(sizeof(*f), GFP_KERNEL);
> + if (!f) {
> err = -ENOMEM;
> goto err_fd;
> }
>
> - file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,
> - O_RDWR, NULL);
> + file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, f, O_RDWR, NULL);
> if (IS_ERR(file)) {
> err = PTR_ERR(file);
> goto err_gmem;
> @@ -531,15 +539,15 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> 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);
> + f->kvm = kvm;
> + xa_init(&f->bindings);
> + list_add(&f->entry, &inode->i_mapping->i_private_list);
>
> fd_install(fd, file);
> return fd;
>
> err_gmem:
> - kfree(gmem);
> + kfree(f);
> err_fd:
> put_unused_fd(fd);
> return err;
> @@ -564,7 +572,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> {
> loff_t size = slot->npages << PAGE_SHIFT;
> unsigned long start, end;
> - struct kvm_gmem *gmem;
> + struct gmem_file *f;
> struct inode *inode;
> struct file *file;
> int r = -EINVAL;
> @@ -578,8 +586,8 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> if (file->f_op != &kvm_gmem_fops)
> goto err;
>
> - gmem = file->private_data;
> - if (gmem->kvm != kvm)
> + f = file->private_data;
> + if (f->kvm != kvm)
> goto err;
>
> inode = file_inode(file);
> @@ -593,8 +601,8 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> start = offset >> PAGE_SHIFT;
> end = start + slot->npages;
>
> - if (!xa_empty(&gmem->bindings) &&
> - xa_find(&gmem->bindings, &start, end - 1, XA_PRESENT)) {
> + if (!xa_empty(&f->bindings) &&
> + xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
> filemap_invalidate_unlock(inode->i_mapping);
> goto err;
> }
> @@ -609,7 +617,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> if (kvm_gmem_supports_mmap(inode))
> slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
>
> - xa_store_range(&gmem->bindings, start, end - 1, slot, GFP_KERNEL);
> + xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> filemap_invalidate_unlock(inode->i_mapping);
>
> /*
> @@ -627,7 +635,7 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
> {
> unsigned long start = slot->gmem.pgoff;
> unsigned long end = start + slot->npages;
> - struct kvm_gmem *gmem;
> + struct gmem_file *f;
> struct file *file;
>
> /*
> @@ -638,10 +646,10 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
> if (!file)
> return;
>
> - gmem = file->private_data;
> + f = file->private_data;
>
> filemap_invalidate_lock(file->f_mapping);
> - xa_store_range(&gmem->bindings, start, end - 1, NULL, GFP_KERNEL);
> + xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
>
> /*
> * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
> @@ -659,18 +667,18 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
> pgoff_t index, kvm_pfn_t *pfn,
> bool *is_prepared, int *max_order)
> {
> - struct file *gmem_file = READ_ONCE(slot->gmem.file);
> - struct kvm_gmem *gmem = file->private_data;
> + struct file *slot_file = READ_ONCE(slot->gmem.file);
> + struct gmem_file *f = file->private_data;
^^^
> struct folio *folio;
>
> - if (file != gmem_file) {
> - WARN_ON_ONCE(gmem_file);
> + if (file != slot_file) {
> + WARN_ON_ONCE(slot_file);
> return ERR_PTR(-EFAULT);
> }
>
> - gmem = file->private_data;
> - if (xa_load(&gmem->bindings, index) != slot) {
> - WARN_ON_ONCE(xa_load(&gmem->bindings, index));
> + f = file->private_data;
This redundant initialization can be dropped.
I sent a cleanup patch including this change a few weeks ago:
https://lore.kernel.org/kvm/20250902080307.153171-2-shivankg@amd.com
Could you please review it?
Everything else looks good to me!
Reviewed-by: Shivank Garg <shivankg@amd.com>
> + if (xa_load(&f->bindings, index) != slot) {
> + WARN_ON_ONCE(xa_load(&f->bindings, index));
> return ERR_PTR(-EIO);
> }
>
Thanks,
Shivank
next prev parent reply other threads:[~2025-10-08 5:26 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-07 22:14 [PATCH v12 00/12] KVM: guest_memfd: Add NUMA mempolicy support Sean Christopherson
2025-10-07 22:14 ` [PATCH v12 01/12] KVM: guest_memfd: Rename "struct kvm_gmem" to "struct gmem_file" Sean Christopherson
2025-10-08 5:25 ` Garg, Shivank [this message]
2025-10-09 21:08 ` Ackerley Tng
2025-10-10 15:07 ` David Hildenbrand
2025-10-07 22:14 ` [PATCH v12 02/12] KVM: guest_memfd: Add macro to iterate over gmem_files for a mapping/inode Sean Christopherson
2025-10-08 5:30 ` Garg, Shivank
2025-10-09 21:27 ` Ackerley Tng
2025-10-07 22:14 ` [PATCH v12 03/12] KVM: guest_memfd: Use guest mem inodes instead of anonymous inodes Sean Christopherson
2025-10-07 22:14 ` [PATCH v12 04/12] KVM: guest_memfd: Add slab-allocated inode cache Sean Christopherson
2025-10-09 21:39 ` Ackerley Tng
2025-10-09 22:16 ` Ackerley Tng
2025-10-07 22:14 ` [PATCH v12 05/12] KVM: guest_memfd: Enforce NUMA mempolicy using shared policy Sean Christopherson
2025-10-09 22:15 ` Ackerley Tng
2025-10-10 7:57 ` Garg, Shivank
2025-10-10 20:33 ` Sean Christopherson
2025-10-10 21:57 ` Ackerley Tng
2025-10-12 20:00 ` Garg, Shivank
2025-10-15 16:56 ` Sean Christopherson
2025-10-07 22:14 ` [PATCH v12 06/12] KVM: selftests: Define wrappers for common syscalls to assert success Sean Christopherson
2025-10-09 21:44 ` Ackerley Tng
2025-10-07 22:14 ` [PATCH v12 07/12] KVM: selftests: Report stacktraces SIGBUS, SIGSEGV, SIGILL, and SIGFPE by default Sean Christopherson
2025-10-09 22:31 ` Ackerley Tng
2025-10-07 22:14 ` [PATCH v12 08/12] KVM: selftests: Add additional equivalents to libnuma APIs in KVM's numaif.h Sean Christopherson
2025-10-09 22:34 ` Ackerley Tng
2025-10-07 22:14 ` [PATCH v12 09/12] KVM: selftests: Use proper uAPI headers to pick up mempolicy.h definitions Sean Christopherson
2025-10-10 17:59 ` Ackerley Tng
2025-10-07 22:14 ` [PATCH v12 10/12] KVM: selftests: Add helpers to probe for NUMA support, and multi-node systems Sean Christopherson
2025-10-07 22:14 ` [PATCH v12 11/12] KVM: selftests: Add guest_memfd tests for mmap and NUMA policy support Sean Christopherson
2025-10-09 23:08 ` Ackerley Tng
2025-10-07 22:14 ` [PATCH v12 12/12] KVM: guest_memfd: Add gmem_inode.flags field instead of using i_private Sean Christopherson
2025-10-09 20:58 ` [PATCH v12 00/12] KVM: guest_memfd: Add NUMA mempolicy support Ackerley Tng
2025-10-10 4:59 ` Garg, Shivank
2025-10-10 17:56 ` Ackerley Tng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dd948073-0839-4f75-8cec-1f3041231ed7@amd.com \
--to=shivankg@amd.com \
--cc=ackerleytng@google.com \
--cc=ashish.kalra@amd.com \
--cc=david@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.linux.dev \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=tabba@google.com \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome