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

[Philippe Mathieu-Daudé]

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):

-- >8 --
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8f033f59..e7f566f05d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1102,6 +1102,7 @@ typedef struct ARMCPUInfo {
     const char *name;
     void (*initfn)(Object *obj);
     void (*class_init)(ObjectClass *oc, void *data);
+    bool (*can_register)(void);
 } ARMCPUInfo;

 /**
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 558a85e6d7..109fb160b5 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2512,6 +2512,7 @@ void arm_cpu_register(const ARMCPUInfo *info)
         .instance_init = arm_cpu_instance_init,
         .class_init = info->class_init ?: cpu_register_class_init,
         .class_data = (void *)info,
+        .can_register = info->can_register,
     };

     type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
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
diff --git a/target/arm/tcg/cpu32.c b/target/arm/tcg/cpu32.c
index d9e0e2a4dd..dcad399a60 100644
--- a/target/arm/tcg/cpu32.c
+++ b/target/arm/tcg/cpu32.c
@@ -1049,7 +1049,6 @@ static void arm_v7m_class_init(ObjectClass *oc, 
void *data)
     cc->gdb_core_xml_file = "arm-m-profile.xml";
 }

-#ifndef TARGET_AARCH64
 /*
  * -cpu max: a CPU with as many features enabled as our emulation 
supports.
  * The version of '-cpu max' for qemu-system-aarch64 is defined in 
cpu64.c;
@@ -1112,7 +1111,11 @@ static void arm_max_initfn(Object *obj)
     cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);
 #endif
 }
-#endif /* !TARGET_AARCH64 */
+
+static bool aa32_only(void)
+{
+    return !target_aarch64_available();
+}

 static const ARMCPUInfo arm_tcg_cpus[] = {
     { .name = "arm926",      .initfn = arm926_initfn },
@@ -1162,9 +1165,8 @@ static const ARMCPUInfo arm_tcg_cpus[] = {
     { .name = "pxa270-b1",   .initfn = pxa270b1_initfn },
     { .name = "pxa270-c0",   .initfn = pxa270c0_initfn },
     { .name = "pxa270-c5",   .initfn = pxa270c5_initfn },
-#ifndef TARGET_AARCH64
-    { .name = "max",         .initfn = arm_max_initfn },
-#endif
+    { .name = "max",         .initfn = arm_max_initfn,
+                             .can_register = aa32_only },
 #ifdef CONFIG_USER_ONLY
     { .name = "any",         .initfn = arm_max_initfn },
 #endif
---