Re: [PATCH v14 09/21] KVM: guest_memfd: Track guest_memfd mmap support in memslot

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 7/16/2025 4:21 PM, Fuad Tabba wrote:
> Hi Xiaoyao,
> 
> On Wed, 16 Jul 2025 at 07:11, Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>
>> On 7/15/2025 5:33 PM, Fuad Tabba wrote:
>>> Add a new internal flag, KVM_MEMSLOT_GMEM_ONLY, to the top half of
>>> memslot->flags. This flag tracks when a guest_memfd-backed memory slot
>>> supports host userspace mmap operations. It's strictly for KVM's
>>> internal use.
>>
>> I would expect some clarification of why naming it with
>> KVM_MEMSLOT_GMEM_ONLY, not something like KVM_MEMSLOT_GMEM_MMAP_ENABLED
>>
>> There was a patch to check the userspace_addr of the memslot refers to
>> the same memory as guest memfd[1], but that patch was dropped. Without
>> the background that when guest memfd is mmapable, userspace doesn't need
>> to provide separate memory via userspace_addr, it's hard to understand
>> and accept the name of GMEM_ONLY.
> 
> The commit message could have clarified this a bit more. Regarding the
> rationale for the naming, there have been various threads and live
> discussions in the biweekly guest_memfd meeting . Instead of rehashing
> the discussion here, I can refer you to a couple [1, 2].

I don't object the name. Just want the clarification in commit message 
and even better add comment in code. That will be truly helpful for 
future readers.

> [1] https://docs.google.com/document/d/1M6766BzdY1Lhk7LiR5IqVR8B8mG3cr-cxTxOrAosPOk/edit?tab=t.0#heading=h.a15es1buok51
> [2] https://lore.kernel.org/all/aFwChljXL5QJYLM_@google.com/
> 
> Thanks,
> /fuad
> 
>> [1] https://lore.kernel.org/all/20250513163438.3942405-9-tabba@google.com/
>>
>>> This optimization avoids repeatedly checking the underlying guest_memfd
>>> file for mmap support, which would otherwise require taking and
>>> releasing a reference on the file for each check. By caching this
>>> information directly in the memslot, we reduce overhead and simplify the
>>> logic involved in handling guest_memfd-backed pages for host mappings.
>>>
>>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>>> Reviewed-by: Shivank Garg <shivankg@amd.com>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Fuad Tabba <tabba@google.com>
>>> ---
>>>    include/linux/kvm_host.h | 11 ++++++++++-
>>>    virt/kvm/guest_memfd.c   |  2 ++
>>>    2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>> index 9ac21985f3b5..d2218ec57ceb 100644
>>> --- a/include/linux/kvm_host.h
>>> +++ b/include/linux/kvm_host.h
>>> @@ -54,7 +54,8 @@
>>>     * used in kvm, other bits are visible for userspace which are defined in
>>>     * include/uapi/linux/kvm.h.
>>>     */
>>> -#define KVM_MEMSLOT_INVALID  (1UL << 16)
>>> +#define KVM_MEMSLOT_INVALID                  (1UL << 16)
>>> +#define KVM_MEMSLOT_GMEM_ONLY                        (1UL << 17)
>>>
>>>    /*
>>>     * Bit 63 of the memslot generation number is an "update in-progress flag",
>>> @@ -2536,6 +2537,14 @@ static inline void kvm_prepare_memory_fault_exit(struct kvm_vcpu *vcpu,
>>>                vcpu->run->memory_fault.flags |= KVM_MEMORY_EXIT_FLAG_PRIVATE;
>>>    }
>>>
>>> +static inline bool kvm_memslot_is_gmem_only(const struct kvm_memory_slot *slot)
>>> +{
>>> +     if (!IS_ENABLED(CONFIG_KVM_GMEM_SUPPORTS_MMAP))
>>> +             return false;
>>> +
>>> +     return slot->flags & KVM_MEMSLOT_GMEM_ONLY;
>>> +}
>>> +
>>>    #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
>>>    static inline unsigned long kvm_get_memory_attributes(struct kvm *kvm, gfn_t gfn)
>>>    {
>>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>>> index 07a4b165471d..2b00f8796a15 100644
>>> --- a/virt/kvm/guest_memfd.c
>>> +++ b/virt/kvm/guest_memfd.c
>>> @@ -592,6 +592,8 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>>>         */
>>>        WRITE_ONCE(slot->gmem.file, file);
>>>        slot->gmem.pgoff = start;
>>> +     if (kvm_gmem_supports_mmap(inode))
>>> +             slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
>>>
>>>        xa_store_range(&gmem->bindings, start, end - 1, slot, GFP_KERNEL);
>>>        filemap_invalidate_unlock(inode->i_mapping);
>>