Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[BALATON Zoltan]

On Thu, 27 Apr 2023, Mark Cave-Ayland wrote:
> On 27/04/2023 08:58, Bernhard Beschow wrote:
> > Am 26. April 2023 12:50:08 UTC schrieb Mark Cave-Ayland 
> > <mark.cave-ayland@ilande.co.uk>:
> > > On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:
> > >
> > > > Since pc_init1() has access to the ISABus*, retrieve the
> > > > ISA IRQs with isa_bus_get_irq().
> > > >
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > ---
> > > >    hw/i386/pc_piix.c | 8 +++++++-
> > > >    1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > index 126b6c11df..1e90b9ff0d 100644
> > > > --- a/hw/i386/pc_piix.c
> > > > +++ b/hw/i386/pc_piix.c
> > > > @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
> > > >        if (pcmc->pci_enabled) {
> > > >            PCIDevice *dev;
> > > >    -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, 
> > > > TYPE_PIIX3_IDE);
> > > > +        dev = pci_new_multifunction(piix3_devfn + 1, false, 
> > > > TYPE_PIIX3_IDE);
> > > > +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
> > > > +                                    isa_bus_get_irq(isa_bus, 14));
> > > > +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
> > > > +                                    isa_bus_get_irq(isa_bus, 15));
> > > > +        pci_realize_and_unref(dev, pci_bus, &error_fatal);
> > > > +
> > > >            pci_ide_create_devs(dev);
> > > >            idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
> > > >            idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
> > >
> > > Another reason this probably isn't a good idea: you're having to call 
> > > qdev_connect_gpio_*() before realizing the device :(
> >
> > Just curious: Resources like memory regions are assigned before 
> > realization, see e.g. i440fx or q35. Interrupts are also resources. What 
> > makes them special?
>
> I think I've covered the .instance_init() vs. realize() part in my reply to 
> Zoltan, but in terms of qdev_connect_gpio_out_named() the reasoning here is

Well, not quite I'm afaid as I still have questions as it's not clear what 
should be in init and in realize.

I've looked at i440fx and it has no init just realize which does what 
init method shoulc do anf the i440fx_init there is a legacy init function 
which should probably be realize so this does not look to be a good 
example. The q35 model is more complex and I proably don't understand it 
fully but still has a realize where most of the memory regions are created 
and an init method where some tweaks are done that are mentioned in a 
comment as needed but not the norm so it may also not be the best example 
so I'm not even getting why Bernhard's cited these in the first place.

Maybe some QOM/qdev experts could give some advice here.

Regards,
BALATON Zoltan

> that a device shouldn't change it's internal behaviour depending upon how it 
> is wired. In other words a standalone device will behave exactly the same as 
> a device connected into a machine.
>
> > I'm asking since this issue seems to be the main blocker for the piix 
> > consolidation to be merged.
>
> I did have a brief catch-up with Phil at the start of the week, and he's 
> tagged your series for review but he is really busy at the moment. As before 
> I repeat my offer to help out if needed as I think it's a good series, but 
> for now we're waiting for Phil to let us know what the next steps are...
>
>
> ATB,
>
> Mark.