| Message ID | 20240528103823.146231-1-sgarzare@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD) | expand |
Stefano Garzarella <sgarzare@redhat.com> 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 <david@redhat.com> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > v5 > - fixed documentation in qapi/qom.json and qemu-options.hx [Markus] > 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 | 19 +++++ > backends/hostmem-shm.c | 123 +++++++++++++++++++++++++++++ > backends/meson.build | 1 + > qemu-options.hx | 16 ++++ > 5 files changed, 162 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..d40592d863 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -721,6 +721,21 @@ > '*hugetlbsize': 'size', > '*seal': 'bool' } } > > +## > +# @MemoryBackendShmProperties: > +# > +# Properties for memory-backend-shm objects. > +# > +# The @share boolean option is true by default with shm. Setting it to false > +# will cause a failure during allocation because it is not supported by this > +# backend. docs/devel/qapi-code-gen.rst: For legibility, wrap text paragraphs so every line is at most 70 characters long. Separate sentences with two spaces. Result: # Properties for memory-backend-shm objects. # # The @share boolean option is true by default with shm. Setting it # to false will cause a failure during allocation because it is not # supported by this backend. However, 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 +1000,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 +1073,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 8ca7f34ef0..ad6521ef5e 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -5240,6 +5240,22 @@ SRST > > The ``share`` boolean option is on by default with memfd. > > + ``-object memory-backend-shm,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave`` > + Creates a POSIX shared memory backend object, which allows > + QEMU to share the memory with an external process (e.g. when > + using vhost-user). > + > + ``memory-backend-shm`` is a more portable and less featureful version > + of ``memory-backend-memfd``. It can then be used in any POSIX system, > + especially when memfd is not supported. This actually explains the purpose, unlike the doc comment in qom.json. Same for the existing memory backends; can't fault you for doing your new one the same way. We ought to fix them all. I'm not demanding you do it. > + > + Please refer to ``memory-backend-file`` for a description of the > + options. > + > + The ``share`` boolean option is on by default with shm. Setting it to > + off will cause a failure during allocation because it is not supported > + by this backend. > + Not this patch's fault: documentation for -object memory-backend-epc is missing. > ``-object iommufd,id=id[,fd=fd]`` > Creates an iommufd backend which allows control of DMA mapping > through the ``/dev/iommu`` device.
On Wed, May 29, 2024 at 04:50:20PM GMT, Markus Armbruster wrote: >Stefano Garzarella <sgarzare@redhat.com> 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 <david@redhat.com> >> Acked-by: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> v5 >> - fixed documentation in qapi/qom.json and qemu-options.hx [Markus] >> 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 | 19 +++++ >> backends/hostmem-shm.c | 123 +++++++++++++++++++++++++++++ >> backends/meson.build | 1 + >> qemu-options.hx | 16 ++++ >> 5 files changed, 162 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..d40592d863 100644 >> --- a/qapi/qom.json >> +++ b/qapi/qom.json >> @@ -721,6 +721,21 @@ >> '*hugetlbsize': 'size', >> '*seal': 'bool' } } >> >> +## >> +# @MemoryBackendShmProperties: >> +# >> +# Properties for memory-backend-shm objects. >> +# >> +# The @share boolean option is true by default with shm. Setting it to false >> +# will cause a failure during allocation because it is not supported by this >> +# backend. > >docs/devel/qapi-code-gen.rst: > > For legibility, wrap text paragraphs so every line is at most 70 > characters long. > > Separate sentences with two spaces. > >Result: > > # Properties for memory-backend-shm objects. > # > # The @share boolean option is true by default with shm. Setting it > # to false will cause a failure during allocation because it is not > # supported by this backend. Ops, sorry, I'll fix! > >However, 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. Yes, I had already seen your comment to an earlier version and sent another separate patch: https://patchew.org/QEMU/20240523133302.103858-1-sgarzare@redhat.com/ Is that okay? > >> +# >> +# Since: 9.1 >> +## >> +{ 'struct': 'MemoryBackendShmProperties', >> + 'base': 'MemoryBackendProperties', >> + 'data': { } } > >Let's add 'if': 'CONFIG_POSIX' here. > I think my response to your review at v4 fell through a crack :-) https://patchew.org/QEMU/20240508074457.12367-1-sgarzare@redhat.com/20240508074457.12367-11-sgarzare@redhat.com/#z3lbtmkn6zlwdhdea7owav3mblttxr3asrmlilwxmkla67tdby@732gn3uuupoq I'll bring back my doubts 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 +1000,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 +1073,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 8ca7f34ef0..ad6521ef5e 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -5240,6 +5240,22 @@ SRST >> >> The ``share`` boolean option is on by default with memfd. >> >> + ``-object memory-backend-shm,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave`` >> + Creates a POSIX shared memory backend object, which allows >> + QEMU to share the memory with an external process (e.g. when >> + using vhost-user). >> + >> + ``memory-backend-shm`` is a more portable and less featureful version >> + of ``memory-backend-memfd``. It can then be used in any POSIX system, >> + especially when memfd is not supported. > >This actually explains the purpose, unlike the doc comment in qom.json. >Same for the existing memory backends; can't fault you for doing your >new one the same way. We ought to fix them all. I'm not demanding you >do it. Okay, I'll try to fix all of them separately. > >> + >> + Please refer to ``memory-backend-file`` for a description of the >> + options. >> + >> + The ``share`` boolean option is on by default with shm. Setting it to >> + off will cause a failure during allocation because it is not supported >> + by this backend. >> + > >Not this patch's fault: documentation for -object memory-backend-epc is >missing. Good point! I don't know epc, @David do you have any thoughts to write here? Thanks, Stefano > >> ``-object iommufd,id=id[,fd=fd]`` >> Creates an iommufd backend which allows control of DMA mapping >> through the ``/dev/iommu`` device. >
Stefano Garzarella <sgarzare@redhat.com> writes: > On Wed, May 29, 2024 at 04:50:20PM GMT, Markus Armbruster wrote: >>Stefano Garzarella <sgarzare@redhat.com> 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 <david@redhat.com> >>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>> --- >>> v5 >>> - fixed documentation in qapi/qom.json and qemu-options.hx [Markus] >>> 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 | 19 +++++ >>> backends/hostmem-shm.c | 123 +++++++++++++++++++++++++++++ >>> backends/meson.build | 1 + >>> qemu-options.hx | 16 ++++ >>> 5 files changed, 162 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..d40592d863 100644 >>> --- a/qapi/qom.json >>> +++ b/qapi/qom.json >>> @@ -721,6 +721,21 @@ >>> '*hugetlbsize': 'size', >>> '*seal': 'bool' } } >>> >>> +## >>> +# @MemoryBackendShmProperties: >>> +# >>> +# Properties for memory-backend-shm objects. >>> +# >>> +# The @share boolean option is true by default with shm. Setting it to false >>> +# will cause a failure during allocation because it is not supported by this >>> +# backend. >> >>docs/devel/qapi-code-gen.rst: >> >> For legibility, wrap text paragraphs so every line is at most 70 >> characters long. >> >> Separate sentences with two spaces. >> >>Result: >> >> # Properties for memory-backend-shm objects. >> # >> # The @share boolean option is true by default with shm. Setting it >> # to false will cause a failure during allocation because it is not >> # supported by this backend. > > Ops, sorry, I'll fix! > >> >>However, 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. > > Yes, I had already seen your comment to an earlier version and sent another separate patch: > https://patchew.org/QEMU/20240523133302.103858-1-sgarzare@redhat.com/ > > Is that okay? Looks like I'm going through my post-vacation review backlog in suboptimal order... Replied there! >>> +# >>> +# Since: 9.1 >>> +## >>> +{ 'struct': 'MemoryBackendShmProperties', >>> + 'base': 'MemoryBackendProperties', >>> + 'data': { } } >> >>Let's add 'if': 'CONFIG_POSIX' here. >> > > I think my response to your review at v4 fell through a crack :-) > https://patchew.org/QEMU/20240508074457.12367-1-sgarzare@redhat.com/20240508074457.12367-11-sgarzare@redhat.com/#z3lbtmkn6zlwdhdea7owav3mblttxr3asrmlilwxmkla67tdby@732gn3uuupoq Dang, it did %-} > I'll bring back my doubts 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? Yes, please. The QAPI schema's primary purpose is to define the QMP interface. The tooling lets you define QAPI types that aren't actually used in the QMP interface. We use this intentionally, e.g. to generate types & visitors for complex QOM properties. Accidental use is also possible, say when we define a type unconditionally, but use it only conditionally. We then end up generating dead code. No big deal, but let's avoid it whenever practical. [...]
On Mon, Jun 03, 2024 at 11:42:35AM GMT, Markus Armbruster wrote: >Stefano Garzarella <sgarzare@redhat.com> writes: > >> On Wed, May 29, 2024 at 04:50:20PM GMT, Markus Armbruster wrote: >>>Stefano Garzarella <sgarzare@redhat.com> 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 <david@redhat.com> >>>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>>> --- >>>> v5 >>>> - fixed documentation in qapi/qom.json and qemu-options.hx [Markus] >>>> 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 | 19 +++++ >>>> backends/hostmem-shm.c | 123 +++++++++++++++++++++++++++++ >>>> backends/meson.build | 1 + >>>> qemu-options.hx | 16 ++++ >>>> 5 files changed, 162 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..d40592d863 100644 >>>> --- a/qapi/qom.json >>>> +++ b/qapi/qom.json >>>> @@ -721,6 +721,21 @@ >>>> '*hugetlbsize': 'size', >>>> '*seal': 'bool' } } >>>> >>>> +## >>>> +# @MemoryBackendShmProperties: >>>> +# >>>> +# Properties for memory-backend-shm objects. >>>> +# >>>> +# The @share boolean option is true by default with shm. Setting it to false >>>> +# will cause a failure during allocation because it is not supported by this >>>> +# backend. >>> >>>docs/devel/qapi-code-gen.rst: >>> >>> For legibility, wrap text paragraphs so every line is at most 70 >>> characters long. >>> >>> Separate sentences with two spaces. >>> >>>Result: >>> >>> # Properties for memory-backend-shm objects. >>> # >>> # The @share boolean option is true by default with shm. Setting it >>> # to false will cause a failure during allocation because it is not >>> # supported by this backend. >> >> Ops, sorry, I'll fix! >> >>> >>>However, 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. >> >> Yes, I had already seen your comment to an earlier version and sent another separate patch: >> https://patchew.org/QEMU/20240523133302.103858-1-sgarzare@redhat.com/ >> >> Is that okay? > >Looks like I'm going through my post-vacation review backlog in >suboptimal order... > >Replied there! Thanks! > >>>> +# >>>> +# Since: 9.1 >>>> +## >>>> +{ 'struct': 'MemoryBackendShmProperties', >>>> + 'base': 'MemoryBackendProperties', >>>> + 'data': { } } >>> >>>Let's add 'if': 'CONFIG_POSIX' here. >>> >> >> I think my response to your review at v4 fell through a crack :-) >> https://patchew.org/QEMU/20240508074457.12367-1-sgarzare@redhat.com/20240508074457.12367-11-sgarzare@redhat.com/#z3lbtmkn6zlwdhdea7owav3mblttxr3asrmlilwxmkla67tdby@732gn3uuupoq > >Dang, it did %-} > >> I'll bring back my doubts 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? > >Yes, please. Okay, I'll send a separated patch for them and fix MemoryBackendShmProperties here in v7. I saw some examples to follow in qapi/char.json. > >The QAPI schema's primary purpose is to define the QMP interface. The >tooling lets you define QAPI types that aren't actually used in the QMP >interface. We use this intentionally, e.g. to generate types & visitors >for complex QOM properties. Accidental use is also possible, say when >we define a type unconditionally, but use it only conditionally. We >then end up generating dead code. No big deal, but let's avoid it >whenever practical. Got it, thanks for the info and the review! Stefano
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..d40592d863 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -721,6 +721,21 @@ '*hugetlbsize': 'size', '*seal': 'bool' } } +## +# @MemoryBackendShmProperties: +# +# Properties for memory-backend-shm objects. +# +# The @share boolean option is true by default with shm. Setting it to false +# will cause a failure during allocation because it is not supported by this +# backend. +# +# Since: 9.1 +## +{ 'struct': 'MemoryBackendShmProperties', + 'base': 'MemoryBackendProperties', + 'data': { } } + ## # @MemoryBackendEpcProperties: # @@ -985,6 +1000,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 +1073,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 0000000000..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 <sgarzare@redhat.com> + * + * 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 processes to open our + * memory unless we share the file descriptor with them. + */ + mode = 0; + oflag = O_RDWR | O_CREAT | O_EXCL; + backend_name = host_memory_backend_get_name(backend); + + /* + * Some operating systems allow creating anonymous POSIX shared memory + * objects (e.g. FreeBSD provides the SHM_ANON constant), but this is not + * defined by POSIX, so let's create a unique name. + * + * From Linux's shm_open(3) man-page: + * For portable use, a shared memory object should be identified + * by a name of the form /somename;" + */ + g_string_printf(shm_name, "/qemu-" FMT_pid "-shm-%s", getpid(), + backend_name); + + fd = shm_open(shm_name->str, oflag, mode); + if (fd < 0) { + error_setg_errno(errp, errno, + "failed to create POSIX shared memory"); + return false; + } + + /* + * We have the file descriptor, so we no longer need to expose the + * POSIX shared memory object. However it will remain allocated as long as + * there are file descriptors pointing to it. + */ + shm_unlink(shm_name->str); + + if (ftruncate(fd, backend->size) == -1) { + error_setg_errno(errp, errno, + "failed to resize POSIX shared memory to %" PRIu64, + backend->size); + close(fd); + return false; + } + + ram_flags = RAM_SHARED; + ram_flags |= backend->reserve ? 0 : RAM_NORESERVE; + + return memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), + backend_name, backend->size, + ram_flags, fd, 0, errp); +} + +static void +shm_backend_instance_init(Object *obj) +{ + HostMemoryBackendShm *m = MEMORY_BACKEND_SHM(obj); + + MEMORY_BACKEND(m)->share = true; +} + +static void +shm_backend_class_init(ObjectClass *oc, void *data) +{ + HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc); + + bc->alloc = shm_backend_memory_alloc; +} + +static const TypeInfo shm_backend_info = { + .name = TYPE_MEMORY_BACKEND_SHM, + .parent = TYPE_MEMORY_BACKEND, + .instance_init = shm_backend_instance_init, + .class_init = shm_backend_class_init, + .instance_size = sizeof(HostMemoryBackendShm), +}; + +static void register_types(void) +{ + type_register_static(&shm_backend_info); +} + +type_init(register_types); diff --git a/backends/meson.build b/backends/meson.build index 8b2b111497..3867b0d363 100644 --- a/backends/meson.build +++ b/backends/meson.build @@ -13,6 +13,7 @@ system_ss.add([files( if host_os != 'windows' system_ss.add(files('rng-random.c')) system_ss.add(files('hostmem-file.c')) + system_ss.add([files('hostmem-shm.c'), rt]) endif if host_os == 'linux' system_ss.add(files('hostmem-memfd.c')) diff --git a/qemu-options.hx b/qemu-options.hx index 8ca7f34ef0..ad6521ef5e 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -5240,6 +5240,22 @@ SRST The ``share`` boolean option is on by default with memfd. + ``-object memory-backend-shm,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave`` + Creates a POSIX shared memory backend object, which allows + QEMU to share the memory with an external process (e.g. when + using vhost-user). + + ``memory-backend-shm`` is a more portable and less featureful version + of ``memory-backend-memfd``. It can then be used in any POSIX system, + especially when memfd is not supported. + + Please refer to ``memory-backend-file`` for a description of the + options. + + The ``share`` boolean option is on by default with shm. Setting it to + off will cause a failure during allocation because it is not supported + by this backend. + ``-object iommufd,id=id[,fd=fd]`` Creates an iommufd backend which allows control of DMA mapping through the ``/dev/iommu`` device.