Re: [PATCH v12 08/19] multi-process: define MPQemuMsg format and transmission functions

[Elena Ufimtseva <elena.ufimtseva@oracle.com>]

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