Re: [PATCH-for-11.1 03/10] cpus: Introduce CPUClass::legacy_monitor_defs hook

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/23/26 2:19 AM, Philippe Mathieu-Daudé wrote:
> On 21/3/26 20:51, Pierrick Bouvier wrote:
>> 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?
> 
> As a software engineer I prefer to deal with code. Unfortunately in my
> experience with QEMU development, removing legacy code means endless
> bikesheeding debates. I'm not judging they are bad, just they drain my
> time and energy. I agree legacy code should be removed, but again this
> is the QEMU project here, where we have bad habbits of adding new APIs
> with the promise of removing the previous one but never finish API
> conversions, not even marking legacy one. With that in mind, I think
> mentioning "legacy" is still a good first step.
>

I agree, and it's *not* your responsibility to refactor any piece of 
code you have to touch for another purpose. In this case, I was just 
suggesting to make the callback consistent between all targets (i.e. 
force all them to implement it) instead of having a conditional legacy 
callback.

All the rest of the series (symbol renaming, etc) stays intact.

> Anyway, I suppose what you are asking is me starting a poll for each
> target array and ask "Who is still using this SPARC 'canrestore' monitor
> command?" etc., wait 2 months for replies, then if possible deprecate
> them, wait 2 more releases, and eventually remove. Then we'll be able
> to link this file in single binary. As of today I'm not motivated enough
> for that so I'm looking for alternatives. Looking at mailing list
> archives I apparently started to tackle this 9 weeks ago, long enough to
> keep working on the single binary with HMP code disable until we get
> a working proof of concept; then revisit.

I was just curious about your mention "My preference would be to remove 
this legacy code altogether", but no one expects you to do it.
I think having consistent callbacks through all targets is a very 
acceptable solution. The only difference with your series is to rename 
"legacy*" and set a default callback for all other targets.

If it's creating too much friction, I can also hear it and accept your 
current series as it is.

Regards,
Pierrick