diff mbox series

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

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

Commit Message

Jag Raman Dec. 1, 2020, 8:22 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.
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

Comments

Marc-André Lureau Dec. 7, 2020, 1:18 p.m. UTC | #1
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
>
>
Elena Ufimtseva Dec. 10, 2020, 1:40 a.m. UTC | #2
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
Marc-André Lureau Dec. 10, 2020, 8:20 a.m. UTC | #3
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.
Elena Ufimtseva Dec. 10, 2020, 12:53 p.m. UTC | #4
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 mbox series

Patch

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)