Re: [PATCH] hw/arm/bcm283x: Initialize CPU objects in SoC common DeviceRealize()

[Thomas Huth <thuth@redhat.com>]

On 29/10/2025 15.14, Peter Maydell wrote:
> On Wed, 29 Oct 2025 at 07:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> QOM .instance_init() handler can not fail. QDev DeviceRealize
>> can.
>>
>> The device-introspect QTest enumerates all QDev types and
>> instantiate each one, without realizing it, then introspects
>> the instance properties.
>>
>> When switching to a single QEMU binary, all QDev types are
>> available in the binary, but only a filtered subset might be
>> available, depending on which previous target the binary is
>> trying to mimic.
>>
>> In particular with the Raspi machines, the TYPE_RASPI4B_MACHINE
>> and ARM_CPU_TYPE_NAME("cortex-a72") will be built in the
>> qemu-system-arm binary, while not available (because filtered
>> as being 64-bit, for the qemu-system-aarch64 binary).
>>
>> However the TYPE_BCM2838 SoC is not filtered out, and will
>> abort when being initialized, because the "cortex-a72" CPU type
>> is filtered out, leading to device-introspect failure:
>>
>>    1/1 qemu:qtest+qtest-arm / qtest-arm/device-introspect-test        ERROR            2.46s   killed by signal 6 SIGABRT
>>    stderr:
>>    unknown type 'cortex-a72-arm-cpu'
>>    Broken pipe
>>    ../../tests/qtest/libqtest.c:199: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
>>    (test program exited with status code -6)
>>    TAP parsing error: Too few tests run (expected 167, got 5)
>>
>> In order to avoid that, move the CPU *initialization* in the
>> SoC DeviceRealize handler, so the SoC initialization won't
>> fail, while realization still will.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/bcm2836.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
>> index cd61ba15054..6e4066f137d 100644
>> --- a/hw/arm/bcm2836.c
>> +++ b/hw/arm/bcm2836.c
>> @@ -25,12 +25,7 @@ static void bcm283x_base_init(Object *obj)
>>   {
>>       BCM283XBaseState *s = BCM283X_BASE(obj);
>>       BCM283XBaseClass *bc = BCM283X_BASE_GET_CLASS(obj);
>> -    int n;
>>
>> -    for (n = 0; n < bc->core_count; n++) {
>> -        object_initialize_child(obj, "cpu[*]", &s->cpu[n].core,
>> -                                bc->cpu_type);
>> -    }
>>       if (bc->core_count > 1) {
>>           qdev_property_add_static(DEVICE(obj), &bcm2836_enabled_cores_property);
>>           qdev_prop_set_uint32(DEVICE(obj), "enabled-cpus", bc->core_count);
>> @@ -65,6 +60,11 @@ bool bcm283x_common_realize(DeviceState *dev, BCMSocPeripheralBaseState *ps,
>>       BCM283XBaseClass *bc = BCM283X_BASE_GET_CLASS(dev);
>>       Object *obj;
>>
>> +    for (int n = 0; n < bc->core_count; n++) {
>> +        object_initialize_child(OBJECT(dev), "cpu[*]", &s->cpu[n].core,
>> +                                bc->cpu_type);
>> +    }
>> +
> 
> This seems a bit odd to me. Yes, object instance_init isn't
> allowed to fail. But it's OK for one object to init another
> in its own init method, exactly because of this. And even
> if we do move this, the failure won't cause the realize
> method to fail cleanly, because object_initialize_child()
> doesn't return a failure message.
> 
> The problem as described in the commit message seems to be
> fairly general: we have effectively blacklisted some types
> as "not really creatable", but we haven't got a mechanism for
> propagating that to other types that unconditionally use those.
> Working around this problem by moving child init from
> init to realize in parent classes is going to result in
> a lot of weird parent classes that do work in realize that
> ought to be in init.
> 
> I think we should either:
> (1) find a way to propagate the "this type doesn't really
> exist for this binary" downwards
> (2) allow the "shouldn't really exist types" to be created
> programmatically, but just don't advertise them to the user.

Maybe we could have a DEFINE_TYPES_IF_ARCH(arch, ...) macro that uses
"if (qemu_arch_available(arch)) { ... }" internally, and then replace the 
DEFINE_TYPES() in hw/arm/bcm2836.c with that macro?

  Thomas