Re: [PATCH v4 10/12] hostmem: add a new memory backend based on POSIX shm_open()

2024-05-10 Thread Stefano Garzarella

On Wed, May 08, 2024 at 01:59:33PM GMT, Markus Armbruster wrote:

Stefano Garzarella  writes:


shm_open() creates and opens a new POSIX shared memory object.
A POSIX shared memory object allows creating memory backend with an
associated file descriptor that can be shared with external processes
(e.g. vhost-user).

The new `memory-backend-shm` can be used as an alternative when
`memory-backend-memfd` is not available (Linux only), since shm_open()
should be provided by any POSIX-compliant operating system.

This backend mimics memfd, allocating memory that is practically
anonymous. In theory shm_open() requires a name, but this is allocated
for a short time interval and shm_unlink() is called right after
shm_open(). After that, only fd is shared with external processes
(e.g., vhost-user) as if it were associated with anonymous memory.

In the future we may also allow the user to specify the name to be
passed to shm_open(), but for now we keep the backend simple, mimicking
anonymous memory such as memfd.

Acked-by: David Hildenbrand 
Signed-off-by: Stefano Garzarella 
---
v4
- fail if we find "share=off" in shm_backend_memory_alloc() [David]
v3
- enriched commit message and documentation to highlight that we
  want to mimic memfd (David)
---
 docs/system/devices/vhost-user.rst |   5 +-
 qapi/qom.json  |  17 
 backends/hostmem-shm.c | 123 +
 backends/meson.build   |   1 +
 qemu-options.hx|  13 +++
 5 files changed, 157 insertions(+), 2 deletions(-)
 create mode 100644 backends/hostmem-shm.c

diff --git a/docs/system/devices/vhost-user.rst 
b/docs/system/devices/vhost-user.rst
index 9b2da106ce..35259d8ec7 100644
--- a/docs/system/devices/vhost-user.rst
+++ b/docs/system/devices/vhost-user.rst
@@ -98,8 +98,9 @@ Shared memory object

 In order for the daemon to access the VirtIO queues to process the
 requests it needs access to the guest's address space. This is
-achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
-objects. A reference to a file-descriptor which can access this object
+achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
+``memory-backend-shm`` objects.
+A reference to a file-descriptor which can access this object
 will be passed via the socket as part of the protocol negotiation.

 Currently the shared memory object needs to match the size of the main
diff --git a/qapi/qom.json b/qapi/qom.json
index 38dde6d785..52df052df8 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -721,6 +721,19 @@
 '*hugetlbsize': 'size',
 '*seal': 'bool' } }

+##
+# @MemoryBackendShmProperties:
+#
+# Properties for memory-backend-shm objects.
+#
+# The @share boolean option is true by default with shm.


This contradicts the doc comment for @share:

  # @share: if false, the memory is private to QEMU; if true, it is
  # shared (default: false)

Your intention is to override that text.  But that's less than clear.
Moreover, the documentation of @share is pretty far from this override.
John Snow is working on patches that'll pull it closer.

Hmm, MemoryBackendMemfdProperties has the same override.


Yep, I followed @MemoryBackendMemfdProperties and 
@MemoryBackendEpcProperties.




I think we should change the doc comment for @share to something like

  # @share: if false, the memory is private to QEMU; if true, it is
  # shared (default depends on the backend type)

and then document the actual default with each backend type.


I agree on that, but I think we should do in a separate series/patch.
If you prefer, I can send that patch.




+#
+# Since: 9.1
+##
+{ 'struct': 'MemoryBackendShmProperties',
+  'base': 'MemoryBackendProperties',
+  'data': { } }


Let's add 'if': 'CONFIG_POSIX' here.



Do you mean something like this:

{ 'struct': 'MemoryBackendShmProperties',
  'if': 'CONFIG_POSIX',
  'base': 'MemoryBackendProperties',
  'data': { } }

I didn't because for MemoryBackendMemfdProperties and
MemoryBackendEpcProperties we have 'if': 'CONFIG_POSIX' only later in
the ObjectOptions union, so I did the same.

Should we fix them as well?


+
 ##
 # @MemoryBackendEpcProperties:
 #
@@ -985,6 +998,8 @@
 { 'name': 'memory-backend-memfd',
   'if': 'CONFIG_LINUX' },
 'memory-backend-ram',
+{ 'name': 'memory-backend-shm',
+  'if': 'CONFIG_POSIX' },
 'pef-guest',
 { 'name': 'pr-manager-helper',
   'if': 'CONFIG_LINUX' },
@@ -1056,6 +1071,8 @@
   'memory-backend-memfd':   { 'type': 'MemoryBackendMemfdProperties',
   'if': 'CONFIG_LINUX' },
   'memory-backend-ram': 'MemoryBackendProperties',
+  'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
+  'if': 'CONFIG_POSIX' },
   'pr-manager-helper':  { 'type': 'PrManagerHelperProperties',
   'if': 'CONFIG_LINUX' },
   

Re: [PATCH v4 10/12] hostmem: add a new memory backend based on POSIX shm_open()

2024-05-08 Thread Markus Armbruster
Stefano Garzarella  writes:

> shm_open() creates and opens a new POSIX shared memory object.
> A POSIX shared memory object allows creating memory backend with an
> associated file descriptor that can be shared with external processes
> (e.g. vhost-user).
>
> The new `memory-backend-shm` can be used as an alternative when
> `memory-backend-memfd` is not available (Linux only), since shm_open()
> should be provided by any POSIX-compliant operating system.
>
> This backend mimics memfd, allocating memory that is practically
> anonymous. In theory shm_open() requires a name, but this is allocated
> for a short time interval and shm_unlink() is called right after
> shm_open(). After that, only fd is shared with external processes
> (e.g., vhost-user) as if it were associated with anonymous memory.
>
> In the future we may also allow the user to specify the name to be
> passed to shm_open(), but for now we keep the backend simple, mimicking
> anonymous memory such as memfd.
>
> Acked-by: David Hildenbrand 
> Signed-off-by: Stefano Garzarella 
> ---
> v4
> - fail if we find "share=off" in shm_backend_memory_alloc() [David]
> v3
> - enriched commit message and documentation to highlight that we
>   want to mimic memfd (David)
> ---
>  docs/system/devices/vhost-user.rst |   5 +-
>  qapi/qom.json  |  17 
>  backends/hostmem-shm.c | 123 +
>  backends/meson.build   |   1 +
>  qemu-options.hx|  13 +++
>  5 files changed, 157 insertions(+), 2 deletions(-)
>  create mode 100644 backends/hostmem-shm.c
>
> diff --git a/docs/system/devices/vhost-user.rst 
> b/docs/system/devices/vhost-user.rst
> index 9b2da106ce..35259d8ec7 100644
> --- a/docs/system/devices/vhost-user.rst
> +++ b/docs/system/devices/vhost-user.rst
> @@ -98,8 +98,9 @@ Shared memory object
>  
>  In order for the daemon to access the VirtIO queues to process the
>  requests it needs access to the guest's address space. This is
> -achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
> -objects. A reference to a file-descriptor which can access this object
> +achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
> +``memory-backend-shm`` objects.
> +A reference to a file-descriptor which can access this object
>  will be passed via the socket as part of the protocol negotiation.
>  
>  Currently the shared memory object needs to match the size of the main
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 38dde6d785..52df052df8 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -721,6 +721,19 @@
>  '*hugetlbsize': 'size',
>  '*seal': 'bool' } }
>  
> +##
> +# @MemoryBackendShmProperties:
> +#
> +# Properties for memory-backend-shm objects.
> +#
> +# The @share boolean option is true by default with shm.

This contradicts the doc comment for @share:

   # @share: if false, the memory is private to QEMU; if true, it is
   # shared (default: false)

Your intention is to override that text.  But that's less than clear.
Moreover, the documentation of @share is pretty far from this override.
John Snow is working on patches that'll pull it closer.

Hmm, MemoryBackendMemfdProperties has the same override.

I think we should change the doc comment for @share to something like

   # @share: if false, the memory is private to QEMU; if true, it is
   # shared (default depends on the backend type)

and then document the actual default with each backend type.

> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'MemoryBackendShmProperties',
> +  'base': 'MemoryBackendProperties',
> +  'data': { } }

Let's add 'if': 'CONFIG_POSIX' here.

> +
>  ##
>  # @MemoryBackendEpcProperties:
>  #
> @@ -985,6 +998,8 @@
>  { 'name': 'memory-backend-memfd',
>'if': 'CONFIG_LINUX' },
>  'memory-backend-ram',
> +{ 'name': 'memory-backend-shm',
> +  'if': 'CONFIG_POSIX' },
>  'pef-guest',
>  { 'name': 'pr-manager-helper',
>'if': 'CONFIG_LINUX' },
> @@ -1056,6 +1071,8 @@
>'memory-backend-memfd':   { 'type': 'MemoryBackendMemfdProperties',
>'if': 'CONFIG_LINUX' },
>'memory-backend-ram': 'MemoryBackendProperties',
> +  'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
> +  'if': 'CONFIG_POSIX' },
>'pr-manager-helper':  { 'type': 'PrManagerHelperProperties',
>'if': 'CONFIG_LINUX' },
>'qtest':  'QtestProperties',

[...]

> diff --git a/qemu-options.hx b/qemu-options.hx
> index cf61f6b863..2226172fb0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -5227,6 +5227,19 @@ SRST
>  
>  The ``share`` boolean option is on by default with memfd.
>  
> +``-object 
> 

[PATCH v4 10/12] hostmem: add a new memory backend based on POSIX shm_open()

2024-05-08 Thread Stefano Garzarella
shm_open() creates and opens a new POSIX shared memory object.
A POSIX shared memory object allows creating memory backend with an
associated file descriptor that can be shared with external processes
(e.g. vhost-user).

The new `memory-backend-shm` can be used as an alternative when
`memory-backend-memfd` is not available (Linux only), since shm_open()
should be provided by any POSIX-compliant operating system.

This backend mimics memfd, allocating memory that is practically
anonymous. In theory shm_open() requires a name, but this is allocated
for a short time interval and shm_unlink() is called right after
shm_open(). After that, only fd is shared with external processes
(e.g., vhost-user) as if it were associated with anonymous memory.

In the future we may also allow the user to specify the name to be
passed to shm_open(), but for now we keep the backend simple, mimicking
anonymous memory such as memfd.

Acked-by: David Hildenbrand 
Signed-off-by: Stefano Garzarella 
---
v4
- fail if we find "share=off" in shm_backend_memory_alloc() [David]
v3
- enriched commit message and documentation to highlight that we
  want to mimic memfd (David)
---
 docs/system/devices/vhost-user.rst |   5 +-
 qapi/qom.json  |  17 
 backends/hostmem-shm.c | 123 +
 backends/meson.build   |   1 +
 qemu-options.hx|  13 +++
 5 files changed, 157 insertions(+), 2 deletions(-)
 create mode 100644 backends/hostmem-shm.c

diff --git a/docs/system/devices/vhost-user.rst 
b/docs/system/devices/vhost-user.rst
index 9b2da106ce..35259d8ec7 100644
--- a/docs/system/devices/vhost-user.rst
+++ b/docs/system/devices/vhost-user.rst
@@ -98,8 +98,9 @@ Shared memory object
 
 In order for the daemon to access the VirtIO queues to process the
 requests it needs access to the guest's address space. This is
-achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
-objects. A reference to a file-descriptor which can access this object
+achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
+``memory-backend-shm`` objects.
+A reference to a file-descriptor which can access this object
 will be passed via the socket as part of the protocol negotiation.
 
 Currently the shared memory object needs to match the size of the main
diff --git a/qapi/qom.json b/qapi/qom.json
index 38dde6d785..52df052df8 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -721,6 +721,19 @@
 '*hugetlbsize': 'size',
 '*seal': 'bool' } }
 
+##
+# @MemoryBackendShmProperties:
+#
+# Properties for memory-backend-shm objects.
+#
+# The @share boolean option is true by default with shm.
+#
+# Since: 9.1
+##
+{ 'struct': 'MemoryBackendShmProperties',
+  'base': 'MemoryBackendProperties',
+  'data': { } }
+
 ##
 # @MemoryBackendEpcProperties:
 #
@@ -985,6 +998,8 @@
 { 'name': 'memory-backend-memfd',
   'if': 'CONFIG_LINUX' },
 'memory-backend-ram',
+{ 'name': 'memory-backend-shm',
+  'if': 'CONFIG_POSIX' },
 'pef-guest',
 { 'name': 'pr-manager-helper',
   'if': 'CONFIG_LINUX' },
@@ -1056,6 +1071,8 @@
   'memory-backend-memfd':   { 'type': 'MemoryBackendMemfdProperties',
   'if': 'CONFIG_LINUX' },
   'memory-backend-ram': 'MemoryBackendProperties',
+  'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
+  'if': 'CONFIG_POSIX' },
   'pr-manager-helper':  { 'type': 'PrManagerHelperProperties',
   'if': 'CONFIG_LINUX' },
   'qtest':  'QtestProperties',
diff --git a/backends/hostmem-shm.c b/backends/hostmem-shm.c
new file mode 100644
index 00..374edc3db8
--- /dev/null
+++ b/backends/hostmem-shm.c
@@ -0,0 +1,123 @@
+/*
+ * QEMU host POSIX shared memory object backend
+ *
+ * Copyright (C) 2024 Red Hat Inc
+ *
+ * Authors:
+ *   Stefano Garzarella 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/hostmem.h"
+#include "qapi/error.h"
+
+#define TYPE_MEMORY_BACKEND_SHM "memory-backend-shm"
+
+OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendShm, MEMORY_BACKEND_SHM)
+
+struct HostMemoryBackendShm {
+HostMemoryBackend parent_obj;
+};
+
+static bool
+shm_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
+{
+g_autoptr(GString) shm_name = g_string_new(NULL);
+g_autofree char *backend_name = NULL;
+uint32_t ram_flags;
+int fd, oflag;
+mode_t mode;
+
+if (!backend->size) {
+error_setg(errp, "can't create shm backend with size 0");
+return false;
+}
+
+if (!backend->share) {
+error_setg(errp, "can't create shm backend with `share=off`");
+return false;
+}
+
+/*
+ * Let's use `mode = 0` because we don't want other