[Qemu-devel] [PATCH] qmp: add pmemload command

[Qemu-devel] [PATCH] 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.

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

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

Based-on-patch-by: Baojun Wang <wangbj@gmail.com>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---

Hello,

I'm using pmemload to manipulate physical memory in Qemu with
this patch; the existing pmemsave can be used to dump the
physical memory.

I've only used pmemload from qemu's monitor and not via qapi.
This part was taken unchanged from the original patch.

Regards
Simon

 cpus.c           | 30 ++++++++++++++++++++++++++++++
 hmp-commands.hx  | 14 ++++++++++++++
 hmp.c            | 11 +++++++++++
 hmp.h            |  1 +
 qapi-schema.json | 18 ++++++++++++++++++
 5 files changed, 74 insertions(+)

diff --git a/cpus.c b/cpus.c
index 114c29b6a0..b2325dd7bb 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2039,6 +2039,36 @@ exit:
     fclose(f);
 }
 
+void qmp_pmemload(int64_t addr, int64_t size, const char *filename,
+                  Error **errp)
+{
+    FILE *f;
+    size_t l;
+    uint8_t buf[1024];
+
+    f = fopen(filename, "rb");
+    if (!f) {
+        error_setg_file_open(errp, errno, filename);
+        return;
+    }
+
+    while (size != 0) {
+        l = sizeof(buf);
+        if (l > size)
+            l = size;
+        if (fread(buf, 1, l, f) != l) {
+            error_setg(errp, QERR_IO_ERROR);
+            goto exit;
+        }
+        cpu_physical_memory_write(addr, buf, l);
+        addr += l;
+        size -= l;
+    }
+
+exit:
+    fclose(f);
+}
+
 void qmp_inject_nmi(Error **errp)
 {
     nmi_monitor_handle(monitor_get_cpu_index(), errp);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4afd57cf5f..cc1956252e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -853,6 +853,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:i,filename:s",
+        .params     = "addr size file",
+        .help       = "load from disk physical memory dump starting at 'addr' of size 'size'",
+        .cmd        = hmp_pmemload,
+    },
+
+STEXI
+@item pmemload @var{addr} @var{size} @var{file}
+@findex pmemload
+load from disk physical memory dump starting at @var{addr} of size @var{size}.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index 35a7041824..bec5eac621 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1107,6 +1107,17 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+void hmp_pmemload(Monitor *mon, const QDict *qdict)
+{
+    uint32_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;
+
+    qmp_pmemload(addr, size, filename, &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 a6f56b1f29..b433d919e9 100644
--- a/hmp.h
+++ b/hmp.h
@@ -49,6 +49,7 @@ void hmp_system_powerdown(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);
diff --git a/qapi-schema.json b/qapi-schema.json
index 18457954a8..a013d590c5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1136,6 +1136,24 @@
 { '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
+#
+# @size: the size of memory region to load
+#
+# @filename: the file to load the memory from as binary data
+#
+# Returns: Nothing on success
+#
+# Since: 2.10
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
+
 ##
 # @cont:
 #
-- 
2.15.0

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [Qemu-devel] [PATCH] qmp: add pmemload command

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 0e59c79ddc01e195ddc59d77d9df2b95bf89b600.1523395243.git.simon@ruderich.org
Subject: [Qemu-devel] [PATCH] qmp: add pmemload command

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/0e59c79ddc01e195ddc59d77d9df2b95bf89b600.1523395243.git.simon@ruderich.org -> patchew/0e59c79ddc01e195ddc59d77d9df2b95bf89b600.1523395243.git.simon@ruderich.org
Switched to a new branch 'test'
199564756b qmp: add pmemload command

=== OUTPUT BEGIN ===
Checking PATCH 1/1: qmp: add pmemload command...
ERROR: braces {} are necessary for all arms of this statement
#45: FILE: cpus.c:2330:
+        if (l > size)
[...]

total: 1 errors, 0 warnings, 104 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] qmp: add pmemload command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]

On 04/10/2018 04:24 PM, Simon Ruderich wrote:
> 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.
> 
> pmemload is necessary to directly write physical memory which is not
> possible with gdb alone as it uses only logical addresses.
> 
> [1]: https://lists.gnu.org/archive/html/qemu-trivial/2014-04/msg00073.html
> 
> Based-on-patch-by: Baojun Wang <wangbj@gmail.com>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---

> +void qmp_pmemload(int64_t addr, int64_t size, const char *filename,
> +                  Error **errp)
> +{
> +    FILE *f;
> +    size_t l;
> +    uint8_t buf[1024];
> +
> +    f = fopen(filename, "rb");

Use qemu_fopen() here, so that you can automagically support /dev/fdset/
magic filenames that work on file descriptors passed via SCM_RIGHTS.


> +++ b/qapi-schema.json
> @@ -1136,6 +1136,24 @@
>  { '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

Is 'val' really the best name for this, or would 'phys-addr' or similar
be a more descriptive name?

> +#
> +# @size: the size of memory region to load
> +#
> +# @filename: the file to load the memory from as binary data
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.10

You've missed 2.10 by a long shot.  The earliest this new feature could
appear is 2.13.

Do you additionally need an offset where to start reading from within
the file (that is, since you already have the 'size' parameter to avoid
reading the entire file, and the 'val' parameter to target anywhere in
physical memory, how do I start reading anywhere from the file)?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] qmp: add pmemload command

[Simon Ruderich]

On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote:
>> +void qmp_pmemload(int64_t addr, int64_t size, const char *filename,
>> +                  Error **errp)
>> +{
>> +    FILE *f;
>> +    size_t l;
>> +    uint8_t buf[1024];
>> +
>> +    f = fopen(filename, "rb");
>
> Use qemu_fopen() here, so that you can automagically support /dev/fdset/
> magic filenames that work on file descriptors passed via SCM_RIGHTS.

Hello,

I can't find qemu_fopen() in the source. Did you mean
qemu_open()? From reading qemu_close() I guess that I can't use
fdopen() (as there's no qemu_fclose()) but must work with the raw
fd. Or is there an easy way to fdopen? (I prefer the FILE *
interface which is easier to work with.)

But I just copied the code from qmp_pmemsave. Should I change it
as well to use qemu_open()?

>> +++ b/qapi-schema.json
>> @@ -1136,6 +1136,24 @@
>>  { '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
>
> Is 'val' really the best name for this, or would 'phys-addr' or similar
> be a more descriptive name?

I copied it from pmemsave to keep the argument names consistent.
Should I change it only for pmemload? Changing it for pmemsave is
problematic as it will break the existing API.

>> +#
>> +# @size: the size of memory region to load
>> +#
>> +# @filename: the file to load the memory from as binary data
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since: 2.10
>
> You've missed 2.10 by a long shot.  The earliest this new feature could
> appear is 2.13.

Will change.

> Do you additionally need an offset where to start reading from within
> the file (that is, since you already have the 'size' parameter to avoid
> reading the entire file, and the 'val' parameter to target anywhere in
> physical memory, how do I start reading anywhere from the file)?

I didn't need it for my usage and wanted to keep the patch as
simple as possible. But I see how it can be useful and will add
it to my patch in the next iteration.

Thank you for the review!

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [Qemu-devel] [PATCH] qmp: add pmemload command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]

On 04/11/2018 02:36 AM, Simon Ruderich wrote:
> On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote:
>>> +void qmp_pmemload(int64_t addr, int64_t size, const char *filename,
>>> +                  Error **errp)
>>> +{
>>> +    FILE *f;
>>> +    size_t l;
>>> +    uint8_t buf[1024];
>>> +
>>> +    f = fopen(filename, "rb");
>>
>> Use qemu_fopen() here, so that you can automagically support /dev/fdset/
>> magic filenames that work on file descriptors passed via SCM_RIGHTS.
> 
> Hello,
> 
> I can't find qemu_fopen() in the source. Did you mean
> qemu_open()?

Looks like you're right; we don't have an automatic FILE wrapper.

> From reading qemu_close() I guess that I can't use
> fdopen() (as there's no qemu_fclose()) but must work with the raw
> fd. Or is there an easy way to fdopen? (I prefer the FILE *
> interface which is easier to work with.)

You could always add qemu_fopen/qemu_fclose to match the existing
qemu_open/qemu_close.  But you do have a point that you can't call
qemu_close/fclose (because fclose would be left with a stale fd that
might spuriously close something that has been opened in another thread
during the race window), nor fclose/qemu_close.

The FILE interface may sometimes be easier to work with, but it also
comes with buffering issues that you don't have to worry about when
using the fd interface.

> 
> But I just copied the code from qmp_pmemsave. Should I change it
> as well to use qemu_open()?

Good point - but yes, ideally it should always be possible to pass in an
fd instead of having qemu itself open a file, because there are
execution environments where qemu is intentionally prohibited from
directly calling open() for security reasons (where the management app,
such as libvirt, opens and then passes fds to qemu instead).


>>> +##
>>> +# @pmemload:
>>> +#
>>> +# Load a portion of guest physical memory from a file.
>>> +#
>>> +# @val: the physical address of the guest to start from
>>
>> Is 'val' really the best name for this, or would 'phys-addr' or similar
>> be a more descriptive name?
> 
> I copied it from pmemsave to keep the argument names consistent.
> Should I change it only for pmemload? Changing it for pmemsave is
> problematic as it will break the existing API.

You are correct that we can't really change the existing interface; so
documenting in the commit message that your choice of names is for
consistency reasons may be sufficient.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] qmp: add pmemload command

[Simon Ruderich]

On Wed, Apr 11, 2018 at 08:02:58AM -0500, Eric Blake wrote:
> You could always add qemu_fopen/qemu_fclose to match the existing
> qemu_open/qemu_close.  But you do have a point that you can't call
> qemu_close/fclose (because fclose would be left with a stale fd that
> might spuriously close something that has been opened in another thread
> during the race window), nor fclose/qemu_close.
>
> The FILE interface may sometimes be easier to work with, but it also
> comes with buffering issues that you don't have to worry about when
> using the fd interface.

Yeah. I'm using plain qemu_open()/qemu_close() and read/write
directly.

> Good point - but yes, ideally it should always be possible to pass in an
> fd instead of having qemu itself open a file, because there are
> execution environments where qemu is intentionally prohibited from
> directly calling open() for security reasons (where the management app,
> such as libvirt, opens and then passes fds to qemu instead).

I've converted qmp_memsave/qmp_pmemsave in extra patches to use
qemu_open() (and some additional cleanup).

> You are correct that we can't really change the existing interface; so
> documenting in the commit message that your choice of names is for
> consistency reasons may be sufficient.

Will do.

Revised patches which should implement all of your suggestions
(and the style issue noticed by the bot) as replies to this mail.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

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

[Simon Ruderich]

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 38eba8bff3..c78f430532 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2263,8 +2263,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);
@@ -2297,8 +2298,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.15.0

[Qemu-devel] [PATCH v2 2/5] 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>
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 c78f430532..292d5b94b1 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2238,7 +2238,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];
@@ -2255,8 +2255,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;
     }
@@ -2271,7 +2271,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;
         }
@@ -2280,18 +2280,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;
     }
@@ -2302,7 +2302,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;
         }
@@ -2311,7 +2311,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.15.0

Re: [Qemu-devel] [PATCH v2 2/5] cpus: convert qmp_memsave/qmp_pmemsave to use qemu_open

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On 04/12/2018 07:50 AM, Simon Ruderich wrote:
> qemu_open() allow passing file descriptors to qemu which is used in

s/allow/allows/

> restricted environments like libvirt where open() is prohibited.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>  cpus.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v2 3/5] 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).

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 292d5b94b1..d256d8e9b4 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2239,7 +2239,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;
@@ -2287,7 +2287,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.15.0

Re: [Qemu-devel] [PATCH v2 3/5] cpus: use size_t in qmp_memsave/qmp_pmemsave

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On 04/12/2018 07:50 AM, Simon Ruderich wrote:
> It's the natural type for object sizes and matches the return value of
> sizeof(buf).
> 
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>  cpus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v2 4/5] hmp: don't truncate size in hmp_memsave/hmp_pmemsave

[Simon Ruderich]

The called function takes an uint64_t as size parameter and
qdict_get_int() returns an uint64_t. Don't truncate it needlessly to an
uint32_t.

Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 hmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hmp.c b/hmp.c
index a25c7bd9a8..1e392055f7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1064,7 +1064,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;
@@ -1081,7 +1081,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.15.0

Re: [Qemu-devel] [PATCH v2 4/5] hmp: don't truncate size in hmp_memsave/hmp_pmemsave

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On 04/12/2018 07:50 AM, Simon Ruderich wrote:
> The called function takes an uint64_t as size parameter and
> qdict_get_int() returns an uint64_t. Don't truncate it needlessly to an
> uint32_t.
> 
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>  hmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v2 5/5] 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.

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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c          | 41 +++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx | 14 ++++++++++++++
 hmp.c           | 12 ++++++++++++
 hmp.h           |  1 +
 qapi/misc.json  | 20 ++++++++++++++++++++
 5 files changed, 88 insertions(+)

diff --git a/cpus.c b/cpus.c
index d256d8e9b4..c690dc205f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2314,6 +2314,47 @@ exit:
     qemu_close(fd);
 }
 
+void qmp_pmemload(int64_t addr, int64_t size, int64_t offset,
+                  const char *filename, 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 (offset > 0) {
+        if (lseek(fd, offset, SEEK_SET) != offset) {
+            error_setg_errno(errp, errno,
+                             "could not seek to offset %" PRIx64, offset);
+            goto exit;
+        }
+    }
+
+    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/hmp-commands.hx b/hmp-commands.hx
index 35d862a5d2..a392d0e379 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -822,6 +822,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:i,offset:i,filename:s",
+        .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 1e392055f7..c9b37dbfbd 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1090,6 +1090,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, size, offset, filename, &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 4e2ec375b0..0a69e371ca 100644
--- a/hmp.h
+++ b/hmp.h
@@ -47,6 +47,7 @@ void hmp_system_powerdown(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);
diff --git a/qapi/misc.json b/qapi/misc.json
index 5636f4a149..2255d219fa 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1185,6 +1185,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
+#
+# @size: the size of memory region to load
+#
+# @offset: the offset in the file to start from
+#
+# @filename: the file to load the memory from as binary data
+#
+# Returns: Nothing on success
+#
+# Since: 2.13
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
+
 ##
 # @cont:
 #
-- 
2.15.0

Re: [Qemu-devel] [PATCH v2 5/5] qmp: add pmemload command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

On 04/12/2018 07:50 AM, Simon Ruderich wrote:
> 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.
> 
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---

Focusing on just the interface:

> +++ b/qapi/misc.json
> @@ -1185,6 +1185,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
> +#
> +# @size: the size of memory region to load

Should size be an optional parameter (default read to the end of the file)?

> +#
> +# @offset: the offset in the file to start from

Should offset be an optional parameter (default start reading from
offset 0 of the file)?

> +#
> +# @filename: the file to load the memory from as binary data
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.13
> +##
> +{ 'command': 'pmemload',
> +  'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
> +
>  ##
>  # @cont:
>  #
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 5/5] qmp: add pmemload command

[Simon Ruderich]

On Tue, Apr 17, 2018 at 04:18:43PM -0500, Eric Blake wrote:
> Focusing on just the interface:
>
>> +++ b/qapi/misc.json
>> @@ -1185,6 +1185,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
>> +#
>> +# @size: the size of memory region to load
>
> Should size be an optional parameter (default read to the end of the file)?
>
>> +#
>> +# @offset: the offset in the file to start from
>
> Should offset be an optional parameter (default start reading from
> offset 0 of the file)?

>From the QAPI point-of-view making both optional seems like a
good idea, however then pmemsave should also mark its "size"
parameter optional for consistency (this is backwards compatible
as existing callers are not affected).

On the monitor API side this is more problematic as the order of
the pmemsave/pmemload parameters make optional parameters
impossible.

Personally I'd keep it as is because it's simple and consistent
with the existing interface (and can be changed in the future
without breaking compatibility).

In case you prefer those parameters to be optional I'll require
some help to implement that: I don't understand how
qapi-schema.json, hmp-commands.hx and hmp.h interact (I've to
admit I just copied that from the original patch and didn't give
it much thought as it worked fine) and would require some
pointers on how to implement optional arguments for
qapi-schema.json.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [Qemu-devel] [PATCH v2 5/5] qmp: add pmemload command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]

On 04/22/2018 04:47 AM, Simon Ruderich wrote:
> On Tue, Apr 17, 2018 at 04:18:43PM -0500, Eric Blake wrote:
>> Focusing on just the interface:
>>
>>> +++ b/qapi/misc.json
>>> @@ -1185,6 +1185,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
>>> +#
>>> +# @size: the size of memory region to load
>>
>> Should size be an optional parameter (default read to the end of the file)?
>>
>>> +#
>>> +# @offset: the offset in the file to start from
>>
>> Should offset be an optional parameter (default start reading from
>> offset 0 of the file)?
> 
>>From the QAPI point-of-view making both optional seems like a
> good idea, however then pmemsave should also mark its "size"
> parameter optional for consistency (this is backwards compatible
> as existing callers are not affected).

Correct.

> 
> On the monitor API side this is more problematic as the order of
> the pmemsave/pmemload parameters make optional parameters
> impossible.

Back-compat in the QMP interface matters.  The HMP interface, however,
exists to serve humans not machines, and we can break it at will to
something that makes more sense to humans.  So don't let HMP concerns
hold you back from a sane interface.

> 
> Personally I'd keep it as is because it's simple and consistent
> with the existing interface (and can be changed in the future
> without breaking compatibility).
> 
> In case you prefer those parameters to be optional I'll require
> some help to implement that: I don't understand how
> qapi-schema.json, hmp-commands.hx and hmp.h interact (I've to
> admit I just copied that from the original patch and didn't give
> it much thought as it worked fine) and would require some
> pointers on how to implement optional arguments for
> qapi-schema.json.

Optional parameters are listed as '*name':'type' in the .json file,
which tells the generator to create a 'has_name' bool parameter
alongside the 'name' parameter in the C code for the QMP interface.  The
HMP interface should then call into the QMP interface.

Recent HMP patches that I've authored may offer some inspiration: commit
08fb10a added a new command, and commit dba4932 added an optional
parameter to an existing command.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 5/5] qmp: add pmemload command

[Simon Ruderich]

On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote:
> Back-compat in the QMP interface matters.  The HMP interface, however,
> exists to serve humans not machines, and we can break it at will to
> something that makes more sense to humans.  So don't let HMP concerns
> hold you back from a sane interface.

I see. However I don't like breaking existing interfaces unless I
have to. In this case I think not having the optional parameters
is fine and the consistency between the existing memsave/pmemsave
functions is more important.

> Optional parameters are listed as '*name':'type' in the .json file,
> which tells the generator to create a 'has_name' bool parameter
> alongside the 'name' parameter in the C code for the QMP interface.  The
> HMP interface should then call into the QMP interface.
>
> Recent HMP patches that I've authored may offer some inspiration: commit
> 08fb10a added a new command, and commit dba4932 added an optional
> parameter to an existing command.

Thank you for the explanation, this looks straight-forward.

Do you have strong opinions regarding the optional parameters or
would you accept the patch as is (minus possible implementation
issues)? I like the symmetry to the existing functions (I noticed
that size can only be optional for pmemload because saving the
complete memory doesn't sound useful) and having to specify
size/offset doesn't hurt the caller too much.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [Qemu-devel] [PATCH v2 1/5] cpus: correct coding style in qmp_memsave/qmp_pmemsave

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

On 04/12/2018 07:50 AM, Simon Ruderich wrote:
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>  cpus.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

However, as a meta-comment, this message was sent with:

> Message-Id: <68c390f22ae2afc6539cd7b127063e3d9534d50b.1523537181.git.simon@ruderich.org>
> In-Reply-To: <20180412124834.GA2025@ruderich.org>
> References: <20180412124834.GA2025@ruderich.org>

Looking on patchew, I see:
http://patchew.org/QEMU/20180412124834.GA2025@ruderich.org/
The requested URL /QEMU/20180412124834.GA2025@ruderich.org/ was not
found on this server.

For most messages, looking up the In-Reply-To: message-id gives the
cover letter (for example,
http://patchew.org/QEMU/20180412115838.10208-1-alex.bennee@linaro.org/)

Similarly, looking on the list archives, I don't see the 0/5 cover
letter, but rather that your messages are treated as threaded to your v1
thread:

https://lists.gnu.org/archive/html/qemu-devel/2018-04/threads.html#01388

For the sake of tooling, it's best to send a v2 series with a new cover
letter as a new top-level thread.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 1/5] cpus: correct coding style in qmp_memsave/qmp_pmemsave

[Simon Ruderich]

On Tue, Apr 17, 2018 at 03:56:58PM -0500, Eric Blake wrote:
> On 04/12/2018 07:50 AM, Simon Ruderich wrote:
>> Signed-off-by: Simon Ruderich <simon@ruderich.org>
>> ---
>>  cpus.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> However, as a meta-comment, this message was sent with:
>
>> Message-Id: <68c390f22ae2afc6539cd7b127063e3d9534d50b.1523537181.git.simon@ruderich.org>
>> In-Reply-To: <20180412124834.GA2025@ruderich.org>
>> References: <20180412124834.GA2025@ruderich.org>
>
> Looking on patchew, I see:
> http://patchew.org/QEMU/20180412124834.GA2025@ruderich.org/
> The requested URL /QEMU/20180412124834.GA2025@ruderich.org/ was not
> found on this server.
>
> For most messages, looking up the In-Reply-To: message-id gives the
> cover letter (for example,
> http://patchew.org/QEMU/20180412115838.10208-1-alex.bennee@linaro.org/)
>
> Similarly, looking on the list archives, I don't see the 0/5 cover
> letter, but rather that your messages are treated as threaded to your v1
> thread:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-04/threads.html#01388
>
> For the sake of tooling, it's best to send a v2 series with a new cover
> letter as a new top-level thread.

Yeah, I screwed that part up. Sorry for the extra work. Will fix
when I resend the series.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[Qemu-devel] [PATCH v3 0/5] qmp: add pmemload command

[Simon Ruderich]

Hello,

This is third version of this patch set, rebased on current
master.

As I've received no answers to [1] (and I'd prefer to keep the
patch as is for now if possible) this doesn't include any changes
to address the comments to [2].

If there's anything else I can do to get these patches merged
please tell me.

Regards
Simon

[1]: <20180424145053.GA21648@ruderich.org>
     https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03894.html
[2]: <6f775e11a75a2faa1c66a86e6d23a97f695c2ca1.1523537181.git.simon@ruderich.org>
     https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg01757.html

Simon Ruderich (5):
  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: don't truncate size in hmp_memsave/hmp_pmemsave
  qmp: add pmemload command

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

-- 
2.15.0

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

[Simon Ruderich]

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

diff --git a/cpus.c b/cpus.c
index d1f16296de..4b1609fe90 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2316,8 +2316,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);
@@ -2350,8 +2351,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.15.0

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

[Simon Ruderich]

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

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

diff --git a/cpus.c b/cpus.c
index 4b1609fe90..7fd8d3c32e 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2291,7 +2291,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];
@@ -2308,8 +2308,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;
     }
@@ -2324,7 +2324,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;
         }
@@ -2333,18 +2333,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;
     }
@@ -2355,7 +2355,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;
         }
@@ -2364,7 +2364,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.15.0

[Qemu-devel] [PATCH v3 3/5] 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).

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

diff --git a/cpus.c b/cpus.c
index 7fd8d3c32e..49d4d44916 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2292,7 +2292,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;
@@ -2340,7 +2340,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.15.0

[Qemu-devel] [PATCH v3 4/5] hmp: don't truncate size in hmp_memsave/hmp_pmemsave

[Simon Ruderich]

The called function takes an uint64_t as size parameter and
qdict_get_int() returns an uint64_t. Don't truncate it needlessly to an
uint32_t.

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

diff --git a/hmp.c b/hmp.c
index ef93f4878b..a4d28913bb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1079,7 +1079,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;
@@ -1096,7 +1096,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.15.0

[Qemu-devel] [PATCH v3 5/5] 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.

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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c          | 41 +++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx | 14 ++++++++++++++
 hmp.c           | 12 ++++++++++++
 hmp.h           |  1 +
 qapi/misc.json  | 20 ++++++++++++++++++++
 5 files changed, 88 insertions(+)

diff --git a/cpus.c b/cpus.c
index 49d4d44916..9b105336af 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2367,6 +2367,47 @@ exit:
     qemu_close(fd);
 }
 
+void qmp_pmemload(int64_t addr, int64_t size, int64_t offset,
+                  const char *filename, 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 (offset > 0) {
+        if (lseek(fd, offset, SEEK_SET) != offset) {
+            error_setg_errno(errp, errno,
+                             "could not seek to offset %" PRIx64, offset);
+            goto exit;
+        }
+    }
+
+    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/hmp-commands.hx b/hmp-commands.hx
index 0734fea931..84647c7c1d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -822,6 +822,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:i,offset:i,filename:s",
+        .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 a4d28913bb..b85c943b63 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1105,6 +1105,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, size, offset, filename, &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 20f27439d3..31767ea4a8 100644
--- a/hmp.h
+++ b/hmp.h
@@ -47,6 +47,7 @@ void hmp_system_powerdown(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);
diff --git a/qapi/misc.json b/qapi/misc.json
index f5988cc0b5..b4c0065b02 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1219,6 +1219,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
+#
+# @size: the size of memory region to load
+#
+# @offset: the offset in the file to start from
+#
+# @filename: the file to load the memory from as binary data
+#
+# Returns: Nothing on success
+#
+# Since: 2.13
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
+
 ##
 # @cont:
 #
-- 
2.15.0

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Eric Blake]

On 05/21/2018 10:36 AM, Simon Ruderich wrote:
> 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.
> 
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>   cpus.c          | 41 +++++++++++++++++++++++++++++++++++++++++
>   hmp-commands.hx | 14 ++++++++++++++
>   hmp.c           | 12 ++++++++++++
>   hmp.h           |  1 +
>   qapi/misc.json  | 20 ++++++++++++++++++++
>   5 files changed, 88 insertions(+)

I haven't closely reviewed this, but a quick note:


> +++ b/qapi/misc.json
> @@ -1219,6 +1219,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
> +#
> +# @size: the size of memory region to load
> +#
> +# @offset: the offset in the file to start from
> +#
> +# @filename: the file to load the memory from as binary data
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.13

There was no 2.13 release, and this feature has missed the 3.0 
deadlines, so it would need to say 'Since: 3.1'

> +##
> +{ 'command': 'pmemload',
> +  'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
> +
>   ##
>   # @cont:
>   #
> 

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

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

On Thu, Aug 09, 2018 at 11:46:50AM -0500, Eric Blake wrote:
> There was no 2.13 release, and this feature has missed the 3.0 deadlines, so
> it would need to say 'Since: 3.1'

Will do.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Dr. David Alan Gilbert]

* Simon Ruderich (simon@ruderich.org) wrote:
> 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.
> 
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>  cpus.c          | 41 +++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx | 14 ++++++++++++++
>  hmp.c           | 12 ++++++++++++
>  hmp.h           |  1 +
>  qapi/misc.json  | 20 ++++++++++++++++++++
>  5 files changed, 88 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index 49d4d44916..9b105336af 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2367,6 +2367,47 @@ exit:
>      qemu_close(fd);
>  }
>  
> +void qmp_pmemload(int64_t addr, int64_t size, int64_t offset,
> +                  const char *filename, 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 (offset > 0) {
> +        if (lseek(fd, offset, SEEK_SET) != offset) {
> +            error_setg_errno(errp, errno,
> +                             "could not seek to offset %" PRIx64, offset);
> +            goto exit;
> +        }
> +    }
> +
> +    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/hmp-commands.hx b/hmp-commands.hx
> index 0734fea931..84647c7c1d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -822,6 +822,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:i,offset:i,filename:s",
> +        .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,
> +    },
> +

I'm guessing that size and offset should be 'l' to allow large
sizes and offsets, and there's an 'F' type for filenames
(see monitor.c which has a big comment table near the start).

Also, had you considered rearranging and making them optional,
for example if you do:

val:l,filename:F,offset:i?,size:i?

I think that would mean you can do the fairly obvious:
  pmemload addr "myfile"

with the assumption that loads the whole file.

Dave

> +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 a4d28913bb..b85c943b63 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1105,6 +1105,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, size, offset, filename, &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 20f27439d3..31767ea4a8 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -47,6 +47,7 @@ void hmp_system_powerdown(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);
> diff --git a/qapi/misc.json b/qapi/misc.json
> index f5988cc0b5..b4c0065b02 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1219,6 +1219,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
> +#
> +# @size: the size of memory region to load
> +#
> +# @offset: the offset in the file to start from
> +#
> +# @filename: the file to load the memory from as binary data
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.13
> +##
> +{ 'command': 'pmemload',
> +  'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
> +
>  ##
>  # @cont:
>  #
> -- 
> 2.15.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 5093 bytes --]

On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -822,6 +822,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:i,offset:i,filename:s",
>> +        .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,
>> +    },
>> +
>
> I'm guessing that size and offset should be 'l' to allow large
> sizes and offsets, and there's an 'F' type for filenames

I've copied that from "pmemsave" and "memsave" which use 'i' for
size. I'll add patches which will adapt them to use both 'l' and
'F' and adapt my pmemload patch as well.

qapi/misc.json seems to always use 'int' for integer types. Is
this value large enough on 64-bit architectures?

> (see monitor.c which has a big comment table near the start).

Thanks.

Just curious, what is the difference between 's' and 'F'. Is that
only for documentation purposes (and maybe tab completion) or is
the usage different? I noticed existing code uses qdict_get_str()
for both 's' and 'F'.

> Also, had you considered rearranging and making them optional,
> for example if you do:
>
> val:l,filename:F,offset:i?,size:i?
>
> I think that would mean you can do the fairly obvious:
>   pmemload addr "myfile"
>
> with the assumption that loads the whole file.

I tried to keep it as similar to the existing functions
"memsave"/"pmemsave", only adding "offset". Eric Blake already
raised this issue in the thread, here's my response for
reference:

On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote:
> Back-compat in the QMP interface matters.  The HMP interface, however,
> exists to serve humans not machines, and we can break it at will to
> something that makes more sense to humans.  So don't let HMP concerns
> hold you back from a sane interface.

I see. However I don't like breaking existing interfaces unless I
have to. In this case I think not having the optional parameters
is fine and the consistency between the existing memsave/pmemsave
functions is more important.

> Optional parameters are listed as '*name':'type' in the .json file,
> which tells the generator to create a 'has_name' bool parameter
> alongside the 'name' parameter in the C code for the QMP interface.  The
> HMP interface should then call into the QMP interface.
>
> Recent HMP patches that I've authored may offer some inspiration: commit
> 08fb10a added a new command, and commit dba4932 added an optional
> parameter to an existing command.

Thank you for the explanation, this looks straight-forward.

Do you have strong opinions regarding the optional parameters or
would you accept the patch as is (minus possible implementation
issues)? I like the symmetry to the existing functions (I noticed
that size can only be optional for pmemload because saving the
complete memory doesn't sound useful) and having to specify
size/offset doesn't hurt the caller too much.


Thanks for your review!

Regards
Simon

PS: Diff between v3 and my current local version follows:

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 5a43dae133..c39d745a22 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -818,7 +818,7 @@ ETEXI
 
     {
         .name       = "memsave",
-        .args_type  = "val:l,size:i,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,
@@ -832,7 +832,7 @@ ETEXI
 
     {
         .name       = "pmemsave",
-        .args_type  = "val:l,size:i,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,
@@ -846,7 +846,7 @@ ETEXI
 
     {
         .name       = "pmemload",
-        .args_type  = "val:l,size:i,offset:i,filename:s",
+        .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,
diff --git a/qapi/misc.json b/qapi/misc.json
index 6c34b2ff8b..becc257a76 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1196,7 +1196,7 @@
 #
 # Returns: Nothing on success
 #
-# Since: 2.13
+# Since: 3.1
 ##
 { 'command': 'pmemload',
   'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>>> --- a/hmp-commands.hx
>>> +++ b/hmp-commands.hx

Subject claims "qmp: add", but the patch also adds to hmp.  Recommend to
split the patch into QMP and HMP part.

>>> @@ -822,6 +822,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:i,offset:i,filename:s",
>>> +        .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,
>>> +    },
>>> +
>>
>> I'm guessing that size and offset should be 'l' to allow large
>> sizes and offsets, and there's an 'F' type for filenames
>
> I've copied that from "pmemsave" and "memsave" which use 'i' for
> size. I'll add patches which will adapt them to use both 'l' and
> 'F' and adapt my pmemload patch as well.
>
> qapi/misc.json seems to always use 'int' for integer types. Is
> this value large enough on 64-bit architectures?

Yes.  QAPI's int translates to int64_t.

>> (see monitor.c which has a big comment table near the start).
>
> Thanks.
>
> Just curious, what is the difference between 's' and 'F'. Is that
> only for documentation purposes (and maybe tab completion) or is
> the usage different? I noticed existing code uses qdict_get_str()
> for both 's' and 'F'.

The main behavioral difference is completion.

>> Also, had you considered rearranging and making them optional,
>> for example if you do:
>>
>> val:l,filename:F,offset:i?,size:i?
>>
>> I think that would mean you can do the fairly obvious:
>>   pmemload addr "myfile"
>>
>> with the assumption that loads the whole file.
>
> I tried to keep it as similar to the existing functions
> "memsave"/"pmemsave", only adding "offset". Eric Blake already
> raised this issue in the thread, here's my response for
> reference:
>
> On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote:
>> Back-compat in the QMP interface matters.  The HMP interface, however,
>> exists to serve humans not machines, and we can break it at will to
>> something that makes more sense to humans.  So don't let HMP concerns
>> hold you back from a sane interface.
>
> I see. However I don't like breaking existing interfaces unless I
> have to. In this case I think not having the optional parameters
> is fine and the consistency between the existing memsave/pmemsave
> functions is more important.
>
>> Optional parameters are listed as '*name':'type' in the .json file,
>> which tells the generator to create a 'has_name' bool parameter
>> alongside the 'name' parameter in the C code for the QMP interface.  The
>> HMP interface should then call into the QMP interface.
>>
>> Recent HMP patches that I've authored may offer some inspiration: commit
>> 08fb10a added a new command, and commit dba4932 added an optional
>> parameter to an existing command.
>
> Thank you for the explanation, this looks straight-forward.
>
> Do you have strong opinions regarding the optional parameters or
> would you accept the patch as is (minus possible implementation
> issues)? I like the symmetry to the existing functions (I noticed
> that size can only be optional for pmemload because saving the
> complete memory doesn't sound useful) and having to specify
> size/offset doesn't hurt the caller too much.

I recommend to start with the QMP interface.  Parameters are unordered
there.  memsave and pmemsave both take mandatory @val, @size, @filename.
memsave additionally takes optional @cpu-index.

Your pmemload has pmemsave's arguments plus and mandatory @offset.
Rationale for adding @offset?  You may have answered this question
already; pointer to that answer would be fine.

Once we got the QMP interface nailed down, we can move to the HMP
interface.

> Thanks for your review!
>
> Regards
> Simon
>
> PS: Diff between v3 and my current local version follows:
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 5a43dae133..c39d745a22 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -818,7 +818,7 @@ ETEXI
>  
>      {
>          .name       = "memsave",
> -        .args_type  = "val:l,size:i,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,
> @@ -832,7 +832,7 @@ ETEXI
>  
>      {
>          .name       = "pmemsave",
> -        .args_type  = "val:l,size:i,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,

These two should become a separate bug fix patch.  The bug being fixed
is completion.

> @@ -846,7 +846,7 @@ ETEXI
>  
>      {
>          .name       = "pmemload",
> -        .args_type  = "val:l,size:i,offset:i,filename:s",
> +        .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,
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 6c34b2ff8b..becc257a76 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1196,7 +1196,7 @@
>  #
>  # Returns: Nothing on success
>  #
> -# Since: 2.13
> +# Since: 3.1
>  ##
>  { 'command': 'pmemload',
>    'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

On Tue, Aug 14, 2018 at 05:49:12PM +0200, Markus Armbruster wrote:
>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>>>> --- a/hmp-commands.hx
>>>> +++ b/hmp-commands.hx
>
> Subject claims "qmp: add", but the patch also adds to hmp.  Recommend to
> split the patch into QMP and HMP part.

Hello,

Sure, I can do that.

>> qapi/misc.json seems to always use 'int' for integer types. Is
>> this value large enough on 64-bit architectures?
>
> Yes.  QAPI's int translates to int64_t.

Thanks.

>> Just curious, what is the difference between 's' and 'F'. Is that
>> only for documentation purposes (and maybe tab completion) or is
>> the usage different? I noticed existing code uses qdict_get_str()
>> for both 's' and 'F'.
>
> The main behavioral difference is completion.

Good to know, thanks.

> I recommend to start with the QMP interface.  Parameters are unordered
> there.  memsave and pmemsave both take mandatory @val, @size, @filename.
> memsave additionally takes optional @cpu-index.

Yes.

> Your pmemload has pmemsave's arguments plus and mandatory @offset.
> Rationale for adding @offset?  You may have answered this question
> already; pointer to that answer would be fine.

My initial patch didn't have the offset. It was suggested by Eric
Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>:

On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote:
> Do you additionally need an offset where to start reading from within
> the file (that is, since you already have the 'size' parameter to avoid
> reading the entire file, and the 'val' parameter to target anywhere in
> physical memory, how do I start reading anywhere from the file)?

It sounded useful to me so I added it.

> Once we got the QMP interface nailed down, we can move to the HMP
> interface.

Good point.

> These two should become a separate bug fix patch.  The bug being fixed
> is completion.

Sure, they are in separate patches. Just wanted to show the
general changes I applied from the reviews.

Thanks for the review.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> On Tue, Aug 14, 2018 at 05:49:12PM +0200, Markus Armbruster wrote:
>>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>>>>> --- a/hmp-commands.hx
>>>>> +++ b/hmp-commands.hx
>>
>> Subject claims "qmp: add", but the patch also adds to hmp.  Recommend to
>> split the patch into QMP and HMP part.
>
> Hello,
>
> Sure, I can do that.
>
>>> qapi/misc.json seems to always use 'int' for integer types. Is
>>> this value large enough on 64-bit architectures?
>>
>> Yes.  QAPI's int translates to int64_t.
>
> Thanks.
>
>>> Just curious, what is the difference between 's' and 'F'. Is that
>>> only for documentation purposes (and maybe tab completion) or is
>>> the usage different? I noticed existing code uses qdict_get_str()
>>> for both 's' and 'F'.
>>
>> The main behavioral difference is completion.
>
> Good to know, thanks.
>
>> I recommend to start with the QMP interface.  Parameters are unordered
>> there.  memsave and pmemsave both take mandatory @val, @size, @filename.
>> memsave additionally takes optional @cpu-index.
>
> Yes.
>
>> Your pmemload has pmemsave's arguments plus and mandatory @offset.
>> Rationale for adding @offset?  You may have answered this question
>> already; pointer to that answer would be fine.
>
> My initial patch didn't have the offset. It was suggested by Eric
> Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>:
>
> On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote:
>> Do you additionally need an offset where to start reading from within
>> the file (that is, since you already have the 'size' parameter to avoid
>> reading the entire file, and the 'val' parameter to target anywhere in
>> physical memory, how do I start reading anywhere from the file)?
>
> It sounded useful to me so I added it.

Feels like an optional parameter to me.

>> Once we got the QMP interface nailed down, we can move to the HMP
>> interface.
>
> Good point.
>
>> These two should become a separate bug fix patch.  The bug being fixed
>> is completion.
>
> Sure, they are in separate patches. Just wanted to show the
> general changes I applied from the reviews.
>
> Thanks for the review.
>
> Regards
> Simon

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

On Wed, Aug 15, 2018 at 06:22:51AM +0200, Markus Armbruster wrote:
>> My initial patch didn't have the offset. It was suggested by Eric
>> Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>:
>>
>> On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote:
>>> Do you additionally need an offset where to start reading from within
>>> the file (that is, since you already have the 'size' parameter to avoid
>>> reading the entire file, and the 'val' parameter to target anywhere in
>>> physical memory, how do I start reading anywhere from the file)?
>>
>> It sounded useful to me so I added it.
>
> Feels like an optional parameter to me.

For the HMP or the QMP interface?

If you think 'offset' is not necessary I can also drop it
completely.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> On Wed, Aug 15, 2018 at 06:22:51AM +0200, Markus Armbruster wrote:
>>> My initial patch didn't have the offset. It was suggested by Eric
>>> Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e482@redhat.com>:
>>>
>>> On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote:
>>>> Do you additionally need an offset where to start reading from within
>>>> the file (that is, since you already have the 'size' parameter to avoid
>>>> reading the entire file, and the 'val' parameter to target anywhere in
>>>> physical memory, how do I start reading anywhere from the file)?
>>>
>>> It sounded useful to me so I added it.
>>
>> Feels like an optional parameter to me.
>
> For the HMP or the QMP interface?

Both.

> If you think 'offset' is not necessary I can also drop it
> completely.

I think it's a reasonable feature, and since you already coded it up...

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Wed, Aug 15, 2018 at 04:29:25PM +0200, Markus Armbruster wrote:
>> For the HMP or the QMP interface?
>
> Both.

Ok.

>> If you think 'offset' is not necessary I can also drop it
>> completely.
>
> I think it's a reasonable feature, and since you already coded it up...

In that case, should size also become optional? As already
suggested in this thread (and similar for QMP):

On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
> Also, had you considered rearranging and making them optional,
> for example if you do:
>
> val:l,filename:F,offset:i?,size:i?
>
> I think that would mean you can do the fairly obvious:
>   pmemload addr "myfile"
>
> with the assumption that loads the whole file.

This would deviate from pmemsave/memsave, but feels more natural.

How are multiple optional parameters handled? Filled from
left-to-right?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> On Wed, Aug 15, 2018 at 04:29:25PM +0200, Markus Armbruster wrote:
>>> For the HMP or the QMP interface?
>>
>> Both.
>
> Ok.
>
>>> If you think 'offset' is not necessary I can also drop it
>>> completely.
>>
>> I think it's a reasonable feature, and since you already coded it up...
>
> In that case, should size also become optional? As already
> suggested in this thread (and similar for QMP):
>
> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>> Also, had you considered rearranging and making them optional,
>> for example if you do:
>>
>> val:l,filename:F,offset:i?,size:i?
>>
>> I think that would mean you can do the fairly obvious:
>>   pmemload addr "myfile"
>>
>> with the assumption that loads the whole file.
>
> This would deviate from pmemsave/memsave, but feels more natural.

The different order or arguments in HMP is somewhat ugly.  Okay if it
makes the command more pleasant to use.  Up to you and Dave to decide.

If you decide to deviate, consider

    filename:F,address:l,size:i?,offset:i?

> How are multiple optional parameters handled? Filled from
> left-to-right?

HMP: yes.

QMP has only named parameters.

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Thu, Aug 16, 2018 at 07:43:41AM +0200, Markus Armbruster wrote:
>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>>> Also, had you considered rearranging and making them optional,
>>> for example if you do:
>>>
>>> val:l,filename:F,offset:i?,size:i?
>>>
>>> I think that would mean you can do the fairly obvious:
>>>   pmemload addr "myfile"
>>>
>>> with the assumption that loads the whole file.
>>
>> This would deviate from pmemsave/memsave, but feels more natural.
>
> The different order or arguments in HMP is somewhat ugly.  Okay if it
> makes the command more pleasant to use.  Up to you and Dave to decide.
>
> If you decide to deviate, consider
>
>     filename:F,address:l,size:i?,offset:i?

From what I understand we can't have optional arguments in the
middle. Would you prefer mandatory size/offset parameters in HMP
or optional parameters but inconsistent with pmemsave/memsave?
Both works for me.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> On Thu, Aug 16, 2018 at 07:43:41AM +0200, Markus Armbruster wrote:
>>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>>>> Also, had you considered rearranging and making them optional,
>>>> for example if you do:
>>>>
>>>> val:l,filename:F,offset:i?,size:i?
>>>>
>>>> I think that would mean you can do the fairly obvious:
>>>>   pmemload addr "myfile"
>>>>
>>>> with the assumption that loads the whole file.
>>>
>>> This would deviate from pmemsave/memsave, but feels more natural.
>>
>> The different order or arguments in HMP is somewhat ugly.  Okay if it
>> makes the command more pleasant to use.  Up to you and Dave to decide.
>>
>> If you decide to deviate, consider
>>
>>     filename:F,address:l,size:i?,offset:i?
>
> From what I understand we can't have optional arguments in the
> middle. Would you prefer mandatory size/offset parameters in HMP
> or optional parameters but inconsistent with pmemsave/memsave?

If you guys decide you want consistency with memsave, that's fine with
me.  If you decide you want to rearrange arguments for better usability,
that's also fine.  I just wanted to throw in another rearrangement for
you to consider.

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

On Thu, Aug 16, 2018 at 10:26:18AM +0200, Markus Armbruster wrote:
>> On Thu, Aug 16, 2018 at 07:43:41AM +0200, Markus Armbruster wrote:
>>>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>>>>> Also, had you considered rearranging and making them optional,
>>>>> for example if you do:
>>>>>
>>>>> val:l,filename:F,offset:i?,size:i?
>>>>>
>>>>> I think that would mean you can do the fairly obvious:
>>>>>   pmemload addr "myfile"
>>>>>
>>>>> with the assumption that loads the whole file.
>>>>
>>>> This would deviate from pmemsave/memsave, but feels more natural.
>>>
>>> The different order or arguments in HMP is somewhat ugly.  Okay if it
>>> makes the command more pleasant to use.  Up to you and Dave to decide.
>>>
>>> If you decide to deviate, consider
>>>
>>>     filename:F,address:l,size:i?,offset:i?
>>
>> From what I understand we can't have optional arguments in the
>> middle. Would you prefer mandatory size/offset parameters in HMP
>> or optional parameters but inconsistent with pmemsave/memsave?
>
> If you guys decide you want consistency with memsave, that's fine with
> me.  If you decide you want to rearrange arguments for better usability,
> that's also fine.  I just wanted to throw in another rearrangement for
> you to consider.

Thanks for your suggestions. Then I'll go with
"val:l,size:l,offset:l,filename:F" for now. I'll send the revised
patch series soon.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Dr. David Alan Gilbert]

* Simon Ruderich (simon@ruderich.org) wrote:
> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -822,6 +822,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:i,offset:i,filename:s",
> >> +        .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,
> >> +    },
> >> +
> >
> > I'm guessing that size and offset should be 'l' to allow large
> > sizes and offsets, and there's an 'F' type for filenames
> 
> I've copied that from "pmemsave" and "memsave" which use 'i' for
> size. I'll add patches which will adapt them to use both 'l' and
> 'F' and adapt my pmemload patch as well.
> 
> qapi/misc.json seems to always use 'int' for integer types. Is
> this value large enough on 64-bit architectures?
> 
> > (see monitor.c which has a big comment table near the start).
> 
> Thanks.
> 
> Just curious, what is the difference between 's' and 'F'. Is that
> only for documentation purposes (and maybe tab completion) or is
> the usage different? I noticed existing code uses qdict_get_str()
> for both 's' and 'F'.
> 
> > Also, had you considered rearranging and making them optional,
> > for example if you do:
> >
> > val:l,filename:F,offset:i?,size:i?
> >
> > I think that would mean you can do the fairly obvious:
> >   pmemload addr "myfile"
> >
> > with the assumption that loads the whole file.
> 
> I tried to keep it as similar to the existing functions
> "memsave"/"pmemsave", only adding "offset". Eric Blake already
> raised this issue in the thread, here's my response for
> reference:
> 
> On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote:
> > Back-compat in the QMP interface matters.  The HMP interface, however,
> > exists to serve humans not machines, and we can break it at will to
> > something that makes more sense to humans.  So don't let HMP concerns
> > hold you back from a sane interface.
> 
> I see. However I don't like breaking existing interfaces unless I
> have to. In this case I think not having the optional parameters
> is fine and the consistency between the existing memsave/pmemsave
> functions is more important.
> 
> > Optional parameters are listed as '*name':'type' in the .json file,
> > which tells the generator to create a 'has_name' bool parameter
> > alongside the 'name' parameter in the C code for the QMP interface.  The
> > HMP interface should then call into the QMP interface.
> >
> > Recent HMP patches that I've authored may offer some inspiration: commit
> > 08fb10a added a new command, and commit dba4932 added an optional
> > parameter to an existing command.
> 
> Thank you for the explanation, this looks straight-forward.
> 
> Do you have strong opinions regarding the optional parameters or
> would you accept the patch as is (minus possible implementation
> issues)? I like the symmetry to the existing functions (I noticed
> that size can only be optional for pmemload because saving the
> complete memory doesn't sound useful) and having to specify
> size/offset doesn't hurt the caller too much.

No strong feeling either way; that was just a suggestion.

Dave

> 
> Thanks for your review!
> 
> Regards
> Simon
> 
> PS: Diff between v3 and my current local version follows:
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 5a43dae133..c39d745a22 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -818,7 +818,7 @@ ETEXI
>  
>      {
>          .name       = "memsave",
> -        .args_type  = "val:l,size:i,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,
> @@ -832,7 +832,7 @@ ETEXI
>  
>      {
>          .name       = "pmemsave",
> -        .args_type  = "val:l,size:i,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,
> @@ -846,7 +846,7 @@ ETEXI
>  
>      {
>          .name       = "pmemload",
> -        .args_type  = "val:l,size:i,offset:i,filename:s",
> +        .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,
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 6c34b2ff8b..becc257a76 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1196,7 +1196,7 @@
>  #
>  # Returns: Nothing on success
>  #
> -# Since: 2.13
> +# Since: 3.1
>  ##
>  { 'command': 'pmemload',
>    'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
> 
> -- 
> + privacy is necessary
> + using gnupg http://gnupg.org
> + public key id: 0x92FEFDB7E44C32F9


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 0/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Mon, May 21, 2018 at 05:36:39PM +0200, Simon Ruderich wrote:
> Hello,
>
> This is third version of this patch set, rebased on current
> master.
>
> As I've received no answers to [1] (and I'd prefer to keep the
> patch as is for now if possible) this doesn't include any changes
> to address the comments to [2].

Hello again,

I haven't received any answers in quite some time. Is there
anything I can do to get these patches merged? Are there any
issues left to resolve?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> On Mon, May 21, 2018 at 05:36:39PM +0200, Simon Ruderich wrote:
>> Hello,
>>
>> This is third version of this patch set, rebased on current
>> master.
>>
>> As I've received no answers to [1] (and I'd prefer to keep the
>> patch as is for now if possible) this doesn't include any changes
>> to address the comments to [2].
>
> Hello again,
>
> I haven't received any answers in quite some time. Is there
> anything I can do to get these patches merged? Are there any
> issues left to resolve?

You neglected to cc: maintainers.  I spotted this cry for help only by
chance.

To find maintainers to cc:, you can use get_maintainer.pl like this:

    $ scripts/get_maintainer.pl *patch
    scripts/get_maintainer.pl: file '0000-cover-letter.patch' doesn't appear to be a patch.  Add -f to options?
    Paolo Bonzini <pbonzini@redhat.com> (maintainer:Overall)
    Peter Crosthwaite <crosthwaite.peter@gmail.com> (maintainer:Overall)
    Richard Henderson <rth@twiddle.net> (maintainer:Overall)
    "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Human Monitor (HMP))
    Eric Blake <eblake@redhat.com> (supporter:QAPI Schema)
    Markus Armbruster <armbru@redhat.com> (supporter:QAPI Schema)
    qemu-devel@nongnu.org (open list:Overall)

It's output is advice, not gospel.  Use your judgement.

I'm copying maintainers for you now.  This should get you some review.

Re: [Qemu-devel] [PATCH v3 0/5] qmp: add pmemload command

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

On Fri, Aug 10, 2018 at 08:06:11AM +0200, Markus Armbruster wrote:
> You neglected to cc: maintainers.  I spotted this cry for help only by
> chance.
>
> To find maintainers to cc:, you can use get_maintainer.pl like this:
>
>     $ scripts/get_maintainer.pl *patch
>     scripts/get_maintainer.pl: file '0000-cover-letter.patch' doesn't appear to be a patch.  Add -f to options?
>     Paolo Bonzini <pbonzini@redhat.com> (maintainer:Overall)
>     Peter Crosthwaite <crosthwaite.peter@gmail.com> (maintainer:Overall)
>     Richard Henderson <rth@twiddle.net> (maintainer:Overall)
>     "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Human Monitor (HMP))
>     Eric Blake <eblake@redhat.com> (supporter:QAPI Schema)
>     Markus Armbruster <armbru@redhat.com> (supporter:QAPI Schema)
>     qemu-devel@nongnu.org (open list:Overall)
>
> It's output is advice, not gospel.  Use your judgement.
>
> I'm copying maintainers for you now.  This should get you some review.

Hello,

Thank you for your help. I missed that part when looking through
the patch submission instructions.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] qmp: add pmemload command

[Markus Armbruster]

Simon Ruderich <simon@ruderich.org> writes:

> On Fri, Aug 10, 2018 at 08:06:11AM +0200, Markus Armbruster wrote:
>> You neglected to cc: maintainers.  I spotted this cry for help only by
>> chance.
>>
>> To find maintainers to cc:, you can use get_maintainer.pl like this:
>>
>>     $ scripts/get_maintainer.pl *patch
>>     scripts/get_maintainer.pl: file '0000-cover-letter.patch' doesn't appear to be a patch.  Add -f to options?
>>     Paolo Bonzini <pbonzini@redhat.com> (maintainer:Overall)
>>     Peter Crosthwaite <crosthwaite.peter@gmail.com> (maintainer:Overall)
>>     Richard Henderson <rth@twiddle.net> (maintainer:Overall)
>>     "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Human Monitor (HMP))
>>     Eric Blake <eblake@redhat.com> (supporter:QAPI Schema)
>>     Markus Armbruster <armbru@redhat.com> (supporter:QAPI Schema)
>>     qemu-devel@nongnu.org (open list:Overall)
>>
>> It's output is advice, not gospel.  Use your judgement.
>>
>> I'm copying maintainers for you now.  This should get you some review.
>
> Hello,
>
> Thank you for your help. I missed that part when looking through
> the patch submission instructions.

Don't worry, we don't expect people to get it 100% right every time,
especially not the first time.

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

[Simon Ruderich]

Hello,

Revised patch series rebased on current master which should
incorporate all comments mentioned in the review. I've decided to
make size and offset optional for QMP, but keep it required for
HMP to make pmemload consistent with memsave/pmemsave.

Regards
Simon

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          | 81 ++++++++++++++++++++++++++++++++++++++++---------
 hmp-commands.hx | 18 +++++++++--
 hmp.c           | 16 ++++++++--
 hmp.h           |  1 +
 qapi/misc.json  | 20 ++++++++++++
 5 files changed, 118 insertions(+), 18 deletions(-)

Interdiff:

diff --git a/cpus.c b/cpus.c
index 4141ef766c..d79bf8b485 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2369,8 +2369,10 @@ exit:
     qemu_close(fd);
 }
 
-void qmp_pmemload(int64_t addr, int64_t size, int64_t offset,
-                  const char *filename, Error **errp)
+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;
@@ -2382,13 +2384,21 @@ void qmp_pmemload(int64_t addr, int64_t size, int64_t offset,
         error_setg_file_open(errp, errno, filename);
         return;
     }
-    if (offset > 0) {
+    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;
+        }
+        size = s.st_size;
+    }
 
     while (size != 0) {
         l = sizeof(buf);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 5a43dae133..c39d745a22 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -818,7 +818,7 @@ ETEXI
 
     {
         .name       = "memsave",
-        .args_type  = "val:l,size:i,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,
@@ -832,7 +832,7 @@ ETEXI
 
     {
         .name       = "pmemsave",
-        .args_type  = "val:l,size:i,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,
@@ -846,7 +846,7 @@ ETEXI
 
     {
         .name       = "pmemload",
-        .args_type  = "val:l,size:i,offset:i,filename:s",
+        .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,
diff --git a/hmp.c b/hmp.c
index 73de92e629..293e067ed5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1128,7 +1128,7 @@ void hmp_pmemload(Monitor *mon, const QDict *qdict)
     uint64_t addr = qdict_get_int(qdict, "val");
     Error *err = NULL;
 
-    qmp_pmemload(addr, size, offset, filename, &err);
+    qmp_pmemload(addr, filename, true, size, true, offset, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi/misc.json b/qapi/misc.json
index 6c34b2ff8b..06cf36f3d4 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1188,18 +1188,18 @@
 #
 # @val: the physical address of the guest to start from
 #
-# @size: the size of memory region to load
-#
-# @offset: the offset in the file 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: 2.13
+# Since: 3.1
 ##
 { 'command': 'pmemload',
-  'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }
+  'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }
 
 ##
 # @cont:

-- 
2.17.1

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

[Simon Ruderich]

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 b5844b7103..125adc4da4 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2318,8 +2318,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);
@@ -2352,8 +2353,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.17.1

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

[Eric Blake]

On 08/16/2018 04:01 AM, Simon Ruderich wrote:
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>   cpus.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v4 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>
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 125adc4da4..dd8ba36d2f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2293,7 +2293,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];
@@ -2310,8 +2310,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;
     }
@@ -2326,7 +2326,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;
         }
@@ -2335,18 +2335,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;
     }
@@ -2357,7 +2357,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;
         }
@@ -2366,7 +2366,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.17.1

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

[Eric Blake]

On 08/16/2018 04:01 AM, Simon Ruderich wrote:
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>   cpus.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v4 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).

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 dd8ba36d2f..243f2c0d2e 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2294,7 +2294,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;
@@ -2342,7 +2342,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.17.1

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

[Eric Blake]

On 08/16/2018 04:01 AM, Simon Ruderich wrote:
> It's the natural type for object sizes and matches the return value of
> sizeof(buf).
> 
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>   cpus.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v4 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>
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 91dfe51c37..79c158a140 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -818,7 +818,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,
@@ -832,7 +832,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 2aafb50e8e..1fb0724107 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1094,7 +1094,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;
@@ -1111,7 +1111,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.17.1

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

[Dr. David Alan Gilbert]

* Simon Ruderich (simon@ruderich.org) wrote:
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  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 91dfe51c37..79c158a140 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -818,7 +818,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,
> @@ -832,7 +832,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 2aafb50e8e..1fb0724107 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1094,7 +1094,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;
> @@ -1111,7 +1111,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.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v4 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>
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 79c158a140..5715bd7b51 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -818,7 +818,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,
@@ -832,7 +832,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.17.1

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

[Dr. David Alan Gilbert]

* Simon Ruderich (simon@ruderich.org) wrote:
> This enables completion for the filename arguments.
> 
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp-commands.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 79c158a140..5715bd7b51 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -818,7 +818,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,
> @@ -832,7 +832,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.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v4 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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c         | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/misc.json | 20 ++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/cpus.c b/cpus.c
index 243f2c0d2e..d79bf8b485 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2369,6 +2369,57 @@ 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;
+        }
+        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 d450cfef21..06cf36f3d4 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1181,6 +1181,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: 3.1
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }
+
 ##
 # @cont:
 #
-- 
2.17.1

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

[Eric Blake]

On 08/16/2018 04:01 AM, Simon Ruderich wrote:
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---

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

This works for regular files, but not for block devices.

Do we have a helper function for easily determining the size of an 
arbitrary fd (whether file or block device)?  If not, should we?  As 
there are multiple spots in the code that grab this sort of information.

Otherwise looks okay to me.

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

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

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

On Thu, Aug 16, 2018 at 03:01:31PM -0500, Eric Blake wrote:
>> +    }
>> +    if (!has_size) {
>> +        struct stat s;
>> +        if (fstat(fd, &s)) {
>> +            error_setg_errno(errp, errno, "could not fstat fd to get size");
>> +            goto exit;
>> +        }
>> +        size = s.st_size;
>> +    }
>
> This works for regular files, but not for block devices.
>
> Do we have a helper function for easily determining the size of an arbitrary
> fd (whether file or block device)?  If not, should we?  As there are
> multiple spots in the code that grab this sort of information.

I found raw_getlength() in block/file-posix.c, but its static and
it takes a BlockDriverState* as argument. For most systems it
could be extracted without too much trouble, but some (e.g
__sun__ and BSD) are quite entangled with BlockDriverState code.

> Otherwise looks okay to me.

Should I check for S_ISCHR() and S_ISBLK() and abort with an
error? I don't think it's a common use case for pmemload anyway.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH v5 5/6] 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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---

Hello,

I've adapted the patch to error out if a char/block device is
used. I think that's the simplest fix for the issue mentioned by
Eric Blake.

Are the any other issues remaining? All other patches are
unchanged, should I resend the whole series?

Regards
Simon

Diff of this patch since v4:

    diff --git a/cpus.c b/cpus.c
    index d79bf8b485..1622f00846 100644
    --- a/cpus.c
    +++ b/cpus.c
    @@ -2397,6 +2397,10 @@ void qmp_pmemload(int64_t addr, const char *filename,
                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;
        }

 cpus.c         | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/misc.json | 20 ++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/cpus.c b/cpus.c
index 243f2c0d2e..1622f00846 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2369,6 +2369,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 d450cfef21..06cf36f3d4 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1181,6 +1181,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: 3.1
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }
+
 ##
 # @cont:
 #
-- 
2.17.1

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [Qemu-devel] [PATCH v5 5/6] qmp: add pmemload command

[Simon Ruderich]

On Fri, Sep 21, 2018 at 01:02:05PM +0200, Simon Ruderich wrote:
> On Tue, Aug 21, 2018 at 02:38:02PM +0200, Simon Ruderich wrote:
>> Hello,
>>
>> I've adapted the patch to error out if a char/block device is
>> used. I think that's the simplest fix for the issue mentioned by
>> Eric Blake.
>>
>> Are the any other issues remaining? All other patches are
>> unchanged, should I resend the whole series?
>
> Hello,
>
> Any news? Anything I can do to get the patches merged?

Hello again,

Any news? I'd really like to get this patchset merged.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[Qemu-devel] [PATCH v4 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>
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 5715bd7b51..c39d745a22 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -842,6 +842,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 1fb0724107..293e067ed5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1120,6 +1120,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 33354f1bdd..f2b2a72a29 100644
--- a/hmp.h
+++ b/hmp.h
@@ -48,6 +48,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.17.1

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

[Dr. David Alan Gilbert]

* Simon Ruderich (simon@ruderich.org) wrote:
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  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 5715bd7b51..c39d745a22 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -842,6 +842,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 1fb0724107..293e067ed5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1120,6 +1120,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 33354f1bdd..f2b2a72a29 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -48,6 +48,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.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v6 0/6] qmp: add pmemload command

[Simon Ruderich]

Hello,

As I got no replies to my last mails, here again the full patch
set (rebased on current master) in the hope to get this merged.
The first few patches are cleanup, the last two patches add the
pmemload feature. Only 5/6 requires an ack (although all
mentioned issues should be fixed), all other patches were already
reviewed in the last round.

If there's anything else I can do to get this merged, please tell
me.

Regards
Simon Ruderich

Simon Ruderich (6):
  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          | 79 +++++++++++++++++++++++++++++++++++++++++--------
 hmp-commands.hx | 18 +++++++++--
 hmp.c           | 16 ++++++++--
 hmp.h           |  1 +
 qapi/misc.json  | 20 +++++++++++++
 5 files changed, 118 insertions(+), 16 deletions(-)

Diff between v5 and v6:

    --- a/qapi/misc.json
    +++ b/qapi/misc.json
    @@ -1201,7 +1201,7 @@
    #
    # Returns: Nothing on success
    #
    -# Since: 3.1
    +# Since: 3.2
    ##
    { 'command': 'pmemload',
    'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }

Diff between v4 (last full series) and v6:

    --- a/cpus.c
    +++ b/cpus.c
    @@ -2473,6 +2473,10 @@ void qmp_pmemload(int64_t addr, const char *filename,
                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;
        }

    --- a/qapi/misc.json
    +++ b/qapi/misc.json
    @@ -1201,7 +1201,7 @@
    #
    # Returns: Nothing on success
    #
    -# Since: 3.1
    +# Since: 3.2
    ##
    { 'command': 'pmemload',
    'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }

-- 
2.19.1

[Qemu-devel] [PATCH v6 1/6] 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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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 v6 2/6] 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).

Signed-off-by: Simon Ruderich <simon@ruderich.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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 v6 3/6] 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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 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 v6 4/6] 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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 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 v6 5/6] 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>
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..39f5e7dd38 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: 3.2
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }
+
 ##
 # @cont:
 #
-- 
2.19.1

[Qemu-devel] [PATCH v6 6/6] 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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 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

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

[Simon Ruderich]

Hello again,

Please ignore v6, I forgot one patch. I hope I got it right this
time.

As I got no replies to my last mails, here again the full patch
set (rebased on current master) in the hope to get this merged.
The first few patches are cleanup, the last two patches add the
pmemload feature. Only 5/6 requires an ack (although all
mentioned issues should be fixed), all other patches were already
reviewed in the last round.

If there's anything else I can do to get this merged, please tell
me.

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          | 81 ++++++++++++++++++++++++++++++++++++++++---------
 hmp-commands.hx | 18 +++++++++--
 hmp.c           | 16 ++++++++--
 hmp.h           |  1 +
 qapi/misc.json  | 20 ++++++++++++
 5 files changed, 118 insertions(+), 18 deletions(-)

No changes between v6 and v7, but v6 missed one patch.

Diff between v5 and v7:

    --- a/qapi/misc.json
    +++ b/qapi/misc.json
    @@ -1201,7 +1201,7 @@
    #
    # Returns: Nothing on success
    #
    -# Since: 3.1
    +# Since: 3.2
    ##
    { 'command': 'pmemload',
    'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }


Diff between v4 (last full series) and v7:

    --- a/cpus.c
    +++ b/cpus.c
    @@ -2473,6 +2473,10 @@ void qmp_pmemload(int64_t addr, const char *filename,
                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;
        }

    --- a/qapi/misc.json
    +++ b/qapi/misc.json
    @@ -1201,7 +1201,7 @@
    #
    # Returns: Nothing on success
    #
    -# Since: 3.1
    +# Since: 3.2
    ##
    { 'command': 'pmemload',
    'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }

-- 
2.19.1

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

[Simon Ruderich]

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

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

[Eric Blake]

On 11/15/18 7:22 AM, Simon Ruderich wrote:
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>   cpus.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v7 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>
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

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

[Eric Blake]

On 11/15/18 7:22 AM, Simon Ruderich wrote:
> 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>
> Signed-off-by: Simon Ruderich <simon@ruderich.org>
> ---
>   cpus.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v7 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).

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 v7 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>
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 v7 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>
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 v7 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>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
 cpus.c         | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/misc.json | 20 ++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/cpus.c b/cpus.c
index ee54595733..a12076dc68 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2445,6 +2445,57 @@ 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;
+        }
+        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..6eb9d29339 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: 3.1
+##
+{ 'command': 'pmemload',
+  'data': {'val': 'int', 'filename': 'str', '*size': 'int', '*offset': 'int'} }
+
 ##
 # @cont:
 #
-- 
2.19.1

[Qemu-devel] [PATCH v7 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>
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 v7 0/7] qmp: add pmemload command

[Eric Blake]

On 11/15/18 7:22 AM, Simon Ruderich wrote:
> Hello again,
> 
> Please ignore v6, I forgot one patch. I hope I got it right this
> time.

Our automated tooling doesn't spot patches sent in reply to an earlier 
series; you may want to repost v7 as a new top-level thread (not 
in-reply-to any earlier message) to makes sure it gets seen by patchew.

> 
> As I got no replies to my last mails, here again the full patch
> set (rebased on current master) in the hope to get this merged.
> The first few patches are cleanup, the last two patches add the
> pmemload feature. Only 5/6 requires an ack (although all
> mentioned issues should be fixed), all other patches were already
> reviewed in the last round.
> 
> If there's anything else I can do to get this merged, please tell
> me.
> 
> 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          | 81 ++++++++++++++++++++++++++++++++++++++++---------
>   hmp-commands.hx | 18 +++++++++--
>   hmp.c           | 16 ++++++++--
>   hmp.h           |  1 +
>   qapi/misc.json  | 20 ++++++++++++
>   5 files changed, 118 insertions(+), 18 deletions(-)
> 
> No changes between v6 and v7, but v6 missed one patch.
> 
> Diff between v5 and v7:
> 
>      --- a/qapi/misc.json
>      +++ b/qapi/misc.json
>      @@ -1201,7 +1201,7 @@
>      #
>      # Returns: Nothing on success
>      #
>      -# Since: 3.1
>      +# Since: 3.2

Except the next release will be named 4.0, not 3.2 :)  (It's okay, 
there's a lot of mentions of 3.2 floating on the list right now, and 
we'll probably have to do a global search-and-replace at some point once 
we are into the new year)

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

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

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Thu, Nov 15, 2018 at 07:45:29AM -0600, Eric Blake wrote:
> On 11/15/18 7:22 AM, Simon Ruderich wrote:
>> Hello again,
>>
>> Please ignore v6, I forgot one patch. I hope I got it right this
>> time.
>
> Our automated tooling doesn't spot patches sent in reply to an earlier
> series; you may want to repost v7 as a new top-level thread (not in-reply-to
> any earlier message) to makes sure it gets seen by patchew.

Hello,

Thanks for the quick reply and review.

I didn't know that patchew requires new threads. I've resent it
as v8 as new top-level thread.

>>      --- a/qapi/misc.json
>>      +++ b/qapi/misc.json
>>      @@ -1201,7 +1201,7 @@
>>      #
>>      # Returns: Nothing on success
>>      #
>>      -# Since: 3.1
>>      +# Since: 3.2
>
> Except the next release will be named 4.0, not 3.2 :)  (It's okay, there's a
> lot of mentions of 3.2 floating on the list right now, and we'll probably
> have to do a global search-and-replace at some point once we are into the
> new year)

I've fixed the version in v8.

Regards
Simon Ruderich
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]