From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4CFBEC4361B for ; Thu, 10 Dec 2020 08:21:50 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8F06423D57 for ; Thu, 10 Dec 2020 08:21:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8F06423D57 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:40742 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1knHCs-00043U-Vx for qemu-devel@archiver.kernel.org; Thu, 10 Dec 2020 03:21:47 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:60914) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1knHBe-0003Vt-4X for qemu-devel@nongnu.org; Thu, 10 Dec 2020 03:20:30 -0500 Received: from mail-ej1-x641.google.com ([2a00:1450:4864:20::641]:45348) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1knHBV-0003IE-4G for qemu-devel@nongnu.org; Thu, 10 Dec 2020 03:20:29 -0500 Received: by mail-ej1-x641.google.com with SMTP id qw4so6050283ejb.12 for ; Thu, 10 Dec 2020 00:20:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=FFjvJixi6u0BUkJOrtev/FcwCEUBhhdxLTAwyPPaVWU=; b=usYr7JC9yBcqNGP7inah76kwHkXbx9bohtd313WK5BoobJv+lJ9OhhcmcTyt+f2MpD uxQA3SamtAP7nDLmhggINKpBVlo+YtgkKRPRkfbGAFJZZF4JC5mj6yH30W9y5wXQdEYn 72ebosIXG1K9jXoyDodDUEyXvGzf2UBCXtFlzU338Jjfp30tsAO3LUBQK00ebjh1Eay+ DXWivzDx22QuxtTFHNDP2DnvNSBp/FJeIIWYHaxNxUKGfTIrAVzbNXhX0fL1G/pnfiXN Z4CiZS7KLK8RIJXkM8f/fVY/BRumu6V2d0t4Zjv20VM74HHAAVS6jxjfFXHjzXJVuSFv SFrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=FFjvJixi6u0BUkJOrtev/FcwCEUBhhdxLTAwyPPaVWU=; b=PX5Ho+z3S/p2JCTyrTnwTbV7jmN+dsx+vAl1OXVd/QzCiDJ894ihDDQXsZ9scbb+dF fFjKh6rf6tR1E3OBR8dj2atUJ054eR2Uu0H8tt/0Ss3BG3A5Ly0KTSpix/3hNeHDtCsc fgYsJHa+DxJEkx5BpYWRuEDlBabtk0bzfvUkVuZ+EacWKmIcxohMIdgL/ubwcbA+nILw 5gZbPORSWjydCGUUVG5T6k5ONdnCQPU+y7Gdtquzq30d2HNU61/45iNX0pfVRyoCj1FW 1HOxvY88CW5xATDSjEmTx1MuZtOlCwXxeQQuPUMvrqARkZAYv0Nh2v/+NTZ0EJpasWGx Io1w== X-Gm-Message-State: AOAM530C00dG30EDHvWCVpgCeDXNELs+2hb+fIkJ2f3zRBiHEBj/GzXV PLpUY3nJQTOrTrXZ/hr/vq/k76uO9XWjQChyyPg= X-Google-Smtp-Source: ABdhPJyL5c9j7yCjmt1RsSDx2lli/Mt6NPAUab7bxdjja7Ho710l1Ji8Li61jmtX2bFYfu/1KqzV31KCaj4sLQ2SDdM= X-Received: by 2002:a17:906:98d4:: with SMTP id zd20mr5378937ejb.532.1607588419427; Thu, 10 Dec 2020 00:20:19 -0800 (PST) MIME-Version: 1.0 References: <20201210014005.GA48815@flaka> In-Reply-To: <20201210014005.GA48815@flaka> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Thu, 10 Dec 2020 12:20:06 +0400 Message-ID: Subject: Re: [PATCH v12 08/19] multi-process: define MPQemuMsg format and transmission functions To: Elena Ufimtseva Content-Type: multipart/alternative; boundary="000000000000af62a005b617d844" Received-SPF: pass client-ip=2a00:1450:4864:20::641; envelope-from=marcandre.lureau@gmail.com; helo=mail-ej1-x641.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Fam Zheng , John G Johnson , Swapnil Ingle , "Michael S. Tsirkin" , QEMU , Gerd Hoffmann , Jagannathan Raman , Juan Quintela , Markus Armbruster , Kanth Ghatraju , Felipe Franciosi , Thomas Huth , Eduardo Habkost , Konrad Rzeszutek Wilk , "Dr. David Alan Gilbert" , Alex Williamson , Stefan Hajnoczi , Thanos Makatos , Richard Henderson , Kevin Wolf , "Daniel P. Berrange" , Max Reitz , Ross Lagerwall , Paolo Bonzini Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000af62a005b617d844 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=C3=A9 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 =C2=A9 2018, 2020 Oracle and/or its affiliates. > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2 o= r > > > 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 =C2=A9 2018, 2020 Oracle and/or its affiliates. > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2 o= r > > > 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 =3D qemu_mutex_iothread_locked(); > > > + bool iothread =3D qemu_get_current_aio_context() =3D=3D > > > qemu_get_aio_context() ? > > > + false : true; > > > > > > > I would introduce a qemu_in_iothread() helper (similar to > > qemu_in_coroutine() etc) > > > > + Error *local_err =3D NULL; > > > + struct iovec send[2] =3D {0}; > > > + int *fds =3D NULL; > > > + size_t nfds =3D 0; > > > + > > > + send[0].iov_base =3D msg; > > > + send[0].iov_len =3D MPQEMU_MSG_HDR_SIZE; > > > + > > > + send[1].iov_base =3D (void *)&msg->data; > > > + send[1].iov_len =3D msg->size; > > > + > > > + if (msg->num_fds) { > > > + nfds =3D msg->num_fds; > > > + fds =3D 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 =3D qemu_mutex_iothread_locked(); bool iothread =3D qemu_get_current_aio_context() =3D=3D qemu_get_aio_context() ? false : true; - Error *local_err =3D NULL; struct iovec send[2] =3D {0}; int *fds =3D NULL; size_t nfds =3D 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) =3D=3D -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 include= d > into > trace_events_subdirs of meson.build with the condition that > CONFIG_MULTIPROCESS is enabled? > > Something like > > > config_devices_mak_file =3D target + '-config-devices.mak' > devconfig =3D keyval.load(meson.current_build_dir() / target + > '-config-devices.mak') > have_multiprocess =3D 'CONFIG_MULTIPROCESS' in devconfig > > if have_multiproces > ...' > > > That shouldn't be necessary, do like the other hw/ traces, adding themself to trace_events_subdirs. --=20 Marc-Andr=C3=A9 Lureau --000000000000af62a005b617d844 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Thu, Dec 10, 2020 at 5:42 AM Ele= na Ufimtseva <elena.ufimts= eva@oracle.com> wrote:
On Mon, Dec 07, 2020 at 05:18:46PM +0400, Marc-Andr=C3=A9 Lur= eau 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 remot= e
> > 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:
> >=C2=A0 =C2=A0- From main loop in co-routine context. Will block th= e main loop if not
> > in
> >=C2=A0 =C2=A0 =C2=A0co-routine context;
> >=C2=A0 =C2=A0- From vCPU thread with no co-routine context and if = the channel is not
> > part
> >=C2=A0 =C2=A0 =C2=A0of the main loop handling;
> >=C2=A0 =C2=A0- From IOThread within co-routine context, outside of= co-routine context
> > will
> >=C2=A0 =C2=A0 =C2=A0block 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>
> > ---
> >=C2=A0 include/hw/remote/mpqemu-link.h |=C2=A0 60 ++++++++++
> >=C2=A0 hw/remote/mpqemu-link.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| = 242
> > ++++++++++++++++++++++++++++++++++++++++
> >=C2=A0 MAINTAINERS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A02 +
> >=C2=A0 hw/remote/meson.build=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 =C2=A01 +
> >=C2=A0 4 files changed, 305 insertions(+)
> >=C2=A0 create mode 100644 include/hw/remote/mpqemu-link.h
> >=C2=A0 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<= br> > > + *
> > + * Copyright =C2=A9 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 {
> > +=C2=A0 =C2=A0 MPQEMU_CMD_INIT,
> > +=C2=A0 =C2=A0 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 fro= m QEMU.
> > + *
> > + */
> > +typedef struct {
> > +=C2=A0 =C2=A0 int cmd;
> > +=C2=A0 =C2=A0 size_t size;
> > +
> > +=C2=A0 =C2=A0 union {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 uint64_t u64;
> > +=C2=A0 =C2=A0 } data;
> > +
> > +=C2=A0 =C2=A0 int fds[REMOTE_MAX_FDS];
> > +=C2=A0 =C2=A0 int num_fds;
> > +} MPQemuMsg;
> > +
> > +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **er= rp);
> > +void mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc, Error **er= rp);
> > +
> > +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<= br> > > + *
> > + * Copyright =C2=A9 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 l= oop if
> > not in
> > + *=C2=A0 =C2=A0co-routine context;
> > + * - From vCPU thread with no co-routine context and if the chan= nel is
> > not part
> > + *=C2=A0 =C2=A0of the main loop handling;
> > + * - From IOThread within co-routine context, outside of co-rout= ine
> > context
> > + *=C2=A0 =C2=A0will block IOThread;
> >
>
> Can drop the extra "From" on each line.
>
> + */
> > +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **er= rp)
> > +{
> > +=C2=A0 =C2=A0 bool iolock =3D qemu_mutex_iothread_locked();
> > +=C2=A0 =C2=A0 bool iothread =3D qemu_get_current_aio_context() = =3D=3D
> > qemu_get_aio_context() ?
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 false : true;
> >
>
> I would introduce a qemu_in_iothread() helper (similar to
> qemu_in_coroutine() etc)
>
> +=C2=A0 =C2=A0 Error *local_err =3D NULL;
> > +=C2=A0 =C2=A0 struct iovec send[2] =3D {0};
> > +=C2=A0 =C2=A0 int *fds =3D NULL;
> > +=C2=A0 =C2=A0 size_t nfds =3D 0;
> > +
> > +=C2=A0 =C2=A0 send[0].iov_base =3D msg;
> > +=C2=A0 =C2=A0 send[0].iov_len =3D MPQEMU_MSG_HDR_SIZE;
> > +
> > +=C2=A0 =C2=A0 send[1].iov_base =3D (void *)&msg->data; > > +=C2=A0 =C2=A0 send[1].iov_len =3D msg->size;
> > +
> > +=C2=A0 =C2=A0 if (msg->num_fds) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 nfds =3D msg->num_fds;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 fds =3D msg->fds;
> > +=C2=A0 =C2=A0 }
> > +=C2=A0 =C2=A0 /*
> > +=C2=A0 =C2=A0 =C2=A0* Dont use in IOThread out of co-routine con= text as
> > +=C2=A0 =C2=A0 =C2=A0* it will block IOThread.
> > +=C2=A0 =C2=A0 =C2=A0*/
> > +=C2=A0 =C2=A0 if (iothread) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 assert(qemu_in_coroutine());
> > +=C2=A0 =C2=A0 }
> >
>
> or simply assert(!iothread || qemu_in_coroutine())
>
> +=C2=A0 =C2=A0 /*
> > +=C2=A0 =C2=A0 =C2=A0* Skip unlocking/locking iothread when in IO= Thread running
> > +=C2=A0 =C2=A0 =C2=A0* in co-routine context. Co-routine context = is asserted above
> > +=C2=A0 =C2=A0 =C2=A0* for IOThread case.
> > +=C2=A0 =C2=A0 =C2=A0* Also skip this while in a co-routine in th= e main context.
> > +=C2=A0 =C2=A0 =C2=A0*/
> > +=C2=A0 =C2=A0 if (iolock && !iothread && !qemu_i= n_coroutine()) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_mutex_unlock_iothread();
> > +=C2=A0 =C2=A0 }
> > +
> > +=C2=A0 =C2=A0 (void)qio_channel_writev_full_all(ioc, send, G_N_E= LEMENTS(send), fds,
> > nfds,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &= local_err);
> >
>
> That extra (void) is probably unnecessary.
>
>
> +
> > +=C2=A0 =C2=A0 if (iolock && !iothread && !qemu_i= n_coroutine()) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* See above comment why skip lockin= g here. */
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_mutex_lock_iothread();
> > +=C2=A0 =C2=A0 }
> > +
> > +=C2=A0 =C2=A0 if (errp) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 error_propagate(errp, local_err); > > +=C2=A0 =C2=A0 } else if (local_err) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 error_report_err(local_err);
> > +=C2=A0 =C2=A0 }
> >
>

Hi Marc-Andre,

Thank you for reviewing the patches.


> Not sure this behaviour is recommended. Instead, a trace and an ERRP_G= UARD
> would be more idiomatic.

Did you mean to suggest using trace_ functions for the general use, not onl= y the
failure path. Just want to make sure I understood correctly.

That's what I wo= uld suggest for error handling: (not tested)

d= iff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c
index d75b= 4782ee..a7ac37627e 100644
--- a/hw/remote/mpqemu-link.c
+++ b/hw/remo= te/mpqemu-link.c
@@ -31,10 +31,10 @@
=C2=A0 */
=C2=A0void mpqemu_m= sg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
=C2=A0{
+ =C2= =A0 =C2=A0ERRP_GUARD();
=C2=A0 =C2=A0 =C2=A0bool iolock =3D qemu_mutex_i= othread_locked();
=C2=A0 =C2=A0 =C2=A0bool iothread =3D qemu_get_current= _aio_context() =3D=3D qemu_get_aio_context() ?
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0false : true;
- =C2= =A0 =C2=A0Error *local_err =3D NULL;
=C2=A0 =C2=A0 =C2=A0struct iovec se= nd[2] =3D {0};
=C2=A0 =C2=A0 =C2=A0int *fds =3D NULL;
=C2=A0 =C2=A0 = =C2=A0size_t nfds =3D 0;
@@ -66,21 +66,15 @@ void mpqemu_msg_send(MPQemu= Msg *msg, QIOChannel *ioc, Error **errp)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0qemu_mutex_unlock_iothread();
=C2=A0 =C2=A0 =C2=A0}

- =C2=A0 = =C2=A0(void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds,= nfds,
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&l= ocal_err);
+ =C2=A0 =C2=A0if (qio_channel_writev_full_all(ioc, send, G_N= _ELEMENTS(send), fds, nfds, errp) =3D=3D -1) {
+ =C2=A0 =C2=A0 =C2=A0 = =C2=A0trace_mpqemu_io_error(msg, ioc, error_get_pretty(*errp));
+ =C2=A0= =C2=A0}

=C2=A0 =C2=A0 =C2=A0if (iolock && !iothread &&a= mp; !qemu_in_coroutine()) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* See abo= ve comment why skip locking here. */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0q= emu_mutex_lock_iothread();
=C2=A0 =C2=A0 =C2=A0}

- =C2=A0 =C2=A0i= f (errp) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0error_propagate(errp, local_err)= ;
- =C2=A0 =C2=A0} else if (local_err) {
- =C2=A0 =C2=A0 =C2=A0 =C2= =A0error_report_err(local_err);
- =C2=A0 =C2=A0}
-
- =C2=A0 =C2=A0= return;
=C2=A0}


=C2=A0

Should the trace file subdirectory (in this case ./hw/remote/) be included = into
trace_events_subdirs of meson.build with the condition that CONFIG_MULTIPRO= CESS is enabled?

Something like
<snip>

config_devices_mak_file =3D target + '-config-devices.mak'
devconfig =3D keyval.load(meson.current_build_dir() / target + '-config= -devices.mak')
have_multiprocess =3D 'CONFIG_MULTIPROCESS' in devconfig

if have_multiproces
...'

</snip>

That shouldn't be nec= essary, do like the other hw/ traces, adding themself to trace_events_subdi= rs.


--
Marc-Andr=C3=A9 Lureau
--000000000000af62a005b617d844--