Re: [RFC PATCH v2 04/11] KVM: guest_memfd: Add KVM capability to check if guest_memfd is shared

[David Hildenbrand <david@redhat.com>]

On 31.01.25 10:52, Fuad Tabba wrote:
> Hi David,
> 
> On Fri, 31 Jan 2025 at 09:11, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 29.01.25 18:23, Fuad Tabba wrote:
>>> Add the KVM capability KVM_CAP_GMEM_SHARED_MEM, which indicates
>>> that the VM supports shared memory in guest_memfd, or that the
>>> host can create VMs that support shared memory. Supporting shared
>>> memory implies that memory can be mapped when shared with the
>>> host.
>>>
>>> For now, this checks only whether the VM type supports sharing
>>> guest_memfd backed memory. In the future, it will be expanded to
>>> check whether the specific memory address is shared with the
>>> host.
>>>
>>> Signed-off-by: Fuad Tabba <tabba@google.com>
>>> ---
>>>    include/uapi/linux/kvm.h |  1 +
>>>    virt/kvm/guest_memfd.c   | 13 +++++++++++++
>>>    virt/kvm/kvm_main.c      |  4 ++++
>>>    3 files changed, 18 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index 502ea63b5d2e..3ac805c5abf1 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -933,6 +933,7 @@ struct kvm_enable_cap {
>>>    #define KVM_CAP_PRE_FAULT_MEMORY 236
>>>    #define KVM_CAP_X86_APIC_BUS_CYCLES_NS 237
>>>    #define KVM_CAP_X86_GUEST_MODE 238
>>> +#define KVM_CAP_GMEM_SHARED_MEM 239
>>>
>>>    struct kvm_irq_routing_irqchip {
>>>        __u32 irqchip;
>>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>>> index 86441581c9ae..4e1144ed3446 100644
>>> --- a/virt/kvm/guest_memfd.c
>>> +++ b/virt/kvm/guest_memfd.c
>>> @@ -308,6 +308,13 @@ static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
>>>    }
>>>
>>>    #ifdef CONFIG_KVM_GMEM_SHARED_MEM
>>> +static bool kvm_gmem_is_shared(struct file *file, pgoff_t pgoff)
>>
>> I assume you want to call this something like:
>>
>> kvm_gmem_folio_is_shared
>> kvm_gmem_offset_is_shared
>> kvm_gmem_range_is_shared
>> ...
>>
>> To make it clearer that you are only checking one piece and not the
>> whole thing.
>>
>> But then, I wonder if that could be handled in kvm_gmem_get_folio(),
>> and e.g., specified via a flag?
>>
>> kvm_gmem_get_folio(inode, vmf->pgoff, KVM_GMEM_GF_SHARED);
>>
>> Maybe existing callers would want to pass KVM_GMEM_GF_PRIVATE, and the
>> ones that "don't care" don't pas anything?
> 
> I agree that naming this function to make it clearer would be a good
> idea. That said, I think with the patches I removed, it doesn't even
> need to be exposed at all, even looking at future patches --- it has
> no callers other than kvm_gmem_fault(). Therefore, I don't think we
> need to add a flag to kvm_gmem_get_folio().
> 
> I'll have a closer look while preparing the respin and fix it either way.

Okay, having some indication in the name that we only want a shared 
folio etc. might make sense.

-- 
Cheers,

David / dhildenb