| Message ID | b9bcba65f98a763669255dd1bc6533bc8310a235.1606853298.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for multi-process Qemu | expand |
Hi On Wed, Dec 2, 2020 at 12:25 AM Jagannathan Raman <jag.raman@oracle.com> wrote: > 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. > Define transmission functions used by proxy and by remote. > There are certain restrictions on where its safe to use these > functions: > - From main loop in co-routine context. Will block the main loop if not > in > co-routine context; > - From vCPU thread with no co-routine context and if the channel is not > part > of the main loop handling; > - From IOThread within co-routine context, outside of co-routine context > will > block IOThread; > > 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> > --- > include/hw/remote/mpqemu-link.h | 60 ++++++++++ > hw/remote/mpqemu-link.c | 242 > ++++++++++++++++++++++++++++++++++++++++ > MAINTAINERS | 2 + > hw/remote/meson.build | 1 + > 4 files changed, 305 insertions(+) > create mode 100644 include/hw/remote/mpqemu-link.h > create mode 100644 hw/remote/mpqemu-link.c > > diff --git a/include/hw/remote/mpqemu-link.h > b/include/hw/remote/mpqemu-link.h > new file mode 100644 > index 0000000..2d79ff8 > --- /dev/null > +++ b/include/hw/remote/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 { > + MPQEMU_CMD_INIT, > + MPQEMU_CMD_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/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c > new file mode 100644 > index 0000000..e535ed2 > --- /dev/null > +++ b/hw/remote/mpqemu-link.c > @@ -0,0 +1,242 @@ > +/* > + * 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 "hw/remote/mpqemu-link.h" > +#include "qapi/error.h" > +#include "qemu/iov.h" > +#include "qemu/error-report.h" > +#include "qemu/main-loop.h" > + > +/* > + * Send message over the ioc QIOChannel. > + * This function is safe to call from: > + * - From main loop in co-routine context. Will block the main loop if > not in > + * co-routine context; > + * - From vCPU thread with no co-routine context and if the channel is > not part > + * of the main loop handling; > + * - From IOThread within co-routine context, outside of co-routine > context > + * will block IOThread; > Can drop the extra "From" on each line. + */ > +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) > +{ > + bool iolock = qemu_mutex_iothread_locked(); > + bool iothread = qemu_get_current_aio_context() == > qemu_get_aio_context() ? > + false : true; > I would introduce a qemu_in_iothread() helper (similar to qemu_in_coroutine() etc) + Error *local_err = NULL; > + struct iovec send[2] = {0}; > + 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; > + } > + /* > + * Dont use in IOThread out of co-routine context as > + * it will block IOThread. > + */ > + if (iothread) { > + assert(qemu_in_coroutine()); > + } > or simply assert(!iothread || qemu_in_coroutine()) + /* > + * Skip unlocking/locking iothread when in IOThread running > + * in co-routine context. Co-routine context is asserted above > + * for IOThread case. > + * Also skip this while in a co-routine in the main context. > + */ > + if (iolock && !iothread && !qemu_in_coroutine()) { > + qemu_mutex_unlock_iothread(); > + } > + > + (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, > nfds, > + &local_err); > That extra (void) is probably unnecessary. + > + if (iolock && !iothread && !qemu_in_coroutine()) { > + /* See above comment why skip locking here. */ > + qemu_mutex_lock_iothread(); > + } > + > + if (errp) { > + error_propagate(errp, local_err); > + } else if (local_err) { > + error_report_err(local_err); > + } > Not sure this behaviour is recommended. Instead, a trace and an ERRP_GUARD would be more idiomatic. > + > + return; > That's an unnecessary return. Why not return true/false based on error? +} > + > +/* > + * Read message from the ioc QIOChannel. > + * This function is safe to call from: > + * - From main loop in co-routine context. Will block the main loop if > not in > + * co-routine context; > + * - From vCPU thread with no co-routine context and if the channel is > not part > + * of the main loop handling; > + * - From IOThread within co-routine context, outside of co-routine > context > + * will block IOThread; > + */ > +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(); > + bool iothread = qemu_get_current_aio_context() == > qemu_get_aio_context() > + ? false : true; > + 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; > + > + size = iov.iov_len; > + > + /* > + * Dont use in IOThread out of co-routine context as > + * it will block IOThread. > + */ > + if (iothread) { > + assert(qemu_in_coroutine()); > + } > as above > + > + while (size > 0) { > + bytes = qio_channel_readv_full(ioc, iovp, niov, l_fds, l_nfds, > + &local_err); > + if (bytes == QIO_CHANNEL_ERR_BLOCK) { > + /* > + * Skip unlocking/locking iothread when in IOThread running > + * in co-routine context. Co-routine context is asserted above > + * for IOThread case. > + * Also skip this while in a co-routine in the main context. > + */ > + if (iolock && !iothread && !qemu_in_coroutine()) { > + qemu_mutex_unlock_iothread(); > Why not lock the iothread at the beginning of the function and call a readv_full_all like we do for writes? + } > + if (qemu_in_coroutine()) { > + qio_channel_yield(ioc, G_IO_IN); > + } else { > + qio_channel_wait(ioc, G_IO_IN); > + } > + /* See above comment why skip locking here. */ > + if (iolock && !iothread && !qemu_in_coroutine()) { > + 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); > needless cast + } > + > + 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, > This cast is not necessary + &local_err); > + if (!local_err) { > + if (len == -EIO) { > + error_setg(&local_err, "Connection closed."); > + goto fail; > + } > + if (len < 0) { > + error_setg(&local_err, "Message length is less than 0"); > + goto fail; > + } > + if (len != MPQEMU_MSG_HDR_SIZE) { > + error_setg(&local_err, "Message header corrupted"); > + goto fail; > + } > + } else { > + 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, > that one too + &local_err) < 0) { > + goto fail; > + } > + > + msg->num_fds = nfds; > + if (nfds > G_N_ELEMENTS(msg->fds)) { > + error_setg(&local_err, > + "Overflow error: received %zu fds, more than max of %d > fds", > + nfds, REMOTE_MAX_FDS); > + goto fail; > + } else if (nfds) { > + memcpy(msg->fds, fds, nfds * sizeof(int)); > + } > + > +fail: > + while (local_err && nfds) { > + close(fds[nfds - 1]); > + nfds--; > + } > + > + g_free(fds); > + > + 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 >= MPQEMU_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; > +} > diff --git a/MAINTAINERS b/MAINTAINERS > index c45ac1d..d0c891a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3141,6 +3141,8 @@ F: hw/pci-host/remote.c > F: include/hw/pci-host/remote.h > F: hw/remote/machine.c > F: include/hw/remote/machine.h > +F: hw/remote/mpqemu-link.c > +F: include/hw/remote/mpqemu-link.h > > Build and test automation > ------------------------- > diff --git a/hw/remote/meson.build b/hw/remote/meson.build > index 197b038..a2b2fc0 100644 > --- a/hw/remote/meson.build > +++ b/hw/remote/meson.build > @@ -1,5 +1,6 @@ > remote_ss = ss.source_set() > > remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('machine.c')) > +remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: > files('mpqemu-link.c')) > > softmmu_ss.add_all(when: 'CONFIG_MULTIPROCESS', if_true: remote_ss) > -- > 1.8.3.1 > >
On Mon, Dec 07, 2020 at 05:18:46PM +0400, Marc-André Lureau wrote: > Hi > > On Wed, Dec 2, 2020 at 12:25 AM Jagannathan Raman <jag.raman@oracle.com> > wrote: > > > 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. > > Define transmission functions used by proxy and by remote. > > There are certain restrictions on where its safe to use these > > functions: > > - From main loop in co-routine context. Will block the main loop if not > > in > > co-routine context; > > - From vCPU thread with no co-routine context and if the channel is not > > part > > of the main loop handling; > > - From IOThread within co-routine context, outside of co-routine context > > will > > block IOThread; > > > > 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> > > --- > > include/hw/remote/mpqemu-link.h | 60 ++++++++++ > > hw/remote/mpqemu-link.c | 242 > > ++++++++++++++++++++++++++++++++++++++++ > > MAINTAINERS | 2 + > > hw/remote/meson.build | 1 + > > 4 files changed, 305 insertions(+) > > create mode 100644 include/hw/remote/mpqemu-link.h > > create mode 100644 hw/remote/mpqemu-link.c > > > > diff --git a/include/hw/remote/mpqemu-link.h > > b/include/hw/remote/mpqemu-link.h > > new file mode 100644 > > index 0000000..2d79ff8 > > --- /dev/null > > +++ b/include/hw/remote/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 { > > + MPQEMU_CMD_INIT, > > + MPQEMU_CMD_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/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c > > new file mode 100644 > > index 0000000..e535ed2 > > --- /dev/null > > +++ b/hw/remote/mpqemu-link.c > > @@ -0,0 +1,242 @@ > > +/* > > + * 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 "hw/remote/mpqemu-link.h" > > +#include "qapi/error.h" > > +#include "qemu/iov.h" > > +#include "qemu/error-report.h" > > +#include "qemu/main-loop.h" > > + > > +/* > > + * Send message over the ioc QIOChannel. > > + * This function is safe to call from: > > + * - From main loop in co-routine context. Will block the main loop if > > not in > > + * co-routine context; > > + * - From vCPU thread with no co-routine context and if the channel is > > not part > > + * of the main loop handling; > > + * - From IOThread within co-routine context, outside of co-routine > > context > > + * will block IOThread; > > > > Can drop the extra "From" on each line. > > + */ > > +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) > > +{ > > + bool iolock = qemu_mutex_iothread_locked(); > > + bool iothread = qemu_get_current_aio_context() == > > qemu_get_aio_context() ? > > + false : true; > > > > I would introduce a qemu_in_iothread() helper (similar to > qemu_in_coroutine() etc) > > + Error *local_err = NULL; > > + struct iovec send[2] = {0}; > > + 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; > > + } > > + /* > > + * Dont use in IOThread out of co-routine context as > > + * it will block IOThread. > > + */ > > + if (iothread) { > > + assert(qemu_in_coroutine()); > > + } > > > > or simply assert(!iothread || qemu_in_coroutine()) > > + /* > > + * Skip unlocking/locking iothread when in IOThread running > > + * in co-routine context. Co-routine context is asserted above > > + * for IOThread case. > > + * Also skip this while in a co-routine in the main context. > > + */ > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > + qemu_mutex_unlock_iothread(); > > + } > > + > > + (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, > > nfds, > > + &local_err); > > > > That extra (void) is probably unnecessary. > > > + > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > + /* See above comment why skip locking here. */ > > + qemu_mutex_lock_iothread(); > > + } > > + > > + if (errp) { > > + error_propagate(errp, local_err); > > + } else if (local_err) { > > + error_report_err(local_err); > > + } > > > Hi Marc-Andre, Thank you for reviewing the patches. > Not sure this behaviour is recommended. Instead, a trace and an ERRP_GUARD > would be more idiomatic. Did you mean to suggest using trace_ functions for the general use, not only the failure path. Just want to make sure I understood correctly. Should the trace file subdirectory (in this case ./hw/remote/) be included into trace_events_subdirs of meson.build with the condition that CONFIG_MULTIPROCESS is enabled? Something like <snip> config_devices_mak_file = target + '-config-devices.mak' devconfig = keyval.load(meson.current_build_dir() / target + '-config-devices.mak') have_multiprocess = 'CONFIG_MULTIPROCESS' in devconfig if have_multiproces ...' </snip> Thank you! Elena > > > > + > > + return; > > > > That's an unnecessary return. Why not return true/false based on error? > > +} > > + > > +/* > > + * Read message from the ioc QIOChannel. > > + * This function is safe to call from: > > + * - From main loop in co-routine context. Will block the main loop if > > not in > > + * co-routine context; > > + * - From vCPU thread with no co-routine context and if the channel is > > not part > > + * of the main loop handling; > > + * - From IOThread within co-routine context, outside of co-routine > > context > > + * will block IOThread; > > + */ > > +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(); > > + bool iothread = qemu_get_current_aio_context() == > > qemu_get_aio_context() > > + ? false : true; > > + 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; > > + > > + size = iov.iov_len; > > + > > + /* > > + * Dont use in IOThread out of co-routine context as > > + * it will block IOThread. > > + */ > > + if (iothread) { > > + assert(qemu_in_coroutine()); > > + } > > > > as above > > > > + > > + while (size > 0) { > > + bytes = qio_channel_readv_full(ioc, iovp, niov, l_fds, l_nfds, > > + &local_err); > > + if (bytes == QIO_CHANNEL_ERR_BLOCK) { > > + /* > > + * Skip unlocking/locking iothread when in IOThread running > > + * in co-routine context. Co-routine context is asserted above > > + * for IOThread case. > > + * Also skip this while in a co-routine in the main context. > > + */ > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > + qemu_mutex_unlock_iothread(); > > > > Why not lock the iothread at the beginning of the function and call a > readv_full_all like we do for writes? > > + } > > + if (qemu_in_coroutine()) { > > + qio_channel_yield(ioc, G_IO_IN); > > + } else { > > + qio_channel_wait(ioc, G_IO_IN); > > + } > > + /* See above comment why skip locking here. */ > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > + 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); > > > > needless cast > > + } > > + > > + 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, > > > > This cast is not necessary > > + &local_err); > > + if (!local_err) { > > + if (len == -EIO) { > > + error_setg(&local_err, "Connection closed."); > > + goto fail; > > + } > > + if (len < 0) { > > + error_setg(&local_err, "Message length is less than 0"); > > + goto fail; > > + } > > + if (len != MPQEMU_MSG_HDR_SIZE) { > > + error_setg(&local_err, "Message header corrupted"); > > + goto fail; > > + } > > + } else { > > + 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, > > > > that one too > > + &local_err) < 0) { > > + goto fail; > > + } > > + > > + msg->num_fds = nfds; > > + if (nfds > G_N_ELEMENTS(msg->fds)) { > > + error_setg(&local_err, > > + "Overflow error: received %zu fds, more than max of %d > > fds", > > + nfds, REMOTE_MAX_FDS); > > + goto fail; > > + } else if (nfds) { > > + memcpy(msg->fds, fds, nfds * sizeof(int)); > > + } > > + > > +fail: > > + while (local_err && nfds) { > > + close(fds[nfds - 1]); > > + nfds--; > > + } > > + > > + g_free(fds); > > + > > + 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 >= MPQEMU_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; > > +} > > diff --git a/MAINTAINERS b/MAINTAINERS > > index c45ac1d..d0c891a 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3141,6 +3141,8 @@ F: hw/pci-host/remote.c > > F: include/hw/pci-host/remote.h > > F: hw/remote/machine.c > > F: include/hw/remote/machine.h > > +F: hw/remote/mpqemu-link.c > > +F: include/hw/remote/mpqemu-link.h > > > > Build and test automation > > ------------------------- > > diff --git a/hw/remote/meson.build b/hw/remote/meson.build > > index 197b038..a2b2fc0 100644 > > --- a/hw/remote/meson.build > > +++ b/hw/remote/meson.build > > @@ -1,5 +1,6 @@ > > remote_ss = ss.source_set() > > > > remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('machine.c')) > > +remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: > > files('mpqemu-link.c')) > > > > softmmu_ss.add_all(when: 'CONFIG_MULTIPROCESS', if_true: remote_ss) > > -- > > 1.8.3.1 > > > > > > -- > Marc-André Lureau
Hi On Thu, Dec 10, 2020 at 5:42 AM Elena Ufimtseva <elena.ufimtseva@oracle.com> wrote: > On Mon, Dec 07, 2020 at 05:18:46PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Wed, Dec 2, 2020 at 12:25 AM Jagannathan Raman <jag.raman@oracle.com> > > wrote: > > > > > 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. > > > Define transmission functions used by proxy and by remote. > > > There are certain restrictions on where its safe to use these > > > functions: > > > - From main loop in co-routine context. Will block the main loop if > not > > > in > > > co-routine context; > > > - From vCPU thread with no co-routine context and if the channel is > not > > > part > > > of the main loop handling; > > > - From IOThread within co-routine context, outside of co-routine > context > > > will > > > block IOThread; > > > > > > 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> > > > --- > > > include/hw/remote/mpqemu-link.h | 60 ++++++++++ > > > hw/remote/mpqemu-link.c | 242 > > > ++++++++++++++++++++++++++++++++++++++++ > > > MAINTAINERS | 2 + > > > hw/remote/meson.build | 1 + > > > 4 files changed, 305 insertions(+) > > > create mode 100644 include/hw/remote/mpqemu-link.h > > > create mode 100644 hw/remote/mpqemu-link.c > > > > > > diff --git a/include/hw/remote/mpqemu-link.h > > > b/include/hw/remote/mpqemu-link.h > > > new file mode 100644 > > > index 0000000..2d79ff8 > > > --- /dev/null > > > +++ b/include/hw/remote/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 { > > > + MPQEMU_CMD_INIT, > > > + MPQEMU_CMD_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/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c > > > new file mode 100644 > > > index 0000000..e535ed2 > > > --- /dev/null > > > +++ b/hw/remote/mpqemu-link.c > > > @@ -0,0 +1,242 @@ > > > +/* > > > + * 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 "hw/remote/mpqemu-link.h" > > > +#include "qapi/error.h" > > > +#include "qemu/iov.h" > > > +#include "qemu/error-report.h" > > > +#include "qemu/main-loop.h" > > > + > > > +/* > > > + * Send message over the ioc QIOChannel. > > > + * This function is safe to call from: > > > + * - From main loop in co-routine context. Will block the main loop if > > > not in > > > + * co-routine context; > > > + * - From vCPU thread with no co-routine context and if the channel is > > > not part > > > + * of the main loop handling; > > > + * - From IOThread within co-routine context, outside of co-routine > > > context > > > + * will block IOThread; > > > > > > > Can drop the extra "From" on each line. > > > > + */ > > > +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) > > > +{ > > > + bool iolock = qemu_mutex_iothread_locked(); > > > + bool iothread = qemu_get_current_aio_context() == > > > qemu_get_aio_context() ? > > > + false : true; > > > > > > > I would introduce a qemu_in_iothread() helper (similar to > > qemu_in_coroutine() etc) > > > > + Error *local_err = NULL; > > > + struct iovec send[2] = {0}; > > > + 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; > > > + } > > > + /* > > > + * Dont use in IOThread out of co-routine context as > > > + * it will block IOThread. > > > + */ > > > + if (iothread) { > > > + assert(qemu_in_coroutine()); > > > + } > > > > > > > or simply assert(!iothread || qemu_in_coroutine()) > > > > + /* > > > + * Skip unlocking/locking iothread when in IOThread running > > > + * in co-routine context. Co-routine context is asserted above > > > + * for IOThread case. > > > + * Also skip this while in a co-routine in the main context. > > > + */ > > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > > + qemu_mutex_unlock_iothread(); > > > + } > > > + > > > + (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), > fds, > > > nfds, > > > + &local_err); > > > > > > > That extra (void) is probably unnecessary. > > > > > > + > > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > > + /* See above comment why skip locking here. */ > > > + qemu_mutex_lock_iothread(); > > > + } > > > + > > > + if (errp) { > > > + error_propagate(errp, local_err); > > > + } else if (local_err) { > > > + error_report_err(local_err); > > > + } > > > > > > > Hi Marc-Andre, > > Thank you for reviewing the patches. > > > > Not sure this behaviour is recommended. Instead, a trace and an > ERRP_GUARD > > would be more idiomatic. > > Did you mean to suggest using trace_ functions for the general use, not > only the > failure path. Just want to make sure I understood correctly. > That's what I would suggest for error handling: (not tested) diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c index d75b4782ee..a7ac37627e 100644 --- a/hw/remote/mpqemu-link.c +++ b/hw/remote/mpqemu-link.c @@ -31,10 +31,10 @@ */ void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) { + ERRP_GUARD(); bool iolock = qemu_mutex_iothread_locked(); bool iothread = qemu_get_current_aio_context() == qemu_get_aio_context() ? false : true; - Error *local_err = NULL; struct iovec send[2] = {0}; int *fds = NULL; size_t nfds = 0; @@ -66,21 +66,15 @@ void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) qemu_mutex_unlock_iothread(); } - (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, nfds, - &local_err); + if (qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, nfds, errp) == -1) { + trace_mpqemu_io_error(msg, ioc, error_get_pretty(*errp)); + } if (iolock && !iothread && !qemu_in_coroutine()) { /* See above comment why skip locking here. */ qemu_mutex_lock_iothread(); } - if (errp) { - error_propagate(errp, local_err); - } else if (local_err) { - error_report_err(local_err); - } - - return; } > > Should the trace file subdirectory (in this case ./hw/remote/) be included > into > trace_events_subdirs of meson.build with the condition that > CONFIG_MULTIPROCESS is enabled? > > Something like > <snip> > > config_devices_mak_file = target + '-config-devices.mak' > devconfig = keyval.load(meson.current_build_dir() / target + > '-config-devices.mak') > have_multiprocess = 'CONFIG_MULTIPROCESS' in devconfig > > if have_multiproces > ...' > > </snip> > That shouldn't be necessary, do like the other hw/ traces, adding themself to trace_events_subdirs.
On Thu, Dec 10, 2020 at 12:20:06PM +0400, Marc-André Lureau wrote: > Hi > > On Thu, Dec 10, 2020 at 5:42 AM Elena Ufimtseva <elena.ufimtseva@oracle.com> > wrote: > > > On Mon, Dec 07, 2020 at 05:18:46PM +0400, Marc-André Lureau wrote: > > > Hi > > > > > > On Wed, Dec 2, 2020 at 12:25 AM Jagannathan Raman <jag.raman@oracle.com> > > > wrote: > > > > > > > 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. > > > > Define transmission functions used by proxy and by remote. > > > > There are certain restrictions on where its safe to use these > > > > functions: > > > > - From main loop in co-routine context. Will block the main loop if > > not > > > > in > > > > co-routine context; > > > > - From vCPU thread with no co-routine context and if the channel is > > not > > > > part > > > > of the main loop handling; > > > > - From IOThread within co-routine context, outside of co-routine > > context > > > > will > > > > block IOThread; > > > > > > > > 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> > > > > --- > > > > include/hw/remote/mpqemu-link.h | 60 ++++++++++ > > > > hw/remote/mpqemu-link.c | 242 > > > > ++++++++++++++++++++++++++++++++++++++++ > > > > MAINTAINERS | 2 + > > > > hw/remote/meson.build | 1 + > > > > 4 files changed, 305 insertions(+) > > > > create mode 100644 include/hw/remote/mpqemu-link.h > > > > create mode 100644 hw/remote/mpqemu-link.c > > > > > > > > diff --git a/include/hw/remote/mpqemu-link.h > > > > b/include/hw/remote/mpqemu-link.h > > > > new file mode 100644 > > > > index 0000000..2d79ff8 > > > > --- /dev/null > > > > +++ b/include/hw/remote/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 { > > > > + MPQEMU_CMD_INIT, > > > > + MPQEMU_CMD_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/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c > > > > new file mode 100644 > > > > index 0000000..e535ed2 > > > > --- /dev/null > > > > +++ b/hw/remote/mpqemu-link.c > > > > @@ -0,0 +1,242 @@ > > > > +/* > > > > + * 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 "hw/remote/mpqemu-link.h" > > > > +#include "qapi/error.h" > > > > +#include "qemu/iov.h" > > > > +#include "qemu/error-report.h" > > > > +#include "qemu/main-loop.h" > > > > + > > > > +/* > > > > + * Send message over the ioc QIOChannel. > > > > + * This function is safe to call from: > > > > + * - From main loop in co-routine context. Will block the main loop if > > > > not in > > > > + * co-routine context; > > > > + * - From vCPU thread with no co-routine context and if the channel is > > > > not part > > > > + * of the main loop handling; > > > > + * - From IOThread within co-routine context, outside of co-routine > > > > context > > > > + * will block IOThread; > > > > > > > > > > Can drop the extra "From" on each line. > > > > > > + */ > > > > +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) > > > > +{ > > > > + bool iolock = qemu_mutex_iothread_locked(); > > > > + bool iothread = qemu_get_current_aio_context() == > > > > qemu_get_aio_context() ? > > > > + false : true; > > > > > > > > > > I would introduce a qemu_in_iothread() helper (similar to > > > qemu_in_coroutine() etc) > > > > > > + Error *local_err = NULL; > > > > + struct iovec send[2] = {0}; > > > > + 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; > > > > + } > > > > + /* > > > > + * Dont use in IOThread out of co-routine context as > > > > + * it will block IOThread. > > > > + */ > > > > + if (iothread) { > > > > + assert(qemu_in_coroutine()); > > > > + } > > > > > > > > > > or simply assert(!iothread || qemu_in_coroutine()) > > > > > > + /* > > > > + * Skip unlocking/locking iothread when in IOThread running > > > > + * in co-routine context. Co-routine context is asserted above > > > > + * for IOThread case. > > > > + * Also skip this while in a co-routine in the main context. > > > > + */ > > > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > > > + qemu_mutex_unlock_iothread(); > > > > + } > > > > + > > > > + (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), > > fds, > > > > nfds, > > > > + &local_err); > > > > > > > > > > That extra (void) is probably unnecessary. > > > > > > > > > + > > > > + if (iolock && !iothread && !qemu_in_coroutine()) { > > > > + /* See above comment why skip locking here. */ > > > > + qemu_mutex_lock_iothread(); > > > > + } > > > > + > > > > + if (errp) { > > > > + error_propagate(errp, local_err); > > > > + } else if (local_err) { > > > > + error_report_err(local_err); > > > > + } > > > > > > > > > > > Hi Marc-Andre, > > > > Thank you for reviewing the patches. > > > > > > > Not sure this behaviour is recommended. Instead, a trace and an > > ERRP_GUARD > > > would be more idiomatic. > > > > Did you mean to suggest using trace_ functions for the general use, not > > only the > > failure path. Just want to make sure I understood correctly. > > > > That's what I would suggest for error handling: (not tested) > > diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c > index d75b4782ee..a7ac37627e 100644 > --- a/hw/remote/mpqemu-link.c > +++ b/hw/remote/mpqemu-link.c > @@ -31,10 +31,10 @@ > */ > void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) > { > + ERRP_GUARD(); > bool iolock = qemu_mutex_iothread_locked(); > bool iothread = qemu_get_current_aio_context() == > qemu_get_aio_context() ? > false : true; > - Error *local_err = NULL; > struct iovec send[2] = {0}; > int *fds = NULL; > size_t nfds = 0; > @@ -66,21 +66,15 @@ void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, > Error **errp) > qemu_mutex_unlock_iothread(); > } > > - (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, > nfds, > - &local_err); > + if (qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, > nfds, errp) == -1) { > + trace_mpqemu_io_error(msg, ioc, error_get_pretty(*errp)); > + } Thanks, that answers my question. I didn't see the examples that convinced me using trace events as the means of error reporting. Now I do :) > > if (iolock && !iothread && !qemu_in_coroutine()) { > /* See above comment why skip locking here. */ > qemu_mutex_lock_iothread(); > } > > - if (errp) { > - error_propagate(errp, local_err); > - } else if (local_err) { > - error_report_err(local_err); > - } > - > - return; > } > > > > > > > > Should the trace file subdirectory (in this case ./hw/remote/) be included > > into > > trace_events_subdirs of meson.build with the condition that > > CONFIG_MULTIPROCESS is enabled? > > > > Something like > > <snip> > > > > config_devices_mak_file = target + '-config-devices.mak' > > devconfig = keyval.load(meson.current_build_dir() / target + > > '-config-devices.mak') > > have_multiprocess = 'CONFIG_MULTIPROCESS' in devconfig > > > > if have_multiproces > > ...' > > > > </snip> > > > > That shouldn't be necessary, do like the other hw/ traces, adding themself > to trace_events_subdirs. Got it, thank you! > > > -- > Marc-André Lureau
diff --git a/include/hw/remote/mpqemu-link.h b/include/hw/remote/mpqemu-link.h new file mode 100644 index 0000000..2d79ff8 --- /dev/null +++ b/include/hw/remote/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 { + MPQEMU_CMD_INIT, + MPQEMU_CMD_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/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c new file mode 100644 index 0000000..e535ed2 --- /dev/null +++ b/hw/remote/mpqemu-link.c @@ -0,0 +1,242 @@ +/* + * 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 "hw/remote/mpqemu-link.h" +#include "qapi/error.h" +#include "qemu/iov.h" +#include "qemu/error-report.h" +#include "qemu/main-loop.h" + +/* + * Send message over the ioc QIOChannel. + * This function is safe to call from: + * - From main loop in co-routine context. Will block the main loop if not in + * co-routine context; + * - From vCPU thread with no co-routine context and if the channel is not part + * of the main loop handling; + * - From IOThread within co-routine context, outside of co-routine context + * will block IOThread; + */ +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp) +{ + bool iolock = qemu_mutex_iothread_locked(); + bool iothread = qemu_get_current_aio_context() == qemu_get_aio_context() ? + false : true; + Error *local_err = NULL; + struct iovec send[2] = {0}; + 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; + } + /* + * Dont use in IOThread out of co-routine context as + * it will block IOThread. + */ + if (iothread) { + assert(qemu_in_coroutine()); + } + /* + * Skip unlocking/locking iothread when in IOThread running + * in co-routine context. Co-routine context is asserted above + * for IOThread case. + * Also skip this while in a co-routine in the main context. + */ + if (iolock && !iothread && !qemu_in_coroutine()) { + qemu_mutex_unlock_iothread(); + } + + (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, nfds, + &local_err); + + if (iolock && !iothread && !qemu_in_coroutine()) { + /* See above comment why skip locking here. */ + qemu_mutex_lock_iothread(); + } + + if (errp) { + error_propagate(errp, local_err); + } else if (local_err) { + error_report_err(local_err); + } + + return; +} + +/* + * Read message from the ioc QIOChannel. + * This function is safe to call from: + * - From main loop in co-routine context. Will block the main loop if not in + * co-routine context; + * - From vCPU thread with no co-routine context and if the channel is not part + * of the main loop handling; + * - From IOThread within co-routine context, outside of co-routine context + * will block IOThread; + */ +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(); + bool iothread = qemu_get_current_aio_context() == qemu_get_aio_context() + ? false : true; + 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; + + size = iov.iov_len; + + /* + * Dont use in IOThread out of co-routine context as + * it will block IOThread. + */ + if (iothread) { + assert(qemu_in_coroutine()); + } + + while (size > 0) { + bytes = qio_channel_readv_full(ioc, iovp, niov, l_fds, l_nfds, + &local_err); + if (bytes == QIO_CHANNEL_ERR_BLOCK) { + /* + * Skip unlocking/locking iothread when in IOThread running + * in co-routine context. Co-routine context is asserted above + * for IOThread case. + * Also skip this while in a co-routine in the main context. + */ + if (iolock && !iothread && !qemu_in_coroutine()) { + qemu_mutex_unlock_iothread(); + } + if (qemu_in_coroutine()) { + qio_channel_yield(ioc, G_IO_IN); + } else { + qio_channel_wait(ioc, G_IO_IN); + } + /* See above comment why skip locking here. */ + if (iolock && !iothread && !qemu_in_coroutine()) { + 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 (!local_err) { + if (len == -EIO) { + error_setg(&local_err, "Connection closed."); + goto fail; + } + if (len < 0) { + error_setg(&local_err, "Message length is less than 0"); + goto fail; + } + if (len != MPQEMU_MSG_HDR_SIZE) { + error_setg(&local_err, "Message header corrupted"); + goto fail; + } + } else { + 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 > G_N_ELEMENTS(msg->fds)) { + error_setg(&local_err, + "Overflow error: received %zu fds, more than max of %d fds", + nfds, REMOTE_MAX_FDS); + goto fail; + } else if (nfds) { + memcpy(msg->fds, fds, nfds * sizeof(int)); + } + +fail: + while (local_err && nfds) { + close(fds[nfds - 1]); + nfds--; + } + + g_free(fds); + + 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 >= MPQEMU_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; +} diff --git a/MAINTAINERS b/MAINTAINERS index c45ac1d..d0c891a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3141,6 +3141,8 @@ F: hw/pci-host/remote.c F: include/hw/pci-host/remote.h F: hw/remote/machine.c F: include/hw/remote/machine.h +F: hw/remote/mpqemu-link.c +F: include/hw/remote/mpqemu-link.h Build and test automation ------------------------- diff --git a/hw/remote/meson.build b/hw/remote/meson.build index 197b038..a2b2fc0 100644 --- a/hw/remote/meson.build +++ b/hw/remote/meson.build @@ -1,5 +1,6 @@ remote_ss = ss.source_set() remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('machine.c')) +remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('mpqemu-link.c')) softmmu_ss.add_all(when: 'CONFIG_MULTIPROCESS', if_true: remote_ss)