| Message ID | 20230302224058.43315-4-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ide: Untangle ISA/PCI abuses of ide_init_ioport() | expand |
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 :( ATB, Mark.
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'm asking since this issue seems to be the main blocker for the piix consolidation to be merged. Best regards, Bernhard > > >ATB, > >Mark.
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'm asking since this issue seems to be the main blocker for the piix consolidation to be merged. Best regards, Bernhard > > >ATB, > >Mark.
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 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.
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. > >
Am 27. April 2023 13:04:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >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. Let's not confuse the topics ".instance_init() vs. .realize()" and "resources". I440fx seems to be very old code -- so old that it still uses legacy init methods (not to be confused with .instance_init()" methods). I've chosen the examples in context of the "resources" topic. Best regards, Bernhard > >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. >> >>
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");
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(-)