Re: [PATCH v3 05/10] hw/isa/vt82c686: Instantiate IDE function in host device

[BALATON Zoltan]

On Wed, 31 Aug 2022, BB wrote:
> Am 31. August 2022 15:12:26 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Wed, 31 Aug 2022, Bernhard Beschow wrote:
> > > The IDE function is closely tied to the ISA function (e.g. the IDE
> > > interrupt routing happens there), so it makes sense that the IDE
> > > function is instantiated within the south bridge itself.
> > >
> > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > ---
> > > configs/devices/mips64el-softmmu/default.mak |  1 -
> > > hw/isa/Kconfig                               |  1 +
> > > hw/isa/vt82c686.c                            | 17 +++++++++++++++++
> > > hw/mips/fuloong2e.c                          |  8 ++++----
> > > hw/ppc/Kconfig                               |  1 -
> > > hw/ppc/pegasos2.c                            |  9 ++++-----
> > > 6 files changed, 26 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/configs/devices/mips64el-softmmu/default.mak 
> > > b/configs/devices/mips64el-softmmu/default.mak
> > > index c610749ac1..d5188f7ea5 100644
> > > --- a/configs/devices/mips64el-softmmu/default.mak
> > > +++ b/configs/devices/mips64el-softmmu/default.mak
> > > @@ -1,7 +1,6 @@
> > > # Default configuration for mips64el-softmmu
> > >
> > > include ../mips-softmmu/common.mak
> > > -CONFIG_IDE_VIA=y
> > > CONFIG_FULOONG=y
> > > CONFIG_LOONGSON3V=y
> > > CONFIG_ATI_VGA=y
> > > diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> > > index d42143a991..20de7e9294 100644
> > > --- a/hw/isa/Kconfig
> > > +++ b/hw/isa/Kconfig
> > > @@ -53,6 +53,7 @@ config VT82C686
> > >     select I8254
> > >     select I8257
> > >     select I8259
> > > +    select IDE_VIA
> > >     select MC146818RTC
> > >     select PARALLEL
> > >
> > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > > index 37e37b3855..9d946cea54 100644
> > > --- a/hw/isa/vt82c686.c
> > > +++ b/hw/isa/vt82c686.c
> > > @@ -17,6 +17,7 @@
> > > #include "hw/isa/vt82c686.h"
> > > #include "hw/pci/pci.h"
> > > #include "hw/qdev-properties.h"
> > > +#include "hw/ide/pci.h"
> > > #include "hw/isa/isa.h"
> > > #include "hw/isa/superio.h"
> > > #include "hw/intc/i8259.h"
> > > @@ -544,6 +545,7 @@ struct ViaISAState {
> > >     qemu_irq cpu_intr;
> > >     qemu_irq *isa_irqs;
> > >     ViaSuperIOState via_sio;
> > > +    PCIIDEState ide;
> > > };
> > >
> > > static const VMStateDescription vmstate_via = {
> > > @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
> > >     }
> > > };
> > >
> > > +static void via_isa_init(Object *obj)
> > > +{
> > > +    ViaISAState *s = VIA_ISA(obj);
> > > +
> > > +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
> > > +}
> > > +
> > > static const TypeInfo via_isa_info = {
> > >     .name          = TYPE_VIA_ISA,
> > >     .parent        = TYPE_PCI_DEVICE,
> > >     .instance_size = sizeof(ViaISAState),
> > > +    .instance_init = via_isa_init,
> >
> > Did you verify this is actually called? I guess you could add a debug printf in 
> > the init method above or check the output of info qom-tree to see if ide apears 
> > below via-isa. I'm not sure because I think QOM does not call superclass 
> > methods if you override them, that's why the subclass realize methods called 
> > via_isa_realize before. In this case it may not cause a problem because ide-via 
> > does not have an init method so it will work with just realize called so the 
> > only effect may be that qom-tree is not like it should be. Or if this is called 
> > then I still don't get QOM.
>
> We discussed the semantics of init() vs. realize() when discussing patch 
> 1 in v1 which consolidates realize() methods. My understanding is that 
> init() behaves like C++ constructors which are called implicitly parent 
> first, child next. OTOH realize() methods behave like virtual methods 
> which get replaced by the most specific one, i.e. one needs to call the 
> parent implementation explicitly.

Indeed, it seems object_init inits all parents recursively. This is a bit 
confusing and maybe not well ecplained in docs/devel/qom.rst at least it 
does not mention instance init func or I missed it but not sure how this 
could be clarified.

Regards,
BALATON Zoltan