| Message ID | 20231122183048.17150-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: Step toward building qemu-system-{arm, aarch64} altogether | expand |
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? Thomas
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 ---
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
On 23/11/23 17:24, Thomas Huth wrote: > 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. OK. > Anyway, that's just my personal taste - if others agree with your > solution instead, I won't insist on my idea. This is a RFC so let's discuss :) I think there is a need to filter QOM types at runtime (at least in "Single Binary" or "Heterogeneous machines"), but I might be wrong. Maybe we can filter that elsewhere (here seemed the simplest / more natural place). I'll keep looking. Regards, Phil.
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. * @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; + } return type_register_internal(info); }
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(+)