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

[Bernhard Beschow <shentey@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7531 bytes --]

On Wed, Aug 24, 2022 at 1:36 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> 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.


Ah yes, of course.


> 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.
>

Okay, I'll keep it as it is.

Thanks,
Bernhard

>
> 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,
> >>> };
> >>>
> >>>
> >>
> >
>

[-- Attachment #2: Type: text/html, Size: 10745 bytes --]