[PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Daniel P. Berrangé]

We are still adding HMP commands without any QMP counterparts. This is
done because there are a reasonable number of scenarios where the cost
of designing a QAPI data type for the command is not justified.

This has the downside, however, that we will never be able to fully
isolate the monitor code from the remainder of QEMU internals. It is
desirable to be able to get to a point where subsystems in QEMU are
exclusively implemented using QAPI types and never need to have any
knowledge of the monitor APIs.

The way to get there is to stop adding commands to HMP only. All
commands must be implemented using QMP, and any HMP implementation
be a shim around the QMP implementation.

We don't want to compromise our supportability of QMP long term though.

This series proposes that we relax our requirements around fine grained
QAPI data design, but with the caveat that any command taking this
design approach is mandated to use the 'x-' name prefix.

This tradeoff should be suitable for any commands we have been adding
exclusively to HMP in recent times, and thus mean we have mandate QMP
support for all new commands going forward.

This series illustrates the concept by converting the "info registers"
HMP to invoke a new 'x-query-registers' QMP command. Note that only
the i386 CPU target is converted to work with this new approach, so
this series needs to be considered incomplete. If we go forward with
this idea, then a subsequent version of this series would need to
obviously convert all other CPU targets.

After doing that conversion the only use of qemu_fprintf() would be
the disas.c file. Remaining uses of qemu_fprintf and qemu_printf
could be tackled in a similar way and eventually eliminate the need
for any of these printf wrappers in QEMU.

NB: I added docs to devel/writing-qmp-commands.rst about the two
design approaches to QMP. I didn't see another good place to put
an explicit note that we will not add any more HMP-only commands.
Obviously HMP/QMP maintainers control this in their reviews of
patches, and maybe that's sufficient ?

NB: if we take this approach we'll want to figure out how many
HMP-only commands we actually have left and then perhaps have
a task to track their conversion to QMP. This could possibly
be a useful task for newbies if we make it clear that they
wouldn't be required to undertake complex QAPI modelling in
doing this conversion.

Daniel P. Berrangé (5):
  docs/devel: document expectations for QAPI data modelling for QMP
  hw/core: introduce 'format_state' callback to replace 'dump_state'
  target/i386: convert to use format_state instead of dump_state
  qapi: introduce x-query-registers QMP command
  monitor: rewrite 'info registers' in terms of 'x-query-registers'

 docs/devel/writing-qmp-commands.rst |  25 +++
 hw/core/cpu-common.c                |  15 ++
 hw/core/machine-qmp-cmds.c          |  28 +++
 include/hw/core/cpu.h               |  13 +-
 monitor/misc.c                      |  25 ++-
 qapi/machine.json                   |  37 ++++
 target/i386/cpu-dump.c              | 325 +++++++++++++++-------------
 target/i386/cpu.c                   |   2 +-
 target/i386/cpu.h                   |   2 +-
 9 files changed, 307 insertions(+), 165 deletions(-)

-- 
2.31.1

[PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

Traditionally we have required that newly added QMP commands will model
any returned data using fine grained QAPI types. This is good for
commands that are intended to be consumed by machines, where clear data
representation is very important. Commands that don't satisfy this have
generally been added to HMP only.

In effect the decision of whether to add a new command to QMP vs HMP has
been used as a proxy for the decision of whether the cost of designing a
fine grained QAPI type is justified by the potential benefits.

As a result the commands present in QMP and HMP are non-overlapping
sets, although HMP comamnds can be accessed indirectly via the QMP
command 'human-monitor-command'.

One of the downsides of 'human-monitor-command' is that the QEMU monitor
APIs remain tied into various internal parts of the QEMU code. For
example any exclusively HMP command will need to use 'monitor_printf'
to get data out. It would be desirable to be able to fully isolate the
monitor implementation from QEMU internals, however, this is only
possible if all commands are exclusively based on QAPI with direct
QMP exposure.

The way to achieve this desired end goal is to finese the requirements
for QMP command design. For cases where the output of a command is only
intended for human consumption, it is reasonable to want to simplify
the implementation by returning a plain string containing formatted
data instead of designing a fine grained QAPI data type. This can be
permitted if-and-only-if the command is exposed under the 'x-' name
prefix. This indicates that the command data format is liable to
future change and that it is not following QAPI design best practice.

The poster child example for this would be the 'info registers' HMP
command which returns printf formatted data representing CPU state.
This information varies enourmously across target architectures and
changes relatively frequently as new CPU features are implemented.
It is there as debugging data for human operators, and any machine
usage would treat it as an opaque blob. It is thus reasonable to
expose this in QMP as 'x-query-registers' returning a 'str' field.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/docs/devel/writing-qmp-commands.rst b/docs/devel/writing-qmp-commands.rst
index 6a10a06c48..d032daa62d 100644
--- a/docs/devel/writing-qmp-commands.rst
+++ b/docs/devel/writing-qmp-commands.rst
@@ -350,6 +350,31 @@ In this section we will focus on user defined types. Please, check the QAPI
 documentation for information about the other types.
 
 
+Modelling data in QAPI
+~~~~~~~~~~~~~~~~~~~~~~
+
+For a QMP command that to be considered stable and supported long term there
+is a requirement returned data should be explicitly modelled using fine grained
+QAPI types. As a general guide, a caller of the QMP command should never need
+to parse individual returned data fields. If a field appears to need parsing,
+them it should be split into separate fields corresponding to each distinct
+data item. This should be the common case for any new QMP command that is
+intended to be used by machines, as opposed to exclusively human operators.
+
+Some QMP commands, however, are only intended as adhoc debugging aids for human
+operators. While they may return large amounts of formatted data, it is not
+expected that machines will need to parse the result. The overhead of defining
+a fine grained QAPI type for the data may not be justified by the potential
+benefit. In such cases, it is permitted to have a command return a simple string
+that contains formatted data, however, it is mandatory for the command to use
+the 'x-' name prefix. This indicates that the command is not guaranteed to be
+long term stable / liable to change in future and is not following QAPI design
+best practices. An example where this approach is taken is the QMP command
+"x-query-registers". This returns a printf formatted dump of the architecture
+specific CPU state. The way the data is formatted varies across QEMU targets,
+is liable to change over time, and is only intended to be consumed as an opaque
+string by machines.
+
 User Defined Types
 ~~~~~~~~~~~~~~~~~~
 
-- 
2.31.1

[PATCH 2/5] hw/core: introduce 'format_state' callback to replace 'dump_state'

[Daniel P. Berrangé]

The 'dump_state' callback assumes it will be outputting to a FILE
object. This is fine for HMP, but not so useful for QMP. Introduce
a new 'format_state' callback that returns a formatted GString
instead.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/cpu-common.c  | 15 +++++++++++++++
 include/hw/core/cpu.h | 13 ++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index e2f5a64604..c2cd33a817 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -106,6 +106,21 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
     if (cc->dump_state) {
         cpu_synchronize_state(cpu);
         cc->dump_state(cpu, f, flags);
+    } else if (cc->format_state) {
+        g_autoptr(GString) buf = g_string_new("");
+        cpu_synchronize_state(cpu);
+        cc->format_state(cpu, buf, flags);
+        qemu_fprintf(f, "%s", buf->str);
+    }
+}
+
+void cpu_format_state(CPUState *cpu, GString *buf, int flags)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->format_state) {
+        cpu_synchronize_state(cpu);
+        cc->format_state(cpu, buf, flags);
     }
 }
 
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index bc864564ce..1599ef9df3 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -91,7 +91,8 @@ struct SysemuCPUOps;
  * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
  * @has_work: Callback for checking if there is work to do.
  * @memory_rw_debug: Callback for GDB memory access.
- * @dump_state: Callback for dumping state.
+ * @dump_state: Callback for dumping state. Deprecated, use @format_state.
+ * @format_state: Callback for formatting state.
  * @get_arch_id: Callback for getting architecture-dependent CPU ID.
  * @set_pc: Callback for setting the Program Counter register. This
  *       should have the semantics used by the target architecture when
@@ -136,6 +137,7 @@ struct CPUClass {
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
     void (*dump_state)(CPUState *cpu, FILE *, int flags);
+    void (*format_state)(CPUState *cpu, GString *buf, int flags);
     int64_t (*get_arch_id)(CPUState *cpu);
     void (*set_pc)(CPUState *cpu, vaddr value);
     int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int reg);
@@ -537,6 +539,15 @@ enum CPUDumpFlags {
  */
 void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 
+/**
+ * cpu_format_state:
+ * @cpu: The CPU whose state is to be formatted.
+ * @buf: buffer to format state into
+ *
+ * Formats the CPU state.
+ */
+void cpu_format_state(CPUState *cpu, GString *buf, int flags);
+
 #ifndef CONFIG_USER_ONLY
 /**
  * cpu_get_phys_page_attrs_debug:
-- 
2.31.1

[PATCH 3/5] target/i386: convert to use format_state instead of dump_state

[Daniel P. Berrangé]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 target/i386/cpu-dump.c | 325 ++++++++++++++++++++++-------------------
 target/i386/cpu.c      |   2 +-
 target/i386/cpu.h      |   2 +-
 3 files changed, 174 insertions(+), 155 deletions(-)

diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
index 02b635a52c..8e19485a20 100644
--- a/target/i386/cpu-dump.c
+++ b/target/i386/cpu-dump.c
@@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = {
 };
 
 static void
-cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f,
+cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf,
                        const char *name, struct SegmentCache *sc)
 {
 #ifdef TARGET_X86_64
     if (env->hflags & HF_CS64_MASK) {
-        qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
-                     sc->selector, sc->base, sc->limit,
-                     sc->flags & 0x00ffff00);
+        g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
+                               sc->selector, sc->base, sc->limit,
+                               sc->flags & 0x00ffff00);
     } else
 #endif
     {
-        qemu_fprintf(f, "%-3s=%04x %08x %08x %08x", name, sc->selector,
-                     (uint32_t)sc->base, sc->limit,
-                     sc->flags & 0x00ffff00);
+        g_string_append_printf(buf, "%-3s=%04x %08x %08x %08x",
+                               name, sc->selector,
+                               (uint32_t)sc->base, sc->limit,
+                               sc->flags & 0x00ffff00);
     }
 
     if (!(env->hflags & HF_PE_MASK) || !(sc->flags & DESC_P_MASK))
         goto done;
 
-    qemu_fprintf(f, " DPL=%d ",
+    g_string_append_printf(buf, " DPL=%d ",
                  (sc->flags & DESC_DPL_MASK) >> DESC_DPL_SHIFT);
     if (sc->flags & DESC_S_MASK) {
         if (sc->flags & DESC_CS_MASK) {
-            qemu_fprintf(f, (sc->flags & DESC_L_MASK) ? "CS64" :
-                         ((sc->flags & DESC_B_MASK) ? "CS32" : "CS16"));
-            qemu_fprintf(f, " [%c%c", (sc->flags & DESC_C_MASK) ? 'C' : '-',
-                         (sc->flags & DESC_R_MASK) ? 'R' : '-');
+            g_string_append_printf(buf, (sc->flags & DESC_L_MASK) ? "CS64" :
+                                   ((sc->flags & DESC_B_MASK) ? "CS32" : "CS16"));
+            g_string_append_printf(buf, " [%c%c",
+                                   (sc->flags & DESC_C_MASK) ? 'C' : '-',
+                                   (sc->flags & DESC_R_MASK) ? 'R' : '-');
         } else {
-            qemu_fprintf(f, (sc->flags & DESC_B_MASK
+            g_string_append_printf(buf, (sc->flags & DESC_B_MASK
                              || env->hflags & HF_LMA_MASK)
                          ? "DS  " : "DS16");
-            qemu_fprintf(f, " [%c%c", (sc->flags & DESC_E_MASK) ? 'E' : '-',
-                         (sc->flags & DESC_W_MASK) ? 'W' : '-');
+            g_string_append_printf(buf, " [%c%c",
+                                   (sc->flags & DESC_E_MASK) ? 'E' : '-',
+                                   (sc->flags & DESC_W_MASK) ? 'W' : '-');
         }
-        qemu_fprintf(f, "%c]", (sc->flags & DESC_A_MASK) ? 'A' : '-');
+        g_string_append_printf(buf, "%c]",
+                               (sc->flags & DESC_A_MASK) ? 'A' : '-');
     } else {
         static const char *sys_type_name[2][16] = {
             { /* 32 bit mode */
@@ -144,12 +148,12 @@ cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f,
                 "Reserved", "IntGate64", "TrapGate64"
             }
         };
-        qemu_fprintf(f, "%s",
-                     sys_type_name[(env->hflags & HF_LMA_MASK) ? 1 : 0]
-                     [(sc->flags & DESC_TYPE_MASK) >> DESC_TYPE_SHIFT]);
+        g_string_append_printf(buf, "%s",
+                               sys_type_name[(env->hflags & HF_LMA_MASK) ? 1 : 0]
+                               [(sc->flags & DESC_TYPE_MASK) >> DESC_TYPE_SHIFT]);
     }
 done:
-    qemu_fprintf(f, "\n");
+    g_string_append_printf(buf, "\n");
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -344,7 +348,7 @@ void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
 #define DUMP_CODE_BYTES_TOTAL    50
 #define DUMP_CODE_BYTES_BACKWARD 20
 
-void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags)
+void x86_cpu_format_state(CPUState *cs, GString *buf, int flags)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
@@ -355,107 +359,116 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     eflags = cpu_compute_eflags(env);
 #ifdef TARGET_X86_64
     if (env->hflags & HF_CS64_MASK) {
-        qemu_fprintf(f, "RAX=%016" PRIx64 " RBX=%016" PRIx64 " RCX=%016" PRIx64 " RDX=%016" PRIx64 "\n"
-                     "RSI=%016" PRIx64 " RDI=%016" PRIx64 " RBP=%016" PRIx64 " RSP=%016" PRIx64 "\n"
-                     "R8 =%016" PRIx64 " R9 =%016" PRIx64 " R10=%016" PRIx64 " R11=%016" PRIx64 "\n"
-                     "R12=%016" PRIx64 " R13=%016" PRIx64 " R14=%016" PRIx64 " R15=%016" PRIx64 "\n"
-                     "RIP=%016" PRIx64 " RFL=%08x [%c%c%c%c%c%c%c] CPL=%d II=%d A20=%d SMM=%d HLT=%d\n",
-                     env->regs[R_EAX],
-                     env->regs[R_EBX],
-                     env->regs[R_ECX],
-                     env->regs[R_EDX],
-                     env->regs[R_ESI],
-                     env->regs[R_EDI],
-                     env->regs[R_EBP],
-                     env->regs[R_ESP],
-                     env->regs[8],
-                     env->regs[9],
-                     env->regs[10],
-                     env->regs[11],
-                     env->regs[12],
-                     env->regs[13],
-                     env->regs[14],
-                     env->regs[15],
-                     env->eip, eflags,
-                     eflags & DF_MASK ? 'D' : '-',
-                     eflags & CC_O ? 'O' : '-',
-                     eflags & CC_S ? 'S' : '-',
-                     eflags & CC_Z ? 'Z' : '-',
-                     eflags & CC_A ? 'A' : '-',
-                     eflags & CC_P ? 'P' : '-',
-                     eflags & CC_C ? 'C' : '-',
-                     env->hflags & HF_CPL_MASK,
-                     (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1,
-                     (env->a20_mask >> 20) & 1,
-                     (env->hflags >> HF_SMM_SHIFT) & 1,
-                     cs->halted);
+        g_string_append_printf(buf, "RAX=%016" PRIx64 " RBX=%016" PRIx64
+                               " RCX=%016" PRIx64 " RDX=%016" PRIx64 "\n"
+                               "RSI=%016" PRIx64 " RDI=%016" PRIx64
+                               " RBP=%016" PRIx64 " RSP=%016" PRIx64 "\n"
+                               "R8 =%016" PRIx64 " R9 =%016" PRIx64
+                               " R10=%016" PRIx64 " R11=%016" PRIx64 "\n"
+                               "R12=%016" PRIx64 " R13=%016" PRIx64
+                               " R14=%016" PRIx64 " R15=%016" PRIx64 "\n"
+                               "RIP=%016" PRIx64 " RFL=%08x [%c%c%c%c%c%c%c] "
+                               "CPL=%d II=%d A20=%d SMM=%d HLT=%d\n",
+                               env->regs[R_EAX],
+                               env->regs[R_EBX],
+                               env->regs[R_ECX],
+                               env->regs[R_EDX],
+                               env->regs[R_ESI],
+                               env->regs[R_EDI],
+                               env->regs[R_EBP],
+                               env->regs[R_ESP],
+                               env->regs[8],
+                               env->regs[9],
+                               env->regs[10],
+                               env->regs[11],
+                               env->regs[12],
+                               env->regs[13],
+                               env->regs[14],
+                               env->regs[15],
+                               env->eip, eflags,
+                               eflags & DF_MASK ? 'D' : '-',
+                               eflags & CC_O ? 'O' : '-',
+                               eflags & CC_S ? 'S' : '-',
+                               eflags & CC_Z ? 'Z' : '-',
+                               eflags & CC_A ? 'A' : '-',
+                               eflags & CC_P ? 'P' : '-',
+                               eflags & CC_C ? 'C' : '-',
+                               env->hflags & HF_CPL_MASK,
+                               (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1,
+                               (env->a20_mask >> 20) & 1,
+                               (env->hflags >> HF_SMM_SHIFT) & 1,
+                               cs->halted);
     } else
 #endif
     {
-        qemu_fprintf(f, "EAX=%08x EBX=%08x ECX=%08x EDX=%08x\n"
-                     "ESI=%08x EDI=%08x EBP=%08x ESP=%08x\n"
-                     "EIP=%08x EFL=%08x [%c%c%c%c%c%c%c] CPL=%d II=%d A20=%d SMM=%d HLT=%d\n",
-                     (uint32_t)env->regs[R_EAX],
-                     (uint32_t)env->regs[R_EBX],
-                     (uint32_t)env->regs[R_ECX],
-                     (uint32_t)env->regs[R_EDX],
-                     (uint32_t)env->regs[R_ESI],
-                     (uint32_t)env->regs[R_EDI],
-                     (uint32_t)env->regs[R_EBP],
-                     (uint32_t)env->regs[R_ESP],
-                     (uint32_t)env->eip, eflags,
-                     eflags & DF_MASK ? 'D' : '-',
-                     eflags & CC_O ? 'O' : '-',
-                     eflags & CC_S ? 'S' : '-',
-                     eflags & CC_Z ? 'Z' : '-',
-                     eflags & CC_A ? 'A' : '-',
-                     eflags & CC_P ? 'P' : '-',
-                     eflags & CC_C ? 'C' : '-',
-                     env->hflags & HF_CPL_MASK,
-                     (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1,
-                     (env->a20_mask >> 20) & 1,
-                     (env->hflags >> HF_SMM_SHIFT) & 1,
-                     cs->halted);
+        g_string_append_printf(buf, "EAX=%08x EBX=%08x ECX=%08x EDX=%08x\n"
+                               "ESI=%08x EDI=%08x EBP=%08x ESP=%08x\n"
+                               "EIP=%08x EFL=%08x [%c%c%c%c%c%c%c] "
+                               "CPL=%d II=%d A20=%d SMM=%d HLT=%d\n",
+                               (uint32_t)env->regs[R_EAX],
+                               (uint32_t)env->regs[R_EBX],
+                               (uint32_t)env->regs[R_ECX],
+                               (uint32_t)env->regs[R_EDX],
+                               (uint32_t)env->regs[R_ESI],
+                               (uint32_t)env->regs[R_EDI],
+                               (uint32_t)env->regs[R_EBP],
+                               (uint32_t)env->regs[R_ESP],
+                               (uint32_t)env->eip, eflags,
+                               eflags & DF_MASK ? 'D' : '-',
+                               eflags & CC_O ? 'O' : '-',
+                               eflags & CC_S ? 'S' : '-',
+                               eflags & CC_Z ? 'Z' : '-',
+                               eflags & CC_A ? 'A' : '-',
+                               eflags & CC_P ? 'P' : '-',
+                               eflags & CC_C ? 'C' : '-',
+                               env->hflags & HF_CPL_MASK,
+                               (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1,
+                               (env->a20_mask >> 20) & 1,
+                               (env->hflags >> HF_SMM_SHIFT) & 1,
+                               cs->halted);
     }
 
     for(i = 0; i < 6; i++) {
-        cpu_x86_dump_seg_cache(env, f, seg_name[i], &env->segs[i]);
+        cpu_x86_dump_seg_cache(env, buf, seg_name[i], &env->segs[i]);
     }
-    cpu_x86_dump_seg_cache(env, f, "LDT", &env->ldt);
-    cpu_x86_dump_seg_cache(env, f, "TR", &env->tr);
+    cpu_x86_dump_seg_cache(env, buf, "LDT", &env->ldt);
+    cpu_x86_dump_seg_cache(env, buf, "TR", &env->tr);
 
 #ifdef TARGET_X86_64
     if (env->hflags & HF_LMA_MASK) {
-        qemu_fprintf(f, "GDT=     %016" PRIx64 " %08x\n",
-                     env->gdt.base, env->gdt.limit);
-        qemu_fprintf(f, "IDT=     %016" PRIx64 " %08x\n",
-                     env->idt.base, env->idt.limit);
-        qemu_fprintf(f, "CR0=%08x CR2=%016" PRIx64 " CR3=%016" PRIx64 " CR4=%08x\n",
-                     (uint32_t)env->cr[0],
-                     env->cr[2],
-                     env->cr[3],
-                     (uint32_t)env->cr[4]);
+        g_string_append_printf(buf, "GDT=     %016" PRIx64 " %08x\n",
+                               env->gdt.base, env->gdt.limit);
+        g_string_append_printf(buf, "IDT=     %016" PRIx64 " %08x\n",
+                               env->idt.base, env->idt.limit);
+        g_string_append_printf(buf, "CR0=%08x CR2=%016" PRIx64
+                               " CR3=%016" PRIx64 " CR4=%08x\n",
+                               (uint32_t)env->cr[0],
+                               env->cr[2],
+                               env->cr[3],
+                               (uint32_t)env->cr[4]);
         for(i = 0; i < 4; i++)
-            qemu_fprintf(f, "DR%d=%016" PRIx64 " ", i, env->dr[i]);
-        qemu_fprintf(f, "\nDR6=%016" PRIx64 " DR7=%016" PRIx64 "\n",
-                     env->dr[6], env->dr[7]);
+            g_string_append_printf(buf, "DR%d=%016" PRIx64 " ", i, env->dr[i]);
+        g_string_append_printf(buf, "\nDR6=%016" PRIx64 " DR7=%016" PRIx64 "\n",
+                               env->dr[6], env->dr[7]);
     } else
 #endif
     {
-        qemu_fprintf(f, "GDT=     %08x %08x\n",
-                     (uint32_t)env->gdt.base, env->gdt.limit);
-        qemu_fprintf(f, "IDT=     %08x %08x\n",
-                     (uint32_t)env->idt.base, env->idt.limit);
-        qemu_fprintf(f, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n",
-                     (uint32_t)env->cr[0],
-                     (uint32_t)env->cr[2],
-                     (uint32_t)env->cr[3],
-                     (uint32_t)env->cr[4]);
+        g_string_append_printf(buf, "GDT=     %08x %08x\n",
+                               (uint32_t)env->gdt.base, env->gdt.limit);
+        g_string_append_printf(buf, "IDT=     %08x %08x\n",
+                               (uint32_t)env->idt.base, env->idt.limit);
+        g_string_append_printf(buf, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n",
+                               (uint32_t)env->cr[0],
+                               (uint32_t)env->cr[2],
+                               (uint32_t)env->cr[3],
+                               (uint32_t)env->cr[4]);
         for(i = 0; i < 4; i++) {
-            qemu_fprintf(f, "DR%d=" TARGET_FMT_lx " ", i, env->dr[i]);
+            g_string_append_printf(buf, "DR%d=" TARGET_FMT_lx
+                                   " ", i, env->dr[i]);
         }
-        qemu_fprintf(f, "\nDR6=" TARGET_FMT_lx " DR7=" TARGET_FMT_lx "\n",
-                     env->dr[6], env->dr[7]);
+        g_string_append_printf(buf, "\nDR6=" TARGET_FMT_lx
+                               " DR7=" TARGET_FMT_lx "\n",
+                               env->dr[6], env->dr[7]);
     }
     if (flags & CPU_DUMP_CCOP) {
         if ((unsigned)env->cc_op < CC_OP_NB)
@@ -464,18 +477,19 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags)
             snprintf(cc_op_name, sizeof(cc_op_name), "[%d]", env->cc_op);
 #ifdef TARGET_X86_64
         if (env->hflags & HF_CS64_MASK) {
-            qemu_fprintf(f, "CCS=%016" PRIx64 " CCD=%016" PRIx64 " CCO=%-8s\n",
-                         env->cc_src, env->cc_dst,
-                         cc_op_name);
+            g_string_append_printf(buf, "CCS=%016" PRIx64
+                                   " CCD=%016" PRIx64 " CCO=%-8s\n",
+                                   env->cc_src, env->cc_dst,
+                                   cc_op_name);
         } else
 #endif
         {
-            qemu_fprintf(f, "CCS=%08x CCD=%08x CCO=%-8s\n",
-                         (uint32_t)env->cc_src, (uint32_t)env->cc_dst,
-                         cc_op_name);
+            g_string_append_printf(buf, "CCS=%08x CCD=%08x CCO=%-8s\n",
+                                   (uint32_t)env->cc_src, (uint32_t)env->cc_dst,
+                                   cc_op_name);
         }
     }
-    qemu_fprintf(f, "EFER=%016" PRIx64 "\n", env->efer);
+    g_string_append_printf(buf, "EFER=%016" PRIx64 "\n", env->efer);
     if (flags & CPU_DUMP_FPU) {
         int fptag;
         const uint64_t avx512_mask = XSTATE_OPMASK_MASK | \
@@ -488,64 +502,68 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags)
             fptag |= ((!env->fptags[i]) << i);
         }
         update_mxcsr_from_sse_status(env);
-        qemu_fprintf(f, "FCW=%04x FSW=%04x [ST=%d] FTW=%02x MXCSR=%08x\n",
-                     env->fpuc,
-                     (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11,
-                     env->fpstt,
-                     fptag,
-                     env->mxcsr);
+        g_string_append_printf(
+            buf, "FCW=%04x FSW=%04x [ST=%d] FTW=%02x MXCSR=%08x\n",
+            env->fpuc,
+            (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11,
+            env->fpstt,
+            fptag,
+            env->mxcsr);
         for(i=0;i<8;i++) {
             CPU_LDoubleU u;
             u.d = env->fpregs[i].d;
-            qemu_fprintf(f, "FPR%d=%016" PRIx64 " %04x",
-                         i, u.l.lower, u.l.upper);
+            g_string_append_printf(buf, "FPR%d=%016" PRIx64 " %04x",
+                                   i, u.l.lower, u.l.upper);
             if ((i & 1) == 1)
-                qemu_fprintf(f, "\n");
+                g_string_append_printf(buf, "\n");
             else
-                qemu_fprintf(f, " ");
+                g_string_append_printf(buf, " ");
         }
-
         if ((env->xcr0 & avx512_mask) == avx512_mask) {
             /* XSAVE enabled AVX512 */
             for (i = 0; i < NB_OPMASK_REGS; i++) {
-                qemu_fprintf(f, "Opmask%02d=%016"PRIx64"%s", i,
-                             env->opmask_regs[i], ((i & 3) == 3) ? "\n" : " ");
+                g_string_append_printf(buf, "Opmask%02d=%016"PRIx64"%s", i,
+                                       env->opmask_regs[i],
+                                       ((i & 3) == 3) ? "\n" : " ");
             }
 
             nb = (env->hflags & HF_CS64_MASK) ? 32 : 8;
             for (i = 0; i < nb; i++) {
-                qemu_fprintf(f, "ZMM%02d=%016"PRIx64" %016"PRIx64" %016"PRIx64
-                             " %016"PRIx64" %016"PRIx64" %016"PRIx64
-                             " %016"PRIx64" %016"PRIx64"\n",
-                             i,
-                             env->xmm_regs[i].ZMM_Q(7),
-                             env->xmm_regs[i].ZMM_Q(6),
-                             env->xmm_regs[i].ZMM_Q(5),
-                             env->xmm_regs[i].ZMM_Q(4),
-                             env->xmm_regs[i].ZMM_Q(3),
-                             env->xmm_regs[i].ZMM_Q(2),
-                             env->xmm_regs[i].ZMM_Q(1),
-                             env->xmm_regs[i].ZMM_Q(0));
+                g_string_append_printf(buf, "ZMM%02d=%016"PRIx64
+                                       " %016"PRIx64" %016"PRIx64
+                                       " %016"PRIx64" %016"PRIx64" %016"PRIx64
+                                       " %016"PRIx64" %016"PRIx64"\n",
+                                       i,
+                                       env->xmm_regs[i].ZMM_Q(7),
+                                       env->xmm_regs[i].ZMM_Q(6),
+                                       env->xmm_regs[i].ZMM_Q(5),
+                                       env->xmm_regs[i].ZMM_Q(4),
+                                       env->xmm_regs[i].ZMM_Q(3),
+                                       env->xmm_regs[i].ZMM_Q(2),
+                                       env->xmm_regs[i].ZMM_Q(1),
+                                       env->xmm_regs[i].ZMM_Q(0));
             }
         } else if ((env->xcr0 & avx_mask)  == avx_mask) {
             /* XSAVE enabled AVX */
             nb = env->hflags & HF_CS64_MASK ? 16 : 8;
             for (i = 0; i < nb; i++) {
-                qemu_fprintf(f, "YMM%02d=%016"PRIx64" %016"PRIx64" %016"PRIx64
-                             " %016"PRIx64"\n", i,
-                             env->xmm_regs[i].ZMM_Q(3),
-                             env->xmm_regs[i].ZMM_Q(2),
-                             env->xmm_regs[i].ZMM_Q(1),
-                             env->xmm_regs[i].ZMM_Q(0));
+                g_string_append_printf(buf, "YMM%02d=%016"PRIx64
+                                       " %016"PRIx64" %016"PRIx64
+                                       " %016"PRIx64"\n", i,
+                                       env->xmm_regs[i].ZMM_Q(3),
+                                       env->xmm_regs[i].ZMM_Q(2),
+                                       env->xmm_regs[i].ZMM_Q(1),
+                                       env->xmm_regs[i].ZMM_Q(0));
             }
         } else { /* SSE and below cases */
             nb = env->hflags & HF_CS64_MASK ? 16 : 8;
             for (i = 0; i < nb; i++) {
-                qemu_fprintf(f, "XMM%02d=%016"PRIx64" %016"PRIx64"%s",
-                             i,
-                             env->xmm_regs[i].ZMM_Q(1),
-                             env->xmm_regs[i].ZMM_Q(0),
-                             (i & 1) ? "\n" : " ");
+                g_string_append_printf(buf,
+                                       "XMM%02d=%016"PRIx64" %016"PRIx64"%s",
+                                       i,
+                                       env->xmm_regs[i].ZMM_Q(1),
+                                       env->xmm_regs[i].ZMM_Q(0),
+                                       (i & 1) ? "\n" : " ");
             }
         }
     }
@@ -555,16 +573,17 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags)
         uint8_t code;
         char codestr[3];
 
-        qemu_fprintf(f, "Code=");
+        g_string_append_printf(buf, "Code=");
         for (i = 0; i < DUMP_CODE_BYTES_TOTAL; i++) {
             if (cpu_memory_rw_debug(cs, base - offs + i, &code, 1, 0) == 0) {
                 snprintf(codestr, sizeof(codestr), "%02x", code);
             } else {
                 snprintf(codestr, sizeof(codestr), "??");
             }
-            qemu_fprintf(f, "%s%s%s%s", i > 0 ? " " : "",
-                         i == offs ? "<" : "", codestr, i == offs ? ">" : "");
+            g_string_append_printf(buf, "%s%s%s%s", i > 0 ? " " : "",
+                                   i == offs ? "<" : "", codestr,
+                                   i == offs ? ">" : "");
         }
-        qemu_fprintf(f, "\n");
+        g_string_append_printf(buf, "\n");
     }
 }
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 97e250e876..f31a304abb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6757,7 +6757,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->class_by_name = x86_cpu_class_by_name;
     cc->parse_features = x86_cpu_parse_featurestr;
     cc->has_work = x86_cpu_has_work;
-    cc->dump_state = x86_cpu_dump_state;
+    cc->format_state = x86_cpu_format_state;
     cc->set_pc = x86_cpu_set_pc;
     cc->gdb_read_register = x86_cpu_gdb_read_register;
     cc->gdb_write_register = x86_cpu_gdb_write_register;
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 6c50d3ab4f..01ca4e715d 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1821,7 +1821,7 @@ int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu,
 void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
                                 Error **errp);
 
-void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags);
+void x86_cpu_format_state(CPUState *cs, GString *buf, int flags);
 
 hwaddr x86_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
                                          MemTxAttrs *attrs);
-- 
2.31.1

[PATCH 4/5] qapi: introduce x-query-registers QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info registers" command. It is being
added with an "x-" prefix because this QMP command is intended as an
adhoc debugging tool and will thus not be modelled in QAPI as fully
structured data, nor will it have long term guaranteed stability.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++
 qapi/machine.json          | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 216fdfaf3a..0d9943ff60 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -204,3 +204,31 @@ MemdevList *qmp_query_memdev(Error **errp)
     object_child_foreach(obj, query_memdev, &list);
     return list;
 }
+
+RegisterInfo *qmp_x_query_registers(bool has_cpu, int64_t cpu, Error **errp)
+{
+    RegisterInfo *info = g_new0(RegisterInfo, 1);
+    g_autoptr(GString) buf = g_string_new("");
+    CPUState *cs = NULL, *tmp;
+
+    if (has_cpu) {
+        CPU_FOREACH(tmp) {
+            if (cpu == tmp->cpu_index) {
+                cs = tmp;
+            }
+        }
+        if (!cs) {
+            error_setg(errp, "CPU %"PRId64" not available", cpu);
+            return NULL;
+        }
+        cpu_format_state(cs, buf, CPU_DUMP_FPU);
+    } else {
+        CPU_FOREACH(cs) {
+            g_string_append_printf(buf, "\nCPU#%d\n", cs->cpu_index);
+            cpu_format_state(cs, buf, CPU_DUMP_FPU);
+        }
+    }
+
+    info->state = g_steal_pointer(&buf->str);
+    return info;
+}
diff --git a/qapi/machine.json b/qapi/machine.json
index 157712f006..27b922f2ce 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1312,3 +1312,40 @@
      '*cores': 'int',
      '*threads': 'int',
      '*maxcpus': 'int' } }
+
+##
+# @RegisterParams:
+#
+# Information about the CPU to query state of
+#
+# @cpu: the CPU number to query. If omitted, queries all CPUs
+#
+# Since: 6.2.0
+#
+##
+{ 'struct': 'RegisterParams', 'data': {'*cpu': 'int' } }
+
+##
+# @RegisterInfo:
+#
+# Information about the CPU state
+#
+# @state: the CPU state in an architecture specific format
+#
+# Since: 6.2.0
+#
+##
+{ 'struct': 'RegisterInfo', 'data': {'state': 'str' } }
+
+##
+# @x-query-registers:
+#
+# Return information on the CPU registers
+#
+# Returns: the CPU state
+#
+# Since: 6.2.0
+##
+{ 'command': 'x-query-registers',
+  'data': 'RegisterParams',
+  'returns': 'RegisterInfo' }
-- 
2.31.1

[PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers'

[Daniel P. Berrangé]

Now that we have a QMP command 'x-query-registers', the HMP counterpart
'info registers' can be refactored to call the former.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/misc.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/monitor/misc.c b/monitor/misc.c
index ffe7966870..f0b94c3084 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -67,6 +67,7 @@
 #include "block/block-hmp-cmds.h"
 #include "qapi/qapi-commands-char.h"
 #include "qapi/qapi-commands-control.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qapi-commands-qom.h"
@@ -301,23 +302,29 @@ int monitor_get_cpu_index(Monitor *mon)
 static void hmp_info_registers(Monitor *mon, const QDict *qdict)
 {
     bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false);
-    CPUState *cs;
+    bool has_cpu = !all_cpus;
+    int64_t cpu = 0;
+    Error *local_err = NULL;
+    g_autoptr(RegisterInfo) info = NULL;
 
-    if (all_cpus) {
-        CPU_FOREACH(cs) {
-            monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index);
-            cpu_dump_state(cs, NULL, CPU_DUMP_FPU);
-        }
-    } else {
-        cs = mon_get_cpu(mon);
+    if (has_cpu) {
+        CPUState *cs = mon_get_cpu(mon);
 
         if (!cs) {
             monitor_printf(mon, "No CPU available\n");
             return;
         }
 
-        cpu_dump_state(cs, NULL, CPU_DUMP_FPU);
+        cpu = cs->cpu_index;
+    }
+
+    info = qmp_x_query_registers(has_cpu, cpu, &local_err);
+    if (!info) {
+        error_report_err(local_err);
+        return;
     }
+
+    monitor_printf(mon, "%s", info->state);
 }
 
 static void hmp_info_sync_profile(Monitor *mon, const QDict *qdict)
-- 
2.31.1

Re: [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers'

[Philippe Mathieu-Daudé]

On 9/8/21 12:37 PM, Daniel P. Berrangé wrote:
> Now that we have a QMP command 'x-query-registers', the HMP counterpart
> 'info registers' can be refactored to call the former.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  monitor/misc.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/monitor/misc.c b/monitor/misc.c
> index ffe7966870..f0b94c3084 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -67,6 +67,7 @@
>  #include "block/block-hmp-cmds.h"
>  #include "qapi/qapi-commands-char.h"
>  #include "qapi/qapi-commands-control.h"
> +#include "qapi/qapi-commands-machine.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/qapi-commands-misc.h"
>  #include "qapi/qapi-commands-qom.h"
> @@ -301,23 +302,29 @@ int monitor_get_cpu_index(Monitor *mon)
>  static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>  {
>      bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false);
> -    CPUState *cs;
> +    bool has_cpu = !all_cpus;
> +    int64_t cpu = 0;
> +    Error *local_err = NULL;
> +    g_autoptr(RegisterInfo) info = NULL;
>  
> -    if (all_cpus) {
> -        CPU_FOREACH(cs) {
> -            monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index);
> -            cpu_dump_state(cs, NULL, CPU_DUMP_FPU);

And once all targets are converted we can remove cpu_dump_state().

Re: [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers'

[Daniel P. Berrangé]

On Wed, Sep 08, 2021 at 01:01:21PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/8/21 12:37 PM, Daniel P. Berrangé wrote:
> > Now that we have a QMP command 'x-query-registers', the HMP counterpart
> > 'info registers' can be refactored to call the former.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  monitor/misc.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index ffe7966870..f0b94c3084 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -67,6 +67,7 @@
> >  #include "block/block-hmp-cmds.h"
> >  #include "qapi/qapi-commands-char.h"
> >  #include "qapi/qapi-commands-control.h"
> > +#include "qapi/qapi-commands-machine.h"
> >  #include "qapi/qapi-commands-migration.h"
> >  #include "qapi/qapi-commands-misc.h"
> >  #include "qapi/qapi-commands-qom.h"
> > @@ -301,23 +302,29 @@ int monitor_get_cpu_index(Monitor *mon)
> >  static void hmp_info_registers(Monitor *mon, const QDict *qdict)
> >  {
> >      bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false);
> > -    CPUState *cs;
> > +    bool has_cpu = !all_cpus;
> > +    int64_t cpu = 0;
> > +    Error *local_err = NULL;
> > +    g_autoptr(RegisterInfo) info = NULL;
> >  
> > -    if (all_cpus) {
> > -        CPU_FOREACH(cs) {
> > -            monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index);
> > -            cpu_dump_state(cs, NULL, CPU_DUMP_FPU);
> 
> And once all targets are converted we can remove cpu_dump_state().

Yes, if we take approach in this series, then there would be another
20-ish patches inserted before this one, to convert all the other
targets, and eliminate the cpu_dump_state method / callback.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state

[Ján Tomko]

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

On a Wednesday in 2021, Daniel P. Berrangé wrote:
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> target/i386/cpu-dump.c | 325 ++++++++++++++++++++++-------------------
> target/i386/cpu.c      |   2 +-
> target/i386/cpu.h      |   2 +-
> 3 files changed, 174 insertions(+), 155 deletions(-)
>
>diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
>index 02b635a52c..8e19485a20 100644
>--- a/target/i386/cpu-dump.c
>+++ b/target/i386/cpu-dump.c

[...]

>@@ -355,107 +359,116 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags)

[...]

>     {
>-        qemu_fprintf(f, "GDT=     %08x %08x\n",
>-                     (uint32_t)env->gdt.base, env->gdt.limit);
>-        qemu_fprintf(f, "IDT=     %08x %08x\n",
>-                     (uint32_t)env->idt.base, env->idt.limit);
>-        qemu_fprintf(f, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n",
>-                     (uint32_t)env->cr[0],
>-                     (uint32_t)env->cr[2],
>-                     (uint32_t)env->cr[3],
>-                     (uint32_t)env->cr[4]);
>+        g_string_append_printf(buf, "GDT=     %08x %08x\n",
>+                               (uint32_t)env->gdt.base, env->gdt.limit);
>+        g_string_append_printf(buf, "IDT=     %08x %08x\n",
>+                               (uint32_t)env->idt.base, env->idt.limit);
>+        g_string_append_printf(buf, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n",
>+                               (uint32_t)env->cr[0],
>+                               (uint32_t)env->cr[2],
>+                               (uint32_t)env->cr[3],
>+                               (uint32_t)env->cr[4]);
>         for(i = 0; i < 4; i++) {
>-            qemu_fprintf(f, "DR%d=" TARGET_FMT_lx " ", i, env->dr[i]);
>+            g_string_append_printf(buf, "DR%d=" TARGET_FMT_lx
>+                                   " ", i, env->dr[i]);

The formatting string can comfortably fit on the first line here.

Jano

>         }
>-        qemu_fprintf(f, "\nDR6=" TARGET_FMT_lx " DR7=" TARGET_FMT_lx "\n",
>-                     env->dr[6], env->dr[7]);
>+        g_string_append_printf(buf, "\nDR6=" TARGET_FMT_lx
>+                               " DR7=" TARGET_FMT_lx "\n",
>+                               env->dr[6], env->dr[7]);

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

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Ján Tomko]

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

On a Wednesday in 2021, Daniel P. Berrangé wrote:
>We are still adding HMP commands without any QMP counterparts. This is
>done because there are a reasonable number of scenarios where the cost
>of designing a QAPI data type for the command is not justified.
>
>This has the downside, however, that we will never be able to fully
>isolate the monitor code from the remainder of QEMU internals. It is
>desirable to be able to get to a point where subsystems in QEMU are
>exclusively implemented using QAPI types and never need to have any
>knowledge of the monitor APIs.
>
>The way to get there is to stop adding commands to HMP only. All
>commands must be implemented using QMP, and any HMP implementation
>be a shim around the QMP implementation.
>
>We don't want to compromise our supportability of QMP long term though.
>
>This series proposes that we relax our requirements around fine grained
>QAPI data design, but with the caveat that any command taking this
>design approach is mandated to use the 'x-' name prefix.
>
>This tradeoff should be suitable for any commands we have been adding
>exclusively to HMP in recent times, and thus mean we have mandate QMP
>support for all new commands going forward.
>
>This series illustrates the concept by converting the "info registers"
>HMP to invoke a new 'x-query-registers' QMP command. Note that only
>the i386 CPU target is converted to work with this new approach, so
>this series needs to be considered incomplete. If we go forward with
>this idea, then a subsequent version of this series would need to
>obviously convert all other CPU targets.
>
>After doing that conversion the only use of qemu_fprintf() would be
>the disas.c file. Remaining uses of qemu_fprintf and qemu_printf
>could be tackled in a similar way and eventually eliminate the need
>for any of these printf wrappers in QEMU.
>
>NB: I added docs to devel/writing-qmp-commands.rst about the two
>design approaches to QMP. I didn't see another good place to put
>an explicit note that we will not add any more HMP-only commands.
>Obviously HMP/QMP maintainers control this in their reviews of
>patches, and maybe that's sufficient ?
>
>NB: if we take this approach we'll want to figure out how many
>HMP-only commands we actually have left and then perhaps have
>a task to track their conversion to QMP. This could possibly
>be a useful task for newbies if we make it clear that they
>wouldn't be required to undertake complex QAPI modelling in
>doing this conversion.
>
>Daniel P. Berrangé (5):
>  docs/devel: document expectations for QAPI data modelling for QMP
>  hw/core: introduce 'format_state' callback to replace 'dump_state'
>  target/i386: convert to use format_state instead of dump_state
>  qapi: introduce x-query-registers QMP command
>  monitor: rewrite 'info registers' in terms of 'x-query-registers'
>
> docs/devel/writing-qmp-commands.rst |  25 +++
> hw/core/cpu-common.c                |  15 ++
> hw/core/machine-qmp-cmds.c          |  28 +++
> include/hw/core/cpu.h               |  13 +-
> monitor/misc.c                      |  25 ++-
> qapi/machine.json                   |  37 ++++
> target/i386/cpu-dump.c              | 325 +++++++++++++++-------------
> target/i386/cpu.c                   |   2 +-
> target/i386/cpu.h                   |   2 +-
> 9 files changed, 307 insertions(+), 165 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> We are still adding HMP commands without any QMP counterparts. This is
> done because there are a reasonable number of scenarios where the cost
> of designing a QAPI data type for the command is not justified.
>
> This has the downside, however, that we will never be able to fully
> isolate the monitor code from the remainder of QEMU internals. It is
> desirable to be able to get to a point where subsystems in QEMU are
> exclusively implemented using QAPI types and never need to have any
> knowledge of the monitor APIs.
>
> The way to get there is to stop adding commands to HMP only. All
> commands must be implemented using QMP, and any HMP implementation
> be a shim around the QMP implementation.
>
> We don't want to compromise our supportability of QMP long term though.
>
> This series proposes that we relax our requirements around fine grained
> QAPI data design,

Specifics?  QMP command returns a string, HMP wrapper prints that
string?

>                   but with the caveat that any command taking this
> design approach is mandated to use the 'x-' name prefix.
>
> This tradeoff should be suitable for any commands we have been adding
> exclusively to HMP in recent times, and thus mean we have mandate QMP
> support for all new commands going forward.
>
> This series illustrates the concept by converting the "info registers"
> HMP to invoke a new 'x-query-registers' QMP command. Note that only
> the i386 CPU target is converted to work with this new approach, so
> this series needs to be considered incomplete. If we go forward with
> this idea, then a subsequent version of this series would need to
> obviously convert all other CPU targets.
>
> After doing that conversion the only use of qemu_fprintf() would be
> the disas.c file. Remaining uses of qemu_fprintf and qemu_printf
> could be tackled in a similar way and eventually eliminate the need
> for any of these printf wrappers in QEMU.
>
> NB: I added docs to devel/writing-qmp-commands.rst about the two
> design approaches to QMP. I didn't see another good place to put
> an explicit note that we will not add any more HMP-only commands.
> Obviously HMP/QMP maintainers control this in their reviews of
> patches, and maybe that's sufficient ?

We could add devel/writing-hmp-commands.rst, or go with a single
document devel/writing-monitor-commands.rst.

> NB: if we take this approach we'll want to figure out how many
> HMP-only commands we actually have left and then perhaps have
> HMP-only commands we actually have left

Yes.

For many HMP commands, a QMP commands with the same name exists, and the
former is probably a wrapper around the latter.  Same for HMP "info FOO"
and QMP query-FOO.

HMP commands without such a match:

    boot_set
    change
    commit
    cpu
    delvm
    drive_add
    drive_del
    exit_preconfig
    gdbserver
    gpa2hpa
    gpa2hva
    gva2gpa
    help
    hostfwd_add
    hostfwd_remove
    i
    info
    info capture
    info cmma
    info cpus
    info history
    info ioapic
    info irq
    info jit
    info lapic
    info mem
    info mtree
    info network
    info numa
    info opcount
    info pic
    info profile
    info qdm
    info qom-tree
    info qtree
    info ramblock
    info rdma
    info registers
    info roms
    info skeys
    info snapshots
    info sync-profile
    info tlb
    info trace-events
    info usb
    info usbhost
    info usernet
    loadvm
    log
    logfile
    mce
    migrate_set_capability
    migrate_set_parameter
    migration_mode
    mouse_button
    mouse_move
    mouse_set
    nmi
    o
    pcie_aer_inject_error
    print
    qemu-io
    savevm
    sendkey
    singlestep
    snapshot_blkdev
    snapshot_blkdev_internal
    snapshot_delete_blkdev_internal
    stopcapture
    sum
    sync-profile
    trace-event
    trace-file
    watchdog_action
    wavcapture
    x
    xp

This is 77 out of 170 HMP commands.  I was hoping for fewer.

>                                         and then perhaps have
> a task to track their conversion to QMP. This could possibly
> be a useful task for newbies if we make it clear that they
> wouldn't be required to undertake complex QAPI modelling in
> doing this conversion.

Yes.

Thanks for tackling this!

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Daniel P. Berrangé]

On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > We are still adding HMP commands without any QMP counterparts. This is
> > done because there are a reasonable number of scenarios where the cost
> > of designing a QAPI data type for the command is not justified.
> >
> > This has the downside, however, that we will never be able to fully
> > isolate the monitor code from the remainder of QEMU internals. It is
> > desirable to be able to get to a point where subsystems in QEMU are
> > exclusively implemented using QAPI types and never need to have any
> > knowledge of the monitor APIs.
> >
> > The way to get there is to stop adding commands to HMP only. All
> > commands must be implemented using QMP, and any HMP implementation
> > be a shim around the QMP implementation.
> >
> > We don't want to compromise our supportability of QMP long term though.
> >
> > This series proposes that we relax our requirements around fine grained
> > QAPI data design,
> 
> Specifics?  QMP command returns a string, HMP wrapper prints that
> string?

yes, a command returning a single opaque printf formatted string would
be the common case.  At a more general POV though, the JSON doc received
by the client should be usable "as received", immediately after JSON
de-serialization without needing any further custom parsing on top.

ie if a value needs to be parsed by the client, then it must be split
into multiple distinct values in the QAPI data type design to remove
the need for parsing by the client. 

If a command's design violates that, then it must remain under the
"x-" prefix.  "info registers" is a example because we're printf
formatting each register value into a opaque string. Any client
needing a specific register value would have to scanf parse this
string. The justification for not representing each register as
a distinct QAPI field is that we don't think machines genuinely
need to parse this, as its just adhoc human operator debug info.
So we take the easy option and just printf to a string and put
it under "x-" prefix



> For many HMP commands, a QMP commands with the same name exists, and the
> former is probably a wrapper around the latter.  Same for HMP "info FOO"
> and QMP query-FOO.
> 
> HMP commands without such a match:
> 
>     boot_set
>     change
>     commit
>     cpu
>     delvm
>     drive_add
>     drive_del
>     exit_preconfig
>     gdbserver
>     gpa2hpa
>     gpa2hva
>     gva2gpa
>     help
>     hostfwd_add
>     hostfwd_remove
>     i
>     info
>     info capture
>     info cmma
>     info cpus
>     info history
>     info ioapic
>     info irq
>     info jit
>     info lapic
>     info mem
>     info mtree
>     info network
>     info numa
>     info opcount
>     info pic
>     info profile
>     info qdm
>     info qom-tree
>     info qtree
>     info ramblock
>     info rdma
>     info registers
>     info roms
>     info skeys
>     info snapshots
>     info sync-profile
>     info tlb
>     info trace-events
>     info usb
>     info usbhost
>     info usernet
>     loadvm
>     log
>     logfile
>     mce
>     migrate_set_capability
>     migrate_set_parameter
>     migration_mode
>     mouse_button
>     mouse_move
>     mouse_set
>     nmi
>     o
>     pcie_aer_inject_error
>     print
>     qemu-io
>     savevm
>     sendkey
>     singlestep
>     snapshot_blkdev
>     snapshot_blkdev_internal
>     snapshot_delete_blkdev_internal
>     stopcapture
>     sum
>     sync-profile
>     trace-event
>     trace-file
>     watchdog_action
>     wavcapture
>     x
>     xp
> 
> This is 77 out of 170 HMP commands.  I was hoping for fewer.

Wow, yes, I'm surprised ! A few of those do indeed have QMP
equivalents, but with slight differences eg savevm ->
snapshot-save, but clearly we have a big list regardless


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Philippe Mathieu-Daudé]

On 9/8/21 5:09 PM, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> We are still adding HMP commands without any QMP counterparts. This is
>> done because there are a reasonable number of scenarios where the cost
>> of designing a QAPI data type for the command is not justified.
>>
>> This has the downside, however, that we will never be able to fully
>> isolate the monitor code from the remainder of QEMU internals. It is
>> desirable to be able to get to a point where subsystems in QEMU are
>> exclusively implemented using QAPI types and never need to have any
>> knowledge of the monitor APIs.
>>
>> The way to get there is to stop adding commands to HMP only. All
>> commands must be implemented using QMP, and any HMP implementation
>> be a shim around the QMP implementation.
>>
>> We don't want to compromise our supportability of QMP long term though.
>>
>> This series proposes that we relax our requirements around fine grained
>> QAPI data design,
> 
> Specifics?  QMP command returns a string, HMP wrapper prints that
> string?
> 
>>                   but with the caveat that any command taking this
>> design approach is mandated to use the 'x-' name prefix.
>>
>> This tradeoff should be suitable for any commands we have been adding
>> exclusively to HMP in recent times, and thus mean we have mandate QMP
>> support for all new commands going forward.
>>
>> This series illustrates the concept by converting the "info registers"
>> HMP to invoke a new 'x-query-registers' QMP command. Note that only
>> the i386 CPU target is converted to work with this new approach, so
>> this series needs to be considered incomplete. If we go forward with
>> this idea, then a subsequent version of this series would need to
>> obviously convert all other CPU targets.
>>
>> After doing that conversion the only use of qemu_fprintf() would be
>> the disas.c file. Remaining uses of qemu_fprintf and qemu_printf
>> could be tackled in a similar way and eventually eliminate the need
>> for any of these printf wrappers in QEMU.
>>
>> NB: I added docs to devel/writing-qmp-commands.rst about the two
>> design approaches to QMP. I didn't see another good place to put
>> an explicit note that we will not add any more HMP-only commands.
>> Obviously HMP/QMP maintainers control this in their reviews of
>> patches, and maybe that's sufficient ?
> 
> We could add devel/writing-hmp-commands.rst, or go with a single
> document devel/writing-monitor-commands.rst.
> 
>> NB: if we take this approach we'll want to figure out how many
>> HMP-only commands we actually have left and then perhaps have
>> HMP-only commands we actually have left
> 
> Yes.
> 
> For many HMP commands, a QMP commands with the same name exists, and the
> former is probably a wrapper around the latter.  Same for HMP "info FOO"
> and QMP query-FOO.
> 
> HMP commands without such a match:

(1) Handy HMP commands while debugging:

- help
- info *
- i/o
- loadvm/savevm
- trace-event/trace-file
- wavcapture/stopcapture
- xp

Eventually also:

- hostfwd_add/hostfwd_remove
- log
- logfile
- print
- sendkey

(2) I suppose these are pre-QMP and wonder about their
    usefulness in the monitor (I'd certainly use a QMP
    equivalent to test).

- migrate_set_capability
- migrate_set_parameter
- migration_mode
- mouse_button
- mouse_move
- mouse_set
- nmi
- pcie_aer_inject_error
- exit_preconfig
- singlestep
- snapshot_blkdev_internal
- snapshot_delete_blkdev_internal

(3) I don't use them, I expect them to belong
    in either (1) or (2).

>     boot_set
>     change
>     commit
>     cpu
>     delvm
>     drive_add
>     drive_del
>     gdbserver
>     gpa2hpa
>     gpa2hva
>     gva2gpa
>     mce
>     qemu-io
>     snapshot_blkdev
>     sum
>     sync-profile
>     watchdog_action

> 
> This is 77 out of 170 HMP commands.  I was hoping for fewer.

Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP

[Eric Blake]

On Wed, Sep 08, 2021 at 11:37:07AM +0100, Daniel P. Berrangé wrote:
> Traditionally we have required that newly added QMP commands will model
> any returned data using fine grained QAPI types. This is good for
> commands that are intended to be consumed by machines, where clear data
> representation is very important. Commands that don't satisfy this have
> generally been added to HMP only.
> 
> In effect the decision of whether to add a new command to QMP vs HMP has
> been used as a proxy for the decision of whether the cost of designing a
> fine grained QAPI type is justified by the potential benefits.
> 
> As a result the commands present in QMP and HMP are non-overlapping
> sets, although HMP comamnds can be accessed indirectly via the QMP
> command 'human-monitor-command'.
> 
> One of the downsides of 'human-monitor-command' is that the QEMU monitor
> APIs remain tied into various internal parts of the QEMU code. For
> example any exclusively HMP command will need to use 'monitor_printf'
> to get data out. It would be desirable to be able to fully isolate the
> monitor implementation from QEMU internals, however, this is only
> possible if all commands are exclusively based on QAPI with direct
> QMP exposure.
> 
> The way to achieve this desired end goal is to finese the requirements
> for QMP command design. For cases where the output of a command is only
> intended for human consumption, it is reasonable to want to simplify
> the implementation by returning a plain string containing formatted
> data instead of designing a fine grained QAPI data type. This can be
> permitted if-and-only-if the command is exposed under the 'x-' name
> prefix. This indicates that the command data format is liable to
> future change and that it is not following QAPI design best practice.
> 
> The poster child example for this would be the 'info registers' HMP
> command which returns printf formatted data representing CPU state.
> This information varies enourmously across target architectures and
> changes relatively frequently as new CPU features are implemented.
> It is there as debugging data for human operators, and any machine
> usage would treat it as an opaque blob. It is thus reasonable to
> expose this in QMP as 'x-query-registers' returning a 'str' field.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/docs/devel/writing-qmp-commands.rst b/docs/devel/writing-qmp-commands.rst
> index 6a10a06c48..d032daa62d 100644
> --- a/docs/devel/writing-qmp-commands.rst
> +++ b/docs/devel/writing-qmp-commands.rst
> @@ -350,6 +350,31 @@ In this section we will focus on user defined types. Please, check the QAPI
>  documentation for information about the other types.
>  
>  
> +Modelling data in QAPI
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +For a QMP command that to be considered stable and supported long term there
> +is a requirement returned data should be explicitly modelled using fine grained
> +QAPI types. As a general guide, a caller of the QMP command should never need
> +to parse individual returned data fields. If a field appears to need parsing,
> +them it should be split into separate fields corresponding to each distinct

then

> +data item. This should be the common case for any new QMP command that is
> +intended to be used by machines, as opposed to exclusively human operators.
> +
> +Some QMP commands, however, are only intended as adhoc debugging aids for human

ad hoc

> +operators. While they may return large amounts of formatted data, it is not
> +expected that machines will need to parse the result. The overhead of defining
> +a fine grained QAPI type for the data may not be justified by the potential
> +benefit. In such cases, it is permitted to have a command return a simple string
> +that contains formatted data, however, it is mandatory for the command to use
> +the 'x-' name prefix. This indicates that the command is not guaranteed to be
> +long term stable / liable to change in future and is not following QAPI design
> +best practices. An example where this approach is taken is the QMP command
> +"x-query-registers". This returns a printf formatted dump of the architecture
> +specific CPU state. The way the data is formatted varies across QEMU targets,
> +is liable to change over time, and is only intended to be consumed as an opaque
> +string by machines.
> +
>  User Defined Types
>  ~~~~~~~~~~~~~~~~~~
>  
> -- 
> 2.31.1
>

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: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state

[Eric Blake]

On Wed, Sep 08, 2021 at 11:37:09AM +0100, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  target/i386/cpu-dump.c | 325 ++++++++++++++++++++++-------------------
>  target/i386/cpu.c      |   2 +-
>  target/i386/cpu.h      |   2 +-
>  3 files changed, 174 insertions(+), 155 deletions(-)
> 
> diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
> index 02b635a52c..8e19485a20 100644
> --- a/target/i386/cpu-dump.c
> +++ b/target/i386/cpu-dump.c
> @@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = {
>  };
>  
>  static void
> -cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f,
> +cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf,
>                         const char *name, struct SegmentCache *sc)
>  {
>  #ifdef TARGET_X86_64
>      if (env->hflags & HF_CS64_MASK) {
> -        qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
> -                     sc->selector, sc->base, sc->limit,
> -                     sc->flags & 0x00ffff00);
> +        g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
> +                               sc->selector, sc->base, sc->limit,
> +                               sc->flags & 0x00ffff00);

Did you consider using open_memstream() to get a FILE* that can then
be passed into these callbacks unchanged, rather than rewriting all
the callbacks to a new signature?

Then again, I like the GString signature better than FILE*, even if it
makes for longer lines.

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

Re: [PATCH 4/5] qapi: introduce x-query-registers QMP command

[Eric Blake]

On Wed, Sep 08, 2021 at 11:37:10AM +0100, Daniel P. Berrangé wrote:
> This is a counterpart to the HMP "info registers" command. It is being
> added with an "x-" prefix because this QMP command is intended as an
> adhoc debugging tool and will thus not be modelled in QAPI as fully

ad hoc

> structured data, nor will it have long term guaranteed stability.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++
>  qapi/machine.json          | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 

> +++ b/qapi/machine.json
> @@ -1312,3 +1312,40 @@
>       '*cores': 'int',
>       '*threads': 'int',
>       '*maxcpus': 'int' } }
> +
> +##
> +# @RegisterParams:
> +#
> +# Information about the CPU to query state of
> +#
> +# @cpu: the CPU number to query. If omitted, queries all CPUs
> +#
> +# Since: 6.2.0

Consensus seems to be "6.2", not "6.2.0".

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

Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state

[Daniel P. Berrangé]

On Wed, Sep 08, 2021 at 01:05:13PM -0500, Eric Blake wrote:
> On Wed, Sep 08, 2021 at 11:37:09AM +0100, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  target/i386/cpu-dump.c | 325 ++++++++++++++++++++++-------------------
> >  target/i386/cpu.c      |   2 +-
> >  target/i386/cpu.h      |   2 +-
> >  3 files changed, 174 insertions(+), 155 deletions(-)
> > 
> > diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
> > index 02b635a52c..8e19485a20 100644
> > --- a/target/i386/cpu-dump.c
> > +++ b/target/i386/cpu-dump.c
> > @@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = {
> >  };
> >  
> >  static void
> > -cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f,
> > +cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf,
> >                         const char *name, struct SegmentCache *sc)
> >  {
> >  #ifdef TARGET_X86_64
> >      if (env->hflags & HF_CS64_MASK) {
> > -        qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
> > -                     sc->selector, sc->base, sc->limit,
> > -                     sc->flags & 0x00ffff00);
> > +        g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
> > +                               sc->selector, sc->base, sc->limit,
> > +                               sc->flags & 0x00ffff00);
> 
> Did you consider using open_memstream() to get a FILE* that can then
> be passed into these callbacks unchanged, rather than rewriting all
> the callbacks to a new signature?

That is certainly an option, but it wouldn't eliminate the need to do
a rewrite. I would still want to replace qemu_fprintf with fprintf in
that scenario. It is desirable to be able to eliminate the QEMU
specific printf wrappers which only exist because they need to treat
a NULL FILE* object as an indication to output to the HMP chardev.
Admittedly that would result in shorter lines than today.

> Then again, I like the GString signature better than FILE*, even if it
> makes for longer lines.

Yes, the verbosity is not ideal. I like the GString API as a general
purpose API for formatting text output to a buffer overall.

I don't feel too strongly either way though, as long as we get to a place
where we eliminate the custom QEMU printf wrappers that integrate with
the monitor APIs.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > We are still adding HMP commands without any QMP counterparts. This is
>> > done because there are a reasonable number of scenarios where the cost
>> > of designing a QAPI data type for the command is not justified.
>> >
>> > This has the downside, however, that we will never be able to fully
>> > isolate the monitor code from the remainder of QEMU internals. It is
>> > desirable to be able to get to a point where subsystems in QEMU are
>> > exclusively implemented using QAPI types and never need to have any
>> > knowledge of the monitor APIs.
>> >
>> > The way to get there is to stop adding commands to HMP only. All
>> > commands must be implemented using QMP, and any HMP implementation
>> > be a shim around the QMP implementation.
>> >
>> > We don't want to compromise our supportability of QMP long term though.
>> >
>> > This series proposes that we relax our requirements around fine grained
>> > QAPI data design,
>> 
>> Specifics?  QMP command returns a string, HMP wrapper prints that
>> string?
>
> yes, a command returning a single opaque printf formatted string would
> be the common case.  At a more general POV though, the JSON doc received
> by the client should be usable "as received", immediately after JSON
> de-serialization without needing any further custom parsing on top.
>
> ie if a value needs to be parsed by the client, then it must be split
> into multiple distinct values in the QAPI data type design to remove
> the need for parsing by the client. 

Yes, that's QMP doctrine.

> If a command's design violates that, then it must remain under the
> "x-" prefix.  "info registers" is a example because we're printf
> formatting each register value into a opaque string. Any client
> needing a specific register value would have to scanf parse this
> string. The justification for not representing each register as
> a distinct QAPI field is that we don't think machines genuinely
> need to parse this, as its just adhoc human operator debug info.
> So we take the easy option and just printf to a string and put
> it under "x-" prefix

Got it.

Limitations:

1. If we convert a long-running HMP command to this technique, we print
   its output only after it completed its work.  We also end up with a
   long-running QMP command, which is bad, because it stops the main
   loop and makes the QMP monitor unresponsive (except for OOB commands,
   if the client is careful).  The former can be mitigated with
   'coroutine': true.  The latter can't.

2. We can't prompt for input.

   The only current use I can see is HMP "change vnc passwd" prompting
   for a password.  Except you currently have to say "change vnc passwd
   wtf" to get it to prompt (suspect logic error in commit cfb5387a1de).


[...]

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Paolo Bonzini]

On 08/09/21 17:09, Markus Armbruster wrote:
> This is 77 out of 170 HMP commands.  I was hoping for fewer.

There are several that are not quite 1:1, but still not HMP-specific.

>      exit_preconfig

This is x-exit-preconfig.

>      migrate_set_capability
>      migrate_set_parameter

These are migrate-set-{capabilities,parameters}

>      mouse_button
>      mouse_move
>      mouse_set
>      sendkey

These are input-send-event, but they are not implemented in terms of it.

>      nmi

This is inject-nmi.

>      loadvm
>      savevm

These are snapshot-{load,save}.

>      watchdog_action

This is set-action, but it is not implemented in terms of it.

Paolo

>>                                          and then perhaps have
>> a task to track their conversion to QMP. This could possibly
>> be a useful task for newbies if we make it clear that they
>> wouldn't be required to undertake complex QAPI modelling in
>> doing this conversion.
> 
> Yes.
> 
> Thanks for tackling this!
> 
> 

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

[...]

> Limitations:
>
> 1. If we convert a long-running HMP command to this technique, we print
>    its output only after it completed its work.  We also end up with a
>    long-running QMP command, which is bad, because it stops the main
>    loop and makes the QMP monitor unresponsive (except for OOB commands,
>    if the client is careful).  The former can be mitigated with
>    'coroutine': true.  The latter can't.
>
> 2. We can't prompt for input.
>
>    The only current use I can see is HMP "change vnc passwd" prompting
>    for a password.  Except you currently have to say "change vnc passwd
>    wtf" to get it to prompt (suspect logic error in commit cfb5387a1de).

Subject: [PATCH 1/2] hmp: Unbreak "change vnc"
Message-Id: <20210909081219.308065-2-armbru@redhat.com>

>
>
> [...]

Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 06:48:21AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > We are still adding HMP commands without any QMP counterparts. This is
> >> > done because there are a reasonable number of scenarios where the cost
> >> > of designing a QAPI data type for the command is not justified.
> >> >
> >> > This has the downside, however, that we will never be able to fully
> >> > isolate the monitor code from the remainder of QEMU internals. It is
> >> > desirable to be able to get to a point where subsystems in QEMU are
> >> > exclusively implemented using QAPI types and never need to have any
> >> > knowledge of the monitor APIs.
> >> >
> >> > The way to get there is to stop adding commands to HMP only. All
> >> > commands must be implemented using QMP, and any HMP implementation
> >> > be a shim around the QMP implementation.
> >> >
> >> > We don't want to compromise our supportability of QMP long term though.
> >> >
> >> > This series proposes that we relax our requirements around fine grained
> >> > QAPI data design,
> >> 
> >> Specifics?  QMP command returns a string, HMP wrapper prints that
> >> string?
> >
> > yes, a command returning a single opaque printf formatted string would
> > be the common case.  At a more general POV though, the JSON doc received
> > by the client should be usable "as received", immediately after JSON
> > de-serialization without needing any further custom parsing on top.
> >
> > ie if a value needs to be parsed by the client, then it must be split
> > into multiple distinct values in the QAPI data type design to remove
> > the need for parsing by the client. 
> 
> Yes, that's QMP doctrine.
> 
> > If a command's design violates that, then it must remain under the
> > "x-" prefix.  "info registers" is a example because we're printf
> > formatting each register value into a opaque string. Any client
> > needing a specific register value would have to scanf parse this
> > string. The justification for not representing each register as
> > a distinct QAPI field is that we don't think machines genuinely
> > need to parse this, as its just adhoc human operator debug info.
> > So we take the easy option and just printf to a string and put
> > it under "x-" prefix
> 
> Got it.
> 
> Limitations:
> 
> 1. If we convert a long-running HMP command to this technique, we print
>    its output only after it completed its work.  We also end up with a
>    long-running QMP command, which is bad, because it stops the main
>    loop and makes the QMP monitor unresponsive (except for OOB commands,
>    if the client is careful).  The former can be mitigated with
>    'coroutine': true.  The latter can't.

I'm essentially ignoring this problem because it already exists
in QMP, because anyone needing a HMP-only command today will be
running it via the QMP 'human-monitor-command' and thus have the
behaviour you describe.

> 2. We can't prompt for input.
> 
>    The only current use I can see is HMP "change vnc passwd" prompting
>    for a password.  Except you currently have to say "change vnc passwd
>    wtf" to get it to prompt (suspect logic error in commit cfb5387a1de).

Prompting just needs to be killed off. The VNC code supports the 'secret'
object type now, in the same way as the rest of the QEMU codebase that
needs passwords. So we just need a way to tell VNC the QOM ID of a new
"secret" instance instead.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] qapi: introduce x-query-registers QMP command

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This is a counterpart to the HMP "info registers" command. It is being
> added with an "x-" prefix because this QMP command is intended as an
> adhoc debugging tool and will thus not be modelled in QAPI as fully
> structured data, nor will it have long term guaranteed stability.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++
>  qapi/machine.json          | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 216fdfaf3a..0d9943ff60 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -204,3 +204,31 @@ MemdevList *qmp_query_memdev(Error **errp)
>      object_child_foreach(obj, query_memdev, &list);
>      return list;
>  }
> +
> +RegisterInfo *qmp_x_query_registers(bool has_cpu, int64_t cpu, Error **errp)
> +{
> +    RegisterInfo *info = g_new0(RegisterInfo, 1);
> +    g_autoptr(GString) buf = g_string_new("");
> +    CPUState *cs = NULL, *tmp;
> +
> +    if (has_cpu) {
> +        CPU_FOREACH(tmp) {
> +            if (cpu == tmp->cpu_index) {
> +                cs = tmp;
> +            }
> +        }
> +        if (!cs) {
> +            error_setg(errp, "CPU %"PRId64" not available", cpu);
> +            return NULL;
> +        }
> +        cpu_format_state(cs, buf, CPU_DUMP_FPU);
> +    } else {
> +        CPU_FOREACH(cs) {
> +            g_string_append_printf(buf, "\nCPU#%d\n", cs->cpu_index);
> +            cpu_format_state(cs, buf, CPU_DUMP_FPU);
> +        }
> +    }
> +
> +    info->state = g_steal_pointer(&buf->str);
> +    return info;
> +}
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 157712f006..27b922f2ce 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1312,3 +1312,40 @@
>       '*cores': 'int',
>       '*threads': 'int',
>       '*maxcpus': 'int' } }
> +
> +##
> +# @RegisterParams:
> +#
> +# Information about the CPU to query state of
> +#
> +# @cpu: the CPU number to query. If omitted, queries all CPUs
> +#
> +# Since: 6.2.0
> +#
> +##
> +{ 'struct': 'RegisterParams', 'data': {'*cpu': 'int' } }
> +
> +##
> +# @RegisterInfo:
> +#
> +# Information about the CPU state
> +#
> +# @state: the CPU state in an architecture specific format
> +#
> +# Since: 6.2.0
> +#
> +##
> +{ 'struct': 'RegisterInfo', 'data': {'state': 'str' } }
> +
> +##
> +# @x-query-registers:
> +#
> +# Return information on the CPU registers
> +#
> +# Returns: the CPU state
> +#
> +# Since: 6.2.0
> +##
> +{ 'command': 'x-query-registers',
> +  'data': 'RegisterParams',

Unless you have further uses of RegisterParams in mind, use an implicit
type:

     'data': { '*cpu': 'int' } }

> +  'returns': 'RegisterInfo' }

I'd like us to adopt a convention for commands returning formatted text
for human consumption.  Like so:

     'returns': 'HumanReadableText' }

with the obvious

   ##
   # @HumanReadableText:
   #
   # @human-readable-text: Formatted output intended for humans.
   #
   # Since: 6.2.0
   ##
   { 'struct': 'HumanReadableText',
     'data': { 'human-readable-text': 'str' } }

When the output needs explaining, do that in the command's doc string.
I figure

   ##
   # @x-query-registers:
   #
   # Returns information about the CPU state
   #
   # Returns: CPU state in an architecture-specific format
   #
   # Since: 6.2.0
   ##

would do in this case.

Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Traditionally we have required that newly added QMP commands will model
> any returned data using fine grained QAPI types. This is good for
> commands that are intended to be consumed by machines, where clear data
> representation is very important. Commands that don't satisfy this have
> generally been added to HMP only.
>
> In effect the decision of whether to add a new command to QMP vs HMP has
> been used as a proxy for the decision of whether the cost of designing a
> fine grained QAPI type is justified by the potential benefits.
>
> As a result the commands present in QMP and HMP are non-overlapping
> sets, although HMP comamnds can be accessed indirectly via the QMP
> command 'human-monitor-command'.
>
> One of the downsides of 'human-monitor-command' is that the QEMU monitor
> APIs remain tied into various internal parts of the QEMU code. For
> example any exclusively HMP command will need to use 'monitor_printf'
> to get data out. It would be desirable to be able to fully isolate the
> monitor implementation from QEMU internals, however, this is only
> possible if all commands are exclusively based on QAPI with direct
> QMP exposure.
>
> The way to achieve this desired end goal is to finese the requirements
> for QMP command design. For cases where the output of a command is only
> intended for human consumption, it is reasonable to want to simplify
> the implementation by returning a plain string containing formatted
> data instead of designing a fine grained QAPI data type. This can be
> permitted if-and-only-if the command is exposed under the 'x-' name
> prefix. This indicates that the command data format is liable to
> future change and that it is not following QAPI design best practice.
>
> The poster child example for this would be the 'info registers' HMP
> command which returns printf formatted data representing CPU state.
> This information varies enourmously across target architectures and
> changes relatively frequently as new CPU features are implemented.
> It is there as debugging data for human operators, and any machine
> usage would treat it as an opaque blob. It is thus reasonable to
> expose this in QMP as 'x-query-registers' returning a 'str' field.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/docs/devel/writing-qmp-commands.rst b/docs/devel/writing-qmp-commands.rst
> index 6a10a06c48..d032daa62d 100644
> --- a/docs/devel/writing-qmp-commands.rst
> +++ b/docs/devel/writing-qmp-commands.rst
> @@ -350,6 +350,31 @@ In this section we will focus on user defined types. Please, check the QAPI
>  documentation for information about the other types.
>  
>  
> +Modelling data in QAPI
> +~~~~~~~~~~~~~~~~~~~~~~

Outline now:

    How to write QMP commands using the QAPI framework
        Overview
        Testing
        Writing a command that doesn't return data
            Arguments
            Errors
            Command Documentation
            Implementing the HMP command
        Writing a command that returns data
-->         Modelling data in QAPI
            User Defined Types
            The HMP command
            Returning Lists

Awkward.  I guess you wanted it next to "User Defined Types", which
makes some sense.

Perhaps minor tweaks (headlines, maybe a bit of text as well) would
suffice:

    How to write QMP commands using the QAPI framework
        Overview
        Testing
        Writing a simple command: hello-world
            Arguments
            Errors
            Command Documentation
            Implementing the HMP command
        Writing more complex commands
            Modelling data in QAPI
            User Defined Types
            The HMP command
            Returning Lists

> +
> +For a QMP command that to be considered stable and supported long term there

"term, there"

> +is a requirement returned data should be explicitly modelled using fine grained

"fine-grained", I think.

> +QAPI types. As a general guide, a caller of the QMP command should never need
> +to parse individual returned data fields. If a field appears to need parsing,
> +them it should be split into separate fields corresponding to each distinct
> +data item. This should be the common case for any new QMP command that is
> +intended to be used by machines, as opposed to exclusively human operators.
> +
> +Some QMP commands, however, are only intended as adhoc debugging aids for human
> +operators. While they may return large amounts of formatted data, it is not
> +expected that machines will need to parse the result. The overhead of defining
> +a fine grained QAPI type for the data may not be justified by the potential
> +benefit. In such cases, it is permitted to have a command return a simple string

There are many existing long lines in this file, so I'm not flagging
yours, except for this one, because it increases the maximum.

> +that contains formatted data, however, it is mandatory for the command to use
> +the 'x-' name prefix. This indicates that the command is not guaranteed to be
> +long term stable / liable to change in future and is not following QAPI design
> +best practices. An example where this approach is taken is the QMP command
> +"x-query-registers". This returns a printf formatted dump of the architecture

Drop "printf".

> +specific CPU state. The way the data is formatted varies across QEMU targets,
> +is liable to change over time, and is only intended to be consumed as an opaque
> +string by machines.
> +
>  User Defined Types
>  ~~~~~~~~~~~~~~~~~~

This file is a tutorial.  It teaches command writing by examples[*].  I
think it should also teach this new class of "ad hoc" QMP commands the
same way.  A section "Writing a debugging aid returning unstructured
text" could go at the very end.


[*] Bit-rotten in places, but that's life for docs.

Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state

[Daniel P. Berrangé]

On Wed, Sep 08, 2021 at 11:06:33PM +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 08, 2021 at 01:05:13PM -0500, Eric Blake wrote:
> > On Wed, Sep 08, 2021 at 11:37:09AM +0100, Daniel P. Berrangé wrote:
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  target/i386/cpu-dump.c | 325 ++++++++++++++++++++++-------------------
> > >  target/i386/cpu.c      |   2 +-
> > >  target/i386/cpu.h      |   2 +-
> > >  3 files changed, 174 insertions(+), 155 deletions(-)
> > > 
> > > diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
> > > index 02b635a52c..8e19485a20 100644
> > > --- a/target/i386/cpu-dump.c
> > > +++ b/target/i386/cpu-dump.c
> > > @@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = {
> > >  };
> > >  
> > >  static void
> > > -cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f,
> > > +cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf,
> > >                         const char *name, struct SegmentCache *sc)
> > >  {
> > >  #ifdef TARGET_X86_64
> > >      if (env->hflags & HF_CS64_MASK) {
> > > -        qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
> > > -                     sc->selector, sc->base, sc->limit,
> > > -                     sc->flags & 0x00ffff00);
> > > +        g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name,
> > > +                               sc->selector, sc->base, sc->limit,
> > > +                               sc->flags & 0x00ffff00);
> > 
> > Did you consider using open_memstream() to get a FILE* that can then
> > be passed into these callbacks unchanged, rather than rewriting all
> > the callbacks to a new signature?
> 
> That is certainly an option, but it wouldn't eliminate the need to do
> a rewrite. I would still want to replace qemu_fprintf with fprintf in
> that scenario. It is desirable to be able to eliminate the QEMU
> specific printf wrappers which only exist because they need to treat
> a NULL FILE* object as an indication to output to the HMP chardev.
> Admittedly that would result in shorter lines than today.
> 
> > Then again, I like the GString signature better than FILE*, even if it
> > makes for longer lines.
> 
> Yes, the verbosity is not ideal. I like the GString API as a general
> purpose API for formatting text output to a buffer overall.
> 
> I don't feel too strongly either way though, as long as we get to a place
> where we eliminate the custom QEMU printf wrappers that integrate with
> the monitor APIs.

I forgot that open_memstream is not portable. The portable alternative
is fmemopen but that needs to know the buffer size upfront which is
too unpleasant to use.  So GString is the better portable option.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] qapi: introduce x-query-registers QMP command

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 11:05:20AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This is a counterpart to the HMP "info registers" command. It is being
> > added with an "x-" prefix because this QMP command is intended as an
> > adhoc debugging tool and will thus not be modelled in QAPI as fully
> > structured data, nor will it have long term guaranteed stability.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++
> >  qapi/machine.json          | 37 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> >
> > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> > index 216fdfaf3a..0d9943ff60 100644
> > --- a/hw/core/machine-qmp-cmds.c
> > +++ b/hw/core/machine-qmp-cmds.c
> > @@ -204,3 +204,31 @@ MemdevList *qmp_query_memdev(Error **errp)
> >      object_child_foreach(obj, query_memdev, &list);
> >      return list;
> >  }
> > +
> > +RegisterInfo *qmp_x_query_registers(bool has_cpu, int64_t cpu, Error **errp)
> > +{
> > +    RegisterInfo *info = g_new0(RegisterInfo, 1);
> > +    g_autoptr(GString) buf = g_string_new("");
> > +    CPUState *cs = NULL, *tmp;
> > +
> > +    if (has_cpu) {
> > +        CPU_FOREACH(tmp) {
> > +            if (cpu == tmp->cpu_index) {
> > +                cs = tmp;
> > +            }
> > +        }
> > +        if (!cs) {
> > +            error_setg(errp, "CPU %"PRId64" not available", cpu);
> > +            return NULL;
> > +        }
> > +        cpu_format_state(cs, buf, CPU_DUMP_FPU);
> > +    } else {
> > +        CPU_FOREACH(cs) {
> > +            g_string_append_printf(buf, "\nCPU#%d\n", cs->cpu_index);
> > +            cpu_format_state(cs, buf, CPU_DUMP_FPU);
> > +        }
> > +    }
> > +
> > +    info->state = g_steal_pointer(&buf->str);
> > +    return info;
> > +}
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 157712f006..27b922f2ce 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1312,3 +1312,40 @@
> >       '*cores': 'int',
> >       '*threads': 'int',
> >       '*maxcpus': 'int' } }
> > +
> > +##
> > +# @RegisterParams:
> > +#
> > +# Information about the CPU to query state of
> > +#
> > +# @cpu: the CPU number to query. If omitted, queries all CPUs
> > +#
> > +# Since: 6.2.0
> > +#
> > +##
> > +{ 'struct': 'RegisterParams', 'data': {'*cpu': 'int' } }
> > +
> > +##
> > +# @RegisterInfo:
> > +#
> > +# Information about the CPU state
> > +#
> > +# @state: the CPU state in an architecture specific format
> > +#
> > +# Since: 6.2.0
> > +#
> > +##
> > +{ 'struct': 'RegisterInfo', 'data': {'state': 'str' } }
> > +
> > +##
> > +# @x-query-registers:
> > +#
> > +# Return information on the CPU registers
> > +#
> > +# Returns: the CPU state
> > +#
> > +# Since: 6.2.0
> > +##
> > +{ 'command': 'x-query-registers',
> > +  'data': 'RegisterParams',
> 
> Unless you have further uses of RegisterParams in mind, use an implicit
> type:
> 
>      'data': { '*cpu': 'int' } }

No further usage, so will do this.

> 
> > +  'returns': 'RegisterInfo' }
> 
> I'd like us to adopt a convention for commands returning formatted text
> for human consumption.  Like so:
> 
>      'returns': 'HumanReadableText' }
> 
> with the obvious
> 
>    ##
>    # @HumanReadableText:
>    #
>    # @human-readable-text: Formatted output intended for humans.
>    #
>    # Since: 6.2.0
>    ##
>    { 'struct': 'HumanReadableText',
>      'data': { 'human-readable-text': 'str' } }

Ah yes that's a nice idea that will apply easily for many/most
of the "info xxxx" commands without current QMP equivs.

> When the output needs explaining, do that in the command's doc string.
> I figure
> 
>    ##
>    # @x-query-registers:
>    #
>    # Returns information about the CPU state
>    #
>    # Returns: CPU state in an architecture-specific format
>    #
>    # Since: 6.2.0
>    ##
> 
> would do in this case.

Yep.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 11:33:20AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Traditionally we have required that newly added QMP commands will model
> > any returned data using fine grained QAPI types. This is good for
> > commands that are intended to be consumed by machines, where clear data
> > representation is very important. Commands that don't satisfy this have
> > generally been added to HMP only.
> >
> > In effect the decision of whether to add a new command to QMP vs HMP has
> > been used as a proxy for the decision of whether the cost of designing a
> > fine grained QAPI type is justified by the potential benefits.
> >
> > As a result the commands present in QMP and HMP are non-overlapping
> > sets, although HMP comamnds can be accessed indirectly via the QMP
> > command 'human-monitor-command'.
> >
> > One of the downsides of 'human-monitor-command' is that the QEMU monitor
> > APIs remain tied into various internal parts of the QEMU code. For
> > example any exclusively HMP command will need to use 'monitor_printf'
> > to get data out. It would be desirable to be able to fully isolate the
> > monitor implementation from QEMU internals, however, this is only
> > possible if all commands are exclusively based on QAPI with direct
> > QMP exposure.
> >
> > The way to achieve this desired end goal is to finese the requirements
> > for QMP command design. For cases where the output of a command is only
> > intended for human consumption, it is reasonable to want to simplify
> > the implementation by returning a plain string containing formatted
> > data instead of designing a fine grained QAPI data type. This can be
> > permitted if-and-only-if the command is exposed under the 'x-' name
> > prefix. This indicates that the command data format is liable to
> > future change and that it is not following QAPI design best practice.
> >
> > The poster child example for this would be the 'info registers' HMP
> > command which returns printf formatted data representing CPU state.
> > This information varies enourmously across target architectures and
> > changes relatively frequently as new CPU features are implemented.
> > It is there as debugging data for human operators, and any machine
> > usage would treat it as an opaque blob. It is thus reasonable to
> > expose this in QMP as 'x-query-registers' returning a 'str' field.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)


> > +QAPI types. As a general guide, a caller of the QMP command should never need
> > +to parse individual returned data fields. If a field appears to need parsing,
> > +them it should be split into separate fields corresponding to each distinct
> > +data item. This should be the common case for any new QMP command that is
> > +intended to be used by machines, as opposed to exclusively human operators.
> > +
> > +Some QMP commands, however, are only intended as adhoc debugging aids for human
> > +operators. While they may return large amounts of formatted data, it is not
> > +expected that machines will need to parse the result. The overhead of defining
> > +a fine grained QAPI type for the data may not be justified by the potential
> > +benefit. In such cases, it is permitted to have a command return a simple string
> 
> There are many existing long lines in this file, so I'm not flagging
> yours, except for this one, because it increases the maximum.

This line is at exactly 80 characters so checkstyle is happy with it.
We don't have any requirement for a differenet line limit for docs
afair ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Sep 09, 2021 at 11:33:20AM +0200, Markus Armbruster wrote:

[...]

>> There are many existing long lines in this file, so I'm not flagging
>> yours, except for this one, because it increases the maximum.
>
> This line is at exactly 80 characters so checkstyle is happy with it.
> We don't have any requirement for a differenet line limit for docs
> afair ?

We don't.  I'm asking for a favour.

Humans tend to have trouble following long lines with our eyes (I sure
do).  Typographic manuals suggest to limit columns to roughly 60
characters for exactly that reason[1].

70 is a reasonable compromise between legibility and "writability".  For
code, we use 80-90, because there the actual width should still be fine
due to indentation.

checkpatch.pl doesn't differentiate betwen code and prose.

This file is already hard to read for me.  Please consider not making it
harder.


[1] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style

Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

On Fri, Sep 10, 2021 at 03:45:11PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Sep 09, 2021 at 11:33:20AM +0200, Markus Armbruster wrote:
> 
> [...]
> 
> >> There are many existing long lines in this file, so I'm not flagging
> >> yours, except for this one, because it increases the maximum.
> >
> > This line is at exactly 80 characters so checkstyle is happy with it.
> > We don't have any requirement for a differenet line limit for docs
> > afair ?
> 
> We don't.  I'm asking for a favour.
> 
> Humans tend to have trouble following long lines with our eyes (I sure
> do).  Typographic manuals suggest to limit columns to roughly 60
> characters for exactly that reason[1].
> 
> 70 is a reasonable compromise between legibility and "writability".  For
> code, we use 80-90, because there the actual width should still be fine
> due to indentation.
> 
> checkpatch.pl doesn't differentiate betwen code and prose.
> 
> This file is already hard to read for me.  Please consider not making it
> harder.

Ok, no worries, I just didn't understand the rationale.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|