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:23, Pierrick Bouvier wrote:
> On 4/18/25 10:10, Philippe Mathieu-Daudé wrote:
>> 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?
>>
> 
> Limit which types are visible from every hw/X, so it's explicit which 
> architecture every board/cpu/device will implement in the end.
> I don't mind if you want to keep it there for now, it doesn't really 
> matter.

Oh, then I think I understood you and we are good:

Instead of having a single include/qemu/target_info-qom.h with
all target definitions, I added the ARM* ones to
include/hw/arm/machines-qom.h. But I kept target_info_types[]
in a single common source file.