Re: [PATCH 1/2] isa/piix4: Resolve RTCState attribute

[Philippe Mathieu-Daudé]

On 5/2/22 19:53, Peter Maydell wrote:
> On Sat, 5 Feb 2022 at 18:05, Bernhard Beschow <shentey@gmail.com> wrote:
> > Assuming that mc146818_rtc_init() is "syntactic sugar" for code that could
> > be converted into configuration in the future, this patch is a step towards
> > this future by freeing piix4 to care about the inner details of mc146818.
> >
> > Furthermore, by reusing mc146818_rtc_init(), piix4's code becomes more
> > homogenious to other code using the mc146818. So piix4 can be refactored in
> > the same way as code already using mc146818_rtc_init().
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> >   hw/isa/piix4.c | 15 +--------------
> >   1 file changed, 1 insertion(+), 14 deletions(-)
> >
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 0fe7b69bc4..08b4262467 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -46,7 +46,6 @@ struct PIIX4State {
> >       qemu_irq cpu_intr;
> >       qemu_irq *isa;
> >
> > -    RTCState rtc;
> >       /* Reset Control Register */
> >       MemoryRegion rcr_mem;
> >       uint8_t rcr;
> > @@ -193,22 +192,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
> >       i8257_dma_init(isa_bus, 0);
> >
> >       /* RTC */
> > -    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
> > -    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
> > -        return;
> > -    }
> > -    isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ);
> > +    mc146818_rtc_init(isa_bus, 2000, NULL);
> >
> >       piix4_dev = dev;
> >   }
> >
> > -static void piix4_init(Object *obj)
> > -{
> > -    PIIX4State *s = PIIX4_PCI_DEVICE(obj);
> > -
> > -    object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC);
> > -}
> > -
> >   static void piix4_class_init(ObjectClass *klass, void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -233,7 +221,6 @@ static const TypeInfo piix4_info = {
> >       .name          = TYPE_PIIX4_PCI_DEVICE,
> >       .parent        = TYPE_PCI_DEVICE,
> >       .instance_size = sizeof(PIIX4State),
> > -    .instance_init = piix4_init,
> >       .class_init    = piix4_class_init,
> >       .interfaces = (InterfaceInfo[]) {
> >           { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>
> This looks like it's going backwards from the way we'd usually
> write code for devices that contain other devices these days.
> The "we have an init function that does stuff" is the older
> style. The newer style has the inner-device as an embedded
> struct in the container-device struct, which is initialized,
> configured and realized using standard functions like object_initialize
> and qdev_realize. (I do wonder whether that ought to be
> object_initialize_child() here, incidentally, but haven't checked the
> details to be certain.)
>
> The existing uses of mc146818_rtc_init() are mostly in older
> code, and also in board-initialization code, which traditionally
> didn't have a convenient struct to embed the device-struct into.
> hw/isa/vt82c686.c is the only use in another device model
> (which could in theory be refactored to the embed-the-device-struct
> style, though the benefit of making the change isn't large, which
> is one reason we still have the mix of both in the tree).

I agree with Peter. The "future" is to remove the ${device}_init()
helpers. Some contributors are helping toward that goal, but 1/ it
involve work and 2/ there is no coordination; this is a "best effort"
project maintenance.
Maybe we should at least list these 'to be removed' functions as a
bit-sized task in GitLab issues.