Re: [PATCH v9 06/20] multi-process: define MPQemuMsg format and transmission functions

[Stefan Hajnoczi]

On Thu, Aug 27, 2020 at 11:12:17AM -0700, elena.ufimtseva@oracle.com wrote:
> +/**
> + * MPQemuCmd:
> + *
> + * MPQemuCmd enum type to specify the command to be executed on the remote
> + * device.
> + */
> +typedef enum {
> +    INIT = 0,
> +    MAX = INT_MAX,

enum members are in a global namespace. INIT and MAX are likely to
collide with other code using these names. Please use MPQEMU_CMD_INIT
and MPQEMU_CMD_MAX.

Why is MAX = INT_MAX? I expected MAX to be related to the number of
commands that have been defined (1 so far).

> +} 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;

I worry about the hole (compiler-inserted padding) between the cmd and
size fields.

It is safer to use fixed-size types and use QEMU_PACKED for structs that
are transferred over the network:

  typedef struct {
      int32_t cmd;
      uint64_t size;
      ...
  } QEMU_PACKED MPQemuMsg;

This way the struct layout is independent of the compilation environment
(32-/64-bit, ABI) and there is no risk of accidentally exposing memory
(information leaks) through holes.

> +
> +    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/io/meson.build b/io/meson.build
> index 768c1b5ec3..3d40cd8867 100644
> --- a/io/meson.build
> +++ b/io/meson.build
> @@ -15,6 +15,8 @@ io_ss.add(files(
>    'task.c',
>  ))
>  
> +io_ss.add(when: 'CONFIG_MPQEMU', if_true: files('mpqemu-link.c'))
> +
>  io_ss = io_ss.apply(config_host, strict: false)
>  libio = static_library('io', io_ss.sources() + genh,
>                         dependencies: [io_ss.dependencies()],
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> new file mode 100644
> index 0000000000..d9be2bdeab
> --- /dev/null
> +++ b/io/mpqemu-link.c
> @@ -0,0 +1,173 @@
> +/*
> + * 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 "io/mpqemu-link.h"
> +#include "qapi/error.h"
> +#include "qemu/iov.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +
> +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
> +{
> +    bool iolock = qemu_mutex_iothread_locked();
> +    Error *local_err = NULL;
> +    struct iovec send[2];
> +    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;
> +    }
> +
> +    if (iolock) {
> +        qemu_mutex_unlock_iothread();
> +    }
> +
> +    (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, 
> nfds,
> +                                      &local_err);

I tried to understand when it is safe to call this function:

Thread    | Coroutine? | Comments
------------------------------------
Main loop | Yes        | Okay
Main loop | No         | Do not use, blocks main loop
vCPU      | Yes        | Careful, can move coroutine to main loop
vCPU      | No         | Okay if no other ioc activity in main loop
IOThread  | Yes        | Broken: we shouldn't touch the global mutex!
IOThread  | No         | Do not use, blocks IOThread

The IOThreads case with coroutines can be fixed by skipping
qemu_mutex_lock_iothread() when running in an IOThread. Please address
this.

Cases that look usable to me are:
1. Main loop with coroutines
2. vCPU without coroutines
3. IOThread with coroutines (needs fix though)

All this is not obvious so a comment and assertions would be good. The
following helpers are available for implementing assertions:
1. bool qemu_in_coroutine() -> true if running in a coroutine
2. qemu_get_current_aio_context() != qemu_get_aio_context() -> true in
   IOThread

> +
> +    if (iolock) {
> +        qemu_mutex_lock_iothread();
> +    }
> +
> +    if (errp) {
> +        error_propagate(errp, local_err);
> +    } else if (local_err) {
> +        error_report_err(local_err);
> +    }
> +
> +    return;
> +}
> +
> +static ssize_t mpqemu_read(QIOChannel *ioc, void *buf, size_t len, int **fds,
> +                           size_t *nfds, Error **errp)

The same constraints apply to this function.

signature.asc
Description: PGP signature