Re: [PATCH 1/9] hw/isa/vt82c686: QOM'ify Super I/O creation

[BALATON Zoltan <balaton@eik.bme.hu>]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> On Tue, Aug 23, 2022 at 2:35 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>> The object creation now happens in chip-specific init methods which
>>> allows the realize methods to be consolidated into one method. Shifting
>>> the logic into the init methods has the addidional advantage that the
>>> parent object's init methods are called implicitly.
>>
>> This and later patches titled "QOM'ify" don't do what I understand on
>> QOMifying which is turining an old device model into a QOM object. These
>> devices are already QOMified, what's really done here is that they are
>> moved to the ViaISAState or embedded into it and created as part of the
>> south bridge rather then individually by the boards. Either my
>> understanding of what QOMify means is wrong or these patches are misnamed.
>>
>
> I think your understanding is correct. Peter refers to it as the
> embed-the-device-struct style [1] which I can take as inspiration for
> renaming my patches in v2.
>
> Technically via_isa_realize() is the realize method of the abstract
>> TYPE_VIA_ISA class which were overriden by the subclasses but since QOM
>> does not call overridden methods implicitly this had to be explicitly
>> called in the overriding realize methods of the subclasses. Now that we
>> don't have to ovverride the method maybe it could be set once on the
>> VIA_ISA class then it may work that way but as it's done here is also OK
>> maybe as a reminder that this super class method should be called by any
>> overriding method if one's added in the future for some reason.
>>
>
> Right. This would involve moving some code around and creating a class
> struct. Do you think it's worth it?

You mean a class_init func to assign realize as the class struct 
via_isa_info already exists. But yes this would need to be moved after 
via_isa_realize then. As I wrote above I don't think this is worth it, 
especially becuase if in the future a realize method was re-added to the 
subclasses then it's easy to forget to revert this and call the superclass 
method so assigning via_isa_realize to the subclass as done here is OK and 
I can think of it as a reminder to call this method when overriding it. 
Also with the added class_init func it's shorter this way even if slightly 
mixing different objects. So in the end I'm OK with this as it is.

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
>
> [1]
> http://patchwork.ozlabs.org/project/qemu-devel/patch/20220205175913.31738-2-shentey@gmail.com/
>
> Regards,
>> BALATON Zoltan
>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 33 ++++++++++++++++++---------------
>>> 1 file changed, 18 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 8f656251b8..0217c98fe4 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -544,7 +544,7 @@ struct ViaISAState {
>>>     qemu_irq cpu_intr;
>>>     qemu_irq *isa_irqs;
>>>     ISABus *isa_bus;
>>> -    ViaSuperIOState *via_sio;
>>> +    ViaSuperIOState via_sio;
>>> };
>>>
>>> static const VMStateDescription vmstate_via = {
>>> @@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error
>> **errp)
>>>             d->wmask[i] = 0;
>>>         }
>>>     }
>>> +
>>> +    /* Super I/O */
>>> +    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
>>> +        return;
>>> +    }
>>> }
>>>
>>> /* TYPE_VT82C686B_ISA */
>>> @@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d,
>> uint32_t addr,
>>>     pci_default_write_config(d, addr, val, len);
>>>     if (addr == 0x85) {
>>>         /* BIT(1): enable or disable superio config io ports */
>>> -        via_superio_io_enable(s->via_sio, val & BIT(1));
>>> +        via_superio_io_enable(&s->via_sio, val & BIT(1));
>>>     }
>>> }
>>>
>>> @@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
>>>     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
>>> }
>>>
>>> -static void vt82c686b_realize(PCIDevice *d, Error **errp)
>>> +static void vt82c686b_init(Object *obj)
>>> {
>>> -    ViaISAState *s = VIA_ISA(d);
>>> +    ViaISAState *s = VIA_ISA(obj);
>>>
>>> -    via_isa_realize(d, errp);
>>> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
>>> -                                               TYPE_VT82C686B_SUPERIO));
>>> +    object_initialize_child(obj, "sio", &s->via_sio,
>> TYPE_VT82C686B_SUPERIO);
>>> }
>>>
>>> static void vt82c686b_class_init(ObjectClass *klass, void *data)
>>> @@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass *klass,
>> void *data)
>>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>
>>> -    k->realize = vt82c686b_realize;
>>> +    k->realize = via_isa_realize;
>>>     k->config_write = vt82c686b_write_config;
>>>     k->vendor_id = PCI_VENDOR_ID_VIA;
>>>     k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
>>> @@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
>>>     .name          = TYPE_VT82C686B_ISA,
>>>     .parent        = TYPE_VIA_ISA,
>>>     .instance_size = sizeof(ViaISAState),
>>> +    .instance_init = vt82c686b_init,
>>>     .class_init    = vt82c686b_class_init,
>>> };
>>>
>>> @@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d,
>> uint32_t addr,
>>>     pci_default_write_config(d, addr, val, len);
>>>     if (addr == 0x50) {
>>>         /* BIT(2): enable or disable superio config io ports */
>>> -        via_superio_io_enable(s->via_sio, val & BIT(2));
>>> +        via_superio_io_enable(&s->via_sio, val & BIT(2));
>>>     }
>>> }
>>>
>>> @@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
>>>     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
>>> }
>>>
>>> -static void vt8231_realize(PCIDevice *d, Error **errp)
>>> +static void vt8231_init(Object *obj)
>>> {
>>> -    ViaISAState *s = VIA_ISA(d);
>>> +    ViaISAState *s = VIA_ISA(obj);
>>>
>>> -    via_isa_realize(d, errp);
>>> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
>>> -                                               TYPE_VT8231_SUPERIO));
>>> +    object_initialize_child(obj, "sio", &s->via_sio,
>> TYPE_VT8231_SUPERIO);
>>> }
>>>
>>> static void vt8231_class_init(ObjectClass *klass, void *data)
>>> @@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass,
>> void *data)
>>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>
>>> -    k->realize = vt8231_realize;
>>> +    k->realize = via_isa_realize;
>>>     k->config_write = vt8231_write_config;
>>>     k->vendor_id = PCI_VENDOR_ID_VIA;
>>>     k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
>>> @@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
>>>     .name          = TYPE_VT8231_ISA,
>>>     .parent        = TYPE_VIA_ISA,
>>>     .instance_size = sizeof(ViaISAState),
>>> +    .instance_init = vt8231_init,
>>>     .class_init    = vt8231_class_init,
>>> };
>>>
>>>
>>
>