On Wed, Dec 2, 2020 at 12:23 AM Jagannathan Raman <jag.ra...@oracle.com>
wrote:
> SyncSysMemMsg message format is defined. It is used to send
> file descriptors of the RAM regions to remote device.
> RAM on the remote device is configured with a set of file descriptors.
> Old RAM regions are deleted and new regions, each with an fd, is
> added to the RAM.
>
> Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> include/hw/remote/memory.h | 19 ++++++++++++++
> include/hw/remote/mpqemu-link.h | 13 +++++++++
> hw/remote/memory.c | 58
> +++++++++++++++++++++++++++++++++++++++++
> hw/remote/mpqemu-link.c | 11 ++++++++
> MAINTAINERS | 2 ++
> hw/remote/meson.build | 2 ++
> 6 files changed, 105 insertions(+)
> create mode 100644 include/hw/remote/memory.h
> create mode 100644 hw/remote/memory.c
>
> diff --git a/include/hw/remote/memory.h b/include/hw/remote/memory.h
> new file mode 100644
> index 0000000..4fd548e
> --- /dev/null
> +++ b/include/hw/remote/memory.h
> @@ -0,0 +1,19 @@
> +/*
> + * Memory manager for remote device
> + *
> + * 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 REMOTE_MEMORY_H
> +#define REMOTE_MEMORY_H
> +
> +#include "exec/hwaddr.h"
> +#include "hw/remote/mpqemu-link.h"
> +
> +void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp);
> +
> +#endif
> diff --git a/include/hw/remote/mpqemu-link.h
> b/include/hw/remote/mpqemu-link.h
> index 2d79ff8..070ac77 100644
> --- a/include/hw/remote/mpqemu-link.h
> +++ b/include/hw/remote/mpqemu-link.h
> @@ -14,6 +14,7 @@
> #include "qom/object.h"
> #include "qemu/thread.h"
> #include "io/channel.h"
> +#include "exec/hwaddr.h"
>
> #define REMOTE_MAX_FDS 8
>
> @@ -24,12 +25,22 @@
> *
> * MPQemuCmd enum type to specify the command to be executed on the remote
> * device.
> + *
> + * SYNC_SYSMEM Shares QEMU's RAM with remote device's RAM
>
My understanding is that it's deliberately a private protocol between qemu
and remote host processes, so that it can break anytime. And in the future
it will be vfio-user based. Correct? It would be worth to leave a note
about it somewhere.
*/
> typedef enum {
> MPQEMU_CMD_INIT,
> + SYNC_SYSMEM,
> + RET_MSG,
> MPQEMU_CMD_MAX,
> } MPQemuCmd;
>
> +typedef struct {
> + hwaddr gpas[REMOTE_MAX_FDS];
> + uint64_t sizes[REMOTE_MAX_FDS];
> + off_t offsets[REMOTE_MAX_FDS];
> +} SyncSysmemMsg;
> +
> /**
> * MPQemuMsg:
> * @cmd: The remote command
> @@ -40,12 +51,14 @@ typedef enum {
> * MPQemuMsg Format of the message sent to the remote device from QEMU.
> *
> */
> +
> typedef struct {
> int cmd;
> size_t size;
>
> union {
> uint64_t u64;
> + SyncSysmemMsg sync_sysmem;
> } data;
>
> int fds[REMOTE_MAX_FDS];
> diff --git a/hw/remote/memory.c b/hw/remote/memory.c
> new file mode 100644
> index 0000000..6d1e830
> --- /dev/null
> +++ b/hw/remote/memory.c
> @@ -0,0 +1,58 @@
> +/*
> + * Memory manager for remote device
> + *
> + * 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 "hw/remote/memory.h"
> +#include "exec/address-spaces.h"
> +#include "exec/ram_addr.h"
> +#include "qapi/error.h"
> +
> +void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
> +{
> + SyncSysmemMsg *sysmem_info = &msg->data.sync_sysmem;
> + MemoryRegion *sysmem, *subregion, *next;
> + static unsigned int suffix;
> + Error *local_err = NULL;
> + char *name;
> + int region;
> +
> + sysmem = get_system_memory();
> +
> + memory_region_transaction_begin();
>
It looks like this transaction business isn't really helping here. Both
del_subregion and add_subregion already handle it.
+
> + QTAILQ_FOREACH_SAFE(subregion, &sysmem->subregions, subregions_link,
> next) {
> + if (subregion->ram) {
> + memory_region_del_subregion(sysmem, subregion);
> + object_unparent(OBJECT(subregion));
> + }
> + }
> +
> + for (region = 0; region < msg->num_fds; region++) {
> + subregion = g_new(MemoryRegion, 1);
> + name = g_strdup_printf("remote-mem-%u", suffix++);
> + memory_region_init_ram_from_fd(subregion, NULL,
> + name, sysmem_info->sizes[region],
> + true, msg->fds[region],
> + sysmem_info->offsets[region],
> + &local_err);
> + g_free(name);
>
We are quite happily using g_auto these days
> + if (local_err) {
> + error_propagate(errp, local_err);
>
here that would help prevent leaking subregion.
And ERRP_GUARD could also help remove local_err and error_propagate
+ break;
> + }
> +
> + memory_region_add_subregion(sysmem, sysmem_info->gpas[region],
> + subregion);
> + }
> +
> + memory_region_transaction_commit();
> +}
> diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c
> index e535ed2..bbd9df3 100644
> --- a/hw/remote/mpqemu-link.c
> +++ b/hw/remote/mpqemu-link.c
> @@ -238,5 +238,16 @@ bool mpqemu_msg_valid(MPQemuMsg *msg)
> }
> }
>
> + /* Verify message specific fields. */
> + switch (msg->cmd) {
> + case SYNC_SYSMEM:
> + if (msg->num_fds == 0 || msg->size != sizeof(SyncSysmemMsg)) {
> + return false;
> + }
> + break;
> + default:
> + break;
> + }
> +
> return true;
> }
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aedfc27..24cb36e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3146,6 +3146,8 @@ F: include/hw/remote/mpqemu-link.h
> F: hw/remote/message.c
> F: include/hw/remote/remote-obj.h
> F: hw/remote/remote-obj.c
> +F: include/hw/remote/memory.h
> +F: hw/remote/memory.c
>
> Build and test automation
> -------------------------
> diff --git a/hw/remote/meson.build b/hw/remote/meson.build
> index 71d0a56..64da16c 100644
> --- a/hw/remote/meson.build
> +++ b/hw/remote/meson.build
> @@ -5,4 +5,6 @@ remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true:
> files('mpqemu-link.c'))
> remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('message.c'))
> remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('remote-obj.c'))
>
> +specific_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('memory.c'))
> +
> softmmu_ss.add_all(when: 'CONFIG_MULTIPROCESS', if_true: remote_ss)
> --
> 1.8.3.1
>
>
I can't comment much on the overall approach. That looks very
straightforward to me, I would be surprised if there are not underlying
subtleties. Again, is this only experimental until a vfio-user
implementation emerge? If not, having unit tests would be important (if not
required).
--
Marc-André Lureau
