Re: [RFC PATCH-for-9.0 01/11] qom: Introduce the TypeInfo::can_register() handler

[Thomas Huth <thuth@redhat.com>]

On 23/11/2023 17.03, Philippe Mathieu-Daudé wrote:
> On 23/11/23 16:09, Thomas Huth wrote:
>> On 22/11/2023 19.30, Philippe Mathieu-Daudé wrote:
>>> Add a helper to decide at runtime whether a type can
>>> be registered to the QOM framework or not.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/qom/object.h | 4 ++++
>>>   qom/object.c         | 3 +++
>>>   2 files changed, 7 insertions(+)
>>>
>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>> index afccd24ca7..0d42fe17de 100644
>>> --- a/include/qom/object.h
>>> +++ b/include/qom/object.h
>>> @@ -372,6 +372,8 @@ struct Object
>>>    * struct TypeInfo:
>>>    * @name: The name of the type.
>>>    * @parent: The name of the parent type.
>>> + * @can_register: This optional function is called before a type is 
>>> registered.
>>> + *   If it exists and returns false, the type is not registered.
>>
>> The second sentence is quite hard to parse, since it is not quite clear 
>> what "it" refers to (type or function) and what "registered" means in this 
>> context (you don't mention type_register() here).
>>
>> Maybe rather something like:
>>
>> If set, type_register() uses this function to decide whether the type can 
>> be registered or not.
>>
>> ?
>>
>>>    * @instance_size: The size of the object (derivative of #Object).  If
>>>    *   @instance_size is 0, then the size of the object will be the size 
>>> of the
>>>    *   parent object.
>>> @@ -414,6 +416,8 @@ struct TypeInfo
>>>       const char *name;
>>>       const char *parent;
>>> +    bool (*can_register)(void);
>>> +
>>>       size_t instance_size;
>>>       size_t instance_align;
>>>       void (*instance_init)(Object *obj);
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 95c0dc8285..f09b6b5a92 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -150,6 +150,9 @@ static TypeImpl *type_register_internal(const 
>>> TypeInfo *info)
>>>   TypeImpl *type_register(const TypeInfo *info)
>>>   {
>>>       assert(info->parent);
>>> +    if (info->can_register && !info->can_register()) {
>>> +        return NULL;
>>> +    }
>>
>> I have to say that I don't like it too much, since you're trying to fix a 
>> problem here in common code that clearly belongs to the code in hw/arm/ 
>> instead.
>>
>> What about dropping it, and changing your last patch to replace the 
>> DEFINE_TYPES(raspi_machine_types) in hw/arm/raspi.c with your own 
>> implementation of type_register_static_array() that checks the condition 
>> there?
> 
> This isn't ARM specific, it happens I started to unify ARM/aarch64
> binaries.
> 
> Types can be registered depending on build-time (config/host specific)
> definitions and runtime ones. How can we check for runtime if not via
> this simple helper?
> 
> Still ARM, but as example what I have then is (module meson):
...
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 1e9c6c85ae..c3b7e5666c 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -744,7 +744,8 @@ static void aarch64_max_initfn(Object *obj)
>   static const ARMCPUInfo aarch64_cpus[] = {
>       { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
>       { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> -    { .name = "max",                .initfn = aarch64_max_initfn },
> +    { .name = "max",                .initfn = aarch64_max_initfn,
> +                                    .can_register = 
> target_aarch64_available },
>   #if defined(CONFIG_KVM) || defined(CONFIG_HVF)
>       { .name = "host",               .initfn = aarch64_host_initfn },
>   #endif

Picking this one as an example, I think I'd rather modify the for-loop in
aarch64_cpu_register_types() to check for the availability there... sounds 
much easier to understand for me than having a callback function.

Anyway, that's just my personal taste - if others agree with your solution 
instead, I won't insist on my idea.

  Thomas