Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Markus Armbruster]

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

> On 7/8/24 07:12, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> qmp_query_gic_capabilities() is not specific to the ARM
>>> architecture but to the GIC device which is modelled in
>>> hw/intc/, so move the code there for clarity. No logical
>>> change intended.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/intc/arm_gic_qmp.c     | 59 +++++++++++++++++++++++++++++++++++++++
>>>   target/arm/arm-qmp-cmds.c | 52 +---------------------------------
>>>   hw/intc/meson.build       |  1 +
>>>   3 files changed, 61 insertions(+), 51 deletions(-)
>>>   create mode 100644 hw/intc/arm_gic_qmp.c
>> [...]
>> 
>>> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>>> index afd1aa51ee..45d3503d49 100644
>>> --- a/hw/intc/meson.build
>>> +++ b/hw/intc/meson.build
>>> @@ -39,6 +39,7 @@ if config_all_devices.has_key('CONFIG_APIC') or \
>>>   endif
>>>     specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 
>>> 'apic_common.c'))
>>> +specific_ss.add(when: 'CONFIG_ARM', if_true: files('arm_gic_qmp.c'))
>>>   specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: 
>>> files('arm_gicv3_cpuif_common.c'))
>>>   specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: 
>>> files('arm_gicv3_cpuif.c'))
>>>   specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: 
>>> files('arm_gic_kvm.c'))
>> You move qmp_query_gic_capabilities() from target/arm/arm-qmp-cmds.c (in
>> arm_system_ss) to hw/intc/arm_gic_qmp.c (in specific_ss when
>> CONFIG_ARM).
>>
>> Both _ss are target-dependent.  In my testing, both get only compiled
>> for target arm and aarch64.  Obvious for arm-qmp-cmds.c in
>> arm_system_ss.  Less so for arm_gic_qmp.c in specific_ss; I guess the
>> CONFIG_ARM does the trick there.  Correct?
>
> Correct, Kconfig CONFIG_ARM_GIC depends on Kconfig CONFIG_ARM,
> itself defined for configure TARGET_ARM (see target/arm/Kconfig).
> (Also CONFIG_AARCH64 selects CONFIG_ARM).
>
> I can clarify that in the patch description if necessary.

Wouldn't hurt.  If figure this patch is meant to be just code motion,
but it's not obvious from the patch that the code being moved gets
compiled into the exact same binaries before and after the move, unless
you know how CONFIG_ARM works.