[v3] qmp: Use unsigned integers for address parameters

[PATCH v3] qmp: Use unsigned integers for address parameters

Posted by Josh Junon 3 months, 3 weeks ago

Fixes higher-half address parsing for QMP commands
`[p]memsave`.

Signed-off-by: Josh Junon <junon@oro.sh>
---
 qapi/machine.json | 11 +++++++++--
 system/cpus.c     | 10 +++++-----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index fcfd249e2d..fb618dc99f 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -852,7 +852,11 @@
 #     <- { "return": {} }
 ##
 { 'command': 'memsave',
-  'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 'int'} }
+  'data': {
+     'val': 'uint64',
+     'size': 'size',
+     'filename': 'str',
+     '*cpu-index': 'int' } }
 
 ##
 # @pmemsave:
@@ -878,7 +882,10 @@
 #     <- { "return": {} }
 ##
 { 'command': 'pmemsave',
-  'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
+  'data': {
+    'val': 'uint64',
+    'size': 'size',
+    'filename': 'str' } }
 
 ##
 # @Memdev:
diff --git a/system/cpus.c b/system/cpus.c
index 5e3a988a0a..f33e4fbff0 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -792,14 +792,14 @@ int vm_stop_force_state(RunState state)
     }
 }
 
-void qmp_memsave(int64_t addr, int64_t size, const char *filename,
+void qmp_memsave(uint64_t addr, uint64_t size, const char *filename,
                  bool has_cpu, int64_t cpu_index, Error **errp)
 {
     FILE *f;
-    uint32_t l;
+    uint64_t l;
     CPUState *cpu;
     uint8_t buf[1024];
-    int64_t orig_addr = addr, orig_size = size;
+    uint64_t orig_addr = addr, orig_size = size;
 
     if (!has_cpu) {
         cpu_index = 0;
@@ -840,11 +840,11 @@ exit:
     fclose(f);
 }
 
-void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
+void qmp_pmemsave(uint64_t addr, uint64_t size, const char *filename,
                   Error **errp)
 {
     FILE *f;
-    uint32_t l;
+    uint64_t l;
     uint8_t buf[1024];
 
     f = fopen(filename, "wb");
-- 
2.34.1

Re: [PATCH v3] qmp: Use unsigned integers for address parameters

Posted by Markus Armbruster 3 months, 3 weeks ago

Josh Junon <junon@oro.sh> writes:

> Fixes higher-half address parsing for QMP commands
> `[p]memsave`.
>
> Signed-off-by: Josh Junon <junon@oro.sh>

Queued for 9.1.  Thanks!

Re: [PATCH v3] qmp: Use unsigned integers for address parameters

Posted by Markus Armbruster 3 months, 3 weeks ago

Let's go back to v1's title

    qmp: Fix higher half vaddrs for [p]memsave

Josh Junon <junon@oro.sh> writes:

> Fixes higher-half address parsing for QMP commands
> `[p]memsave`.
>
> Signed-off-by: Josh Junon <junon@oro.sh>
> ---
>  qapi/machine.json | 11 +++++++++--
>  system/cpus.c     | 10 +++++-----
>  2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index fcfd249e2d..fb618dc99f 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -852,7 +852,11 @@
>  #     <- { "return": {} }
>  ##
>  { 'command': 'memsave',
> -  'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 'int'} }
> +  'data': {
> +     'val': 'uint64',
> +     'size': 'size',
> +     'filename': 'str',
> +     '*cpu-index': 'int' } }
>  
>  ##
>  # @pmemsave:
> @@ -878,7 +882,10 @@
>  #     <- { "return": {} }
>  ##
>  { 'command': 'pmemsave',
> -  'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
> +  'data': {
> +    'val': 'uint64',
> +    'size': 'size',
> +    'filename': 'str' } }
>  
>  ##
>  # @Memdev:
> diff --git a/system/cpus.c b/system/cpus.c
> index 5e3a988a0a..f33e4fbff0 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -792,14 +792,14 @@ int vm_stop_force_state(RunState state)
>      }
>  }
>  
> -void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> +void qmp_memsave(uint64_t addr, uint64_t size, const char *filename,
>                   bool has_cpu, int64_t cpu_index, Error **errp)
>  {
>      FILE *f;
> -    uint32_t l;
> +    uint64_t l;
>      CPUState *cpu;
>      uint8_t buf[1024];
> -    int64_t orig_addr = addr, orig_size = size;
> +    uint64_t orig_addr = addr, orig_size = size;
>  
>      if (!has_cpu) {
>          cpu_index = 0;
       }
   [...]
           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);

@size is now unsigned and should be formatted with PRIu64.

               goto exit;
           }
> @@ -840,11 +840,11 @@ exit:
>      fclose(f);
>  }
>  
> -void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
> +void qmp_pmemsave(uint64_t addr, uint64_t size, const char *filename,
>                    Error **errp)
>  {
>      FILE *f;
> -    uint32_t l;
> +    uint64_t l;
>      uint8_t buf[1024];
>  
>      f = fopen(filename, "wb");

I can address my nitpicks in my tree, no respin necessary.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v3] qmp: Use unsigned integers for address parameters

Posted by Josh Junon 3 months, 3 weeks ago

On Fri, Aug 02, 2024 at 04:18:40PM +0200, Markus Armbruster wrote:
> Let's go back to v1's title
> 
>     qmp: Fix higher half vaddrs for [p]memsave
> 
> Josh Junon <junon@oro.sh> writes:
> 
> > Fixes higher-half address parsing for QMP commands
> > `[p]memsave`.
> >
> > Signed-off-by: Josh Junon <junon@oro.sh>
> > ---
> >  qapi/machine.json | 11 +++++++++--
> >  system/cpus.c     | 10 +++++-----
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index fcfd249e2d..fb618dc99f 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -852,7 +852,11 @@
> >  #     <- { "return": {} }
> >  ##
> >  { 'command': 'memsave',
> > -  'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 'int'} }
> > +  'data': {
> > +     'val': 'uint64',
> > +     'size': 'size',
> > +     'filename': 'str',
> > +     '*cpu-index': 'int' } }
> >  
> >  ##
> >  # @pmemsave:
> > @@ -878,7 +882,10 @@
> >  #     <- { "return": {} }
> >  ##
> >  { 'command': 'pmemsave',
> > -  'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
> > +  'data': {
> > +    'val': 'uint64',
> > +    'size': 'size',
> > +    'filename': 'str' } }
> >  
> >  ##
> >  # @Memdev:
> > diff --git a/system/cpus.c b/system/cpus.c
> > index 5e3a988a0a..f33e4fbff0 100644
> > --- a/system/cpus.c
> > +++ b/system/cpus.c
> > @@ -792,14 +792,14 @@ int vm_stop_force_state(RunState state)
> >      }
> >  }
> >  
> > -void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> > +void qmp_memsave(uint64_t addr, uint64_t size, const char *filename,
> >                   bool has_cpu, int64_t cpu_index, Error **errp)
> >  {
> >      FILE *f;
> > -    uint32_t l;
> > +    uint64_t l;
> >      CPUState *cpu;
> >      uint8_t buf[1024];
> > -    int64_t orig_addr = addr, orig_size = size;
> > +    uint64_t orig_addr = addr, orig_size = size;
> >  
> >      if (!has_cpu) {
> >          cpu_index = 0;
>        }
>    [...]
>            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);
> 
> @size is now unsigned and should be formatted with PRIu64.

Ah yes, missed that one last round. Apologies!

> 
>                goto exit;
>            }
> > @@ -840,11 +840,11 @@ exit:
> >      fclose(f);
> >  }
> >  
> > -void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
> > +void qmp_pmemsave(uint64_t addr, uint64_t size, const char *filename,
> >                    Error **errp)
> >  {
> >      FILE *f;
> > -    uint32_t l;
> > +    uint64_t l;
> >      uint8_t buf[1024];
> >  
> >      f = fopen(filename, "wb");
> 
> I can address my nitpicks in my tree, no respin necessary.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Sounds good to me, thank you for the reviews!

- Josh