[PATCH for-9.1 v2 00/11] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)

[PATCH for-9.1 v2 00/11] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)

[Stefano Garzarella]

v1: https://patchew.org/QEMU/20240228114759.44758-1-sgarzare@redhat.com/
v2:
  - fixed Author email and little typos in some patches
  - added memory-backend-shm (patches 9, 10, 11) [Daniel, David, Markus]
  - removed changes to memory-backend-file (ex patch 9)
  - used memory-backend-shm in tests/qtest/vhost-user-blk-test (patch 10)
  - added test case in tests/qtest/vhost-user-test (patch 11)

The vhost-user protocol is not really Linux-specific, so let's try support
QEMU's frontends and backends (including libvhost-user) in any POSIX system
with this series. The main use case is to be able to use virtio devices that
we don't have built-in in QEMU (e.g. virtiofsd, vhost-user-vsock, etc.) even
in non-Linux systems.

The first 5 patches are more like fixes discovered at runtime on macOS or
FreeBSD that could go even independently of this series.

Patches 6, 7, and 8 enable building of frontends and backends (including
libvhost-user) with associated code changes to succeed in compilation.

Patch 9 adds `memory-backend-shm` that uses the POSIX shm_open() API to
create shared memory which is identified by an fd that can be shared with
vhost-user backends. This is useful on those systems (like macOS) where
we don't have memfd_create() or special filesystems like "/dev/shm".

Patches 10 and 11 use `memory-backend-shm` in some vhost-user tests.

Maybe the first 5 patches can go separately, but I only discovered those
problems after testing patches 6 - 9, so I have included them in this series
for now. Please let me know if you prefer that I send them separately.

I tested this series using vhost-user-blk and QSD on macOS Sonoma 14.4
(aarch64), FreeBSD 14 (x86_64), OpenBSD 7.4 (x86_64), and Fedora 39 (x86_64)
in this way:

- Start vhost-user-blk or QSD (same commands for all systems)

  vhost-user-blk -s /tmp/vhost.socket \
    -b Fedora-Cloud-Base-39-1.5.x86_64.raw

  qemu-storage-daemon \
    --blockdev file,filename=Fedora-Cloud-Base-39-1.5.x86_64.qcow2,node-name=file \
    --blockdev qcow2,file=file,node-name=qcow2 \
    --export vhost-user-blk,addr.type=unix,addr.path=/tmp/vhost.socket,id=vub,num-queues=1,node-name=qcow2,writable=on

- macOS (aarch64): start QEMU (using hvf accelerator)

  qemu-system-aarch64 -smp 2 -cpu host -M virt,accel=hvf,memory-backend=mem \
    -drive file=./build/pc-bios/edk2-aarch64-code.fd,if=pflash,format=raw,readonly=on \
    -device virtio-net-device,netdev=net0 -netdev user,id=net0 \
    -device ramfb -device usb-ehci -device usb-kbd \
    -object memory-backend-shm,id=mem,size=512M \
    -device vhost-user-blk-pci,num-queues=1,disable-legacy=on,chardev=char0 \
    -chardev socket,id=char0,path=/tmp/vhost.socket

- FreeBSD/OpenBSD (x86_64): start QEMU (no accelerators available)

  qemu-system-x86_64 -smp 2 -M q35,memory-backend=mem \
    -object memory-backend-shm,id=mem,size="512M" \
    -device vhost-user-blk-pci,num-queues=1,chardev=char0 \
    -chardev socket,id=char0,path=/tmp/vhost.socket

- Fedora (x86_64): start QEMU (using kvm accelerator)

  qemu-system-x86_64 -smp 2 -M q35,accel=kvm,memory-backend=mem \
    -object memory-backend-shm,size="512M" \
    -device vhost-user-blk-pci,num-queues=1,chardev=char0 \
    -chardev socket,id=char0,path=/tmp/vhost.socket

Branch pushed (and CI started) at https://gitlab.com/sgarzarella/qemu/-/tree/macos-vhost-user?ref_type=heads

Thanks,
Stefano

Stefano Garzarella (11):
  libvhost-user: set msg.msg_control to NULL when it is empty
  libvhost-user: fail vu_message_write() if sendmsg() is failing
  libvhost-user: mask F_INFLIGHT_SHMFD if memfd is not supported
  vhost-user-server: don't abort if we can't set fd non-blocking
  contrib/vhost-user-blk: fix bind() using the right size of the address
  vhost-user: enable frontends on any POSIX system
  libvhost-user: enable it on any POSIX system
  contrib/vhost-user-blk: enable it on any POSIX system
  hostmem: add a new memory backend based on POSIX shm_open()
  tests/qtest/vhost-user-blk-test: use memory-backend-shm
  tests/qtest/vhost-user-test: add a test case for memory-backend-shm

 docs/system/devices/vhost-user.rst        |   5 +-
 meson.build                               |   5 +-
 qapi/qom.json                             |  17 ++++
 subprojects/libvhost-user/libvhost-user.h |   2 +-
 backends/hostmem-shm.c                    | 118 ++++++++++++++++++++++
 contrib/vhost-user-blk/vhost-user-blk.c   |  23 ++++-
 hw/net/vhost_net.c                        |   5 +
 subprojects/libvhost-user/libvhost-user.c |  76 +++++++++++++-
 tests/qtest/vhost-user-blk-test.c         |   2 +-
 tests/qtest/vhost-user-test.c             |  23 +++++
 util/vhost-user-server.c                  |   6 +-
 backends/meson.build                      |   1 +
 hw/block/Kconfig                          |   2 +-
 qemu-options.hx                           |  10 ++
 util/meson.build                          |   4 +-
 15 files changed, 280 insertions(+), 19 deletions(-)
 create mode 100644 backends/hostmem-shm.c

-- 
2.44.0

[PATCH for-9.1 v2 01/11] libvhost-user: set msg.msg_control to NULL when it is empty

[Stefano Garzarella]

On some OS (e.g. macOS) sendmsg() returns -1 (errno EINVAL) if
the `struct msghdr` has the field `msg_controllen` set to 0, but
`msg_control` is not NULL.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index a879149fef..22bea0c775 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -632,6 +632,7 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
         memcpy(CMSG_DATA(cmsg), vmsg->fds, fdsize);
     } else {
         msg.msg_controllen = 0;
+        msg.msg_control = NULL;
     }
 
     do {
-- 
2.44.0

[PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

[Stefano Garzarella]

In vu_message_write() we use sendmsg() to send the message header,
then a write() to send the payload.

If sendmsg() fails we should avoid sending the payload, since we
were unable to send the header.

Discovered before fixing the issue with the previous patch, where
sendmsg() failed on macOS due to wrong parameters, but the frontend
still sent the payload which the backend incorrectly interpreted
as a wrong header.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 22bea0c775..a11afd1960 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
         rc = sendmsg(conn_fd, &msg, 0);
     } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
 
+    if (rc <= 0) {
+        vu_panic(dev, "Error while writing: %s", strerror(errno));
+        return false;
+    }
+
     if (vmsg->size) {
         do {
             if (vmsg->data) {
-- 
2.44.0

[PATCH for-9.1 v2 03/11] libvhost-user: mask F_INFLIGHT_SHMFD if memfd is not supported

[Stefano Garzarella]

libvhost-user will panic when receiving VHOST_USER_GET_INFLIGHT_FD
message if MFD_ALLOW_SEALING is not defined, since it's not able
to create a memfd.

VHOST_USER_GET_INFLIGHT_FD is used only if
VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD is negotiated. So, let's mask
that feature if the backend is not able to properly handle these
messages.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index a11afd1960..1c361ffd51 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1674,6 +1674,16 @@ vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg)
         features |= dev->iface->get_protocol_features(dev);
     }
 
+    /*
+     * If MFD_ALLOW_SEALING is not defined, we are not able to handle
+     * VHOST_USER_GET_INFLIGHT_FD messages, since we can't create a memfd.
+     * Those messages are used only if VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD
+     * is negotiated. A device implementation can enable it, so let's mask
+     * it to avoid a runtime panic.
+     */
+#ifndef MFD_ALLOW_SEALING
+    features &= ~(1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD);
+#endif
     vmsg_set_reply_u64(vmsg, features);
     return true;
 }
-- 
2.44.0

[PATCH for-9.1 v2 04/11] vhost-user-server: don't abort if we can't set fd non-blocking

[Stefano Garzarella]

In vhost-user-server we set all fd received from the other peer
in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.)
if we fail, it's not really a problem, because we don't use these
fd with blocking operations, but only to map memory.

In these cases a failure is not bad, so let's just report a warning
instead of panicking if we fail to set some fd in non-blocking mode.

This for example occurs in macOS where setting shm_open() fd
non-blocking is failing (errno: 25).

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 util/vhost-user-server.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 3bfb1ad3ec..064999f0b7 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -66,7 +66,11 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg)
 {
     int i;
     for (i = 0; i < vmsg->fd_num; i++) {
-        qemu_socket_set_nonblock(vmsg->fds[i]);
+        int ret = qemu_socket_try_set_nonblock(vmsg->fds[i]);
+        if (ret) {
+            warn_report("Failed to set fd %d nonblock for request %d: %s",
+                        vmsg->fds[i], vmsg->request, strerror(-ret));
+        }
     }
 }
 
-- 
2.44.0

[PATCH for-9.1 v2 05/11] contrib/vhost-user-blk: fix bind() using the right size of the address

[Stefano Garzarella]

On macOS passing `-s /tmp/vhost.socket` parameter to the vhost-user-blk
application, the bind was done on `/tmp/vhost.socke` pathname,
missing the last character.

This sounds like one of the portability problems described in the
unix(7) manpage:

   Pathname sockets
       When  binding  a socket to a pathname, a few rules should
       be observed for maximum portability and ease of coding:

       •  The pathname in sun_path should be null-terminated.

       •  The length of the pathname, including the  terminating
          null byte, should not exceed the size of sun_path.

       •  The  addrlen  argument  that  describes  the enclosing
          sockaddr_un structure should have a value of at least:

              offsetof(struct sockaddr_un, sun_path) +
              strlen(addr.sun_path)+1

          or,  more  simply,  addrlen  can   be   specified   as
          sizeof(struct sockaddr_un).

So let's follow the last advice and simplify the code as well.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 contrib/vhost-user-blk/vhost-user-blk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
index 89e5f11a64..a8ab9269a2 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -469,7 +469,6 @@ static int unix_sock_new(char *unix_fn)
 {
     int sock;
     struct sockaddr_un un;
-    size_t len;
 
     assert(unix_fn);
 
@@ -481,10 +480,9 @@ static int unix_sock_new(char *unix_fn)
 
     un.sun_family = AF_UNIX;
     (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn);
-    len = sizeof(un.sun_family) + strlen(un.sun_path);
 
     (void)unlink(unix_fn);
-    if (bind(sock, (struct sockaddr *)&un, len) < 0) {
+    if (bind(sock, (struct sockaddr *)&un, sizeof(un)) < 0) {
         perror("bind");
         goto fail;
     }
-- 
2.44.0

[PATCH for-9.1 v2 06/11] vhost-user: enable frontends on any POSIX system

[Stefano Garzarella]

The vhost-user protocol is not really Linux-specific so let's enable
vhost-user frontends for any POSIX system.

In vhost_net.c we use VHOST_FILE_UNBIND which is defined in a Linux
specific header, let's define it for other systems as well.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 meson.build        | 1 -
 hw/net/vhost_net.c | 5 +++++
 hw/block/Kconfig   | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index c9c3217ba4..c19d51501a 100644
--- a/meson.build
+++ b/meson.build
@@ -151,7 +151,6 @@ have_tpm = get_option('tpm') \
 
 # vhost
 have_vhost_user = get_option('vhost_user') \
-  .disable_auto_if(host_os != 'linux') \
   .require(host_os != 'windows',
            error_message: 'vhost-user is not available on Windows').allowed()
 have_vhost_vdpa = get_option('vhost_vdpa') \
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e8e1661646..b8a59b9e90 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -34,8 +34,13 @@
 #include "standard-headers/linux/virtio_ring.h"
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/virtio-bus.h"
+#if defined(__linux__)
 #include "linux-headers/linux/vhost.h"
+#endif
 
+#ifndef VHOST_FILE_UNBIND
+#define VHOST_FILE_UNBIND -1
+#endif
 
 /* Features supported by host kernel. */
 static const int kernel_feature_bits[] = {
diff --git a/hw/block/Kconfig b/hw/block/Kconfig
index 9e8f28f982..29ee09e434 100644
--- a/hw/block/Kconfig
+++ b/hw/block/Kconfig
@@ -40,7 +40,7 @@ config VHOST_USER_BLK
     bool
     # Only PCI devices are provided for now
     default y if VIRTIO_PCI
-    depends on VIRTIO && VHOST_USER && LINUX
+    depends on VIRTIO && VHOST_USER
 
 config SWIM
     bool
-- 
2.44.0

[PATCH for-9.1 v2 07/11] libvhost-user: enable it on any POSIX system

[Stefano Garzarella]

The vhost-user protocol is not really Linux-specific so let's enable
libvhost-user for any POSIX system.

Compiling it on macOS and FreeBSD some problems came up:
- avoid to include linux/vhost.h which is avaibale only on Linux
  (vhost_types.h contains many of the things we need)
- macOS doesn't provide sys/endian.h, so let's define them
  (note: libvhost-user doesn't include qemu's headers, so we can't use
   use "qemu/bswap.h")
- define eventfd_[write|read] as write/read wrapper when system doesn't
  provide those (e.g. macOS)
- copy SEAL defines from include/qemu/memfd.h to make the code works
  on FreeBSD where MFD_ALLOW_SEALING is defined
- define MAP_NORESERVE if it's not defined (e.g. on FreeBSD)

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 meson.build                               |  2 +-
 subprojects/libvhost-user/libvhost-user.h |  2 +-
 subprojects/libvhost-user/libvhost-user.c | 60 +++++++++++++++++++++--
 3 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index c19d51501a..3197a2f62e 100644
--- a/meson.build
+++ b/meson.build
@@ -3194,7 +3194,7 @@ endif
 config_host_data.set('CONFIG_FDT', fdt.found())
 
 vhost_user = not_found
-if host_os == 'linux' and have_vhost_user
+if have_vhost_user
   libvhost_user = subproject('libvhost-user')
   vhost_user = libvhost_user.get_variable('vhost_user_dep')
 endif
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index deb40e77b3..e13e1d3931 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -18,9 +18,9 @@
 #include <stdbool.h>
 #include <stddef.h>
 #include <poll.h>
-#include <linux/vhost.h>
 #include <pthread.h>
 #include "standard-headers/linux/virtio_ring.h"
+#include "standard-headers/linux/vhost_types.h"
 
 /* Based on qemu/hw/virtio/vhost-user.c */
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 1c361ffd51..03edb4bf64 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -28,9 +28,7 @@
 #include <inttypes.h>
 #include <sys/types.h>
 #include <sys/socket.h>
-#include <sys/eventfd.h>
 #include <sys/mman.h>
-#include <endian.h>
 
 /* Necessary to provide VIRTIO_F_VERSION_1 on system
  * with older linux headers. Must appear before
@@ -39,8 +37,8 @@
 #include "standard-headers/linux/virtio_config.h"
 
 #if defined(__linux__)
+#include <endian.h>
 #include <sys/syscall.h>
-#include <fcntl.h>
 #include <sys/ioctl.h>
 #include <linux/vhost.h>
 #include <sys/vfs.h>
@@ -52,6 +50,62 @@
 
 #endif
 
+#if defined(__APPLE__) && (__MACH__)
+#include <libkern/OSByteOrder.h>
+#define htobe16(x) OSSwapHostToBigInt16(x)
+#define htole16(x) OSSwapHostToLittleInt16(x)
+#define be16toh(x) OSSwapBigToHostInt16(x)
+#define le16toh(x) OSSwapLittleToHostInt16(x)
+
+#define htobe32(x) OSSwapHostToBigInt32(x)
+#define htole32(x) OSSwapHostToLittleInt32(x)
+#define be32toh(x) OSSwapBigToHostInt32(x)
+#define le32toh(x) OSSwapLittleToHostInt32(x)
+
+#define htobe64(x) OSSwapHostToBigInt64(x)
+#define htole64(x) OSSwapHostToLittleInt64(x)
+#define be64toh(x) OSSwapBigToHostInt64(x)
+#define le64toh(x) OSSwapLittleToHostInt64(x)
+#endif
+
+#ifdef CONFIG_EVENTFD
+#include <sys/eventfd.h>
+#else
+#define eventfd_t uint64_t
+
+int eventfd_write(int fd, eventfd_t value)
+{
+    return (write(fd, &value, sizeof(value)) == sizeof(value)) ? 0 : -1;
+}
+
+int eventfd_read(int fd, eventfd_t *value)
+{
+    return (read(fd, value, sizeof(*value)) == sizeof(*value)) ? 0 : -1;
+}
+#endif
+
+#ifdef MFD_ALLOW_SEALING
+#include <fcntl.h>
+
+#ifndef F_LINUX_SPECIFIC_BASE
+#define F_LINUX_SPECIFIC_BASE 1024
+#endif
+
+#ifndef F_ADD_SEALS
+#define F_ADD_SEALS (F_LINUX_SPECIFIC_BASE + 9)
+#define F_GET_SEALS (F_LINUX_SPECIFIC_BASE + 10)
+
+#define F_SEAL_SEAL     0x0001  /* prevent further seals from being set */
+#define F_SEAL_SHRINK   0x0002  /* prevent file from shrinking */
+#define F_SEAL_GROW     0x0004  /* prevent file from growing */
+#define F_SEAL_WRITE    0x0008  /* prevent writes */
+#endif
+#endif
+
+#ifndef MAP_NORESERVE
+#define MAP_NORESERVE 0
+#endif
+
 #include "include/atomic.h"
 
 #include "libvhost-user.h"
-- 
2.44.0

[PATCH for-9.1 v2 08/11] contrib/vhost-user-blk: enable it on any POSIX system

[Stefano Garzarella]

Let's make the code more portable by using the "qemu/bswap.h" API
and adding defines from block/file-posix.c to support O_DIRECT in
other systems (e.g. macOS).

vhost-user-server.c is a dependency, let's enable it for any POSIX
system.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 meson.build                             |  2 --
 contrib/vhost-user-blk/vhost-user-blk.c | 19 +++++++++++++++++--
 util/meson.build                        |  4 +++-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index 3197a2f62e..b541e5c875 100644
--- a/meson.build
+++ b/meson.build
@@ -1956,8 +1956,6 @@ has_statx = cc.has_header_symbol('sys/stat.h', 'STATX_BASIC_STATS', prefix: gnu_
 has_statx_mnt_id = cc.has_header_symbol('sys/stat.h', 'STATX_MNT_ID', prefix: gnu_source_prefix)
 
 have_vhost_user_blk_server = get_option('vhost_user_blk_server') \
-  .require(host_os == 'linux',
-           error_message: 'vhost_user_blk_server requires linux') \
   .require(have_vhost_user,
            error_message: 'vhost_user_blk_server requires vhost-user support') \
   .disable_auto_if(not have_tools and not have_system) \
diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
index a8ab9269a2..462e584857 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -16,6 +16,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
 #include "standard-headers/linux/virtio_blk.h"
 #include "libvhost-user-glib.h"
 
@@ -24,6 +25,20 @@
 #include <sys/ioctl.h>
 #endif
 
+/* OS X does not have O_DSYNC */
+#ifndef O_DSYNC
+#ifdef O_SYNC
+#define O_DSYNC O_SYNC
+#elif defined(O_FSYNC)
+#define O_DSYNC O_FSYNC
+#endif
+#endif
+
+/* Approximate O_DIRECT with O_DSYNC if O_DIRECT isn't available */
+#ifndef O_DIRECT
+#define O_DIRECT O_DSYNC
+#endif
+
 enum {
     VHOST_USER_BLK_MAX_QUEUES = 8,
 };
@@ -267,13 +282,13 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
     req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base;
     in_num--;
 
-    type = le32toh(req->out->type);
+    type = le32_to_cpu(req->out->type);
     switch (type & ~VIRTIO_BLK_T_BARRIER) {
     case VIRTIO_BLK_T_IN:
     case VIRTIO_BLK_T_OUT: {
         ssize_t ret = 0;
         bool is_write = type & VIRTIO_BLK_T_OUT;
-        req->sector_num = le64toh(req->out->sector);
+        req->sector_num = le64_to_cpu(req->out->sector);
         if (is_write) {
             ret  = vub_writev(req, &elem->out_sg[1], out_num);
         } else {
diff --git a/util/meson.build b/util/meson.build
index 0ef9886be0..f52682ce96 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -113,10 +113,12 @@ if have_block
     util_ss.add(files('filemonitor-stub.c'))
   endif
   if host_os == 'linux'
-    util_ss.add(files('vhost-user-server.c'), vhost_user)
     util_ss.add(files('vfio-helpers.c'))
     util_ss.add(files('chardev_open.c'))
   endif
+  if host_os != 'windows'
+    util_ss.add(files('vhost-user-server.c'), vhost_user)
+  endif
 endif
 
 if cpu == 'aarch64'
-- 
2.44.0

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

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

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 docs/system/devices/vhost-user.rst |   5 +-
 qapi/qom.json                      |  17 +++++
 backends/hostmem-shm.c             | 118 +++++++++++++++++++++++++++++
 backends/meson.build               |   1 +
 qemu-options.hx                    |  10 +++
 5 files changed, 149 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 baae3a183f..88136b861d 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -720,6 +720,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:
 #
@@ -976,6 +989,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' },
@@ -1047,6 +1062,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..7595204d29
--- /dev/null
+++ b/backends/hostmem-shm.c
@@ -0,0 +1,118 @@
+/*
+ * 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 backend with size 0");
+        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 = backend->share ? RAM_SHARED : 0;
+    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 7fd1713fa8..70fa98ba19 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5230,6 +5230,16 @@ 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).
+
+        Please refer to ``memory-backend-file`` for a description of the
+        options.
+
+        The ``share`` boolean option is on by default with shm.
+
     ``-object iommufd,id=id[,fd=fd]``
         Creates an iommufd backend which allows control of DMA mapping
         through the ``/dev/iommu`` device.
-- 
2.44.0

[PATCH for-9.1 v2 10/11] tests/qtest/vhost-user-blk-test: use memory-backend-shm

[Stefano Garzarella]

`memory-backend-memfd` is available only on Linux while the new
`memory-backend-shm` can be used on any POSIX-compliant operating
system. Let's use it so we can run the test in multiple environments.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tests/qtest/vhost-user-blk-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c
index 117b9acd10..e945f6abf2 100644
--- a/tests/qtest/vhost-user-blk-test.c
+++ b/tests/qtest/vhost-user-blk-test.c
@@ -906,7 +906,7 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances,
                            vhost_user_blk_bin);
 
     g_string_append_printf(cmd_line,
-            " -object memory-backend-memfd,id=mem,size=256M,share=on "
+            " -object memory-backend-shm,id=mem,size=256M,share=on "
             " -M memory-backend=mem -m 256M ");
 
     for (i = 0; i < vus_instances; i++) {
-- 
2.44.0

[PATCH for-9.1 v2 11/11] tests/qtest/vhost-user-test: add a test case for memory-backend-shm

[Stefano Garzarella]

`memory-backend-shm` can be used with vhost-user devices, so let's
add a new test case for it.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tests/qtest/vhost-user-test.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index d4e437265f..8c1d903b2a 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -44,6 +44,8 @@
                         "mem-path=%s,share=on -numa node,memdev=mem"
 #define QEMU_CMD_MEMFD  " -m %d -object memory-backend-memfd,id=mem,size=%dM," \
                         " -numa node,memdev=mem"
+#define QEMU_CMD_SHM    " -m %d -object memory-backend-shm,id=mem,size=%dM," \
+                        " -numa node,memdev=mem"
 #define QEMU_CMD_CHR    " -chardev socket,id=%s,path=%s%s"
 #define QEMU_CMD_NETDEV " -netdev vhost-user,id=hs0,chardev=%s,vhostforce=on"
 
@@ -195,6 +197,7 @@ enum test_memfd {
     TEST_MEMFD_AUTO,
     TEST_MEMFD_YES,
     TEST_MEMFD_NO,
+    TEST_MEMFD_SHM,
 };
 
 static void append_vhost_net_opts(TestServer *s, GString *cmd_line,
@@ -228,6 +231,8 @@ static void append_mem_opts(TestServer *server, GString *cmd_line,
 
     if (memfd == TEST_MEMFD_YES) {
         g_string_append_printf(cmd_line, QEMU_CMD_MEMFD, size, size);
+    } else if (memfd == TEST_MEMFD_SHM) {
+        g_string_append_printf(cmd_line, QEMU_CMD_SHM, size, size);
     } else {
         const char *root = init_hugepagefs() ? : server->tmpfs;
 
@@ -788,6 +793,19 @@ static void *vhost_user_test_setup_memfd(GString *cmd_line, void *arg)
     return server;
 }
 
+static void *vhost_user_test_setup_shm(GString *cmd_line, void *arg)
+{
+    TestServer *server = test_server_new("vhost-user-test", arg);
+    test_server_listen(server);
+
+    append_mem_opts(server, cmd_line, 256, TEST_MEMFD_SHM);
+    server->vu_ops->append_opts(server, cmd_line, "");
+
+    g_test_queue_destroy(vhost_user_test_cleanup, server);
+
+    return server;
+}
+
 static void test_read_guest_mem(void *obj, void *arg, QGuestAllocator *alloc)
 {
     TestServer *server = arg;
@@ -1081,6 +1099,11 @@ static void register_vhost_user_test(void)
                  "virtio-net",
                  test_read_guest_mem, &opts);
 
+    opts.before = vhost_user_test_setup_shm;
+    qos_add_test("vhost-user/read-guest-mem/shm",
+                 "virtio-net",
+                 test_read_guest_mem, &opts);
+
     if (qemu_memfd_check(MFD_ALLOW_SEALING)) {
         opts.before = vhost_user_test_setup_memfd;
         qos_add_test("vhost-user/read-guest-mem/memfd",
-- 
2.44.0

Re: [PATCH for-9.1 v2 01/11] libvhost-user: set msg.msg_control to NULL when it is empty

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:26PM +0100, Stefano Garzarella wrote:
> On some OS (e.g. macOS) sendmsg() returns -1 (errno EINVAL) if
> the `struct msghdr` has the field `msg_controllen` set to 0, but
> `msg_control` is not NULL.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index a879149fef..22bea0c775 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -632,6 +632,7 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>          memcpy(CMSG_DATA(cmsg), vmsg->fds, fdsize);
>      } else {
>          msg.msg_controllen = 0;
> +        msg.msg_control = NULL;
>      }
>  
>      do {
> -- 
> 2.44.0
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH for-9.1 v2 01/11] libvhost-user: set msg.msg_control to NULL when it is empty

[David Hildenbrand]

On 26.03.24 14:39, Stefano Garzarella wrote:
> On some OS (e.g. macOS) sendmsg() returns -1 (errno EINVAL) if
> the `struct msghdr` has the field `msg_controllen` set to 0, but
> `msg_control` is not NULL.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>   subprojects/libvhost-user/libvhost-user.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index a879149fef..22bea0c775 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -632,6 +632,7 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>           memcpy(CMSG_DATA(cmsg), vmsg->fds, fdsize);
>       } else {
>           msg.msg_controllen = 0;
> +        msg.msg_control = NULL;
>       }
>   
>       do {

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
> In vu_message_write() we use sendmsg() to send the message header,
> then a write() to send the payload.
> 
> If sendmsg() fails we should avoid sending the payload, since we
> were unable to send the header.
> 
> Discovered before fixing the issue with the previous patch, where
> sendmsg() failed on macOS due to wrong parameters, but the frontend
> still sent the payload which the backend incorrectly interpreted
> as a wrong header.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 22bea0c775..a11afd1960 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>          rc = sendmsg(conn_fd, &msg, 0);
>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>  
> +    if (rc <= 0) {

Is rejecting a 0 return value correct?  Technically, a 0 return means
a successful write of 0 bytes - but then again, it is unwise to
attempt to send an fd or other auxilliary ddata without at least one
regular byte, so we should not be attempting a write of 0 bytes.  So I
guess this one is okay, although I might have used < instead of <=.

> +        vu_panic(dev, "Error while writing: %s", strerror(errno));
> +        return false;
> +    }

At any rate, noticing the error is the correct thing to do.

> +
>      if (vmsg->size) {
>          do {
>              if (vmsg->data) {
> -- 
> 2.44.0
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

[David Hildenbrand]

On 26.03.24 15:34, Eric Blake wrote:
> On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
>> In vu_message_write() we use sendmsg() to send the message header,
>> then a write() to send the payload.
>>
>> If sendmsg() fails we should avoid sending the payload, since we
>> were unable to send the header.
>>
>> Discovered before fixing the issue with the previous patch, where
>> sendmsg() failed on macOS due to wrong parameters, but the frontend
>> still sent the payload which the backend incorrectly interpreted
>> as a wrong header.
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>   subprojects/libvhost-user/libvhost-user.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
>> index 22bea0c775..a11afd1960 100644
>> --- a/subprojects/libvhost-user/libvhost-user.c
>> +++ b/subprojects/libvhost-user/libvhost-user.c
>> @@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>>           rc = sendmsg(conn_fd, &msg, 0);
>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>   
>> +    if (rc <= 0) {
> 
> Is rejecting a 0 return value correct?  Technically, a 0 return means
> a successful write of 0 bytes - but then again, it is unwise to
> attempt to send an fd or other auxilliary ddata without at least one
> regular byte, so we should not be attempting a write of 0 bytes.  So I
> guess this one is okay, although I might have used < instead of <=.

I was wondering if we could see some partial sendmsg()/write succeeding. 
Meaning, we transferred some bytes but not all, and we'd actually need 
to loop ...

-- 
Cheers,

David / dhildenb

Re: [PATCH for-9.1 v2 03/11] libvhost-user: mask F_INFLIGHT_SHMFD if memfd is not supported

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:28PM +0100, Stefano Garzarella wrote:
> libvhost-user will panic when receiving VHOST_USER_GET_INFLIGHT_FD
> message if MFD_ALLOW_SEALING is not defined, since it's not able
> to create a memfd.
> 
> VHOST_USER_GET_INFLIGHT_FD is used only if
> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD is negotiated. So, let's mask
> that feature if the backend is not able to properly handle these
> messages.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index a11afd1960..1c361ffd51 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -1674,6 +1674,16 @@ vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg)
>          features |= dev->iface->get_protocol_features(dev);
>      }
>  
> +    /*
> +     * If MFD_ALLOW_SEALING is not defined, we are not able to handle
> +     * VHOST_USER_GET_INFLIGHT_FD messages, since we can't create a memfd.
> +     * Those messages are used only if VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD
> +     * is negotiated. A device implementation can enable it, so let's mask
> +     * it to avoid a runtime panic.
> +     */
> +#ifndef MFD_ALLOW_SEALING
> +    features &= ~(1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD);
> +#endif

Masking the feature out of advertisement is obviously correct. But
should we also fix the code for handling
VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD to return an error to any client
that requests it in error when the feature was not advertised, instead
of panicking?

>      vmsg_set_reply_u64(vmsg, features);
>      return true;
>  }
> -- 
> 2.44.0
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH for-9.1 v2 04/11] vhost-user-server: don't abort if we can't set fd non-blocking

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:29PM +0100, Stefano Garzarella wrote:
> In vhost-user-server we set all fd received from the other peer
> in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.)
> if we fail, it's not really a problem, because we don't use these
> fd with blocking operations, but only to map memory.
> 
> In these cases a failure is not bad, so let's just report a warning
> instead of panicking if we fail to set some fd in non-blocking mode.
> 
> This for example occurs in macOS where setting shm_open() fd
> non-blocking is failing (errno: 25).

What is errno 25 on MacOS?

> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  util/vhost-user-server.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
> index 3bfb1ad3ec..064999f0b7 100644
> --- a/util/vhost-user-server.c
> +++ b/util/vhost-user-server.c
> @@ -66,7 +66,11 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg)
>  {
>      int i;
>      for (i = 0; i < vmsg->fd_num; i++) {
> -        qemu_socket_set_nonblock(vmsg->fds[i]);
> +        int ret = qemu_socket_try_set_nonblock(vmsg->fds[i]);
> +        if (ret) {

Should this be 'if (ret < 0)'?

> +            warn_report("Failed to set fd %d nonblock for request %d: %s",
> +                        vmsg->fds[i], vmsg->request, strerror(-ret));
> +        }

This now ignores all errors even on pre-existing fds where we NEED
non-blocking, rather than just the specific (expected) error we are
seeing on MacOS.  Should this code be a bit more precise about
checking that -ret == EXXX for the expected errno value we are
ignoring for the specific fds where non-blocking is not essential?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[David Hildenbrand]

> +    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);

Hm, shouldn't we just let the user specify a name, and if no name was 
specified, generate one ourselves?

I'm also not quite sure if "host_memory_backend_get_name()" should be 
used for the purpose here.

-- 
Cheers,

David / dhildenb

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

[Stefano Garzarella]

On Tue, Mar 26, 2024 at 03:36:52PM +0100, David Hildenbrand wrote:
>On 26.03.24 15:34, Eric Blake wrote:
>>On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
>>>In vu_message_write() we use sendmsg() to send the message header,
>>>then a write() to send the payload.
>>>
>>>If sendmsg() fails we should avoid sending the payload, since we
>>>were unable to send the header.
>>>
>>>Discovered before fixing the issue with the previous patch, where
>>>sendmsg() failed on macOS due to wrong parameters, but the frontend
>>>still sent the payload which the backend incorrectly interpreted
>>>as a wrong header.
>>>
>>>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>>---
>>>  subprojects/libvhost-user/libvhost-user.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>>diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
>>>index 22bea0c775..a11afd1960 100644
>>>--- a/subprojects/libvhost-user/libvhost-user.c
>>>+++ b/subprojects/libvhost-user/libvhost-user.c
>>>@@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>>>          rc = sendmsg(conn_fd, &msg, 0);
>>>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>>+    if (rc <= 0) {
>>
>>Is rejecting a 0 return value correct?  Technically, a 0 return means
>>a successful write of 0 bytes - but then again, it is unwise to
>>attempt to send an fd or other auxilliary ddata without at least one
>>regular byte, so we should not be attempting a write of 0 bytes.  So I
>>guess this one is okay, although I might have used < instead of <=.

I blindly copied what they already do at the end of the same function.
However, your observation is correct. That said they have a sendmsg()
to send the header. After this we have a write() loop to send the
payload.

Now if sendmsg() returns 0, but we have not sent all the header, what
should we do? Try sendmsg() again? For how many times?

>
>I was wondering if we could see some partial sendmsg()/write 
>succeeding. Meaning, we transferred some bytes but not all, and we'd 
>actually need to loop ...

Yep, true, but I would fix it in another patch/series if you agree.

Thanks,
Stefano

Re: [PATCH for-9.1 v2 03/11] libvhost-user: mask F_INFLIGHT_SHMFD if memfd is not supported

[Stefano Garzarella]

On Tue, Mar 26, 2024 at 09:36:54AM -0500, Eric Blake wrote:
>On Tue, Mar 26, 2024 at 02:39:28PM +0100, Stefano Garzarella wrote:
>> libvhost-user will panic when receiving VHOST_USER_GET_INFLIGHT_FD
>> message if MFD_ALLOW_SEALING is not defined, since it's not able
>> to create a memfd.
>>
>> VHOST_USER_GET_INFLIGHT_FD is used only if
>> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD is negotiated. So, let's mask
>> that feature if the backend is not able to properly handle these
>> messages.
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  subprojects/libvhost-user/libvhost-user.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
>> index a11afd1960..1c361ffd51 100644
>> --- a/subprojects/libvhost-user/libvhost-user.c
>> +++ b/subprojects/libvhost-user/libvhost-user.c
>> @@ -1674,6 +1674,16 @@ vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg)
>>          features |= dev->iface->get_protocol_features(dev);
>>      }
>>
>> +    /*
>> +     * If MFD_ALLOW_SEALING is not defined, we are not able to handle
>> +     * VHOST_USER_GET_INFLIGHT_FD messages, since we can't create a memfd.
>> +     * Those messages are used only if VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD
>> +     * is negotiated. A device implementation can enable it, so let's mask
>> +     * it to avoid a runtime panic.
>> +     */
>> +#ifndef MFD_ALLOW_SEALING
>> +    features &= ~(1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD);
>> +#endif
>
>Masking the feature out of advertisement is obviously correct. But
>should we also fix the code for handling
>VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD to return an error to any client
>that requests it in error when the feature was not advertised, instead
>of panicking?

Totally agree!

Do I send a separate patch from this series or include it in this
series?
I would do the former because this one is already long enough.

Thanks,
Stefano

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

[David Hildenbrand]

>>
>> I was wondering if we could see some partial sendmsg()/write
>> succeeding. Meaning, we transferred some bytes but not all, and we'd
>> actually need to loop ...
> 
> Yep, true, but I would fix it in another patch/series if you agree.

Absolutely.

-- 
Cheers,

David / dhildenb

Re: [PATCH for-9.1 v2 04/11] vhost-user-server: don't abort if we can't set fd non-blocking

[Stefano Garzarella]

On Tue, Mar 26, 2024 at 09:40:12AM -0500, Eric Blake wrote:
>On Tue, Mar 26, 2024 at 02:39:29PM +0100, Stefano Garzarella wrote:
>> In vhost-user-server we set all fd received from the other peer
>> in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.)
>> if we fail, it's not really a problem, because we don't use these
>> fd with blocking operations, but only to map memory.
>>
>> In these cases a failure is not bad, so let's just report a warning
>> instead of panicking if we fail to set some fd in non-blocking mode.
>>
>> This for example occurs in macOS where setting shm_open() fd
>> non-blocking is failing (errno: 25).
>
>What is errno 25 on MacOS?

It should be ENOTTY.
I'll add in the commit description.

>
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  util/vhost-user-server.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
>> index 3bfb1ad3ec..064999f0b7 100644
>> --- a/util/vhost-user-server.c
>> +++ b/util/vhost-user-server.c
>> @@ -66,7 +66,11 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg)
>>  {
>>      int i;
>>      for (i = 0; i < vmsg->fd_num; i++) {
>> -        qemu_socket_set_nonblock(vmsg->fds[i]);
>> +        int ret = qemu_socket_try_set_nonblock(vmsg->fds[i]);
>> +        if (ret) {
>
>Should this be 'if (ret < 0)'?

I was confused by the assert() in qemu_socket_set_nonblock():
     void qemu_socket_set_nonblock(int fd)
     {
         int f;
         f = qemu_socket_try_set_nonblock(fd);
         assert(f == 0);
     }

BTW, I see most of the code checks ret < 0, so I'll fix it.

>
>> +            warn_report("Failed to set fd %d nonblock for request %d: %s",
>> +                        vmsg->fds[i], vmsg->request, strerror(-ret));
>> +        }
>
>This now ignores all errors even on pre-existing fds where we NEED
>non-blocking, rather than just the specific (expected) error we are
>seeing on MacOS.  Should this code be a bit more precise about
>checking that -ret == EXXX for the expected errno value we are
>ignoring for the specific fds where non-blocking is not essential?

Good point, maybe I'll just avoid calling vmsg_unblock_fds() when the
message is VHOST_USER_ADD_MEM_REG or VHOST_USER_SET_MEM_TABLE.

These should be the cases where carried fds are used for mmap() and so
there is no need to mark them non-blocking.

WDYT?

Stefano

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

[Stefano Garzarella]

On Tue, Mar 26, 2024 at 03:45:52PM +0100, David Hildenbrand wrote:
>>+    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);
>
>Hm, shouldn't we just let the user specify a name, and if no name was 
>specified, generate one ourselves?

I thought about it and initially did it that way, but then some problems 
came up so I tried to keep it as simple as possible for the user and for 
our use case (having an fd associated with memory and sharing it with 
other processes).

The problems I had were:

- what mode_t to use if the object does not exist and needs to be 
   created?

- exclude O_EXCL if the user passes the name since they may have already 
   created it?

- call shm_unlink() only at object deallocation?

So I thought that for now we only support the "anonymous" mode, and if 
in the future we have a use case where the user wants to specify the 
name, we can add it later.

That said, if you think it's already useful from the beginning, I can 
add the name as an optional parameter.

>
>I'm also not quite sure if "host_memory_backend_get_name()" should be 
>used for the purpose here.

What problem do you see? As an alternative I thought of a static 
counter.

Thanks,
Stefano

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

[David Hildenbrand]

On 27.03.24 11:23, Stefano Garzarella wrote:
> On Tue, Mar 26, 2024 at 03:45:52PM +0100, David Hildenbrand wrote:
>>> +    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);
>>
>> Hm, shouldn't we just let the user specify a name, and if no name was
>> specified, generate one ourselves?
> 
> I thought about it and initially did it that way, but then some problems
> came up so I tried to keep it as simple as possible for the user and for
> our use case (having an fd associated with memory and sharing it with
> other processes).
> 
> The problems I had were:
> 
> - what mode_t to use if the object does not exist and needs to be
>     created? >
> - exclude O_EXCL if the user passes the name since they may have already
>     created it?

I'd handle both like we handle files. But I understand now that you 
really just want to "simulate" memfd.

> 
> - call shm_unlink() only at object deallocation?

Right, we don't really do that for ordinary files, they keep existing. 
We only have the "discard-data" property to free up memory.

For memfd, it just happens "automatically". They cannot be looked up by 
name, and once the last user closes the fd, it gets destroyed.

> 
> So I thought that for now we only support the "anonymous" mode, and if
> in the future we have a use case where the user wants to specify the
> name, we can add it later.

Okay, so for now you really only want an anonymous fd that behaves like 
a memfd, got it.

Likely we should somehow fail if the "POSIX shared memory object" 
already exists. Hmm.

> 
> That said, if you think it's already useful from the beginning, I can
> add the name as an optional parameter.
> 
>>
>> I'm also not quite sure if "host_memory_backend_get_name()" should be
>> used for the purpose here.
> 
> What problem do you see? As an alternative I thought of a static
> counter.

If it's really just an "internal UUID", as we'll remove the file using 
shm_unlink(), then the name in fact shouldn't matter and this would be 
fine. Correct?


So I assume if we ever want to have non-anonymous fds here, we'd pass in 
a new property "name", and change the logic how we open/unlink. Makes sense.

-- 
Cheers,

David / dhildenb

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

[Stefano Garzarella]

On Wed, Mar 27, 2024 at 12:51:51PM +0100, David Hildenbrand wrote:
>On 27.03.24 11:23, Stefano Garzarella wrote:
>>On Tue, Mar 26, 2024 at 03:45:52PM +0100, David Hildenbrand wrote:
>>>>+    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);
>>>
>>>Hm, shouldn't we just let the user specify a name, and if no name was
>>>specified, generate one ourselves?
>>
>>I thought about it and initially did it that way, but then some problems
>>came up so I tried to keep it as simple as possible for the user and for
>>our use case (having an fd associated with memory and sharing it with
>>other processes).
>>
>>The problems I had were:
>>
>>- what mode_t to use if the object does not exist and needs to be
>>    created? >
>>- exclude O_EXCL if the user passes the name since they may have already
>>    created it?
>
>I'd handle both like we handle files. But I understand now that you 
>really just want to "simulate" memfd.

Right, maybe I should write that in the commit message and 
documentation.

>
>>
>>- call shm_unlink() only at object deallocation?
>
>Right, we don't really do that for ordinary files, they keep existing. 
>We only have the "discard-data" property to free up memory.
>
>For memfd, it just happens "automatically". They cannot be looked up 
>by name, and once the last user closes the fd, it gets destroyed.

Yep, I see.

>
>>
>>So I thought that for now we only support the "anonymous" mode, and if
>>in the future we have a use case where the user wants to specify the
>>name, we can add it later.
>
>Okay, so for now you really only want an anonymous fd that behaves like 
>a memfd, got it.
>
>Likely we should somehow fail if the "POSIX shared memory object" 
>already exists. Hmm.

`O_CREAT | O_EXCL` should provide just this behavior.
 From shm_open(3) manpage:

     O_EXCL  If O_CREAT was also specified, and a shared memory object
             with the given name already exists, return an error.  The
             check for the existence of the object, and its creation if
             it does not exist, are performed atomically.

>
>>
>>That said, if you think it's already useful from the beginning, I can
>>add the name as an optional parameter.
>>
>>>
>>>I'm also not quite sure if "host_memory_backend_get_name()" should be
>>>used for the purpose here.
>>
>>What problem do you see? As an alternative I thought of a static
>>counter.
>
>If it's really just an "internal UUID", as we'll remove the file using 
>shm_unlink(), then the name in fact shouldn't matter and this would be 
>fine. Correct?

Right. It's a name that will only "appear" in the system for a very 
short time since we call shm_unlink() right after shm_open(). So I just 
need the unique name to prevent several QEMUs launched at the same time 
from colliding with the name.

>
>So I assume if we ever want to have non-anonymous fds here, we'd pass 
>in a new property "name", and change the logic how we open/unlink.  
>Makes sense.

Exactly! I can clarify this in the commit description as well.

Thanks for the helpful comments!
If there is anything I need to change for v3, please tell me ;-)

Stefano

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

[David Hildenbrand]

>>>
>>> So I thought that for now we only support the "anonymous" mode, and if
>>> in the future we have a use case where the user wants to specify the
>>> name, we can add it later.
>>
>> Okay, so for now you really only want an anonymous fd that behaves like
>> a memfd, got it.
>>
>> Likely we should somehow fail if the "POSIX shared memory object"
>> already exists. Hmm.
> 
> `O_CREAT | O_EXCL` should provide just this behavior.
>   From shm_open(3) manpage:
> 
>       O_EXCL  If O_CREAT was also specified, and a shared memory object
>               with the given name already exists, return an error.  The
>               check for the existence of the object, and its creation if
>               it does not exist, are performed atomically.
> 

Cool!

>>
>>>
>>> That said, if you think it's already useful from the beginning, I can
>>> add the name as an optional parameter.
>>>
>>>>
>>>> I'm also not quite sure if "host_memory_backend_get_name()" should be
>>>> used for the purpose here.
>>>
>>> What problem do you see? As an alternative I thought of a static
>>> counter.
>>
>> If it's really just an "internal UUID", as we'll remove the file using
>> shm_unlink(), then the name in fact shouldn't matter and this would be
>> fine. Correct?
> 
> Right. It's a name that will only "appear" in the system for a very
> short time since we call shm_unlink() right after shm_open(). So I just
> need the unique name to prevent several QEMUs launched at the same time
> from colliding with the name.

Okay, essentially emulating tmpfile(), but for weird shmem_open() :)

> 
>>
>> So I assume if we ever want to have non-anonymous fds here, we'd pass
>> in a new property "name", and change the logic how we open/unlink.
>> Makes sense.
> 
> Exactly! I can clarify this in the commit description as well.
> 
> Thanks for the helpful comments!
> If there is anything I need to change for v3, please tell me ;-)

Besides some patch description extension, makes sense to me :)

-- 
Cheers,

David / dhildenb