On Thu, Aug 27, 2020 at 11:12:17AM -0700, elena.ufimts...@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
