[Qemu-devel] [PATCH v8 0/7] qmp: add pmemload command

[Qemu-devel] [PATCH v8 0/7] qmp: add pmemload command

[Simon Ruderich]

Hello,

This time as a new thread to help patchew. The only change is to use the
correct next version.

Regards
Simon Ruderich

Simon Ruderich (7):
  cpus: correct coding style in qmp_memsave/qmp_pmemsave
  cpus: convert qmp_memsave/qmp_pmemsave to use qemu_open
  cpus: use size_t in qmp_memsave/qmp_pmemsave
  hmp: use l for size argument in memsave/pmemsave
  hmp: use F for filename arguments in memsave/pmemsave
  qmp: add pmemload command
  hmp: add pmemload command

 cpus.c          | 85 +++++++++++++++++++++++++++++++++++++++++--------
 hmp-commands.hx | 18 +++++++++--
 hmp.c           | 16 ++++++++--
 hmp.h           |  1 +
 qapi/misc.json  | 20 ++++++++++++
 5 files changed, 122 insertions(+), 18 deletions(-)

Diff between v7 and v8:

    diff --git a/qapi/misc.json b/qapi/misc.json
    index 39f5e7dd38..e4f39e31e5 100644
    --- a/qapi/misc.json
    +++ b/qapi/misc.json
    @@ -1201,7 +1201,7 @@
    #
    # Returns: Nothing on success
    #
    -# Since: 3.2
    +# Since: 4.0
    ##
    { 'command': 'pmemload',
    'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }

-- 
2.19.1

[Qemu-devel] [PATCH v8 1/7] cpus: correct coding style in qmp_memsave/qmp_pmemsave

[Simon Ruderich]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index a2b33ccb29..e67efbb58b 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2394,8 +2394,9 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
 
     while (size != 0) {
         l = sizeof(buf);
-        if (l > size)
+        if (l > size) {
             l = size;
+        }
         if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
             error_setg(errp, "Invalid addr 0x%016" PRIx64 "/size %" PRId64
                              " specified", orig_addr, orig_size);
@@ -2428,8 +2429,9 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
 
     while (size != 0) {
         l = sizeof(buf);
-        if (l > size)
+        if (l > size) {
             l = size;
+        }
         cpu_physical_memory_read(addr, buf, l);
         if (fwrite(buf, 1, l, f) != l) {
             error_setg(errp, QERR_IO_ERROR);
-- 
2.19.1

[Qemu-devel] [PATCH v8 2/7] cpus: convert qmp_memsave/qmp_pmemsave to use qemu_open

[Simon Ruderich]

qemu_open() allow passing file descriptors to qemu which is used in
restricted environments like libvirt where open() is prohibited.

Suggested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/cpus.c b/cpus.c
index e67efbb58b..c0d796f441 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2369,7 +2369,7 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
 void qmp_memsave(int64_t addr, int64_t size, const char *filename,
                  bool has_cpu, int64_t cpu_index, Error **errp)
 {
-    FILE *f;
+    int fd;
     uint32_t l;
     CPUState *cpu;
     uint8_t buf[1024];
@@ -2386,8 +2386,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
         return;
     }
 
-    f = fopen(filename, "wb");
-    if (!f) {
+    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600);
+    if (fd < 0) {
         error_setg_file_open(errp, errno, filename);
         return;
     }
@@ -2402,7 +2402,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
                              " specified", orig_addr, orig_size);
             goto exit;
         }
-        if (fwrite(buf, 1, l, f) != l) {
+        if (qemu_write_full(fd, buf, l) != l) {
             error_setg(errp, QERR_IO_ERROR);
             goto exit;
         }
@@ -2411,18 +2411,18 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
     }
 
 exit:
-    fclose(f);
+    qemu_close(fd);
 }
 
 void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
                   Error **errp)
 {
-    FILE *f;
+    int fd;
     uint32_t l;
     uint8_t buf[1024];
 
-    f = fopen(filename, "wb");
-    if (!f) {
+    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600);
+    if (fd < 0) {
         error_setg_file_open(errp, errno, filename);
         return;
     }
@@ -2433,7 +2433,7 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
             l = size;
         }
         cpu_physical_memory_read(addr, buf, l);
-        if (fwrite(buf, 1, l, f) != l) {
+        if (qemu_write_full(fd, buf, l) != l) {
             error_setg(errp, QERR_IO_ERROR);
             goto exit;
         }
@@ -2442,7 +2442,7 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
     }
 
 exit:
-    fclose(f);
+    qemu_close(fd);
 }
 
 void qmp_inject_nmi(Error **errp)
-- 
2.19.1

[Qemu-devel] [PATCH v8 3/7] cpus: use size_t in qmp_memsave/qmp_pmemsave

[Simon Ruderich]

It's the natural type for object sizes and matches the return value of
sizeof(buf).

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index c0d796f441..ee54595733 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2370,7 +2370,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
                  bool has_cpu, int64_t cpu_index, Error **errp)
 {
     int fd;
-    uint32_t l;
+    size_t l;
     CPUState *cpu;
     uint8_t buf[1024];
     int64_t orig_addr = addr, orig_size = size;
@@ -2418,7 +2418,7 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
                   Error **errp)
 {
     int fd;
-    uint32_t l;
+    size_t l;
     uint8_t buf[1024];
 
     fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600);
-- 
2.19.1

[Qemu-devel] [PATCH v8 4/7] hmp: use l for size argument in memsave/pmemsave

[Simon Ruderich]

i is only 32-bit. To prevent possible truncation when dumping large
memory regions use l which is target long.

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 hmp-commands.hx | 4 ++--
 hmp.c           | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index db0c681f74..ff96c3ad24 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -833,7 +833,7 @@ ETEXI
 
     {
         .name       = "memsave",
-        .args_type  = "val:l,size:i,filename:s",
+        .args_type  = "val:l,size:l,filename:s",
         .params     = "addr size file",
         .help       = "save to disk virtual memory dump starting at 'addr' of size 'size'",
         .cmd        = hmp_memsave,
@@ -847,7 +847,7 @@ ETEXI
 
     {
         .name       = "pmemsave",
-        .args_type  = "val:l,size:i,filename:s",
+        .args_type  = "val:l,size:l,filename:s",
         .params     = "addr size file",
         .help       = "save to disk physical memory dump starting at 'addr' of size 'size'",
         .cmd        = hmp_pmemsave,
diff --git a/hmp.c b/hmp.c
index 7828f93a39..42a5d163cd 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1143,7 +1143,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
 
 void hmp_memsave(Monitor *mon, const QDict *qdict)
 {
-    uint32_t size = qdict_get_int(qdict, "size");
+    uint64_t size = qdict_get_int(qdict, "size");
     const char *filename = qdict_get_str(qdict, "filename");
     uint64_t addr = qdict_get_int(qdict, "val");
     Error *err = NULL;
@@ -1160,7 +1160,7 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
 
 void hmp_pmemsave(Monitor *mon, const QDict *qdict)
 {
-    uint32_t size = qdict_get_int(qdict, "size");
+    uint64_t size = qdict_get_int(qdict, "size");
     const char *filename = qdict_get_str(qdict, "filename");
     uint64_t addr = qdict_get_int(qdict, "val");
     Error *err = NULL;
-- 
2.19.1

[Qemu-devel] [PATCH v8 5/7] hmp: use F for filename arguments in memsave/pmemsave

[Simon Ruderich]

This enables completion for the filename arguments.

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 hmp-commands.hx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index ff96c3ad24..2404a5210d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -833,7 +833,7 @@ ETEXI
 
     {
         .name       = "memsave",
-        .args_type  = "val:l,size:l,filename:s",
+        .args_type  = "val:l,size:l,filename:F",
         .params     = "addr size file",
         .help       = "save to disk virtual memory dump starting at 'addr' of size 'size'",
         .cmd        = hmp_memsave,
@@ -847,7 +847,7 @@ ETEXI
 
     {
         .name       = "pmemsave",
-        .args_type  = "val:l,size:l,filename:s",
+        .args_type  = "val:l,size:l,filename:F",
         .params     = "addr size file",
         .help       = "save to disk physical memory dump starting at 'addr' of size 'size'",
         .cmd        = hmp_pmemsave,
-- 
2.19.1

[Qemu-devel] [PATCH v8 6/7] qmp: add pmemload command

[Simon Ruderich]

Adapted patch from Baojun Wang [1] with the following commit message:

    I found this could be useful to have qemu-softmmu as a cross
    debugger (launch with -s -S command line option), then if we can
    have a command to load guest physical memory, we can use cross gdb
    to do some target debug which gdb cannot do directly.

This patch contains only the qmp changes of the original patch.

pmemload is necessary to directly write physical memory which is not
possible with gdb alone as it uses only logical addresses.

The QAPI for pmemload uses "val" as parameter name for the physical
address. This name is not very descriptive but is consistent with the
existing pmemsave. Changing the parameter name of pmemsave is not
possible without breaking the existing API.

[1]: https://lists.gnu.org/archive/html/qemu-trivial/2014-04/msg00074.html

Based-on-patch-by: Baojun Wang <wangbj@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c         | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/misc.json | 20 ++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/cpus.c b/cpus.c
index ee54595733..b80f331596 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2445,6 +2445,61 @@ exit:
     qemu_close(fd);
 }
 
+void qmp_pmemload(int64_t addr, const char *filename,
+                  bool has_size, int64_t size,
+                  bool has_offset, int64_t offset,
+                  Error **errp)
+{
+    int fd;
+    size_t l;
+    ssize_t r;
+    uint8_t buf[1024];
+
+    fd = qemu_open(filename, O_RDONLY | O_BINARY);
+    if (fd < 0) {
+        error_setg_file_open(errp, errno, filename);
+        return;
+    }
+    if (has_offset && offset > 0) {
+        if (lseek(fd, offset, SEEK_SET) != offset) {
+            error_setg_errno(errp, errno,
+                             "could not seek to offset %" PRIx64, offset);
+            goto exit;
+        }
+    }
+    if (!has_size) {
+        struct stat s;
+        if (fstat(fd, &s)) {
+            error_setg_errno(errp, errno, "could not fstat fd to get size");
+            goto exit;
+        }
+        if (S_ISCHR(s.st_mode) || S_ISBLK(s.st_mode)) {
+            error_setg(errp, "pmemload doesn't support char/block devices");
+            goto exit;
+        }
+        size = s.st_size;
+    }
+
+    while (size != 0) {
+        l = sizeof(buf);
+        if (l > size) {
+            l = size;
+        }
+        r = read(fd, buf, l);
+        if (r <= 0) {
+            error_setg(errp, QERR_IO_ERROR);
+            goto exit;
+        }
+        l = r; /* in case of short read */
+        cpu_physical_memory_write(addr, buf, l);
+        addr += l;
+        size -= l;
+    }
+
+exit:
+    qemu_close(fd);
+}
+
 void qmp_inject_nmi(Error **errp)
 {
     nmi_monitor_handle(monitor_get_cpu_index(), errp);
diff --git a/qapi/misc.json b/qapi/misc.json
index 6c1c5c0a37..e4f39e31e5 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1186,6 +1186,26 @@
 { 'command': 'pmemsave',
   'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
 
+##
+# @pmemload:
+#
+# Load a portion of guest physical memory from a file.
+#
+# @val: the physical address of the guest to start from
+#
+# @filename: the file to load the memory from as binary data
+#
+# @size: the size of memory region to load (defaults to whole file)
+#
+# @offset: the offset in the file to start from (defaults to 0)
+#
+# Returns: Nothing on success
+#
+# Since: 4.0
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }
+
 ##
 # @cont:
 #
-- 
2.19.1

[Qemu-devel] [PATCH v8 7/7] hmp: add pmemload command

[Simon Ruderich]

Adapted patch from Baojun Wang [1] with the following commit message:

    I found this could be useful to have qemu-softmmu as a cross
    debugger (launch with -s -S command line option), then if we can
    have a command to load guest physical memory, we can use cross gdb
    to do some target debug which gdb cannot do directly.

This patch contains only the hmp changes of the original patch.

[1]: https://lists.gnu.org/archive/html/qemu-trivial/2014-04/msg00074.html

Based-on-patch-by: Baojun Wang <wangbj@gmail.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 hmp-commands.hx | 14 ++++++++++++++
 hmp.c           | 12 ++++++++++++
 hmp.h           |  1 +
 3 files changed, 27 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2404a5210d..baadbb3a69 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -857,6 +857,20 @@ STEXI
 @item pmemsave @var{addr} @var{size} @var{file}
 @findex pmemsave
 save to disk physical memory dump starting at @var{addr} of size @var{size}.
+ETEXI
+
+    {
+        .name       = "pmemload",
+        .args_type  = "val:l,size:l,offset:l,filename:F",
+        .params     = "addr size offset file",
+        .help       = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'",
+        .cmd        = hmp_pmemload,
+    },
+
+STEXI
+@item pmemload @var{addr} @var{size} @var{offset} @var{file}
+@findex pmemload
+load from disk physical memory dump starting at @var{addr} of size @var{size} at file offset @var{offset}.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index 42a5d163cd..c2a77fc390 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1169,6 +1169,18 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+void hmp_pmemload(Monitor *mon, const QDict *qdict)
+{
+    uint64_t size = qdict_get_int(qdict, "size");
+    uint64_t offset = qdict_get_int(qdict, "offset");
+    const char *filename = qdict_get_str(qdict, "filename");
+    uint64_t addr = qdict_get_int(qdict, "val");
+    Error *err = NULL;
+
+    qmp_pmemload(addr, filename, true, size, true, offset, &err);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_ringbuf_write(Monitor *mon, const QDict *qdict)
 {
     const char *chardev = qdict_get_str(qdict, "device");
diff --git a/hmp.h b/hmp.h
index 5f1addcca2..6503146a8c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -49,6 +49,7 @@ void hmp_exit_preconfig(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
 void hmp_pmemsave(Monitor *mon, const QDict *qdict);
+void hmp_pmemload(Monitor *mon, const QDict *qdict);
 void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
 void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
 void hmp_cont(Monitor *mon, const QDict *qdict);
-- 
2.19.1

Re: [Qemu-devel] [PATCH v8 6/7] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> Adapted patch from Baojun Wang [1] with the following commit message:
>
>     I found this could be useful to have qemu-softmmu as a cross
>     debugger (launch with -s -S command line option), then if we can
>     have a command to load guest physical memory, we can use cross gdb
>     to do some target debug which gdb cannot do directly.
>
> This patch contains only the qmp changes of the original patch.
>
> pmemload is necessary to directly write physical memory which is not
> possible with gdb alone as it uses only logical addresses.
>
> The QAPI for pmemload uses "val" as parameter name for the physical
> address. This name is not very descriptive but is consistent with the
> existing pmemsave. Changing the parameter name of pmemsave is not
> possible without breaking the existing API.

As much as I dislike the name "val" for a base address, I agree with you
to prioritize consistency.

> [1]: https://lists.gnu.org/archive/html/qemu-trivial/2014-04/msg00074.html
>
> Based-on-patch-by: Baojun Wang <wangbj@gmail.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>  cpus.c         | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/misc.json | 20 ++++++++++++++++++
>  2 files changed, 75 insertions(+)
>
> diff --git a/cpus.c b/cpus.c
> index ee54595733..b80f331596 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2445,6 +2445,61 @@ exit:
>      qemu_close(fd);
>  }
>  
> +void qmp_pmemload(int64_t addr, const char *filename,

I first parameter is called @val in the schema, and @addr here.  I hate
that, but it also matches memsave.

> +                  bool has_size, int64_t size,
> +                  bool has_offset, int64_t offset,
> +                  Error **errp)
> +{
> +    int fd;
> +    size_t l;
> +    ssize_t r;
> +    uint8_t buf[1024];
> +
> +    fd = qemu_open(filename, O_RDONLY | O_BINARY);
> +    if (fd < 0) {
> +        error_setg_file_open(errp, errno, filename);
> +        return;
> +    }
> +    if (has_offset && offset > 0) {
> +        if (lseek(fd, offset, SEEK_SET) != offset) {
> +            error_setg_errno(errp, errno,
> +                             "could not seek to offset %" PRIx64, offset);

I think the value of @filename is more interesting than the value of
@offset here.

> +            goto exit;
> +        }
> +    }
> +    if (!has_size) {
> +        struct stat s;
> +        if (fstat(fd, &s)) {
> +            error_setg_errno(errp, errno, "could not fstat fd to get size");
> +            goto exit;
> +        }

TOCTTOU: the size can change while you read the file.  I figure nothing
truly terrible can happen here that way, but it does set a bad example.

> +        if (S_ISCHR(s.st_mode) || S_ISBLK(s.st_mode)) {
> +            error_setg(errp, "pmemload doesn't support char/block devices");
> +            goto exit;
> +        }

Any particular reason for rejecting character and block devices when
@size is absent, yet accepting them when @size is present?

> +        size = s.st_size;
> +    }
> +
> +    while (size != 0) {
> +        l = sizeof(buf);
> +        if (l > size) {
> +            l = size;
> +        }
> +        r = read(fd, buf, l);
> +        if (r <= 0) {
> +            error_setg(errp, QERR_IO_ERROR);
> +            goto exit;

Please avoid the QERR_ macros, as requested by qerror.h.

Two ways to get r == 0:

* If has_size: the file is shorter than the user's size value.

* If !has_size: the file has shrunk since fstat().

In both cases, reporting an I/O error is not nice.

> +        }
> +        l = r; /* in case of short read */
> +        cpu_physical_memory_write(addr, buf, l);
> +        addr += l;
> +        size -= l;
> +    }

This reads and copies in chunks of at most 1KiB.  Feels rather small.  I
figure 4KiB would still be conservative enough, and at least matches
common page and block sizes.  Paolo, what do you think?

> +
> +exit:
> +    qemu_close(fd);
> +}
> +

Here's my (compile-tested) attempt to sidestep TOCTTOU:

    int fd;
    uint64_t limit, ofs;
    ssize_t n;
    uint8_t buf[4096];

    fd = qemu_open(filename, O_RDONLY | O_BINARY);
    if (fd < 0) {
        error_setg_file_open(errp, errno, filename);
        return;
    }

    if (has_offset && offset > 0) {
        if (lseek(fd, offset, SEEK_SET) != offset) {
            error_setg_errno(errp, errno, "could not seek '%s'", filename);
            goto exit;
        }
    }

    limit = has_size ? size : UINT64_MAX;
    ofs = 0;
    while (ofs < limit) {
        n = MIN(sizeof(buf), limit - ofs);
        n = read(fd, buf, n);
        if (n < 0) {
            error_setg_errno(errp, errno, "could not read from '%s'",
                             filename);
            goto exit;
        }
        if (n == 0) {
            break;
        }
        cpu_physical_memory_write(addr + ofs, buf, n);
        ofs += n;
    }

exit:
    qemu_close(fd);

Note that this *silently* reads less than @size when the file isn't big
enough.  If we decide that's what we want, then the command's contract
in misc.json has to be updated accordingly.

If it isn't, we can turn the break into an error_setg(); goto exit.  Not
terribly satisfying, since we already changed the physical memory.

>  void qmp_inject_nmi(Error **errp)
>  {
>      nmi_monitor_handle(monitor_get_cpu_index(), errp);
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 6c1c5c0a37..e4f39e31e5 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1186,6 +1186,26 @@
>  { 'command': 'pmemsave',
>    'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
>  
> +##
> +# @pmemload:
> +#
> +# Load a portion of guest physical memory from a file.
> +#
> +# @val: the physical address of the guest to start from
> +#
> +# @filename: the file to load the memory from as binary data
> +#
> +# @size: the size of memory region to load (defaults to whole file)
> +#
> +# @offset: the offset in the file to start from (defaults to 0)
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 4.0
> +##
> +{ 'command': 'pmemload',
> +  'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }
> +
>  ##
>  # @cont:
>  #

Using 'int' for a physical address is problematic, because it requires
users to express the upper half of the 64-bit address space as negative
numbers.  I started to fix up these things some time ago, but got stuck.
Here's the relevant part:

    Message-ID: <87fud26pfi.fsf@dusky.pond.sub.org>
    https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01114.html

Perhaps we should cherry-pick just that patch from the series into this
series, with the format string bug David pointed out fixed.

@offset and @size feel a bit like overengineering to me.  If this work
was still at the design stage, I'd advise to start without them, then
see whether there's a genuine need.  At v5, I'm li

Re: [Qemu-devel] [PATCH v8 6/7] qmp: add pmemload command

[Eric Blake]

On 11/28/18 6:37 AM, Markus Armbruster wrote:
...
> 
> @offset and @size feel a bit like overengineering to me.  If this work
> was still at the design stage, I'd advise to start without them, then
> see whether there's a genuine need.  At v5, I'm li
> 
> 

Was there intended to be more to this message?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v8 6/7] qmp: add pmemload command

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/28/18 6:37 AM, Markus Armbruster wrote:
> ...
>>
>> @offset and @size feel a bit like overengineering to me.  If this work
>> was still at the design stage, I'd advise to start without them, then
>> see whether there's a genuine need.  At v5, I'm li
>>
>>
>
> Was there intended to be more to this message?

Oops!  At v5, it's probably better not to argue about this anymore.