Re: [Qemu-devel] [PATCH] hw/sparc64/sun4u: Fix introspection by converting prom instance_init to realize

[Thomas Huth]

On 06.04.2018 16:41, Mark Cave-Ayland wrote:
> On 05/04/18 10:32, Thomas Huth wrote:
> 
>> The instance_init function of devices should always succeed to be able
>> to introspect the device. However, the instance_init function of the
>> "openprom" device can currently fail, for example like this:
>>
>> $ echo "{'execute':'qmp_capabilities'}"\
>>         "{'execute':'device-list-properties',"\
>>         " 'arguments':{'typename':'openprom'}}" \
>>         | sparc64-softmmu/qemu-system-sparc64 -M sun4v,accel=qtest
>> -qmp stdio
>> {"QMP": {"version": {"qemu": {"micro": 91, "minor": 11, "major": 2},
>>   "package": "build-all"}, "capabilities": []}}
>> {"return": {}}
>> RAMBlock "sun4u.prom" already registered, abort!
>> Aborted (core dumped)
>>
>> This should not happen. Fix this problem by moving the affected code from
>> instance_init into a realize function instead.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>   hw/sparc64/sun4u.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 2044a52..d62f5a2 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -425,13 +425,19 @@ static void prom_init(hwaddr addr, const char
>> *bios_name)
>>       }
>>   }
>>   -static void prom_init1(Object *obj)
>> +static void prom_realize(DeviceState *ds, Error **errp)
>>   {
>> -    PROMState *s = OPENPROM(obj);
>> -    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>> +    PROMState *s = OPENPROM(ds);
>> +    SysBusDevice *dev = SYS_BUS_DEVICE(ds);
>> +    Error *local_err = NULL;
>> +
>> +    memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4u.prom",
>> +                                     PROM_SIZE_MAX, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>   -    memory_region_init_ram_nomigrate(&s->prom, obj, "sun4u.prom",
>> PROM_SIZE_MAX,
>> -                           &error_fatal);
>>       vmstate_register_ram_global(&s->prom);
>>       memory_region_set_readonly(&s->prom, true);
>>       sysbus_init_mmio(dev, &s->prom);
>> @@ -446,6 +452,7 @@ static void prom_class_init(ObjectClass *klass,
>> void *data)
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>         dc->props = prom_properties;
>> +    dc->realize = prom_realize;
>>   }
>>     static const TypeInfo prom_info = {
>> @@ -453,7 +460,6 @@ static const TypeInfo prom_info = {
>>       .parent        = TYPE_SYS_BUS_DEVICE,
>>       .instance_size = sizeof(PROMState),
>>       .class_init    = prom_class_init,
>> -    .instance_init = prom_init1,
>>   };
> 
> Looks good to me:
> 
> Reviewed-by: Mark Cave-Ayland <address@hidden>
> 
> I thought this would have been caught by the device introspect test, or
> is this something you've found whilst trying to add sun4v to the list of
> machines to include in the test?

I've found it with this patch here:

https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05033.html

The current device-introspect-test only checks with the "none" machine,
so it did not detect this problem yet since this only happens when you
use the sun4v machine.

 Thomas