Re: [PATCH v12 08/18] KVM: guest_memfd: Allow host to map guest_memfd pages

[David Hildenbrand <david@redhat.com>]

On 16.06.25 15:44, Ira Weiny wrote:
> Sean Christopherson wrote:
>> On Wed, Jun 11, 2025, Fuad Tabba wrote:
>>> This patch enables support for shared memory in guest_memfd, including
>>
>> Please don't lead with with "This patch", simply state what changes are being
>> made as a command.
>>
>>> mapping that memory from host userspace.
>>
>>> This functionality is gated by the KVM_GMEM_SHARED_MEM Kconfig option,
>>> and enabled for a given instance by the GUEST_MEMFD_FLAG_SUPPORT_SHARED
>>> flag at creation time.
>>
>> Why?  I can see that from the patch.
>>
>> This changelog is way, way, waaay too light on details.  Sorry for jumping in at
>> the 11th hour, but we've spent what, 2 years working on this?
>>
>>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>> Co-developed-by: Ackerley Tng <ackerleytng@google.com>
>>> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
>>> Signed-off-by: Fuad Tabba <tabba@google.com>
>>> ---
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index d00b85cb168c..cb19150fd595 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -1570,6 +1570,7 @@ 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	(1ULL << 0)
>>
>> I find the SUPPORT_SHARED terminology to be super confusing.  I had to dig quite
>> deep to undesrtand that "support shared" actually mean "userspace explicitly
>> enable sharing on _this_ guest_memfd instance".  E.g. I was surprised to see
>>
>> IMO, GUEST_MEMFD_FLAG_SHAREABLE would be more appropriate.  But even that is
>> weird to me.  For non-CoCo VMs, there is no concept of shared vs. private.  What's
>> novel and notable is that the memory is _mappable_.  Yeah, yeah, pKVM's use case
>> is to share memory, but that's a _use case_, not the property of guest_memfd that
>> is being controlled by userspace.
>>
>> And kvm_gmem_memslot_supports_shared() is even worse.  It's simply that the
>> memslot is bound to a mappable guest_memfd instance, it's that the guest_memfd
>> instance is the _only_ entry point to the memslot.
>>
>> So my vote would be "GUEST_MEMFD_FLAG_MAPPABLE", and then something like
> 
> If we are going to change this; FLAG_MAPPABLE is not clear to me either.
> The guest can map private memory, right?  I see your point about shared
> being overloaded with file shared but it would not be the first time a
> term is overloaded.  kvm_slot_has_gmem() does makes a lot of sense.
> 
> If it is going to change; how about GUEST_MEMFD_FLAG_USER_MAPPABLE?

If "shared" is not good enough terminology ...

... can we please just find a way to name what this "non-private" memory 
is called? That something is mappable into $whatever is not the right 
way to look at this IMHO. As raised in the past, we can easily support 
read()/write()/etc to this non-private memory.


I'll note, the "non-private" memory in guest-memfd behaves just like ... 
the "shared" memory in shmem ... well, or like other memory in memfd. 
(which is based on mm/shmem.c).

"Private" is also not the best way to describe the "protected\encrypted" 
memory, but that ship has sailed with KVM_MEMORY_ATTRIBUTE_PRIVATE.

I'll further note that in the doc of KVM_SET_USER_MEMORY_REGION2 we talk 
about "private" vs "shared" memory ... so that would have to be improved 
as well.

-- 
Cheers,

David / dhildenb