Re: [PATCH for-9.1 v3 09/11] hostmem: add a new memory backend based on POSIX shm_open()

[David Hildenbrand]

On 08.04.24 09:58, Stefano Garzarella wrote:
> On Thu, Apr 04, 2024 at 04:09:34PM +0200, David Hildenbrand wrote:
> > On 04.04.24 14:23, Stefano Garzarella wrote:
> > > 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.
> > >
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > > 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             | 118 +++++++++++++++++++++++++++++
> > >   backends/meson.build               |   1 +
> > >   qemu-options.hx                    |  11 +++
> > >   5 files changed, 150 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 85e6b4f84a..5252ec69e3 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': { } }
> > > +
> >
> > Acked-by: David Hildenbrand <david@redhat.com>
> >
> > One comment: we should maybe just forbid setting share=off. it doesn't
> > make any sense and it can even result in an unexpected double memory
> > consumption. We missed doing that for memfd, unfortunately.
>
> Good point!
>
> IIUC the `share` property is defined by the parent `hostmem`, so I
> should find a way to override the property here and disable the setter,
> or add an option to `hostmem` to make the property non-writable.

Right, or simply fail later when you would find "share=off" in 
shm_backend_memory_alloc().

When ever supporting named shmem_open(), it could make sense for VM 
snapshotting. Right now it doesn't really make any sense.

--
Cheers,

David / dhildenb