diff mbox series

[v9,06/20] multi-process: define MPQemuMsg format and transmission functions

Message ID 20200827181231.22778-7-elena.ufimtseva@oracle.com (mailing list archive)
State New, archived
Headers show
Series Initial support for multi-process Qemu | expand

Commit Message

Elena Ufimtseva Aug. 27, 2020, 6:12 p.m. UTC 
From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Defines MPQemuMsg, which is the message that is sent to the remote
process. This message is sent over QIOChannel and is used to
command the remote process to perform various tasks.

Also defined the helper functions to send and receive messages over the
QIOChannel

Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 MAINTAINERS              |   2 +
 include/io/mpqemu-link.h |  60 ++++++++++++++
 io/meson.build           |   2 +
 io/mpqemu-link.c         | 173 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 237 insertions(+)
 create mode 100644 include/io/mpqemu-link.h
 create mode 100644 io/mpqemu-link.c

Comments

Stefan Hajnoczi Sept. 23, 2020, 1:47 p.m. UTC | #1 
On Thu, Aug 27, 2020 at 11:12:17AM -0700, elena.ufimtseva@oracle.com wrote:
> +/**
> + * MPQemuCmd:
> + *
> + * MPQemuCmd enum type to specify the command to be executed on the remote
> + * device.
> + */
> +typedef enum {
> +    INIT = 0,
> +    MAX = INT_MAX,

enum members are in a global namespace. INIT and MAX are likely to
collide with other code using these names. Please use MPQEMU_CMD_INIT
and MPQEMU_CMD_MAX.

Why is MAX = INT_MAX? I expected MAX to be related to the number of
commands that have been defined (1 so far).

> +} MPQemuCmd;
> +
> +/**
> + * MPQemuMsg:
> + * @cmd: The remote command
> + * @size: Size of the data to be shared
> + * @data: Structured data
> + * @fds: File descriptors to be shared with remote device
> + *
> + * MPQemuMsg Format of the message sent to the remote device from QEMU.
> + *
> + */
> +typedef struct {
> +    int cmd;
> +    size_t size;

I worry about the hole (compiler-inserted padding) between the cmd and
size fields.

It is safer to use fixed-size types and use QEMU_PACKED for structs that
are transferred over the network:

  typedef struct {
      int32_t cmd;
      uint64_t size;
      ...
  } QEMU_PACKED MPQemuMsg;

This way the struct layout is independent of the compilation environment
(32-/64-bit, ABI) and there is no risk of accidentally exposing memory
(information leaks) through holes.

> +
> +    union {
> +        uint64_t u64;
> +    } data;
> +
> +    int fds[REMOTE_MAX_FDS];
> +    int num_fds;
> +} MPQemuMsg;
> +
> +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp);
> +void mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc, Error **errp);
> +
> +bool mpqemu_msg_valid(MPQemuMsg *msg);
> +
> +#endif
> diff --git a/io/meson.build b/io/meson.build
> index 768c1b5ec3..3d40cd8867 100644
> --- a/io/meson.build
> +++ b/io/meson.build
> @@ -15,6 +15,8 @@ io_ss.add(files(
>    'task.c',
>  ))
>  
> +io_ss.add(when: 'CONFIG_MPQEMU', if_true: files('mpqemu-link.c'))
> +
>  io_ss = io_ss.apply(config_host, strict: false)
>  libio = static_library('io', io_ss.sources() + genh,
>                         dependencies: [io_ss.dependencies()],
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> new file mode 100644
> index 0000000000..d9be2bdeab
> --- /dev/null
> +++ b/io/mpqemu-link.c
> @@ -0,0 +1,173 @@
> +/*
> + * Communication channel between QEMU and remote device process
> + *
> + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> + *
> + * 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 "qemu-common.h"
> +
> +#include "qemu/module.h"
> +#include "io/mpqemu-link.h"
> +#include "qapi/error.h"
> +#include "qemu/iov.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +
> +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
> +{
> +    bool iolock = qemu_mutex_iothread_locked();
> +    Error *local_err = NULL;
> +    struct iovec send[2];
> +    int *fds = NULL;
> +    size_t nfds = 0;
> +
> +    send[0].iov_base = msg;
> +    send[0].iov_len = MPQEMU_MSG_HDR_SIZE;
> +
> +    send[1].iov_base = (void *)&msg->data;
> +    send[1].iov_len = msg->size;
> +
> +    if (msg->num_fds) {
> +        nfds = msg->num_fds;
> +        fds = msg->fds;
> +    }
> +
> +    if (iolock) {
> +        qemu_mutex_unlock_iothread();
> +    }
> +
> +    (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, nfds,
> +                                      &local_err);

I tried to understand when it is safe to call this function:

Thread    | Coroutine? | Comments
------------------------------------
Main loop | Yes        | Okay
Main loop | No         | Do not use, blocks main loop
vCPU      | Yes        | Careful, can move coroutine to main loop
vCPU      | No         | Okay if no other ioc activity in main loop
IOThread  | Yes        | Broken: we shouldn't touch the global mutex!
IOThread  | No         | Do not use, blocks IOThread

The IOThreads case with coroutines can be fixed by skipping
qemu_mutex_lock_iothread() when running in an IOThread. Please address
this.

Cases that look usable to me are:
1. Main loop with coroutines
2. vCPU without coroutines
3. IOThread with coroutines (needs fix though)

All this is not obvious so a comment and assertions would be good. The
following helpers are available for implementing assertions:
1. bool qemu_in_coroutine() -> true if running in a coroutine
2. qemu_get_current_aio_context() != qemu_get_aio_context() -> true in
   IOThread

> +
> +    if (iolock) {
> +        qemu_mutex_lock_iothread();
> +    }
> +
> +    if (errp) {
> +        error_propagate(errp, local_err);
> +    } else if (local_err) {
> +        error_report_err(local_err);
> +    }
> +
> +    return;
> +}
> +
> +static ssize_t mpqemu_read(QIOChannel *ioc, void *buf, size_t len, int **fds,
> +                           size_t *nfds, Error **errp)

The same constraints apply to this function.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 38d19c83cd..1ca1f8ccff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3045,6 +3045,8 @@  F: hw/pci-host/remote.c
 F: include/hw/pci-host/remote.h
 F: hw/i386/remote.c
 F: include/hw/i386/remote.h
+F: io/mpqemu-link.c
+F: include/io/mpqemu-link.h
 
 Build and test automation
 -------------------------
diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
new file mode 100644
index 0000000000..c7de8648bc
--- /dev/null
+++ b/include/io/mpqemu-link.h
@@ -0,0 +1,60 @@ 
+/*
+ * Communication channel between QEMU and remote device process
+ *
+ * Copyright © 2018, 2020 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef MPQEMU_LINK_H
+#define MPQEMU_LINK_H
+
+#include "qom/object.h"
+#include "qemu/thread.h"
+#include "io/channel.h"
+
+#define REMOTE_MAX_FDS 8
+
+#define MPQEMU_MSG_HDR_SIZE offsetof(MPQemuMsg, data.u64)
+
+/**
+ * MPQemuCmd:
+ *
+ * MPQemuCmd enum type to specify the command to be executed on the remote
+ * device.
+ */
+typedef enum {
+    INIT = 0,
+    MAX = INT_MAX,
+} MPQemuCmd;
+
+/**
+ * MPQemuMsg:
+ * @cmd: The remote command
+ * @size: Size of the data to be shared
+ * @data: Structured data
+ * @fds: File descriptors to be shared with remote device
+ *
+ * MPQemuMsg Format of the message sent to the remote device from QEMU.
+ *
+ */
+typedef struct {
+    int cmd;
+    size_t size;
+
+    union {
+        uint64_t u64;
+    } data;
+
+    int fds[REMOTE_MAX_FDS];
+    int num_fds;
+} MPQemuMsg;
+
+void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp);
+void mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc, Error **errp);
+
+bool mpqemu_msg_valid(MPQemuMsg *msg);
+
+#endif
diff --git a/io/meson.build b/io/meson.build
index 768c1b5ec3..3d40cd8867 100644
--- a/io/meson.build
+++ b/io/meson.build
@@ -15,6 +15,8 @@  io_ss.add(files(
   'task.c',
 ))
 
+io_ss.add(when: 'CONFIG_MPQEMU', if_true: files('mpqemu-link.c'))
+
 io_ss = io_ss.apply(config_host, strict: false)
 libio = static_library('io', io_ss.sources() + genh,
                        dependencies: [io_ss.dependencies()],
diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
new file mode 100644
index 0000000000..d9be2bdeab
--- /dev/null
+++ b/io/mpqemu-link.c
@@ -0,0 +1,173 @@ 
+/*
+ * Communication channel between QEMU and remote device process
+ *
+ * Copyright © 2018, 2020 Oracle and/or its affiliates.
+ *
+ * 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 "qemu-common.h"
+
+#include "qemu/module.h"
+#include "io/mpqemu-link.h"
+#include "qapi/error.h"
+#include "qemu/iov.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+
+void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
+{
+    bool iolock = qemu_mutex_iothread_locked();
+    Error *local_err = NULL;
+    struct iovec send[2];
+    int *fds = NULL;
+    size_t nfds = 0;
+
+    send[0].iov_base = msg;
+    send[0].iov_len = MPQEMU_MSG_HDR_SIZE;
+
+    send[1].iov_base = (void *)&msg->data;
+    send[1].iov_len = msg->size;
+
+    if (msg->num_fds) {
+        nfds = msg->num_fds;
+        fds = msg->fds;
+    }
+
+    if (iolock) {
+        qemu_mutex_unlock_iothread();
+    }
+
+    (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, nfds,
+                                      &local_err);
+
+    if (iolock) {
+        qemu_mutex_lock_iothread();
+    }
+
+    if (errp) {
+        error_propagate(errp, local_err);
+    } else if (local_err) {
+        error_report_err(local_err);
+    }
+
+    return;
+}
+
+static ssize_t mpqemu_read(QIOChannel *ioc, void *buf, size_t len, int **fds,
+                           size_t *nfds, Error **errp)
+{
+    struct iovec iov = { .iov_base = buf, .iov_len = len };
+    bool iolock = qemu_mutex_iothread_locked();
+    struct iovec *iovp = &iov;
+    Error *local_err = NULL;
+    unsigned int niov = 1;
+    size_t *l_nfds = nfds;
+    int **l_fds = fds;
+    ssize_t bytes = 0;
+    size_t size;
+
+    iov.iov_base = buf;
+    iov.iov_len = len;
+    size = iov.iov_len;
+
+    while (size > 0) {
+        bytes = qio_channel_readv_full(ioc, iovp, niov, l_fds, l_nfds,
+                                       &local_err);
+
+        if (bytes == QIO_CHANNEL_ERR_BLOCK) {
+            if (iolock) {
+                qemu_mutex_unlock_iothread();
+            }
+
+            if (qemu_in_coroutine()) {
+                qio_channel_yield(ioc, G_IO_IN);
+            } else {
+                qio_channel_wait(ioc, G_IO_IN);
+            }
+
+            if (iolock) {
+                qemu_mutex_lock_iothread();
+            }
+            continue;
+        }
+
+        if (bytes <= 0) {
+            error_propagate(errp, local_err);
+            return -EIO;
+        }
+
+        l_fds = NULL;
+        l_nfds = NULL;
+
+        size -= bytes;
+
+        (void)iov_discard_front(&iovp, &niov, bytes);
+    }
+
+    return len - size;
+}
+
+void mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
+{
+    Error *local_err = NULL;
+    int *fds = NULL;
+    size_t nfds = 0;
+    ssize_t len;
+
+    len = mpqemu_read(ioc, (void *)msg, MPQEMU_MSG_HDR_SIZE, &fds, &nfds,
+                      &local_err);
+    if (len < 0) {
+        goto fail;
+    } else if (len != MPQEMU_MSG_HDR_SIZE) {
+        error_setg(&local_err, "Message header corrupted");
+        goto fail;
+    }
+
+    if (msg->size > sizeof(msg->data)) {
+        error_setg(&local_err, "Invalid size for message");
+        goto fail;
+    }
+
+    if (mpqemu_read(ioc, (void *)&msg->data, msg->size, NULL, NULL,
+                    &local_err) < 0) {
+        goto fail;
+    }
+
+    msg->num_fds = nfds;
+    if (nfds) {
+        memcpy(msg->fds, fds, nfds * sizeof(int));
+    }
+
+fail:
+    if (errp) {
+        error_propagate(errp, local_err);
+    } else if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
+bool mpqemu_msg_valid(MPQemuMsg *msg)
+{
+    if (msg->cmd >= MAX && msg->cmd < 0) {
+        return false;
+    }
+
+    /* Verify FDs. */
+    if (msg->num_fds >= REMOTE_MAX_FDS) {
+        return false;
+    }
+
+    if (msg->num_fds > 0) {
+        for (int i = 0; i < msg->num_fds; i++) {
+            if (fcntl(msg->fds[i], F_GETFL) == -1) {
+                return false;
+            }
+        }
+    }
+
+    return true;
+}