Hi On Thu, Dec 10, 2020 at 5:42 AM Elena Ufimtseva 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 > > wrote: > > > > > From: Elena Ufimtseva > > > > > > 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 > > > Signed-off-by: John G Johnson > > > Signed-off-by: Elena Ufimtseva > > > --- > > > 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 > > > 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 > ...' > > > That shouldn't be necessary, do like the other hw/ traces, adding themself to trace_events_subdirs. -- Marc-André Lureau