Hi
On Wed, Dec 2, 2020 at 12:23 AM Jagannathan Raman <jag.ra...@oracle.com>
wrote:
> Initializes the message handler function in the remote process. It is
> called whenever there's an event pending on QIOChannel that registers
> this function.
>
> Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com>
> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> include/hw/remote/machine.h | 9 +++++++
> hw/remote/message.c | 61
> +++++++++++++++++++++++++++++++++++++++++++++
> MAINTAINERS | 1 +
> hw/remote/meson.build | 1 +
> 4 files changed, 72 insertions(+)
> create mode 100644 hw/remote/message.c
>
> diff --git a/include/hw/remote/machine.h b/include/hw/remote/machine.h
> index d312972..3073db6 100644
> --- a/include/hw/remote/machine.h
> +++ b/include/hw/remote/machine.h
> @@ -14,6 +14,7 @@
> #include "qom/object.h"
> #include "hw/boards.h"
> #include "hw/pci-host/remote.h"
> +#include "io/channel.h"
>
> typedef struct RemoteMachineState {
> MachineState parent_obj;
> @@ -21,8 +22,16 @@ typedef struct RemoteMachineState {
> RemotePCIHost *host;
> } RemoteMachineState;
>
> +/* Used to pass to co-routine device and ioc. */
> +typedef struct RemoteCommDev {
> + PCIDevice *dev;
> + QIOChannel *ioc;
> +} RemoteCommDev;
> +
> #define TYPE_REMOTE_MACHINE "x-remote-machine"
> #define REMOTE_MACHINE(obj) \
> OBJECT_CHECK(RemoteMachineState, (obj), TYPE_REMOTE_MACHINE)
>
> +void coroutine_fn mpqemu_remote_msg_loop_co(void *data);
> +
> #endif
> diff --git a/hw/remote/message.c b/hw/remote/message.c
> new file mode 100644
> index 0000000..5d87bf4
> --- /dev/null
> +++ b/hw/remote/message.c
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright © 2020 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or
> later.
> + *
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +
> +#include "hw/remote/machine.h"
> +#include "io/channel.h"
> +#include "hw/remote/mpqemu-link.h"
> +#include "qapi/error.h"
> +#include "sysemu/runstate.h"
> +
> +void coroutine_fn mpqemu_remote_msg_loop_co(void *data)
> +{
> + RemoteCommDev *com = (RemoteCommDev *)data;
> + PCIDevice *pci_dev = NULL;
> +
> + pci_dev = com->dev;
> + for (;;) {
> + MPQemuMsg msg = {0};
> + Error *local_err = NULL;
> +
> + if (!com->ioc) {
> + error_report("ERROR: No channel available");
> + break;
> + }
>
Shouldn't this be assert() at the top?
> + mpqemu_msg_recv(&msg, com->ioc, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + break;
>
Error handling is not consistent in this function. Could you cleanup error
code paths so error handling & reporting is done in one place?
+ }
> +
> + if (!mpqemu_msg_valid(&msg)) {
> + error_report("Received invalid message from proxy"
> + "in remote process pid=%d", getpid());
> + break;
> + }
> +
> + switch (msg.cmd) {
> + default:
> + error_setg(&local_err,
> + "Unknown command (%d) received for device %s
> (pid=%d)",
> + msg.cmd, DEVICE(pci_dev)->id, getpid());
> + }
> +
> + if (local_err) {
> + error_report_err(local_err);
> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>
Presumably that error handling should be done outside of the for(;;) loop.
SHUTDOWN_CAUSE_HOST_ERROR might be more appropriate in this case, or
perhaps introduce a new ShutdownCause?
+ break;
> + }
> + }
> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +
> + return;
>
needless return statement
+}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d0c891a..b64e4b8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3143,6 +3143,7 @@ F: hw/remote/machine.c
> F: include/hw/remote/machine.h
> F: hw/remote/mpqemu-link.c
> F: include/hw/remote/mpqemu-link.h
> +F: hw/remote/message.c
>
> Build and test automation
> -------------------------
> diff --git a/hw/remote/meson.build b/hw/remote/meson.build
> index a2b2fc0..9f5c57f 100644
> --- a/hw/remote/meson.build
> +++ b/hw/remote/meson.build
> @@ -2,5 +2,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'))
> +remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('message.c'))
>
> softmmu_ss.add_all(when: 'CONFIG_MULTIPROCESS', if_true: remote_ss)
> --
> 1.8.3.1
>
>
--
Marc-André Lureau
