[Qemu-devel] [PATCH] qapi: misc: change the 'pc' to unsinged 64 in CpuInfo

[Qemu-devel] [PATCH] qapi: misc: change the 'pc' to unsinged 64 in CpuInfo

[Li Qiang]

When trigger a 'query-cpus' qmp, the pc is an signed value like
following:
{"arch": "x86", ...  "pc": -1732653994, "halted": true,...}
It is strange. Change it to uint64_t.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 qapi/misc.json | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 6c1c5c0a37..621ec6ce13 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -407,7 +407,7 @@
 #
 # Since: 2.6
 ##
-{ 'struct': 'CpuInfoX86', 'data': { 'pc': 'int' } }
+{ 'struct': 'CpuInfoX86', 'data': { 'pc': 'uint64' } }
 
 ##
 # @CpuInfoSPARC:
@@ -420,7 +420,7 @@
 #
 # Since: 2.6
 ##
-{ 'struct': 'CpuInfoSPARC', 'data': { 'pc': 'int', 'npc': 'int' } }
+{ 'struct': 'CpuInfoSPARC', 'data': { 'pc': 'uint64', 'npc': 'uint64' } }
 
 ##
 # @CpuInfoPPC:
@@ -431,7 +431,7 @@
 #
 # Since: 2.6
 ##
-{ 'struct': 'CpuInfoPPC', 'data': { 'nip': 'int' } }
+{ 'struct': 'CpuInfoPPC', 'data': { 'nip': 'uint64' } }
 
 ##
 # @CpuInfoMIPS:
@@ -442,7 +442,7 @@
 #
 # Since: 2.6
 ##
-{ 'struct': 'CpuInfoMIPS', 'data': { 'PC': 'int' } }
+{ 'struct': 'CpuInfoMIPS', 'data': { 'PC': 'uint64' } }
 
 ##
 # @CpuInfoTricore:
@@ -453,7 +453,7 @@
 #
 # Since: 2.6
 ##
-{ 'struct': 'CpuInfoTricore', 'data': { 'PC': 'int' } }
+{ 'struct': 'CpuInfoTricore', 'data': { 'PC': 'uint64' } }
 
 ##
 # @CpuInfoRISCV:
@@ -464,7 +464,7 @@
 #
 # Since 2.12
 ##
-{ 'struct': 'CpuInfoRISCV', 'data': { 'pc': 'int' } }
+{ 'struct': 'CpuInfoRISCV', 'data': { 'pc': 'uint64' } }
 
 ##
 # @CpuS390State:
-- 
2.11.0

Re: [Qemu-devel] [PATCH] qapi: misc: change the 'pc' to unsinged 64 in CpuInfo

[Eric Blake]

On 11/2/18 6:01 AM, Li Qiang wrote:
> When trigger a 'query-cpus' qmp, the pc is an signed value like
> following:
> {"arch": "x86", ...  "pc": -1732653994, "halted": true,...}
> It is strange. Change it to uint64_t.
> 
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
>   qapi/misc.json | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

I don't see this as causing any major backwards-incompatible behavior to 
clients that can parse full 64-bit unsigned numbers (note that not all 
JSON parsers do so - here's frowning at you, jansson - but libvirt is okay).

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

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

Re: [Qemu-devel] [PATCH] qapi: misc: change the 'pc' to unsinged 64 in CpuInfo

[Peter Maydell]

On 2 November 2018 at 11:01, Li Qiang <liq3ea@gmail.com> wrote:
> When trigger a 'query-cpus' qmp, the pc is an signed value like
> following:
> {"arch": "x86", ...  "pc": -1732653994, "halted": true,...}
> It is strange. Change it to uint64_t.
>
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---

NB: typo in subject line: should be "unsigned".

thanks
-- PMM

[Qemu-devel] QAPI 'size' vs. 'uint64' (was: [PATCH] qapi: misc: change the 'pc' to unsinged 64 in CpuInfo)

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/2/18 6:01 AM, Li Qiang wrote:
>> When trigger a 'query-cpus' qmp, the pc is an signed value like
>> following:
>> {"arch": "x86", ...  "pc": -1732653994, "halted": true,...}
>> It is strange. Change it to uint64_t.
>>
>> Signed-off-by: Li Qiang <liq3ea@gmail.com>
>> ---
>>   qapi/misc.json | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> I don't see this as causing any major backwards-incompatible behavior
> to clients that can parse full 64-bit unsigned numbers (note that not
> all JSON parsers do so - here's frowning at you, jansson - but libvirt
> is okay).
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Client breakage is possible, but feels unlikely.  To break, a client has
to (a) accept negative numbers modulo 2^64, and (b) don't accept numbers
>= 2^63.  That's extra work.  Clients that simply use strtoul() are
unaffected.  We accepted this very risk when we fixed the QObject
visitor for unsigned integers in commit 5923f85fb82.  Relevant part of
the commit message:

    Note: before the patch, uint64_t values above INT64_MAX are sent over
    json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the
    patch, they are sent unmodified.  Clearly a bug fix, but we have to
    consider compatibility issues anyway.  libvirt should cope fine,
    because its parsing of unsigned integers accepts negative values
    modulo 2^64.  There's hope that other clients will, too.

This patch needs to discuss the risk, too.  Suggest to steal from the
commit message I quoted.


There's another question: 'uint64' or 'size'?

Short answer: I think we better stick to 'uint64' here.  Long answer
follows.

Both QAPI types map to uint64_t in C.  The difference is twofold:

* It serves as a documentation of intent in the QAPI schema.  Or rather
  it would, if we used it more consistently.

  Related:
  [RFC PATCH 00/56] qapi: Use 'size' for byte counts & offsets
  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01089.html
  Message-Id: <1502117160-24655-1-git-send-email-armbru@redhat.com>

  From its cover letter:

      Byte sizes, offsets and the like should use QAPI type 'size'
      (uint64_t).  This rule is more honored in the breach than in the
      observance.  Fix the obvious offenders.

  The series doesn't fix the program counters in the CpuInfoFOO.

* A few visitors treat 'size' differently:

  - The opts visitor parses with qemu_strtosz() instead of parse_uint(),
    and it doesn't support ranges.  Let's ignore ranges.  qemu_strtosz()
    accepts convenience syntax like 1.5M for 1.5 * 1024 * 1024.

    This visitor is used for parsing CLI options -numa, -netdev,
    -object, -acpitable, HMP netdev_add, object_add, and the -object
    equivalents of qemu-img, qemu-io, qemu-nbd.
    
  - The QObject input visitor's keyval flavour parses with
    qemu_strtosz() instead of qemu_strtou64().

    This visitor is used for parsing CLI options -display (except for
    its legacy forms), -blockdev, and by block layer in ways that are
    too complex to explain here.

  - The string input visitor[*] parses with qemu_strtosz() (via
    parse_option_size()) instead of qemu_strtou64(), and it doesn't
    support ranges.

    This visitor is used for parsing HMP migrate_set_parameter and QOM
    property values.  It's also used for getting QOM property values
    (don't ask, it's terrible).

  - The string output visitor uses format PRIu64 instead of PRId64 and
    optionally PRIx64 as well, and it doesn't support ranges.  This
    visitor is used for HMP info migrate, info memdev, info network,
    info qtree.  It's also used for getting QOM property values
    (terrible).

  Yes, this stuff could have used adult design supervision.

  As far as I can tell, none of the 'size' differences is relevant for
  this patch.

So, on the one hand, program counters are byte counts, so 'size' feels
more appropriate.  On the other hand, the important part is changing to
unsigned, and 'uint64' gets us there without the additional baggage of
'size'.

The additional baggage looks irrelevant for this patch.  But perhaps we
should make up our mind on usage of 'size' in general.

The true reason it exists is the desire for convenience syntax at the
human interface.  Fair enough, but it doesn't scale: we could use
convenience syntax in many other places, but creating a special type for
each kind is an awful way to get it.  If we had a better, more general
way to specify "use this convenience syntax for human interfaces" in the
schema, we could get rid of size.


[*] With David Hildenbrand's patches applied.