Re: [PATCH v3 07/49] HostMem: Add mechanism to opt in kvm guest memfd via MachineState

[David Hildenbrand <david@redhat.com>]

On 21.01.25 18:39, Peter Xu wrote:
> On Wed, Mar 20, 2024 at 03:39:03AM -0500, Michael Roth wrote:
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> Add a new member "guest_memfd" to memory backends. When it's set
>> to true, it enables RAM_GUEST_MEMFD in ram_flags, thus private kvm
>> guest_memfd will be allocated during RAMBlock allocation.
>>
>> Memory backend's @guest_memfd is wired with @require_guest_memfd
>> field of MachineState. It avoid looking up the machine in phymem.c.
>>
>> MachineState::require_guest_memfd is supposed to be set by any VMs
>> that requires KVM guest memfd as private memory, e.g., TDX VM.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>> Changes in v4:
>>   - rename "require_guest_memfd" to "guest_memfd" in struct
>>     HostMemoryBackend;	(David Hildenbrand)
>> Signed-off-by: Michael Roth <michael.roth@amd.com>
>> ---
>>   backends/hostmem-file.c  | 1 +
>>   backends/hostmem-memfd.c | 1 +
>>   backends/hostmem-ram.c   | 1 +
>>   backends/hostmem.c       | 1 +
>>   hw/core/machine.c        | 5 +++++
>>   include/hw/boards.h      | 2 ++
>>   include/sysemu/hostmem.h | 1 +
>>   7 files changed, 12 insertions(+)
>>
>> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>> index ac3e433cbd..3c69db7946 100644
>> --- a/backends/hostmem-file.c
>> +++ b/backends/hostmem-file.c
>> @@ -85,6 +85,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>       ram_flags |= fb->readonly ? RAM_READONLY_FD : 0;
>>       ram_flags |= fb->rom == ON_OFF_AUTO_ON ? RAM_READONLY : 0;
>>       ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>> +    ram_flags |= backend->guest_memfd ? RAM_GUEST_MEMFD : 0;
>>       ram_flags |= fb->is_pmem ? RAM_PMEM : 0;
>>       ram_flags |= RAM_NAMED_FILE;
>>       return memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name,
>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>> index 3923ea9364..745ead0034 100644
>> --- a/backends/hostmem-memfd.c
>> +++ b/backends/hostmem-memfd.c
>> @@ -55,6 +55,7 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>       name = host_memory_backend_get_name(backend);
>>       ram_flags = backend->share ? RAM_SHARED : 0;
>>       ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>> +    ram_flags |= backend->guest_memfd ? RAM_GUEST_MEMFD : 0;
>>       return memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
>>                                             backend->size, ram_flags, fd, 0, errp);
>>   }
>> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
>> index d121249f0f..f7d81af783 100644
>> --- a/backends/hostmem-ram.c
>> +++ b/backends/hostmem-ram.c
>> @@ -30,6 +30,7 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>       name = host_memory_backend_get_name(backend);
>>       ram_flags = backend->share ? RAM_SHARED : 0;
>>       ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>> +    ram_flags |= backend->guest_memfd ? RAM_GUEST_MEMFD : 0;
>>       return memory_region_init_ram_flags_nomigrate(&backend->mr, OBJECT(backend),
>>                                                     name, backend->size,
>>                                                     ram_flags, errp);
> 
> These change look a bit confusing to me, as I don't see how gmemfd can be
> used with either file or ram typed memory backends..

I recall that the following should work:

"private" memory will come from guest_memfd, "shared" (as in, accessible 
by the host) will come from anonymous memory.

This "anon" memory cannot be "shared" with other processes, but 
virtio-kernel etc. can just use it.

To "share" the memory with other processes, we'd need memfd/file.

> 
> When specified gmemfd=on with those, IIUC it'll allocate both the memory
> (ramblock->host) and gmemfd, but without using ->host.  Meanwhile AFAIU the
> ramblock->host will start to conflict with gmemfd in the future when it
> might be able to be mapp-able (having valid ->host).

These will require a new guest_memfd memory backend (I recall that was 
discussed a couple of times).

> 
> I have a local fix for this (and actually more than below.. but starting
> from it), I'm not sure whether I overlooked something, but from reading the
> cover letter it's only using memfd backend which makes perfect sense to me
> so far. 

Does the anon+guest_memfd combination not work or are you speculating 
about the usability (which I hopefully addressed above).

-- 
Cheers,

David / dhildenb