Re: [RFC PATCH v2 04/11] hw/arm: Register TYPE_TARGET_ARM/AARCH64_CPU QOM interfaces

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 18/4/25 19:07, Pierrick Bouvier wrote:
> On 4/18/25 10:04, Philippe Mathieu-Daudé wrote:
>> On 18/4/25 18:30, Pierrick Bouvier wrote:
>>> On 4/18/25 07:07, Philippe Mathieu-Daudé wrote:
>>>> On 18/4/25 05:07, Pierrick Bouvier wrote:
>>>>> On 4/17/25 17:50, Philippe Mathieu-Daudé wrote:
>>>>>> Define the TYPE_TARGET_ARM_MACHINE and TYPE_TARGET_AARCH64_MACHINE
>>>>>> QOM interface names to allow machines to implement them.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>>     meson.build                    |  1 +
>>>>>>     include/hw/boards.h            |  1 +
>>>>>>     include/qemu/target_info-qom.h | 18 ++++++++++++++++++
>>>>>>     target_info-qom.c              | 24 ++++++++++++++++++++++++
>>>>>>     4 files changed, 44 insertions(+)
>>>>>>     create mode 100644 include/qemu/target_info-qom.h
>>>>>>     create mode 100644 target_info-qom.c
>>>>>>
>>>>>> diff --git a/meson.build b/meson.build
>>>>>> index 49a050a28a3..168b07b5887 100644
>>>>>> --- a/meson.build
>>>>>> +++ b/meson.build
>>>>>> @@ -3808,6 +3808,7 @@ common_ss.add(pagevary)
>>>>>>     specific_ss.add(files('page-target.c', 'page-vary-target.c'))
>>>>>>     common_ss.add(files('target_info.c'))
>>>>>> +system_ss.add(files('target_info-qom.c'))
>>>>>>     specific_ss.add(files('target_info-stub.c'))
>>>>>>     subdir('backends')
>>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>>>> index 02f43ac5d4d..b1bbf3c34d4 100644
>>>>>> --- a/include/hw/boards.h
>>>>>> +++ b/include/hw/boards.h
>>>>>> @@ -7,6 +7,7 @@
>>>>>>     #include "system/hostmem.h"
>>>>>>     #include "system/blockdev.h"
>>>>>>     #include "qapi/qapi-types-machine.h"
>>>>>> +#include "qemu/target_info-qom.h"
>>>>>>     #include "qemu/module.h"
>>>>>>     #include "qom/object.h"
>>>>>>     #include "hw/core/cpu.h"
>>>>>> diff --git a/include/qemu/target_info-qom.h b/include/qemu/
>>>>>> target_info-qom.h
>>>>>> new file mode 100644
>>>>>> index 00000000000..7eb9b6bd254
>>>>>> --- /dev/null
>>>>>> +++ b/include/qemu/target_info-qom.h
>>>>>> @@ -0,0 +1,18 @@
>>>>>> +/*
>>>>>> + * QEMU binary/target API (QOM types)
>>>>>> + *
>>>>>> + *  Copyright (c) Linaro
>>>>>> + *
>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef QEMU_TARGET_INFO_QOM_H
>>>>>> +#define QEMU_TARGET_INFO_QOM_H
>>>>>> +
>>>>>> +#define TYPE_TARGET_ARM_MACHINE \
>>>>>> +        "target-info-arm-machine"
>>>>>> +
>>>>>> +#define TYPE_TARGET_AARCH64_MACHINE \
>>>>>> +        "target-info-aarch64-machine"
>>>>>> +
>>>>>> +#endif
>>>>>> diff --git a/target_info-qom.c b/target_info-qom.c
>>>>>> new file mode 100644
>>>>>> index 00000000000..d3fee57361b
>>>>>> --- /dev/null
>>>>>> +++ b/target_info-qom.c
>>>>>> @@ -0,0 +1,24 @@
>>>>>> +/*
>>>>>> + * QEMU binary/target API (QOM types)
>>>>>> + *
>>>>>> + *  Copyright (c) Linaro
>>>>>> + *
>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>> + */
>>>>>> +
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qemu/target_info-qom.h"
>>>>>> +#include "qom/object.h"
>>>>>> +
>>>>>> +static const TypeInfo target_info_types[] = {
>>>>>> +    {
>>>>>> +        .name           = TYPE_TARGET_ARM_MACHINE,
>>>>>> +        .parent         = TYPE_INTERFACE,
>>>>>> +    },
>>>>>> +    {
>>>>>> +        .name           = TYPE_TARGET_AARCH64_MACHINE,
>>>>>> +        .parent         = TYPE_INTERFACE,
>>>>>> +    },
>>>>>> +};
>>>>>> +
>>>>>> +DEFINE_TYPES(target_info_types)
>>>>>
>>>>> Ideally, this should be in target/arm, as this type will only be 
>>>>> used by
>>>>> boards in hw/arm, and by the target_info specialization.
>>>>
>>>> Not the way QOM works, interfaces must be registered, which is
>>>> why I use this centralized file. Otherwise we get:
>>>>
>>>> $ qemu-system-sh4 -M help
>>>> qemu-system-sh4: -M help: missing interface 'target-info-arm-machine'
>>>> for object 'machine'
>>>>
>>>
>>> If I'm correct, types can be registered anywhere, since they rely on
>>> static initializer, but in qemu-system-sh4, hw/arm or target/arm is not
>>> linked, so it fails.
>>> I guess this is the null board that is creating this situation, since
>>> it's included by all binaries.
>>
>> Correct.
>>
>>> I see two solutions while still moving those types in target/arm:
>>> - include this type file in libcommon, so it's always linked.
>>> - introduce a new TYPE_TARGET_ALL_MACHINE, and always include it in list
>>> of machines. But I think it's not so good as it would require to deal
>>> with a list of types when we want to access a machine.
>>>
>>> So I would just move the file and link it inconditonnally in all 
>>> binaries.
>>
>> Which file? target_info-qom.c is already in system_ss[].
> 
> Move existing target_info-qom in in target/arm/target_info_types.c, and 
> add it to system_ss, instead of arm_ss or arm_common_ss, so it's always 
> linked.

That should work, but what is the benefit over having a single file
where all definitions are concentrated?