Allow targets to register their legacy target_monitor_defs()
in CPUClass; check it first in get_monitor_def() otherwise
fall back to previous per-target helper.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/cpu.h | 3 +++
 monitor/hmp.c         | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 04e1f970caf..072f58bead5 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -148,6 +148,8 @@ struct SysemuCPUOps;
  * @max_as: Maximum valid index used to refer to the address spaces supported by
  *          the architecture, i.e., to refer to CPUAddressSpaces in
  *          CPUState::cpu_ases.
+ * @legacy_monitor_defs: Array of MonitorDef entries. This field is legacy,
+ *                       use @gdb_core_xml_file to dump registers instead.
  *
  * Represents a CPU family or model.
  */
@@ -174,6 +176,7 @@ struct CPUClass {
     const char *gdb_core_xml_file;
     const char * (*gdb_arch_name)(CPUState *cpu);
     const char * (*gdb_get_core_xml_file)(CPUState *cpu);
+    const MonitorDef *legacy_monitor_defs;
 
     void (*disas_set_info)(const CPUState *cpu, disassemble_info *info);
 
diff --git a/monitor/hmp.c b/monitor/hmp.c
index c63da13e310..a2b6269d0ff 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include <dirent.h>
+#include "hw/core/cpu.h"
 #include "hw/core/qdev.h"
 #include "monitor-internal.h"
 #include "monitor/hmp.h"
@@ -1603,8 +1604,8 @@ void monitor_register_hmp_info_hrt(const char *name,
  */
 int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
 {
-    const MonitorDef *md = target_monitor_defs();
     CPUState *cs = mon_get_cpu(mon);
+    const MonitorDef *md = cs->cc->legacy_monitor_defs ?: target_monitor_defs();
     void *ptr;
     uint64_t tmp = 0;
     int ret;
-- 
2.53.0

On 3/20/26 9:08 AM, Philippe Mathieu-Daudé wrote:
> Allow targets to register their legacy target_monitor_defs()
> in CPUClass; check it first in get_monitor_def() otherwise
> fall back to previous per-target helper.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/core/cpu.h | 3 +++
>   monitor/hmp.c         | 3 ++-
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 04e1f970caf..072f58bead5 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -148,6 +148,8 @@ struct SysemuCPUOps;
>    * @max_as: Maximum valid index used to refer to the address spaces supported by
>    *          the architecture, i.e., to refer to CPUAddressSpaces in
>    *          CPUState::cpu_ases.
> + * @legacy_monitor_defs: Array of MonitorDef entries. This field is legacy,
> + *                       use @gdb_core_xml_file to dump registers instead.
>    *
>    * Represents a CPU family or model.
>    */
> @@ -174,6 +176,7 @@ struct CPUClass {
>       const char *gdb_core_xml_file;
>       const char * (*gdb_arch_name)(CPUState *cpu);
>       const char * (*gdb_get_core_xml_file)(CPUState *cpu);
> +    const MonitorDef *legacy_monitor_defs;
>   
>       void (*disas_set_info)(const CPUState *cpu, disassemble_info *info);
>   
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index c63da13e310..a2b6269d0ff 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -24,6 +24,7 @@
>   
>   #include "qemu/osdep.h"
>   #include <dirent.h>
> +#include "hw/core/cpu.h"
>   #include "hw/core/qdev.h"
>   #include "monitor-internal.h"
>   #include "monitor/hmp.h"
> @@ -1603,8 +1604,8 @@ void monitor_register_hmp_info_hrt(const char *name,
>    */
>   int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
>   {
> -    const MonitorDef *md = target_monitor_defs();
>       CPUState *cs = mon_get_cpu(mon);
> +    const MonitorDef *md = cs->cc->legacy_monitor_defs ?: target_monitor_defs();
>       void *ptr;
>       uint64_t tmp = 0;
>       int ret;

I'm not very fond of the "keep legacy callback for the N targets that 
might require it". Either set it for every target, using a default 
implementation if needed, or...

As an alternative, we could use a dispatch function like:

const MonitorDef *target_monitor_defs()
{
   switch (target_arch()) {
   case
     SYS_EMU_TARGET_X:
       return X_target_monitor_defs();
     SYS_EMU_TARGET_Y:
       return Y_target_monitor_defs();
   default:
     return default_target_monitor_defs();
   }
}

Personally, I prefer the dispatch approach, as it's very clear to see 
what happens per target. This solves the symbol duplication by manually 
name them differently.

It's just a suggestion though, and I'm ok to have a per target field, as 
long as we enforce it to be set for all targets, and not using a 
'legacy' callback for a few of them only.

Pick the one you prefer :)

Regards,
Pierrick

On 20/3/26 23:01, Pierrick Bouvier wrote:
> On 3/20/26 9:08 AM, Philippe Mathieu-Daudé wrote:
>> Allow targets to register their legacy target_monitor_defs()
>> in CPUClass; check it first in get_monitor_def() otherwise
>> fall back to previous per-target helper.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/core/cpu.h | 3 +++
>>   monitor/hmp.c         | 3 ++-
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index 04e1f970caf..072f58bead5 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -148,6 +148,8 @@ struct SysemuCPUOps;
>>    * @max_as: Maximum valid index used to refer to the address spaces 
>> supported by
>>    *          the architecture, i.e., to refer to CPUAddressSpaces in
>>    *          CPUState::cpu_ases.
>> + * @legacy_monitor_defs: Array of MonitorDef entries. This field is 
>> legacy,
>> + *                       use @gdb_core_xml_file to dump registers 
>> instead.
>>    *
>>    * Represents a CPU family or model.
>>    */
>> @@ -174,6 +176,7 @@ struct CPUClass {
>>       const char *gdb_core_xml_file;
>>       const char * (*gdb_arch_name)(CPUState *cpu);
>>       const char * (*gdb_get_core_xml_file)(CPUState *cpu);
>> +    const MonitorDef *legacy_monitor_defs;
>>       void (*disas_set_info)(const CPUState *cpu, disassemble_info 
>> *info);
>> diff --git a/monitor/hmp.c b/monitor/hmp.c
>> index c63da13e310..a2b6269d0ff 100644
>> --- a/monitor/hmp.c
>> +++ b/monitor/hmp.c
>> @@ -24,6 +24,7 @@
>>   #include "qemu/osdep.h"
>>   #include <dirent.h>
>> +#include "hw/core/cpu.h"
>>   #include "hw/core/qdev.h"
>>   #include "monitor-internal.h"
>>   #include "monitor/hmp.h"
>> @@ -1603,8 +1604,8 @@ void monitor_register_hmp_info_hrt(const char 
>> *name,
>>    */
>>   int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
>>   {
>> -    const MonitorDef *md = target_monitor_defs();
>>       CPUState *cs = mon_get_cpu(mon);
>> +    const MonitorDef *md = cs->cc->legacy_monitor_defs ?: 
>> target_monitor_defs();
>>       void *ptr;
>>       uint64_t tmp = 0;
>>       int ret;
> 
> I'm not very fond of the "keep legacy callback for the N targets that 
> might require it". Either set it for every target, using a default 
> implementation if needed, or...
> 
> As an alternative, we could use a dispatch function like:
> 
> const MonitorDef *target_monitor_defs()
> {
>    switch (target_arch()) {
>    case
>      SYS_EMU_TARGET_X:
>        return X_target_monitor_defs();
>      SYS_EMU_TARGET_Y:
>        return Y_target_monitor_defs();
>    default:
>      return default_target_monitor_defs();
>    }
> }
> 
> Personally, I prefer the dispatch approach, as it's very clear to see 
> what happens per target. This solves the symbol duplication by manually 
> name them differently.
> 
> It's just a suggestion though, and I'm ok to have a per target field, as 
> long as we enforce it to be set for all targets, and not using a 
> 'legacy' callback for a few of them only.
> 
> Pick the one you prefer :)

My preference would be to remove this legacy code altogether, as
I'm pretty sure it isn't used anymore (see for the example the
issues RISC-V folks had with it before giving up).

On 3/21/26 8:19 AM, Philippe Mathieu-Daudé wrote:
> On 20/3/26 23:01, Pierrick Bouvier wrote:
>> On 3/20/26 9:08 AM, Philippe Mathieu-Daudé wrote:
>>> Allow targets to register their legacy target_monitor_defs()
>>> in CPUClass; check it first in get_monitor_def() otherwise
>>> fall back to previous per-target helper.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    include/hw/core/cpu.h | 3 +++
>>>    monitor/hmp.c         | 3 ++-
>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index 04e1f970caf..072f58bead5 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -148,6 +148,8 @@ struct SysemuCPUOps;
>>>     * @max_as: Maximum valid index used to refer to the address spaces
>>> supported by
>>>     *          the architecture, i.e., to refer to CPUAddressSpaces in
>>>     *          CPUState::cpu_ases.
>>> + * @legacy_monitor_defs: Array of MonitorDef entries. This field is
>>> legacy,
>>> + *                       use @gdb_core_xml_file to dump registers
>>> instead.
>>>     *
>>>     * Represents a CPU family or model.
>>>     */
>>> @@ -174,6 +176,7 @@ struct CPUClass {
>>>        const char *gdb_core_xml_file;
>>>        const char * (*gdb_arch_name)(CPUState *cpu);
>>>        const char * (*gdb_get_core_xml_file)(CPUState *cpu);
>>> +    const MonitorDef *legacy_monitor_defs;
>>>        void (*disas_set_info)(const CPUState *cpu, disassemble_info
>>> *info);
>>> diff --git a/monitor/hmp.c b/monitor/hmp.c
>>> index c63da13e310..a2b6269d0ff 100644
>>> --- a/monitor/hmp.c
>>> +++ b/monitor/hmp.c
>>> @@ -24,6 +24,7 @@
>>>    #include "qemu/osdep.h"
>>>    #include <dirent.h>
>>> +#include "hw/core/cpu.h"
>>>    #include "hw/core/qdev.h"
>>>    #include "monitor-internal.h"
>>>    #include "monitor/hmp.h"
>>> @@ -1603,8 +1604,8 @@ void monitor_register_hmp_info_hrt(const char
>>> *name,
>>>     */
>>>    int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
>>>    {
>>> -    const MonitorDef *md = target_monitor_defs();
>>>        CPUState *cs = mon_get_cpu(mon);
>>> +    const MonitorDef *md = cs->cc->legacy_monitor_defs ?:
>>> target_monitor_defs();
>>>        void *ptr;
>>>        uint64_t tmp = 0;
>>>        int ret;
>>
>> I'm not very fond of the "keep legacy callback for the N targets that
>> might require it". Either set it for every target, using a default
>> implementation if needed, or...
>>
>> As an alternative, we could use a dispatch function like:
>>
>> const MonitorDef *target_monitor_defs()
>> {
>>     switch (target_arch()) {
>>     case
>>       SYS_EMU_TARGET_X:
>>         return X_target_monitor_defs();
>>       SYS_EMU_TARGET_Y:
>>         return Y_target_monitor_defs();
>>     default:
>>       return default_target_monitor_defs();
>>     }
>> }
>>
>> Personally, I prefer the dispatch approach, as it's very clear to see
>> what happens per target. This solves the symbol duplication by manually
>> name them differently.
>>
>> It's just a suggestion though, and I'm ok to have a per target field, as
>> long as we enforce it to be set for all targets, and not using a
>> 'legacy' callback for a few of them only.
>>
>> Pick the one you prefer :)
> 
> My preference would be to remove this legacy code altogether, as
> I'm pretty sure it isn't used anymore (see for the example the
> issues RISC-V folks had with it before giving up).

I'm not sure what you mean.

If it's not used, then we simply remove it.
If it's used, then let's keep it, but just make it consistent with all 
targets without having a special case named "legacy". If it's here to 
stay, it's not legacy.

Do you have something else in mind?