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

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

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