Re: [Qemu-devel] [PATCH] qmp: extend QMP to provide read/write access to physical memory

[Fam Zheng]

On Wed, 11/26 12:27, Bryan D. Payne wrote:
> This patch adds a new QMP command that sets up a domain socket. This
> socket can then be used for fast read/write access to the guest's
> physical memory. The key benefit to this system over existing solutions
> is speed. Using this patch, guest memory can be copied out at a rate of
> ~200MB/sec, depending on the hardware. Existing solutions only achieve
> a small fraction of this speed.

Out of curiosity, what are existing solutions?

> 
> Signed-off-by: Bryan D. Payne <address@hidden>
> ---
>  Makefile.target  |   2 +-
>  memory-access.c  | 200 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  memory-access.h  |  11 +++
>  monitor.c        |   6 ++
>  qapi-schema.json |  12 ++++
>  qmp-commands.hx  |  28 ++++++++
>  6 files changed, 258 insertions(+), 1 deletion(-)
>  create mode 100644 memory-access.c
>  create mode 100644 memory-access.h
> 
> diff --git a/Makefile.target b/Makefile.target
> index e9ff1ee..4b3cd99 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -127,7 +127,7 @@ endif #CONFIG_BSD_USER
>  # System emulator target
>  ifdef CONFIG_SOFTMMU
>  obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o
> -obj-y += qtest.o bootdevice.o
> +obj-y += qtest.o bootdevice.o memory-access.o
>  obj-y += hw/
>  obj-$(CONFIG_FDT) += device_tree.o
>  obj-$(CONFIG_KVM) += kvm-all.o
> diff --git a/memory-access.c b/memory-access.c
> new file mode 100644
> index 0000000..f696d7b
> --- /dev/null
> +++ b/memory-access.c
> @@ -0,0 +1,200 @@
> +/*
> + * Access guest physical memory via a domain socket.
> + *
> + * Copyright (c) 2014 Bryan D. Payne (address@hidden)
> + */
> +
> +#include "memory-access.h"
> +#include "qemu-common.h"
> +#include "exec/cpu-common.h"
> +#include "config.h"
> +
> +#include <glib.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <pthread.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <unistd.h>
> +#include <signal.h>
> +#include <stdint.h>
> +
> +struct request {
> +    uint8_t type;      /* 0 quit, 1 read, 2 write, ... rest reserved */
> +    uint64_t address;  /* address to read from OR write to */
> +    uint64_t length;   /* number of bytes to read OR write */
> +};

Please add QEMU_PACKED to this structure, and probably name it QEMUMARequest,
for name collision avoidance and CamelCase convension.

> +
> +static uint64_t
> +connection_read_memory(uint64_t user_paddr, void *buf, uint64_t user_len)
> +{
> +    hwaddr paddr = (hwaddr) user_paddr;
> +    hwaddr len = (hwaddr) user_len;
> +    void *guestmem = cpu_physical_memory_map(paddr, &len, 0);

Accessing guest memory from another thread is not safe without holding the
lock: qemu_mutex_lock_iothread is needed before processing a request, and
unlock it after responding, to let guest continue.

> +    if (!guestmem) {
> +        return 0;
> +    }
> +    memcpy(buf, guestmem, len);
> +    cpu_physical_memory_unmap(guestmem, len, 0, len);
> +
> +    return len;
> +}
> +
> +static uint64_t
> +connection_write_memory(uint64_t user_paddr,
> +                        const void *buf,
> +                        uint64_t user_len)
> +{
> +    hwaddr paddr = (hwaddr) user_paddr;
> +    hwaddr len = (hwaddr) user_len;
> +    void *guestmem = cpu_physical_memory_map(paddr, &len, 1);
> +    if (!guestmem) {
> +        return 0;
> +    }
> +    memcpy(guestmem, buf, len);
> +    cpu_physical_memory_unmap(guestmem, len, 0, len);
> +
> +    return len;
> +}
> +
> +static void
> +send_success_ack(int connection_fd)
> +{
> +    uint8_t success = 1;
> +    int nbytes = write(connection_fd, &success, 1);
> +    if (nbytes != 1) {
> +        printf("QemuMemoryAccess: failed to send success ack\n");
> +    }
> +}
> +
> +static void
> +send_fail_ack(int connection_fd)
> +{
> +    uint8_t fail = 0;
> +    int nbytes = write(connection_fd, &fail, 1);
> +    if (nbytes != 1) {
> +        printf("QemuMemoryAccess: failed to send fail ack\n");

fprintf(stderr ?

> +    }
> +}
> +
> +static void
> +connection_handler(int connection_fd)
> +{
> +    int nbytes;
> +    struct request req;
> +
> +    while (1) {
> +        /* client request should match the struct request format */
> +        nbytes = read(connection_fd, &req, sizeof(struct request));
> +        if (nbytes != sizeof(struct request)) {
> +            /* error */
> +            send_fail_ack(connection_fd);
> +            continue;
> +        } else if (req.type == 0) {

I suggest using macro names for enum instead of magic values 0, 1, 2.

> +            /* request to quit, goodbye */
> +            break;
> +        } else if (req.type == 1) {
> +            /* request to read */
> +            char *buf = g_malloc(req.length + 1);

Is there a limit for r/w length? We could abort if req.length is too big.

> +            nbytes = connection_read_memory(req.address, buf, req.length);
> +            if (nbytes != req.length) {
> +                /* read failure, return failure message */
> +                buf[req.length] = 0; /* set last byte to 0 for failure */
> +                nbytes = write(connection_fd, buf, 1);
> +            } else {
> +                /* read success, return bytes */
> +                buf[req.length] = 1; /* set last byte to 1 for success */
> +                nbytes = write(connection_fd, buf, nbytes + 1);
> +            }
> +            g_free(buf);
> +        } else if (req.type == 2) {
> +            /* request to write */
> +            void *write_buf = g_malloc(req.length);
> +            nbytes = read(connection_fd, &write_buf, req.length);
> +            if (nbytes != req.length) {

Is short read possible here?

> +                /* failed reading the message to write */
> +                send_fail_ack(connection_fd);
> +            } else{

s/else{/else {

> +                /* do the write */
> +                nbytes = connection_write_memory(req.address,
> +                                                 write_buf,
> +                                                 req.length);
> +                if (nbytes == req.length) {
> +                    send_success_ack(connection_fd);
> +                } else {
> +                    send_fail_ack(connection_fd);
> +                }
> +            }
> +            g_free(write_buf);
> +        } else {
> +            /* unknown command */
> +            printf("QemuMemoryAccess: ignoring unknown command (%d)\n",
> +                   req.type);
> +            send_fail_ack(connection_fd);
> +        }
> +    }
> +
> +    close(connection_fd);
> +}
> +
> +static void *
> +memory_access_thread(void *path)
> +{
> +    struct sockaddr_un address;
> +    int socket_fd, connection_fd, path_len;
> +    socklen_t address_length;
> +
> +    socket_fd = socket(PF_UNIX, SOCK_STREAM, 0);
> +    if (socket_fd < 0) {
> +        printf("QemuMemoryAccess: socket failed\n");
> +        goto error_exit;
> +    }
> +
> +    unlink(path); /* handle unlikely case that this temp file exists */
> +    memset(&address, 0, sizeof(struct sockaddr_un));
> +    address.sun_family = AF_UNIX;
> +    path_len = sprintf(address.sun_path, "%s", (char *) path);
> +    address_length = sizeof(address.sun_family) + path_len;
> +
> +    if (bind(socket_fd, (struct sockaddr *) &address, address_length) != 0) {
> +        printf("QemuMemoryAccess: bind failed\n");
> +        goto error_exit;

socket_fd is left open on error.

> +    }
> +    if (listen(socket_fd, 0) != 0) {
> +        printf("QemuMemoryAccess: listen failed\n");
> +        goto error_exit;
> +    }
> +
> +    connection_fd = accept(socket_fd,
> +                           (struct sockaddr *) &address,
> +                           &address_length);
> +    connection_handler(connection_fd);
> +
> +    close(socket_fd);
> +    unlink(path);
> +error_exit:
> +    g_free(path);
> +    return NULL;
> +}
> +
> +int
> +memory_access_start(const char *path)
> +{
> +    pthread_t thread;
> +    sigset_t set, oldset;
> +    int ret;
> +
> +    /* create a copy of path that we can safely use */
> +    char *pathcopy = g_malloc(strlen(path) + 1);
> +    memcpy(pathcopy, path, strlen(path) + 1);

char *pathcopy = g_strdup(path);

> +
> +    /* start the thread */
> +    sigfillset(&set);
> +    pthread_sigmask(SIG_SETMASK, &set, &oldset);
> +    ret = pthread_create(&thread, NULL, memory_access_thread, pathcopy);
> +    pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> +
> +    return ret;
> +}
> diff --git a/memory-access.h b/memory-access.h
> new file mode 100644
> index 0000000..cb5fe33
> --- /dev/null
> +++ b/memory-access.h
> @@ -0,0 +1,11 @@
> +/*
> + * Access guest physical memory via a domain socket.
> + *
> + * Copyright (c) 2014 Bryan D. Payne (address@hidden)
> + */
> +#ifndef MEMORY_ACCESS_H
> +#define MEMORY_ACCESS_H
> +
> +int memory_access_start(const char *path);
> +
> +#endif /* MEMORY_ACCESS_H */
> diff --git a/monitor.c b/monitor.c
> index fa00594..19724b8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -72,6 +72,7 @@
>  #include "block/qapi.h"
>  #include "qapi/qmp-event.h"
>  #include "qapi-event.h"
> +#include "memory-access.h"
>  
>  /* for pic/irq_info */
>  #if defined(TARGET_SPARC)
> @@ -1371,6 +1372,11 @@ static void do_print(Monitor *mon, const QDict *qdict)
>      monitor_printf(mon, "\n");
>  }
>  
> +void qmp_pmemaccess(const char *path, Error **err)
> +{
> +    memory_access_start(path);
> +}
> +
>  static void do_sum(Monitor *mon, const QDict *qdict)
>  {
>      uint32_t addr;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9ffdcf8..37a6657 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3515,3 +3515,15 @@
>  # Since: 2.1
>  ##
>  { 'command': 'rtc-reset-reinjection' }
> +
> +##
> +# @pmemaccess
> +#
> +# This command enables access to guest physical memory using
> +# a simple protocol over a UNIX domain socket.
> +#
> +# @path Location to use for the UNIX domain socket
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'pmemaccess', 'data': { 'path': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 718dd92..fab1322 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -609,6 +609,34 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "pmemaccess",
> +        .args_type  = "path:s",
> +        .params     = "path",
> +        .help       = "access to guest memory via domain socket at 'path'",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = qmp_marshal_input_pmemaccess,
> +    },
> +
> +SQMP
> +pmemaccess
> +----------
> +
> +This command enables access to guest physical memory using a simple protocol
> +over a UNIX domain socket.

Please document the protocol, as it's the most important part of this command.

Fam

> +
> +Arguments:
> +
> +- "path": path for domain socket (json-string)
> +
> +Example:
> +
> +-> { "execute": "pmemaccess",
> +             "arguments": { "path": "/tmp/guestname" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
>          .mhandler.cmd_new = qmp_marshal_input_migrate,
> -- 
> 1.9.1
> 
>