[PATCH 0/2] monitor/hmp: Reduce target-specific definitions

[PATCH 0/2] monitor/hmp: Reduce target-specific definitions

[Philippe Mathieu-Daudé]

Move most of target-agnostic definitions from
"monitor/hmp-target.h" to "monitor/hmp.h" to
reduce files target-poisoning.

Philippe Mathieu-Daudé (2):
  target/i386: Include missing 'svm.h' header in 'sev.h'
  monitor/hmp: Reduce target-specific definitions

 include/monitor/hmp-target.h  | 28 +++++-----------------------
 include/monitor/hmp.h         | 23 +++++++++++++++++++++++
 target/i386/sev.h             |  2 ++
 hw/i386/sgx-stub.c            |  2 +-
 hw/i386/sgx.c                 |  1 -
 monitor/hmp-cmds.c            |  1 -
 stubs/target-monitor-defs.c   |  2 +-
 target/i386/cpu-apic.c        |  2 +-
 target/i386/sev-system-stub.c |  2 +-
 target/i386/sev.c             |  1 -
 target/m68k/monitor.c         |  1 +
 target/riscv/monitor.c        |  1 +
 target/sh4/monitor.c          |  1 -
 target/xtensa/monitor.c       |  1 -
 14 files changed, 36 insertions(+), 32 deletions(-)

-- 
2.52.0

[PATCH 1/2] target/i386: Include missing 'svm.h' header in 'sev.h'

[Philippe Mathieu-Daudé]

"target/i386/sev.h" uses the vmcb_seg structure type, which
is defined in "target/i386/svm.h". Include the latter, otherwise
the following patch triggers:

  ../target/i386/sev.h:62:21: error: field has incomplete type 'struct vmcb_seg'
     62 |     struct vmcb_seg es;
        |                     ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/sev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/sev.h b/target/i386/sev.h
index 9db1a802f6b..4358df40e48 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -14,6 +14,8 @@
 #ifndef I386_SEV_H
 #define I386_SEV_H
 
+#include "target/i386/svm.h"
+
 #ifndef CONFIG_USER_ONLY
 #include CONFIG_DEVICES /* CONFIG_SEV */
 #endif
-- 
2.52.0

[PATCH 2/2] monitor/hmp: Reduce target-specific definitions

[Philippe Mathieu-Daudé]

From "monitor/hmp-target.h", only the MonitorDef structure
is target specific (by using the 'target_long' type). All
the rest (even target_monitor_defs and target_get_monitor_def)
can be exposed to target-agnostic units, allowing to build
some of them in meson common source set.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/monitor/hmp-target.h  | 28 +++++-----------------------
 include/monitor/hmp.h         | 23 +++++++++++++++++++++++
 hw/i386/sgx-stub.c            |  2 +-
 hw/i386/sgx.c                 |  1 -
 monitor/hmp-cmds.c            |  1 -
 stubs/target-monitor-defs.c   |  2 +-
 target/i386/cpu-apic.c        |  2 +-
 target/i386/sev-system-stub.c |  2 +-
 target/i386/sev.c             |  1 -
 target/m68k/monitor.c         |  1 +
 target/riscv/monitor.c        |  1 +
 target/sh4/monitor.c          |  1 -
 target/xtensa/monitor.c       |  1 -
 13 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index b679aaebbff..d39d8c8abe1 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -25,9 +25,12 @@
 #ifndef MONITOR_HMP_TARGET_H
 #define MONITOR_HMP_TARGET_H
 
-typedef struct MonitorDef MonitorDef;
+#include "monitor/hmp.h"
+
+#ifndef COMPILING_PER_TARGET
+#error hmp-target.h included from common code
+#endif
 
-#ifdef COMPILING_PER_TARGET
 #include "cpu.h"
 struct MonitorDef {
     const char *name;
@@ -36,29 +39,8 @@ struct MonitorDef {
                              int val);
     int type;
 };
-#endif
 
 #define MD_TLONG 0
 #define MD_I32   1
 
-const MonitorDef *target_monitor_defs(void);
-int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
-
-CPUArchState *mon_get_cpu_env(Monitor *mon);
-CPUState *mon_get_cpu(Monitor *mon);
-
-void hmp_info_mem(Monitor *mon, const QDict *qdict);
-void hmp_info_tlb(Monitor *mon, const QDict *qdict);
-void hmp_mce(Monitor *mon, const QDict *qdict);
-void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
-void hmp_info_sev(Monitor *mon, const QDict *qdict);
-void hmp_info_sgx(Monitor *mon, const QDict *qdict);
-void hmp_info_via(Monitor *mon, const QDict *qdict);
-void hmp_memory_dump(Monitor *mon, const QDict *qdict);
-void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict);
-void hmp_info_registers(Monitor *mon, const QDict *qdict);
-void hmp_gva2gpa(Monitor *mon, const QDict *qdict);
-void hmp_gpa2hva(Monitor *mon, const QDict *qdict);
-void hmp_gpa2hpa(Monitor *mon, const QDict *qdict);
-
 #endif /* MONITOR_HMP_TARGET_H */
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 83721b5ffc6..48cd8cefe98 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -17,6 +17,29 @@
 #include "qemu/readline.h"
 #include "qapi/qapi-types-common.h"
 
+typedef struct MonitorDef MonitorDef;
+
+const MonitorDef *target_monitor_defs(void);
+
+int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
+
+CPUArchState *mon_get_cpu_env(Monitor *mon);
+CPUState *mon_get_cpu(Monitor *mon);
+
+void hmp_info_mem(Monitor *mon, const QDict *qdict);
+void hmp_info_tlb(Monitor *mon, const QDict *qdict);
+void hmp_mce(Monitor *mon, const QDict *qdict);
+void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
+void hmp_info_sev(Monitor *mon, const QDict *qdict);
+void hmp_info_sgx(Monitor *mon, const QDict *qdict);
+void hmp_info_via(Monitor *mon, const QDict *qdict);
+void hmp_memory_dump(Monitor *mon, const QDict *qdict);
+void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict);
+void hmp_info_registers(Monitor *mon, const QDict *qdict);
+void hmp_gva2gpa(Monitor *mon, const QDict *qdict);
+void hmp_gpa2hva(Monitor *mon, const QDict *qdict);
+void hmp_gpa2hpa(Monitor *mon, const QDict *qdict);
+
 bool hmp_handle_error(Monitor *mon, Error *err);
 void hmp_help_cmd(Monitor *mon, const char *name);
 strList *hmp_split_at_comma(const char *str);
diff --git a/hw/i386/sgx-stub.c b/hw/i386/sgx-stub.c
index d295e54d239..6e82773a86d 100644
--- a/hw/i386/sgx-stub.c
+++ b/hw/i386/sgx-stub.c
@@ -1,6 +1,6 @@
 #include "qemu/osdep.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp-target.h"
+#include "monitor/hmp.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/sgx-epc.h"
 #include "qapi/qapi-commands-misc-i386.h"
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index e2801546ad6..54d2cae36d8 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -16,7 +16,6 @@
 #include "hw/mem/memory-device.h"
 #include "monitor/qdev.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp-target.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qapi/qapi-commands-misc-i386.h"
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index f7ff6ec90ec..1ab789ff468 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -21,7 +21,6 @@
 #include "gdbstub/enums.h"
 #include "monitor/hmp.h"
 #include "qemu/help_option.h"
-#include "monitor/hmp-target.h"
 #include "monitor/monitor-internal.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-control.h"
diff --git a/stubs/target-monitor-defs.c b/stubs/target-monitor-defs.c
index 35a0a342772..0dd4cdb34f6 100644
--- a/stubs/target-monitor-defs.c
+++ b/stubs/target-monitor-defs.c
@@ -1,5 +1,5 @@
 #include "qemu/osdep.h"
-#include "monitor/hmp-target.h"
+#include "monitor/hmp.h"
 
 const MonitorDef *target_monitor_defs(void)
 {
diff --git a/target/i386/cpu-apic.c b/target/i386/cpu-apic.c
index eeee62b52a2..3b73a04597f 100644
--- a/target/i386/cpu-apic.c
+++ b/target/i386/cpu-apic.c
@@ -10,7 +10,7 @@
 #include "qobject/qdict.h"
 #include "qapi/error.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp-target.h"
+#include "monitor/hmp.h"
 #include "system/hw_accel.h"
 #include "system/kvm.h"
 #include "system/xen.h"
diff --git a/target/i386/sev-system-stub.c b/target/i386/sev-system-stub.c
index 7c5c02a5657..f799a338d60 100644
--- a/target/i386/sev-system-stub.c
+++ b/target/i386/sev-system-stub.c
@@ -13,7 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp-target.h"
+#include "monitor/hmp.h"
 #include "qapi/error.h"
 #include "sev.h"
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index fb5a3b5d778..7e2a5df8867 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -36,7 +36,6 @@
 #include "migration/blocker.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp-target.h"
 #include "qapi/qapi-commands-misc-i386.h"
 #include "confidential-guest.h"
 #include "hw/i386/pc.h"
diff --git a/target/m68k/monitor.c b/target/m68k/monitor.c
index 2bdf6acae0a..784f5730919 100644
--- a/target/m68k/monitor.c
+++ b/target/m68k/monitor.c
@@ -7,6 +7,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "monitor/hmp.h"
 #include "monitor/hmp-target.h"
 #include "monitor/monitor.h"
 
diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
index 8a77476db93..478fd392ac6 100644
--- a/target/riscv/monitor.c
+++ b/target/riscv/monitor.c
@@ -22,6 +22,7 @@
 #include "cpu.h"
 #include "cpu_bits.h"
 #include "monitor/monitor.h"
+#include "monitor/hmp.h"
 #include "monitor/hmp-target.h"
 #include "system/memory.h"
 
diff --git a/target/sh4/monitor.c b/target/sh4/monitor.c
index 2da6a5426eb..50324d3600c 100644
--- a/target/sh4/monitor.c
+++ b/target/sh4/monitor.c
@@ -24,7 +24,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 
 static void print_tlb(Monitor *mon, int idx, tlb_t *tlb)
diff --git a/target/xtensa/monitor.c b/target/xtensa/monitor.c
index fbf60d55530..2af84934f83 100644
--- a/target/xtensa/monitor.c
+++ b/target/xtensa/monitor.c
@@ -24,7 +24,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "monitor/monitor.h"
-#include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 
 void hmp_info_tlb(Monitor *mon, const QDict *qdict)
-- 
2.52.0

Re: [PATCH 2/2] monitor/hmp: Reduce target-specific definitions

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> From "monitor/hmp-target.h", only the MonitorDef structure
> is target specific (by using the 'target_long' type). All
> the rest (even target_monitor_defs and target_get_monitor_def)
> can be exposed to target-agnostic units, allowing to build
> some of them in meson common source set.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

The only use of the ->get_value() callback I can see is in
get_monitor_def(), to implement HMP's $register feature.  I can't see
the callback being set.  Is it dead?

Re: [PATCH 2/2] monitor/hmp: Reduce target-specific definitions

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> > From "monitor/hmp-target.h", only the MonitorDef structure
> > is target specific (by using the 'target_long' type). All
> > the rest (even target_monitor_defs and target_get_monitor_def)
> > can be exposed to target-agnostic units, allowing to build
> > some of them in meson common source set.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> The only use of the ->get_value() callback I can see is in
> get_monitor_def(), to implement HMP's $register feature.  I can't see
> the callback being set.  Is it dead?

I think I see that being used in ppc;
target/ppc/ppc-qmp-cmds.c

const MonitorDef monitor_defs[] = {
    { "fpscr", offsetof(CPUPPCState, fpscr) },
    /* Next instruction pointer */
    { "nip|pc", offsetof(CPUPPCState, nip) },
    { "lr", offsetof(CPUPPCState, lr) },
    { "ctr", offsetof(CPUPPCState, ctr) },
    { "decr", 0, &monitor_get_decr, },
    { "ccr|cr", 0, &monitor_get_ccr, },
    /* Machine state register */
    { "xer", 0, &monitor_get_xer },
    { "msr", offsetof(CPUPPCState, msr) },
    { "tbu", 0, &monitor_get_tbu, },
#if defined(TARGET_PPC64)
    { "tb", 0, &monitor_get_tbl, },
#else
    { "tbl", 0, &monitor_get_tbl, },
#endif
    { NULL },
};

those monitor_get_* functions are that get_value() aren't they?

Dave

-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 2/2] monitor/hmp: Reduce target-specific definitions

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>> > From "monitor/hmp-target.h", only the MonitorDef structure
>> > is target specific (by using the 'target_long' type). All
>> > the rest (even target_monitor_defs and target_get_monitor_def)
>> > can be exposed to target-agnostic units, allowing to build
>> > some of them in meson common source set.
>> >
>> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> 
>> The only use of the ->get_value() callback I can see is in
>> get_monitor_def(), to implement HMP's $register feature.  I can't see
>> the callback being set.  Is it dead?
>
> I think I see that being used in ppc;
> target/ppc/ppc-qmp-cmds.c
>
> const MonitorDef monitor_defs[] = {
>     { "fpscr", offsetof(CPUPPCState, fpscr) },
>     /* Next instruction pointer */
>     { "nip|pc", offsetof(CPUPPCState, nip) },
>     { "lr", offsetof(CPUPPCState, lr) },
>     { "ctr", offsetof(CPUPPCState, ctr) },
>     { "decr", 0, &monitor_get_decr, },
>     { "ccr|cr", 0, &monitor_get_ccr, },
>     /* Machine state register */
>     { "xer", 0, &monitor_get_xer },
>     { "msr", offsetof(CPUPPCState, msr) },
>     { "tbu", 0, &monitor_get_tbu, },
> #if defined(TARGET_PPC64)
>     { "tb", 0, &monitor_get_tbl, },
> #else
>     { "tbl", 0, &monitor_get_tbl, },
> #endif
>     { NULL },
> };
>
> those monitor_get_* functions are that get_value() aren't they?

You're right.

target/sparc/monitor.c and target/i386/monitor.c have more.

I prefer .member = initial-value when member is optional.

Thanks!

Re: [PATCH 2/2] monitor/hmp: Reduce target-specific definitions

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

>>From "monitor/hmp-target.h", only the MonitorDef structure
> is target specific (by using the 'target_long' type). All
> the rest (even target_monitor_defs and target_get_monitor_def)
> can be exposed to target-agnostic units, allowing to build
> some of them in meson common source set.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

target_long is only used as return type of MonitorDef method
get_value().  Its only caller is

    int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
    {
        [...]

        for(; md->name != NULL; md++) {
            if (hmp_compare_cmd(name, md->name)) {
                if (md->get_value) {
--->                *pval = md->get_value(mon, md, md->offset);
                } else {

We store the return value in an int64_t.

Change the return type to match?

Re: [PATCH 0/2] monitor/hmp: Reduce target-specific definitions

[Pierrick Bouvier]

On 1/7/26 10:20 AM, Philippe Mathieu-Daudé wrote:
> Move most of target-agnostic definitions from
> "monitor/hmp-target.h" to "monitor/hmp.h" to
> reduce files target-poisoning.
> 
> Philippe Mathieu-Daudé (2):
>    target/i386: Include missing 'svm.h' header in 'sev.h'
>    monitor/hmp: Reduce target-specific definitions
> 
>   include/monitor/hmp-target.h  | 28 +++++-----------------------
>   include/monitor/hmp.h         | 23 +++++++++++++++++++++++
>   target/i386/sev.h             |  2 ++
>   hw/i386/sgx-stub.c            |  2 +-
>   hw/i386/sgx.c                 |  1 -
>   monitor/hmp-cmds.c            |  1 -
>   stubs/target-monitor-defs.c   |  2 +-
>   target/i386/cpu-apic.c        |  2 +-
>   target/i386/sev-system-stub.c |  2 +-
>   target/i386/sev.c             |  1 -
>   target/m68k/monitor.c         |  1 +
>   target/riscv/monitor.c        |  1 +
>   target/sh4/monitor.c          |  1 -
>   target/xtensa/monitor.c       |  1 -
>   14 files changed, 36 insertions(+), 32 deletions(-)
> 

(in private),
build issues:
https://github.com/pbo-linaro/qemu-ci/actions/runs/20792272184/job/59717050408