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

[Peter Xu]

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..

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).

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.  I also don't know the planning of coco patches merging so I don't
think even if valid this is urgent - I don't want to mess up on merging
plans..  but still want to collect some comments on whether it's valid:

===8<===

>From edfdf019ab01e99fb4ff417e30bb3692b4e3b922 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Tue, 21 Jan 2025 12:31:19 -0500
Subject: [PATCH] hostmem: Disallow guest memfd for FILE or RAM typed backends

Guest memfd has very special semantics, which by default doesn't have a
path at all, meanwhile it won't proactively allocate anonymous memory.

Currently:

  - memory-backend-file: it is about creating a memory object based on a
  path in the file system.  It doesn't apply to gmemfd.

  - memory-backend-ram: it is about (mostly) trying to allocate anonymous
  memories from the system (private or shared).  It also doesn't apply to
  gmemfd.

Forbid the two types of memory backends to gmemfd, but only allow
memory-backend-memfd for it as of now.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 backends/hostmem-file.c | 8 +++++++-
 backends/hostmem-ram.c  | 7 ++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 46321fda84..c94cf8441b 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -52,11 +52,18 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error 
**errp)
         error_setg(errp, "can't create backend with size 0");
         return false;
     }
+
     if (!fb->mem_path) {
         error_setg(errp, "mem-path property not set");
         return false;
     }
 
+    if (backend->guest_memfd) {
+        error_setg(errp, "File backends do not support guest memfd. "
+                   "Please use memfd backend");
+        return false;
+    }
+
     switch (fb->rom) {
     case ON_OFF_AUTO_AUTO:
         /* Traditionally, opening the file readonly always resulted in ROM. */
@@ -86,7 +93,6 @@ 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-ram.c b/backends/hostmem-ram.c
index 39aac6bf35..8125be217c 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -27,10 +27,15 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error 
**errp)
         return false;
     }
 
+    if (backend->guest_memfd) {
+        error_setg(errp, "File backends do not support guest memfd. "
+                   "Please use memfd backend");
+        return false;
+    }
+
     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);
-- 
2.47.0
===8<===

Thanks,

-- 
Peter Xu