[PATCH v2 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[PATCH v2 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Philippe Mathieu-Daudé]

Background thread:
https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/

This series:
- contains few ISA cleanups
- exposes PIIX IDE output IRQs and wires them in parent (PC/Malta)
- renames the current ide_init_ioport() as ide_init_ioport_isa()
- adds a generic ide_init_ioport() which works with PCI devices
- remove ISA bus singleton.

This is required to proceed with more PIIX cleanups.

ide_drive_get() will be remove in a future series.

Based-on: <20230215112712.23110-1-philmd@linaro.org>
          "hw/ide: QOM/QDev housekeeping"
https://lore.kernel.org/qemu-devel/20230215112712.23110-1-philmd@linaro.org/

v1: https://lore.kernel.org/qemu-devel/20230208000743.79415-1-philmd@linaro.org/

Philippe Mathieu-Daudé (18):
  hw/isa: Rename isa_get_dma() -> isa_bus_get_dma()
  hw/isa: Factor isa_bus_get_irq() out of isa_get_irq()
  hw: Replace isa_get_irq() by isa_bus_get_irq() when ISABus is
    available
  hw/ide/piix: Expose output IRQ as properties for late object
    population
  hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15
  hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15
  hw/ide/piix: Ensure IDE output IRQs are wired at realization
  hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()
  hw/isa: Simplify isa_address_space[_io]()
  hw/isa: Use isa_address_space_io() in isa_register_ioport()
  hw/ide: Declare ide_get_[geometry/bios_chs_trans] in
    'hw/ide/internal.h'
  hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa
  hw/ide: Introduce generic ide_init_ioport()
  hw/ide/piix: Use generic ide_bus_init_ioport()
  hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device
  hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()
  hw/isa: Un-inline isa_bus_from_device()
  hw/isa: Remove empty ISADeviceClass structure

 hw/audio/cs4231a.c                |  5 +--
 hw/audio/gus.c                    |  5 +--
 hw/audio/sb16.c                   |  7 ++--
 hw/block/fdc-isa.c                |  5 +--
 hw/dma/i82374.c                   |  2 +-
 hw/i386/pc.c                      |  3 +-
 hw/i386/pc_piix.c                 |  8 ++++-
 hw/ide/ioport.c                   | 16 +++++++--
 hw/ide/isa.c                      |  2 +-
 hw/ide/piix.c                     | 33 ++++++++++++-------
 hw/isa/isa-bus.c                  | 55 ++++++++++++++++---------------
 hw/isa/piix4.c                    |  2 ++
 hw/rtc/m48t59-isa.c               |  2 +-
 include/hw/ide.h                  |  4 ---
 include/hw/ide/internal.h         |  7 +++-
 include/hw/ide/isa.h              |  3 ++
 include/hw/ide/pci.h              |  1 +
 include/hw/ide/piix.h             |  4 +++
 include/hw/isa/i8259_internal.h   |  2 +-
 include/hw/isa/isa.h              | 26 ++++++++-------
 include/hw/isa/superio.h          |  2 +-
 include/hw/timer/i8254.h          |  3 +-
 include/hw/timer/i8254_internal.h |  2 +-
 23 files changed, 122 insertions(+), 77 deletions(-)

-- 
2.38.1

[PATCH v2 01/18] hw/isa: Rename isa_get_dma() -> isa_bus_get_dma()

[Philippe Mathieu-Daudé]

isa_get_dma() returns a DMA channel handler from an ISABus.
To emphasize this, rename it as isa_bus_get_dma().

Mechanical change using:

  $ sed -i -e 's/isa_get_dma/isa_bus_get_dma/g' \
        $(git grep -l isa_get_dma)

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/audio/cs4231a.c   | 2 +-
 hw/audio/gus.c       | 2 +-
 hw/audio/sb16.c      | 4 ++--
 hw/block/fdc-isa.c   | 2 +-
 hw/dma/i82374.c      | 2 +-
 hw/isa/isa-bus.c     | 2 +-
 include/hw/isa/isa.h | 2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/audio/cs4231a.c b/hw/audio/cs4231a.c
index 7f17a72a9c..ec066fcd89 100644
--- a/hw/audio/cs4231a.c
+++ b/hw/audio/cs4231a.c
@@ -671,7 +671,7 @@ static void cs4231a_realizefn (DeviceState *dev, Error **errp)
     CSState *s = CS4231A (dev);
     IsaDmaClass *k;
 
-    s->isa_dma = isa_get_dma(isa_bus_from_device(d), s->dma);
+    s->isa_dma = isa_bus_get_dma(isa_bus_from_device(d), s->dma);
     if (!s->isa_dma) {
         error_setg(errp, "ISA controller does not support DMA");
         return;
diff --git a/hw/audio/gus.c b/hw/audio/gus.c
index 42f010b671..2a08a0f7d7 100644
--- a/hw/audio/gus.c
+++ b/hw/audio/gus.c
@@ -240,7 +240,7 @@ static void gus_realizefn (DeviceState *dev, Error **errp)
     IsaDmaClass *k;
     struct audsettings as;
 
-    s->isa_dma = isa_get_dma(isa_bus_from_device(d), s->emu.gusdma);
+    s->isa_dma = isa_bus_get_dma(isa_bus_from_device(d), s->emu.gusdma);
     if (!s->isa_dma) {
         error_setg(errp, "ISA controller does not support DMA");
         return;
diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 2215386ddb..ae745c7283 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -1401,8 +1401,8 @@ static void sb16_realizefn (DeviceState *dev, Error **errp)
     SB16State *s = SB16 (dev);
     IsaDmaClass *k;
 
-    s->isa_hdma = isa_get_dma(isa_bus_from_device(isadev), s->hdma);
-    s->isa_dma = isa_get_dma(isa_bus_from_device(isadev), s->dma);
+    s->isa_hdma = isa_bus_get_dma(isa_bus_from_device(isadev), s->hdma);
+    s->isa_dma = isa_bus_get_dma(isa_bus_from_device(isadev), s->dma);
     if (!s->isa_dma || !s->isa_hdma) {
         error_setg(errp, "ISA controller does not support DMA");
         return;
diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index fee1ca68a8..a5f07b668d 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -98,7 +98,7 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
     fdctrl->dma_chann = isa->dma;
     if (fdctrl->dma_chann != -1) {
         IsaDmaClass *k;
-        fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
+        fdctrl->dma = isa_bus_get_dma(isa_bus_from_device(isadev), isa->dma);
         if (!fdctrl->dma) {
             error_setg(errp, "ISA controller does not support DMA");
             return;
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index 34c3aaf7d3..63734c22c9 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -125,7 +125,7 @@ static void i82374_realize(DeviceState *dev, Error **errp)
     I82374State *s = I82374(dev);
     ISABus *isa_bus = isa_bus_from_device(ISA_DEVICE(dev));
 
-    if (isa_get_dma(isa_bus, 0)) {
+    if (isa_bus_get_dma(isa_bus, 0)) {
         error_setg(errp, "DMA already initialized on ISA bus");
         return;
     }
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index f155b80010..39111f74cc 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -99,7 +99,7 @@ void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16)
     bus->dma[1] = dma16;
 }
 
-IsaDma *isa_get_dma(ISABus *bus, int nchan)
+IsaDma *isa_bus_get_dma(ISABus *bus, int nchan)
 {
     assert(bus);
     return bus->dma[nchan > 3 ? 1 : 0];
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 25acd5c34c..029d6e90bf 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -77,7 +77,7 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
 qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
 void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16);
-IsaDma *isa_get_dma(ISABus *bus, int nchan);
+IsaDma *isa_bus_get_dma(ISABus *bus, int nchan);
 MemoryRegion *isa_address_space(ISADevice *dev);
 MemoryRegion *isa_address_space_io(ISADevice *dev);
 ISADevice *isa_new(const char *name);
-- 
2.38.1

Re: [PATCH v2 01/18] hw/isa: Rename isa_get_dma() -> isa_bus_get_dma()

[Thomas Huth]

On 15/02/2023 17.16, Philippe Mathieu-Daudé wrote:
> isa_get_dma() returns a DMA channel handler from an ISABus.
> To emphasize this, rename it as isa_bus_get_dma().
> 
> Mechanical change using:
> 
>    $ sed -i -e 's/isa_get_dma/isa_bus_get_dma/g' \
>          $(git grep -l isa_get_dma)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/audio/cs4231a.c   | 2 +-
>   hw/audio/gus.c       | 2 +-
>   hw/audio/sb16.c      | 4 ++--
>   hw/block/fdc-isa.c   | 2 +-
>   hw/dma/i82374.c      | 2 +-
>   hw/isa/isa-bus.c     | 2 +-
>   include/hw/isa/isa.h | 2 +-
>   7 files changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 02/18] hw/isa: Factor isa_bus_get_irq() out of isa_get_irq()

[Philippe Mathieu-Daudé]

isa_get_irq() was added in commit 3a38d437ca
("Add isa_reserve_irq()" Fri Aug 14 11:36:15 2009) as:

    a temporary interface to be used to allocate ISA IRQs for
    devices which have not yet been converted to qdev, and for
    special cases which are not suited for qdev conversions,
    such as the 'ferr'.

We still use it 14 years later, using the global 'isabus'
singleton. In order to get rid of such *temporary* interface,
extract isa_bus_get_irq() which can take any ISABus* object.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c     | 14 ++++++++++----
 include/hw/isa/isa.h |  8 ++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 39111f74cc..96bfee9aa7 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -72,6 +72,13 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
     bus->irqs = irqs;
 }
 
+qemu_irq isa_bus_get_irq(ISABus *bus, unsigned irqnum)
+{
+    assert(irqnum < ISA_NUM_IRQS);
+    assert(bus && bus->irqs);
+    return bus->irqs[irqnum];
+}
+
 /*
  * isa_get_irq() returns the corresponding qemu_irq entry for the i8259.
  *
@@ -81,14 +88,13 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
 qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
 {
     assert(!dev || ISA_BUS(qdev_get_parent_bus(DEVICE(dev))) == isabus);
-    assert(isairq < ISA_NUM_IRQS);
-    return isabus->irqs[isairq];
+    return isa_bus_get_irq(isabus, isairq);
 }
 
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
 {
-    qemu_irq irq = isa_get_irq(isadev, isairq);
-    qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
+    qemu_irq input_irq = isa_get_irq(isadev, isairq);
+    qdev_connect_gpio_out(DEVICE(isadev), gpioirq, input_irq);
 }
 
 void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16)
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 029d6e90bf..42d36b21a3 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -80,6 +80,14 @@ void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16);
 IsaDma *isa_bus_get_dma(ISABus *bus, int nchan);
 MemoryRegion *isa_address_space(ISADevice *dev);
 MemoryRegion *isa_address_space_io(ISADevice *dev);
+/**
+ * isa_bus_get_irq: Return input IRQ on ISA bus.
+ * @bus: the #ISABus to plug ISA devices on.
+ * @irqnum: the ISA IRQ number.
+ *
+ * Return IRQ @irqnum from the PIC associated on ISA @bus.
+ */
+qemu_irq isa_bus_get_irq(ISABus *bus, unsigned irqnum);
 ISADevice *isa_new(const char *name);
 ISADevice *isa_try_new(const char *name);
 bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
-- 
2.38.1

Re: [PATCH v2 02/18] hw/isa: Factor isa_bus_get_irq() out of isa_get_irq()

[Thomas Huth]

On 15/02/2023 17.16, Philippe Mathieu-Daudé wrote:
> isa_get_irq() was added in commit 3a38d437ca
> ("Add isa_reserve_irq()" Fri Aug 14 11:36:15 2009) as:
> 
>      a temporary interface to be used to allocate ISA IRQs for
>      devices which have not yet been converted to qdev, and for
>      special cases which are not suited for qdev conversions,
>      such as the 'ferr'.
> 
> We still use it 14 years later, using the global 'isabus'
> singleton. In order to get rid of such *temporary* interface,
> extract isa_bus_get_irq() which can take any ISABus* object.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 03/18] hw: Replace isa_get_irq() by isa_bus_get_irq() when ISABus is available

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/audio/cs4231a.c       | 5 +++--
 hw/audio/gus.c           | 5 +++--
 hw/audio/sb16.c          | 7 ++++---
 hw/block/fdc-isa.c       | 5 +++--
 include/hw/timer/i8254.h | 3 ++-
 5 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/audio/cs4231a.c b/hw/audio/cs4231a.c
index ec066fcd89..5c6d643732 100644
--- a/hw/audio/cs4231a.c
+++ b/hw/audio/cs4231a.c
@@ -668,16 +668,17 @@ static void cs4231a_initfn (Object *obj)
 static void cs4231a_realizefn (DeviceState *dev, Error **errp)
 {
     ISADevice *d = ISA_DEVICE (dev);
+    ISABus *bus = isa_bus_from_device(d);
     CSState *s = CS4231A (dev);
     IsaDmaClass *k;
 
-    s->isa_dma = isa_bus_get_dma(isa_bus_from_device(d), s->dma);
+    s->isa_dma = isa_bus_get_dma(bus, s->dma);
     if (!s->isa_dma) {
         error_setg(errp, "ISA controller does not support DMA");
         return;
     }
 
-    s->pic = isa_get_irq(d, s->irq);
+    s->pic = isa_bus_get_irq(bus, s->irq);
     k = ISADMA_GET_CLASS(s->isa_dma);
     k->register_channel(s->isa_dma, s->dma, cs_dma_read, s);
 
diff --git a/hw/audio/gus.c b/hw/audio/gus.c
index 2a08a0f7d7..787345ce54 100644
--- a/hw/audio/gus.c
+++ b/hw/audio/gus.c
@@ -236,11 +236,12 @@ static const MemoryRegionPortio gus_portio_list2[] = {
 static void gus_realizefn (DeviceState *dev, Error **errp)
 {
     ISADevice *d = ISA_DEVICE(dev);
+    ISABus *bus = isa_bus_from_device(d);
     GUSState *s = GUS (dev);
     IsaDmaClass *k;
     struct audsettings as;
 
-    s->isa_dma = isa_bus_get_dma(isa_bus_from_device(d), s->emu.gusdma);
+    s->isa_dma = isa_bus_get_dma(bus, s->emu.gusdma);
     if (!s->isa_dma) {
         error_setg(errp, "ISA controller does not support DMA");
         return;
@@ -282,7 +283,7 @@ static void gus_realizefn (DeviceState *dev, Error **errp)
     s->emu.himemaddr = s->himem;
     s->emu.gusdatapos = s->emu.himemaddr + 1024 * 1024 + 32;
     s->emu.opaque = s;
-    s->pic = isa_get_irq(d, s->emu.gusirq);
+    s->pic = isa_bus_get_irq(bus, s->emu.gusirq);
 
     AUD_set_active_out (s->voice, 1);
 }
diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index ae745c7283..535ccccdc9 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -1398,17 +1398,18 @@ static void sb16_initfn (Object *obj)
 static void sb16_realizefn (DeviceState *dev, Error **errp)
 {
     ISADevice *isadev = ISA_DEVICE (dev);
+    ISABus *bus = isa_bus_from_device(isadev);
     SB16State *s = SB16 (dev);
     IsaDmaClass *k;
 
-    s->isa_hdma = isa_bus_get_dma(isa_bus_from_device(isadev), s->hdma);
-    s->isa_dma = isa_bus_get_dma(isa_bus_from_device(isadev), s->dma);
+    s->isa_hdma = isa_bus_get_dma(bus, s->hdma);
+    s->isa_dma = isa_bus_get_dma(bus, s->dma);
     if (!s->isa_dma || !s->isa_hdma) {
         error_setg(errp, "ISA controller does not support DMA");
         return;
     }
 
-    s->pic = isa_get_irq(isadev, s->irq);
+    s->pic = isa_bus_get_irq(bus, s->irq);
 
     s->mixer_regs[0x80] = magic_of_irq (s->irq);
     s->mixer_regs[0x81] = (1 << s->dma) | (1 << s->hdma);
diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index a5f07b668d..7ec075e470 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -86,6 +86,7 @@ static const MemoryRegionPortio fdc_portio_list[] = {
 static void isabus_fdc_realize(DeviceState *dev, Error **errp)
 {
     ISADevice *isadev = ISA_DEVICE(dev);
+    ISABus *bus = isa_bus_from_device(isadev);
     FDCtrlISABus *isa = ISA_FDC(dev);
     FDCtrl *fdctrl = &isa->state;
     Error *err = NULL;
@@ -94,11 +95,11 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
                              isa->iobase, fdc_portio_list, fdctrl,
                              "fdc");
 
-    fdctrl->irq = isa_get_irq(isadev, isa->irq);
+    fdctrl->irq = isa_bus_get_irq(bus, isa->irq);
     fdctrl->dma_chann = isa->dma;
     if (fdctrl->dma_chann != -1) {
         IsaDmaClass *k;
-        fdctrl->dma = isa_bus_get_dma(isa_bus_from_device(isadev), isa->dma);
+        fdctrl->dma = isa_bus_get_dma(bus, isa->dma);
         if (!fdctrl->dma) {
             error_setg(errp, "ISA controller does not support DMA");
             return;
diff --git a/include/hw/timer/i8254.h b/include/hw/timer/i8254.h
index 3e569f42b6..8402caad30 100644
--- a/include/hw/timer/i8254.h
+++ b/include/hw/timer/i8254.h
@@ -56,7 +56,8 @@ static inline ISADevice *i8254_pit_init(ISABus *bus, int base, int isa_irq,
     qdev_prop_set_uint32(dev, "iobase", base);
     isa_realize_and_unref(d, bus, &error_fatal);
     qdev_connect_gpio_out(dev, 0,
-                          isa_irq >= 0 ? isa_get_irq(d, isa_irq) : alt_irq);
+                          isa_irq >= 0 ? isa_bus_get_irq(bus, isa_irq)
+                                       : alt_irq);
 
     return d;
 }
-- 
2.38.1

Re: [PATCH v2 03/18] hw: Replace isa_get_irq() by isa_bus_get_irq() when ISABus is available

[Thomas Huth]

On 15/02/2023 17.16, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/audio/cs4231a.c       | 5 +++--
>   hw/audio/gus.c           | 5 +++--
>   hw/audio/sb16.c          | 7 ++++---
>   hw/block/fdc-isa.c       | 5 +++--
>   include/hw/timer/i8254.h | 3 ++-
>   5 files changed, 15 insertions(+), 10 deletions(-)

Makes perfectly sense!

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 04/18] hw/ide/piix: Expose output IRQ as properties for late object population

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c         | 14 ++++++++++++--
 include/hw/ide/pci.h  |  1 +
 include/hw/ide/piix.h |  4 ++++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 41d60921e3..9d876dd4a7 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -121,6 +121,13 @@ static void piix_ide_reset(DeviceState *dev)
     pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
 }
 
+static void piix_ide_initfn(Object *obj)
+{
+    PCIIDEState *dev = PCI_IDE(obj);
+
+    qdev_init_gpio_out_named(DEVICE(obj), dev->irq, "ide-irq", 2);
+}
+
 static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
 {
     static const struct {
@@ -133,6 +140,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     };
     int ret;
 
+    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
     ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
     ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
                           port_info[i].iobase2);
@@ -141,7 +149,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
                          object_get_typename(OBJECT(d)), i);
         return false;
     }
-    ide_bus_init_output_irq(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+    ide_bus_init_output_irq(&d->bus[i], irq_out);
 
     bmdma_init(&d->bus[i], &d->bmdma[i], d);
     d->bmdma[i].bus = &d->bus[i];
@@ -162,7 +170,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
 
-    for (unsigned i = 0; i < 2; i++) {
+    for (unsigned i = 0; i < ARRAY_SIZE(d->irq); i++) {
         if (!pci_piix_init_bus(d, i, errp)) {
             return;
         }
@@ -199,6 +207,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
 static const TypeInfo piix3_ide_info = {
     .name          = TYPE_PIIX3_IDE,
     .parent        = TYPE_PCI_IDE,
+    .instance_init = piix_ide_initfn,
     .class_init    = piix3_ide_class_init,
 };
 
@@ -221,6 +230,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
 static const TypeInfo piix4_ide_info = {
     .name          = TYPE_PIIX4_IDE,
     .parent        = TYPE_PCI_IDE,
+    .instance_init = piix_ide_initfn,
     .class_init    = piix4_ide_class_init,
 };
 
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 7b5e3f6e1c..548e539add 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -49,6 +49,7 @@ struct PCIIDEState {
 
     IDEBus bus[2];
     BMDMAState bmdma[2];
+    qemu_irq irq[2];
     uint32_t secondary; /* used only for cmd646 */
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
diff --git a/include/hw/ide/piix.h b/include/hw/ide/piix.h
index ef3ef3d62d..533d24d408 100644
--- a/include/hw/ide/piix.h
+++ b/include/hw/ide/piix.h
@@ -1,6 +1,10 @@
 #ifndef HW_IDE_PIIX_H
 #define HW_IDE_PIIX_H
 
+/*
+ * QEMU interface:
+ *  + named GPIO outputs "ide-irq": asserted by each IDE channel
+ */
 #define TYPE_PIIX3_IDE "piix3-ide"
 #define TYPE_PIIX4_IDE "piix4-ide"
 
-- 
2.38.1

[PATCH v2 05/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Philippe Mathieu-Daudé]

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 7085b4bc58..983baf0211 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");
-- 
2.38.1

[PATCH v2 06/18] hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15

[Philippe Mathieu-Daudé]

piix4_realize() initialized an array of 16 ISA IRQs in
PIIX4State::isa[], use it to wire the IDE output IRQs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/piix4.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index de60ceef73..94e5dc7825 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -229,6 +229,8 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
 
     /* IDE */
     qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
+    qdev_connect_gpio_out_named(DEVICE(&s->ide), "ide-irq", 0, s->isa[14]);
+    qdev_connect_gpio_out_named(DEVICE(&s->ide), "ide-irq", 1, s->isa[15]);
     if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
         return;
     }
-- 
2.38.1

[PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Philippe Mathieu-Daudé]

Ensure both IDE output IRQ lines are wired.

We can remove the last use of isa_get_irq(NULL).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9d876dd4a7..b75a4ddcca 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     static const struct {
         int iobase;
         int iobase2;
-        int isairq;
     } port_info[] = {
-        {0x1f0, 0x3f6, 14},
-        {0x170, 0x376, 15},
+        {0x1f0, 0x3f6},
+        {0x170, 0x376},
     };
     int ret;
 
-    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
+    if (!d->irq[i]) {
+        error_setg(errp, "output IDE IRQ %u not connected", i);
+        return false;
+    }
+
     ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
     ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
                           port_info[i].iobase2);
@@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
                          object_get_typename(OBJECT(d)), i);
         return false;
     }
-    ide_bus_init_output_irq(&d->bus[i], irq_out);
+    ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
 
     bmdma_init(&d->bus[i], &d->bmdma[i], d);
     d->bmdma[i].bus = &d->bus[i];
-- 
2.38.1

Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Bernhard Beschow]

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

On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Ensure both IDE output IRQ lines are wired.
>
> We can remove the last use of isa_get_irq(NULL).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ide/piix.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 9d876dd4a7..b75a4ddcca 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d,
> unsigned i, Error **errp)
>      static const struct {
>          int iobase;
>          int iobase2;
> -        int isairq;
>      } port_info[] = {
> -        {0x1f0, 0x3f6, 14},
> -        {0x170, 0x376, 15},
> +        {0x1f0, 0x3f6},
> +        {0x170, 0x376},
>      };
>      int ret;
>
> -    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL,
> port_info[i].isairq);
> +    if (!d->irq[i]) {
> +        error_setg(errp, "output IDE IRQ %u not connected", i);
> +        return false;
> +    }
> +
>      ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>      ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>                            port_info[i].iobase2);
> @@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned
> i, Error **errp)
>                           object_get_typename(OBJECT(d)), i);
>          return false;
>      }
> -    ide_bus_init_output_irq(&d->bus[i], irq_out);
> +    ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
>
>      bmdma_init(&d->bus[i], &d->bmdma[i], d);
>      d->bmdma[i].bus = &d->bus[i];
> --
> 2.38.1
>
>
> This now breaks user-created  piix3-ide:

  qemu-system-x86_64 -M q35 -device piix3-ide

Results in:

  qemu-system-x86_64: -device piix3-ide: output IDE IRQ 0 not connected

Best regards,
Bernhard

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

Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Philippe Mathieu-Daudé]

On 16/2/23 15:43, Bernhard Beschow wrote:
> 
> 
> On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé 
> <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
> 
>     Ensure both IDE output IRQ lines are wired.
> 
>     We can remove the last use of isa_get_irq(NULL).
> 
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org
>     <mailto:philmd@linaro.org>>
>     ---
>       hw/ide/piix.c | 13 ++++++++-----
>       1 file changed, 8 insertions(+), 5 deletions(-)
> 
>     diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>     index 9d876dd4a7..b75a4ddcca 100644
>     --- a/hw/ide/piix.c
>     +++ b/hw/ide/piix.c
>     @@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>     unsigned i, Error **errp)
>           static const struct {
>               int iobase;
>               int iobase2;
>     -        int isairq;
>           } port_info[] = {
>     -        {0x1f0, 0x3f6, 14},
>     -        {0x170, 0x376, 15},
>     +        {0x1f0, 0x3f6},
>     +        {0x170, 0x376},
>           };
>           int ret;
> 
>     -    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL,
>     port_info[i].isairq);
>     +    if (!d->irq[i]) {
>     +        error_setg(errp, "output IDE IRQ %u not connected", i);
>     +        return false;
>     +    }
>     +
>           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>           ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>                                 port_info[i].iobase2);
>     @@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>     unsigned i, Error **errp)
>                                object_get_typename(OBJECT(d)), i);
>               return false;
>           }
>     -    ide_bus_init_output_irq(&d->bus[i], irq_out);
>     +    ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
> 
>           bmdma_init(&d->bus[i], &d->bmdma[i], d);
>           d->bmdma[i].bus = &d->bus[i];
>     -- 
>     2.38.1
> 
> 
> This now breaks user-created  piix3-ide:
> 
>    qemu-system-x86_64 -M q35 -device piix3-ide
> 
> Results in:
> 
>    qemu-system-x86_64: -device piix3-ide: output IDE IRQ 0 not connected

Thank you for this real-life-impossible-but-exists-in-QEMU test-case!

Should we cover it in qtests?

Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Bernhard Beschow]

Am 16. Februar 2023 15:33:47 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 16/2/23 15:43, Bernhard Beschow wrote:
>> 
>> 
>> On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
>> 
>>     Ensure both IDE output IRQ lines are wired.
>> 
>>     We can remove the last use of isa_get_irq(NULL).
>> 
>>     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org
>>     <mailto:philmd@linaro.org>>
>>     ---
>>       hw/ide/piix.c | 13 ++++++++-----
>>       1 file changed, 8 insertions(+), 5 deletions(-)
>> 
>>     diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>     index 9d876dd4a7..b75a4ddcca 100644
>>     --- a/hw/ide/piix.c
>>     +++ b/hw/ide/piix.c
>>     @@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>>     unsigned i, Error **errp)
>>           static const struct {
>>               int iobase;
>>               int iobase2;
>>     -        int isairq;
>>           } port_info[] = {
>>     -        {0x1f0, 0x3f6, 14},
>>     -        {0x170, 0x376, 15},
>>     +        {0x1f0, 0x3f6},
>>     +        {0x170, 0x376},
>>           };
>>           int ret;
>> 
>>     -    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL,
>>     port_info[i].isairq);
>>     +    if (!d->irq[i]) {
>>     +        error_setg(errp, "output IDE IRQ %u not connected", i);
>>     +        return false;
>>     +    }
>>     +
>>           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>           ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>                                 port_info[i].iobase2);
>>     @@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>>     unsigned i, Error **errp)
>>                                object_get_typename(OBJECT(d)), i);
>>               return false;
>>           }
>>     -    ide_bus_init_output_irq(&d->bus[i], irq_out);
>>     +    ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
>> 
>>           bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>           d->bmdma[i].bus = &d->bus[i];
>>     --     2.38.1
>> 
>> 
>> This now breaks user-created  piix3-ide:
>> 
>>    qemu-system-x86_64 -M q35 -device piix3-ide
>> 
>> Results in:
>> 
>>    qemu-system-x86_64: -device piix3-ide: output IDE IRQ 0 not connected
>
>Thank you for this real-life-impossible-but-exists-in-QEMU test-case!
>
>Should we cover it in qtests?

Yes, why not. Preferably along with the x-remote machine.

Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Philippe Mathieu-Daudé]

+Daniel, Igor, Marcel & libvirt

On 16/2/23 16:33, Philippe Mathieu-Daudé wrote:
> On 16/2/23 15:43, Bernhard Beschow wrote:
>>
>>
>> On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé 
>> <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
>>
>>     Ensure both IDE output IRQ lines are wired.
>>
>>     We can remove the last use of isa_get_irq(NULL).
>>
>>     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org
>>     <mailto:philmd@linaro.org>>
>>     ---
>>       hw/ide/piix.c | 13 ++++++++-----
>>       1 file changed, 8 insertions(+), 5 deletions(-)
>>
>>     diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>     index 9d876dd4a7..b75a4ddcca 100644
>>     --- a/hw/ide/piix.c
>>     +++ b/hw/ide/piix.c
>>     @@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>>     unsigned i, Error **errp)
>>           static const struct {
>>               int iobase;
>>               int iobase2;
>>     -        int isairq;
>>           } port_info[] = {
>>     -        {0x1f0, 0x3f6, 14},
>>     -        {0x170, 0x376, 15},
>>     +        {0x1f0, 0x3f6},
>>     +        {0x170, 0x376},
>>           };
>>           int ret;
>>
>>     -    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL,
>>     port_info[i].isairq);
>>     +    if (!d->irq[i]) {
>>     +        error_setg(errp, "output IDE IRQ %u not connected", i);
>>     +        return false;
>>     +    }
>>     +
>>           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>           ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>                                 port_info[i].iobase2);
>>     @@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>>     unsigned i, Error **errp)
>>                                object_get_typename(OBJECT(d)), i);
>>               return false;
>>           }
>>     -    ide_bus_init_output_irq(&d->bus[i], irq_out);
>>     +    ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
>>
>>           bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>           d->bmdma[i].bus = &d->bus[i];
>>     --     2.38.1
>>
>>
>> This now breaks user-created  piix3-ide:
>>
>>    qemu-system-x86_64 -M q35 -device piix3-ide
>>
>> Results in:
>>
>>    qemu-system-x86_64: -device piix3-ide: output IDE IRQ 0 not connected
> 
> Thank you for this real-life-impossible-but-exists-in-QEMU test-case!

Do we really want to maintain this Frankenstein use case?

1/ Q35 comes with a PCIe bus on which sits a ICH chipset, which already
    contains a AHCI function exposing multiple IDE buses.
    What is the point on using an older tech?

2/ Why can we plug a PCI function on a PCIe bridge without using a
    pcie-pci-bridge?

3/ Chipsets come as a whole. Software drivers might expect all PCI
    functions working (Linux considering each function individually
    is not the norm)


I get your use case working with the following diff [*]:

-- >8 --
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 74e2f4288d..cb1628963a 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -140,8 +140,19 @@ static bool pci_piix_init_bus(PCIIDEState *d, 
unsigned i, Error **errp)
      };

      if (!d->irq[i]) {
-        error_setg(errp, "output IDE IRQ %u not connected", i);
-        return false;
+        if (DEVICE_GET_CLASS(d)->user_creatable) {
+            /* Returns NULL unless there is exactly one ISA bus */
+            Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, 
NULL);
+
+            if (!isabus) {
+                error_setg(errp, "Unable to find a single ISA bus");
+                return false;
+            }
+            d->irq[i] = isa_bus_get_irq(ISA_BUS(isabus), 14 + i);
+        } else {
+            error_setg(errp, "output IDE IRQ %u not connected", i);
+            return false;
+        }
      }

      ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
@@ -201,6 +212,13 @@ static void piix3_ide_class_init(ObjectClass 
*klass, void *data)
      k->class_id = PCI_CLASS_STORAGE_IDE;
      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
      dc->hotpluggable = false;
+    /*
+     * This function is part of a Super I/O chip and shouldn't be user
+     * creatable. However QEMU accepts impossible hardware setups such
+     * plugging a PIIX IDE function on a ICH ISA bridge.
+     * Keep this Frankenstein (ab)use working.
+     */
+    dc->user_creatable = true;
  }

  static const TypeInfo piix3_ide_info = {
@@ -224,6 +242,8 @@ static void piix4_ide_class_init(ObjectClass *klass, 
void *data)
      k->class_id = PCI_CLASS_STORAGE_IDE;
      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
      dc->hotpluggable = false;
+    /* Reason: Part of a Super I/O chip */
+    dc->user_creatable = false;
  }
---

But the hardware really looks Frankenstein now:

(qemu) info qom-tree
/machine (pc-q35-8.0-machine)
   /peripheral-anon (container)
     /device[0] (piix3-ide)
       /bmdma[0] (memory-region)
       /bmdma[1] (memory-region)
       /bus master container[0] (memory-region)
       /bus master[0] (memory-region)
       /ide.6 (IDE)
       /ide.7 (IDE)
       /ide[0] (memory-region)
       /ide[1] (memory-region)
       /ide[2] (memory-region)
       /ide[3] (memory-region)
       /piix-bmdma-container[0] (memory-region)
       /piix-bmdma[0] (memory-region)
       /piix-bmdma[1] (memory-region)
   /q35 (q35-pcihost)
   /unattached (container)
     /device[17] (ich9-ahci)
       /ahci-idp[0] (memory-region)
       /ahci[0] (memory-region)
       /bus master container[0] (memory-region)
       /bus master[0] (memory-region)
       /ide.0 (IDE)
       /ide.1 (IDE)
       /ide.2 (IDE)
       /ide.3 (IDE)
       /ide.4 (IDE)
       /ide.5 (IDE)
     /device[2] (ICH9-LPC)
       /bus master container[0] (memory-region)
       /bus master[0] (memory-region)
       /ich9-pm[0] (memory-region)
       /isa.0 (ISA)

I guess the diff [*] is acceptable as a kludge, but we might consider
deprecating such use.

Paolo, Michael & libvirt folks, what do you think?

Regards,

Phil.

Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Bernhard Beschow]

Am 19. Februar 2023 21:54:34 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>+Daniel, Igor, Marcel & libvirt
>
>On 16/2/23 16:33, Philippe Mathieu-Daudé wrote:
>> On 16/2/23 15:43, Bernhard Beschow wrote:
>>> 
>>> 
>>> On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
>>> 
>>>     Ensure both IDE output IRQ lines are wired.
>>> 
>>>     We can remove the last use of isa_get_irq(NULL).
>>> 
>>>     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org
>>>     <mailto:philmd@linaro.org>>
>>>     ---
>>>       hw/ide/piix.c | 13 ++++++++-----
>>>       1 file changed, 8 insertions(+), 5 deletions(-)
>>> 
>>>     diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>     index 9d876dd4a7..b75a4ddcca 100644
>>>     --- a/hw/ide/piix.c
>>>     +++ b/hw/ide/piix.c
>>>     @@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>>>     unsigned i, Error **errp)
>>>           static const struct {
>>>               int iobase;
>>>               int iobase2;
>>>     -        int isairq;
>>>           } port_info[] = {
>>>     -        {0x1f0, 0x3f6, 14},
>>>     -        {0x170, 0x376, 15},
>>>     +        {0x1f0, 0x3f6},
>>>     +        {0x170, 0x376},
>>>           };
>>>           int ret;
>>> 
>>>     -    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL,
>>>     port_info[i].isairq);
>>>     +    if (!d->irq[i]) {
>>>     +        error_setg(errp, "output IDE IRQ %u not connected", i);
>>>     +        return false;
>>>     +    }
>>>     +
>>>           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>           ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>                                 port_info[i].iobase2);
>>>     @@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d,
>>>     unsigned i, Error **errp)
>>>                                object_get_typename(OBJECT(d)), i);
>>>               return false;
>>>           }
>>>     -    ide_bus_init_output_irq(&d->bus[i], irq_out);
>>>     +    ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
>>> 
>>>           bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>           d->bmdma[i].bus = &d->bus[i];
>>>     --     2.38.1
>>> 
>>> 
>>> This now breaks user-created  piix3-ide:
>>> 
>>>    qemu-system-x86_64 -M q35 -device piix3-ide
>>> 
>>> Results in:
>>> 
>>>    qemu-system-x86_64: -device piix3-ide: output IDE IRQ 0 not connected
>> 
>> Thank you for this real-life-impossible-but-exists-in-QEMU test-case!
>
>Do we really want to maintain this Frankenstein use case?
>
>1/ Q35 comes with a PCIe bus on which sits a ICH chipset, which already
>   contains a AHCI function exposing multiple IDE buses.
>   What is the point on using an older tech?

I just chose Q35 in the test case to demonstrate that we'd not meet our own deprecation rules.

IIUC, QEMU guarantees a deprecation period for at least two major versions. So if we deprecated user-creatable piix-ide in 8.1, we are not allowed to remove it before 10.1. Let's stick to our rules to give our users a chance to adapt gracefully.

>
>2/ Why can we plug a PCI function on a PCIe bridge without using a
>   pcie-pci-bridge?
>
>3/ Chipsets come as a whole. Software drivers might expect all PCI
>   functions working (Linux considering each function individually
>   is not the norm)
>
>
>I get your use case working with the following diff [*]:
>
>-- >8 --
>diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>index 74e2f4288d..cb1628963a 100644
>--- a/hw/ide/piix.c
>+++ b/hw/ide/piix.c
>@@ -140,8 +140,19 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>     };
>
>     if (!d->irq[i]) {
>-        error_setg(errp, "output IDE IRQ %u not connected", i);
>-        return false;
>+        if (DEVICE_GET_CLASS(d)->user_creatable) {
>+            /* Returns NULL unless there is exactly one ISA bus */
>+            Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, NULL);
>+
>+            if (!isabus) {
>+                error_setg(errp, "Unable to find a single ISA bus");
>+                return false;
>+            }
>+            d->irq[i] = isa_bus_get_irq(ISA_BUS(isabus), 14 + i);
>+        } else {
>+            error_setg(errp, "output IDE IRQ %u not connected", i);
>+            return false;
>+        }
>     }
>
>     ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>@@ -201,6 +212,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
>     k->class_id = PCI_CLASS_STORAGE_IDE;
>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>     dc->hotpluggable = false;
>+    /*
>+     * This function is part of a Super I/O chip and shouldn't be user
>+     * creatable. However QEMU accepts impossible hardware setups such
>+     * plugging a PIIX IDE function on a ICH ISA bridge.
>+     * Keep this Frankenstein (ab)use working.
>+     */
>+    dc->user_creatable = true;
> }
>
> static const TypeInfo piix3_ide_info = {
>@@ -224,6 +242,8 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>     k->class_id = PCI_CLASS_STORAGE_IDE;
>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>     dc->hotpluggable = false;
>+    /* Reason: Part of a Super I/O chip */
>+    dc->user_creatable = false;

piix3-ide and piix4-ide are implemented in the same file. This means that above test case should also apply for piix4-ide, even when CONFIG_PIIX4=n. To meet our deprecation rules, we're not allowed to treat piix4 differently.

Best regards,
Bernhard

> }
>---
>
>But the hardware really looks Frankenstein now:
>
>(qemu) info qom-tree
>/machine (pc-q35-8.0-machine)
>  /peripheral-anon (container)
>    /device[0] (piix3-ide)
>      /bmdma[0] (memory-region)
>      /bmdma[1] (memory-region)
>      /bus master container[0] (memory-region)
>      /bus master[0] (memory-region)
>      /ide.6 (IDE)
>      /ide.7 (IDE)
>      /ide[0] (memory-region)
>      /ide[1] (memory-region)
>      /ide[2] (memory-region)
>      /ide[3] (memory-region)
>      /piix-bmdma-container[0] (memory-region)
>      /piix-bmdma[0] (memory-region)
>      /piix-bmdma[1] (memory-region)
>  /q35 (q35-pcihost)
>  /unattached (container)
>    /device[17] (ich9-ahci)
>      /ahci-idp[0] (memory-region)
>      /ahci[0] (memory-region)
>      /bus master container[0] (memory-region)
>      /bus master[0] (memory-region)
>      /ide.0 (IDE)
>      /ide.1 (IDE)
>      /ide.2 (IDE)
>      /ide.3 (IDE)
>      /ide.4 (IDE)
>      /ide.5 (IDE)
>    /device[2] (ICH9-LPC)
>      /bus master container[0] (memory-region)
>      /bus master[0] (memory-region)
>      /ich9-pm[0] (memory-region)
>      /isa.0 (ISA)
>
>I guess the diff [*] is acceptable as a kludge, but we might consider
>deprecating such use.
>
>Paolo, Michael & libvirt folks, what do you think?
>
>Regards,
>
>Phil.

Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[BALATON Zoltan]

On Mon, 20 Feb 2023, Bernhard Beschow wrote:
> IIUC, QEMU guarantees a deprecation period for at least two major 
> versions. So if we deprecated user-creatable piix-ide in 8.1, we are not 
> allowed to remove it before 10.1. Let's stick to our rules to give our 
> users a chance to adapt gracefully.

I think that's not 2 major releases just 2 releases so in your example 
could be removed in 9.0. qemu/docs/about/deprecated.rst says:

"The feature will remain functional for the release in which it was 
deprecated and one further release. After these two releases, the feature 
is liable to be removed."

Regards,
BALATON Zoltan

Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Daniel P. Berrangé]

On Sun, Feb 19, 2023 at 10:54:34PM +0100, Philippe Mathieu-Daudé wrote:
> +Daniel, Igor, Marcel & libvirt
> 
> On 16/2/23 16:33, Philippe Mathieu-Daudé wrote:
> > On 16/2/23 15:43, Bernhard Beschow wrote:
> > > 
> > > 
> > > On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé
> > > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
> > > 
> > >     Ensure both IDE output IRQ lines are wired.
> > > 
> > >     We can remove the last use of isa_get_irq(NULL).
> > > 
> > >     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org
> > >     <mailto:philmd@linaro.org>>
> > >     ---
> > >       hw/ide/piix.c | 13 ++++++++-----
> > >       1 file changed, 8 insertions(+), 5 deletions(-)
> > > 
> > >     diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > >     index 9d876dd4a7..b75a4ddcca 100644
> > >     --- a/hw/ide/piix.c
> > >     +++ b/hw/ide/piix.c
> > >     @@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d,
> > >     unsigned i, Error **errp)
> > >           static const struct {
> > >               int iobase;
> > >               int iobase2;
> > >     -        int isairq;
> > >           } port_info[] = {
> > >     -        {0x1f0, 0x3f6, 14},
> > >     -        {0x170, 0x376, 15},
> > >     +        {0x1f0, 0x3f6},
> > >     +        {0x170, 0x376},
> > >           };
> > >           int ret;
> > > 
> > >     -    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL,
> > >     port_info[i].isairq);
> > >     +    if (!d->irq[i]) {
> > >     +        error_setg(errp, "output IDE IRQ %u not connected", i);
> > >     +        return false;
> > >     +    }
> > >     +
> > >           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> > >           ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> > >                                 port_info[i].iobase2);
> > >     @@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d,
> > >     unsigned i, Error **errp)
> > >                                object_get_typename(OBJECT(d)), i);
> > >               return false;
> > >           }
> > >     -    ide_bus_init_output_irq(&d->bus[i], irq_out);
> > >     +    ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
> > > 
> > >           bmdma_init(&d->bus[i], &d->bmdma[i], d);
> > >           d->bmdma[i].bus = &d->bus[i];
> > >     --     2.38.1
> > > 
> > > 
> > > This now breaks user-created  piix3-ide:
> > > 
> > >    qemu-system-x86_64 -M q35 -device piix3-ide
> > > 
> > > Results in:
> > > 
> > >    qemu-system-x86_64: -device piix3-ide: output IDE IRQ 0 not connected
> > 
> > Thank you for this real-life-impossible-but-exists-in-QEMU test-case!
> 
> Do we really want to maintain this Frankenstein use case?
> 
> 1/ Q35 comes with a PCIe bus on which sits a ICH chipset, which already
>    contains a AHCI function exposing multiple IDE buses.
>    What is the point on using an older tech?
> 
> 2/ Why can we plug a PCI function on a PCIe bridge without using a
>    pcie-pci-bridge?

FYI, this scenario is explicitly permitted by QEMU's PCIE docs,
as without any bus= attr, the -device piix3-ide will end up
attached to the PCI-E root complex. It thus becomes an integrated
endpoint and does not support hotplug.

If you want hotpluggable PCI-only device, you need to add a
pcie-pci-bridge and a pci-bridge, attaching the endpoint to
the latter

PCE-E endpoints should always be in a pcie-root-port (or
switch downstream port)

> 3/ Chipsets come as a whole. Software drivers might expect all PCI
>    functions working (Linux considering each function individually
>    is not the norm)


> But the hardware really looks Frankenstein now:
> 
> (qemu) info qom-tree
> /machine (pc-q35-8.0-machine)
>   /peripheral-anon (container)
>     /device[0] (piix3-ide)
>       /bmdma[0] (memory-region)
>       /bmdma[1] (memory-region)
>       /bus master container[0] (memory-region)
>       /bus master[0] (memory-region)
>       /ide.6 (IDE)
>       /ide.7 (IDE)
>       /ide[0] (memory-region)
>       /ide[1] (memory-region)
>       /ide[2] (memory-region)
>       /ide[3] (memory-region)
>       /piix-bmdma-container[0] (memory-region)
>       /piix-bmdma[0] (memory-region)
>       /piix-bmdma[1] (memory-region)
>   /q35 (q35-pcihost)
>   /unattached (container)
>     /device[17] (ich9-ahci)
>       /ahci-idp[0] (memory-region)
>       /ahci[0] (memory-region)
>       /bus master container[0] (memory-region)
>       /bus master[0] (memory-region)
>       /ide.0 (IDE)
>       /ide.1 (IDE)
>       /ide.2 (IDE)
>       /ide.3 (IDE)
>       /ide.4 (IDE)
>       /ide.5 (IDE)
>     /device[2] (ICH9-LPC)
>       /bus master container[0] (memory-region)
>       /bus master[0] (memory-region)
>       /ich9-pm[0] (memory-region)
>       /isa.0 (ISA)
> 
> I guess the diff [*] is acceptable as a kludge, but we might consider
> deprecating such use.
> 
> Paolo, Michael & libvirt folks, what do you think?

AFAICT, libvirt will never use '-device piix3-ide'. I vaguely recall
that we discussed allowing it to enable use of > 4 IDE units, but
decide it was too niche to care about.

With q35 we'll just use ahci only

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 08/18] hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()

[Philippe Mathieu-Daudé]

Last commit removed the last use of isa_get_irq(NULL).
Add an assertion to ensure we won't use that hack again.
Deprecate in favor of the BUS API: isa_bus_get_irq().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c     | 6 +++---
 include/hw/isa/isa.h | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 96bfee9aa7..a9b94e6c8a 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -85,10 +85,10 @@ qemu_irq isa_bus_get_irq(ISABus *bus, unsigned irqnum)
  * This function is only for special cases such as the 'ferr', and
  * temporary use for normal devices until they are converted to qdev.
  */
-qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
+qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum)
 {
-    assert(!dev || ISA_BUS(qdev_get_parent_bus(DEVICE(dev))) == isabus);
-    return isa_bus_get_irq(isabus, isairq);
+    assert(dev);
+    return isa_bus_get_irq(ISA_BUS(qdev_get_parent_bus(DEVICE(dev))), irqnum);
 }
 
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 42d36b21a3..1084d68ead 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -74,7 +74,6 @@ struct ISADevice {
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space,
                     MemoryRegion *address_space_io, Error **errp);
 void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
-qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
 void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16);
 IsaDma *isa_bus_get_dma(ISABus *bus, int nchan);
@@ -95,6 +94,9 @@ ISADevice *isa_create_simple(ISABus *bus, const char *name);
 
 ISADevice *isa_vga_init(ISABus *bus);
 
+/*  isa_get_irq() is deprecated, please use isa_bus_get_irq() instead. */
+qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum);
+
 /**
  * isa_register_ioport: Install an I/O port region on the ISA bus.
  *
-- 
2.38.1

[PATCH v2 09/18] hw/isa: Simplify isa_address_space[_io]()

[Philippe Mathieu-Daudé]

We don't have any caller passing a NULL device argument,
so we can simplify, avoiding to access the global 'isabus'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index a9b94e6c8a..08c84704e9 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -247,20 +247,14 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
 
 MemoryRegion *isa_address_space(ISADevice *dev)
 {
-    if (dev) {
-        return isa_bus_from_device(dev)->address_space;
-    }
-
-    return isabus->address_space;
+    assert(dev);
+    return isa_bus_from_device(dev)->address_space;
 }
 
 MemoryRegion *isa_address_space_io(ISADevice *dev)
 {
-    if (dev) {
-        return isa_bus_from_device(dev)->address_space_io;
-    }
-
-    return isabus->address_space_io;
+    assert(dev);
+    return isa_bus_from_device(dev)->address_space_io;
 }
 
 type_init(isabus_register_types)
-- 
2.38.1

[PATCH v2 10/18] hw/isa: Use isa_address_space_io() in isa_register_ioport()

[Philippe Mathieu-Daudé]

We don't have any caller passing a NULL device argument,
so we can simplify, avoiding to access the global 'isabus'.

(Since the previous commit, isa_address_space_io() assert
 the device argument is not NULL).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 08c84704e9..abc1bd0771 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -120,7 +120,7 @@ static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 
 void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
-    memory_region_add_subregion(isabus->address_space_io, start, io);
+    memory_region_add_subregion(isa_address_space_io(dev), start, io);
     isa_init_ioport(dev, start);
 }
 
-- 
2.38.1

[PATCH v2 11/18] hw/ide: Declare ide_get_[geometry/bios_chs_trans] in 'hw/ide/internal.h'

[Philippe Mathieu-Daudé]

ide_get_geometry() and ide_get_bios_chs_trans() are only
used by the TYPE_PC_MACHINE.
"hw/ide.h" is a mixed bag of lost IDE declarations. In order
to remove this (almost) pointless header soon, move these
declarations to "hw/ide/internal.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc.c              | 3 ++-
 include/hw/ide.h          | 4 ----
 include/hw/ide/internal.h | 4 ++++
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6e592bd969..79297a6ecd 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -34,7 +34,8 @@
 #include "hw/i386/vmport.h"
 #include "sysemu/cpus.h"
 #include "hw/block/fdc.h"
-#include "hw/ide.h"
+#include "hw/ide/internal.h"
+#include "hw/ide/isa.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/pci-bridge/pci_expander_bridge.h"
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 24a7aa2925..db963bdb77 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -3,10 +3,6 @@
 
 #include "exec/memory.h"
 
-int ide_get_geometry(BusState *bus, int unit,
-                     int16_t *cyls, int8_t *heads, int8_t *secs);
-int ide_get_bios_chs_trans(BusState *bus, int unit);
-
 /* ide/core.c */
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index c2b794150f..d9f1f77dd5 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -647,6 +647,10 @@ void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
                   int bus_id, int max_units);
 IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
 
+int ide_get_geometry(BusState *bus, int unit,
+                     int16_t *cyls, int8_t *heads, int8_t *secs);
+int ide_get_bios_chs_trans(BusState *bus, int unit);
+
 int ide_handle_rw_error(IDEState *s, int error, int op);
 
 #endif /* HW_IDE_INTERNAL_H */
-- 
2.38.1

[PATCH v2 12/18] hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa

[Philippe Mathieu-Daudé]

Rename ide_init_ioport() as ide_bus_init_ioport_isa() to make
explicit it expects an ISA device. Move the declaration to
"hw/ide/isa.h" where it belongs.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ioport.c           | 4 +++-
 hw/ide/isa.c              | 2 +-
 hw/ide/piix.c             | 5 +++--
 include/hw/ide/internal.h | 1 -
 include/hw/ide/isa.h      | 3 +++
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index e2ecc6230c..d869f8018a 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -25,6 +25,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/isa/isa.h"
+#include "hw/ide/isa.h"
 #include "hw/ide/internal.h"
 #include "trace.h"
 
@@ -40,7 +41,8 @@ static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
+                            int iobase, int iobase2)
 {
     int ret;
 
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 95053e026f..6eed16bf87 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -71,7 +71,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
     ISAIDEState *s = ISA_IDE(dev);
 
     ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
-    ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
+    ide_bus_init_ioport_isa(&s->bus, isadev, s->iobase, s->iobase2);
     ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum));
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
     ide_bus_register_restart_cb(&s->bus);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b75a4ddcca..9b886fc0d2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -33,6 +33,7 @@
 #include "hw/pci/pci.h"
 #include "hw/ide/piix.h"
 #include "hw/ide/pci.h"
+#include "hw/ide/isa.h"
 #include "trace.h"
 
 static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
@@ -145,8 +146,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     }
 
     ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-    ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                          port_info[i].iobase2);
+    ret = ide_bus_init_ioport_isa(&d->bus[i], NULL,
+                                  port_info[i].iobase, port_info[i].iobase2);
     if (ret) {
         error_setg_errno(errp, -ret, "Failed to realize %s port %u",
                          object_get_typename(OBJECT(d)), i);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d9f1f77dd5..d3b7fdc504 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -618,7 +618,6 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    int chs_trans, Error **errp);
 void ide_exit(IDEState *s);
 void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
-int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_bus_set_irq(IDEBus *bus);
 void ide_bus_register_restart_cb(IDEBus *bus);
 
diff --git a/include/hw/ide/isa.h b/include/hw/ide/isa.h
index 1cd0ff1fa6..7f7a850265 100644
--- a/include/hw/ide/isa.h
+++ b/include/hw/ide/isa.h
@@ -10,11 +10,14 @@
 #define HW_IDE_ISA_H
 
 #include "qom/object.h"
+#include "hw/ide/internal.h"
 
 #define TYPE_ISA_IDE "isa-ide"
 OBJECT_DECLARE_SIMPLE_TYPE(ISAIDEState, ISA_IDE)
 
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int irqnum,
                         DriveInfo *hd0, DriveInfo *hd1);
+int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *isa,
+                            int iobase, int iobase2);
 
 #endif
-- 
2.38.1

[PATCH v2 13/18] hw/ide: Introduce generic ide_init_ioport()

[Philippe Mathieu-Daudé]

Add ide_init_ioport() which is not restricted to the ISA bus.
(Next commit will use it for a PCI device).

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ioport.c           | 12 ++++++++++--
 include/hw/ide/internal.h |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index d869f8018a..1d4711dd91 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -46,8 +46,6 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
 {
     int ret;
 
-    /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
-       bridge has been setup properly to always register with ISA.  */
     ret = isa_register_portio_list(dev, &bus->portio_list,
                                    iobase, ide_portio_list, bus, "ide");
 
@@ -58,3 +56,13 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
 
     return ret;
 }
+
+void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+                         int iobase, int iobase2)
+{
+    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide");
+    portio_list_add(&bus->portio_list, io, iobase);
+
+    portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus, "ide");
+    portio_list_add(&bus->portio2_list, io, iobase2);
+}
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d3b7fdc504..6967ca13e0 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -617,6 +617,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    uint32_t cylinders, uint32_t heads, uint32_t secs,
                    int chs_trans, Error **errp);
 void ide_exit(IDEState *s);
+void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+                         int iobase, int iobase2);
 void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
 void ide_bus_set_irq(IDEBus *bus);
 void ide_bus_register_restart_cb(IDEBus *bus);
-- 
2.38.1

[PATCH v2 14/18] hw/ide/piix: Use generic ide_bus_init_ioport()

[Philippe Mathieu-Daudé]

TYPE_PIIX3_IDE is a PCI function inheriting from QOM
TYPE_PCI_DEVICE. To be able to call the ISA specific
ide_init_ioport_isa(), we call this function passing
a NULL ISADevice argument. Remove this hack by calling
the recently added generic ide_init_ioport(), which
doesn't expect any ISADevice.

Inspired-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9b886fc0d2..74e2f4288d 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -138,7 +138,6 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
         {0x1f0, 0x3f6},
         {0x170, 0x376},
     };
-    int ret;
 
     if (!d->irq[i]) {
         error_setg(errp, "output IDE IRQ %u not connected", i);
@@ -146,13 +145,9 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     }
 
     ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-    ret = ide_bus_init_ioport_isa(&d->bus[i], NULL,
-                                  port_info[i].iobase, port_info[i].iobase2);
-    if (ret) {
-        error_setg_errno(errp, -ret, "Failed to realize %s port %u",
-                         object_get_typename(OBJECT(d)), i);
-        return false;
-    }
+    ide_bus_init_ioport(&d->bus[i], OBJECT(d),
+                        pci_address_space_io(PCI_DEVICE(d)),
+                        port_info[i].iobase, port_info[i].iobase2);
     ide_bus_init_output_irq(&d->bus[i], d->irq[i]);
 
     bmdma_init(&d->bus[i], &d->bmdma[i], d);
-- 
2.38.1

[PATCH v2 15/18] hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device

[Philippe Mathieu-Daudé]

Previous commit removed the single call to isa_register_portio_list()
with dev=NULL. To be sure we won't reintroduce such weird (ab)use,
assert dev is non-NULL.

We can now calls isa_address_space_io() to get the device I/O region.

Note we can then remove the NULL check in isa_init_ioport() because
it is only called in 2 places (and is static to this file):
- isa_register_ioport() which first calls isa_address_space_io(),
  itself asserting dev is not NULL.
- isa_register_portio_list() which also asserts dev is not NULL
  since the previous commit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index abc1bd0771..59f98472d1 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -113,7 +113,7 @@ IsaDma *isa_bus_get_dma(ISABus *bus, int nchan)
 
 static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 {
-    if (dev && (dev->ioport_id == 0 || ioport < dev->ioport_id)) {
+    if (dev->ioport_id == 0 || ioport < dev->ioport_id) {
         dev->ioport_id = ioport;
     }
 }
@@ -129,6 +129,7 @@ int isa_register_portio_list(ISADevice *dev,
                              const MemoryRegionPortio *pio_start,
                              void *opaque, const char *name)
 {
+    assert(dev);
     assert(piolist && !piolist->owner);
 
     if (!isabus) {
@@ -141,7 +142,7 @@ int isa_register_portio_list(ISADevice *dev,
     isa_init_ioport(dev, start);
 
     portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
-    portio_list_add(piolist, isabus->address_space_io, start);
+    portio_list_add(piolist, isa_address_space_io(dev), start);
 
     return 0;
 }
-- 
2.38.1

[PATCH v2 16/18] hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()

[Philippe Mathieu-Daudé]

Previous commit ensured when entering isa_register_portio_list(),
'dev' is not NULL. Being a TYPE_ISA_DEVICE, the device must sit
on a ISA bus. This means isa_bus_new() as already been called
and 'isabus' can not be NULL.

Simplify by removing the 'isabus' NULL check in
isa_register_portio_list(). 'isabus' is now only used in
isa_bus_new(). Reduce its scope by only declaring it the
function using it (this will allows us to create multiple
ISA buses later).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 59f98472d1..719f2e96f2 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -25,8 +25,6 @@
 #include "sysemu/sysemu.h"
 #include "hw/isa/isa.h"
 
-static ISABus *isabus;
-
 static char *isabus_get_fw_dev_path(DeviceState *dev);
 
 static void isa_bus_class_init(ObjectClass *klass, void *data)
@@ -52,6 +50,8 @@ static const TypeInfo isa_bus_info = {
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
                     MemoryRegion *address_space_io, Error **errp)
 {
+    static ISABus *isabus;
+
     if (isabus) {
         error_setg(errp, "Can't create a second ISA bus");
         return NULL;
@@ -132,10 +132,6 @@ int isa_register_portio_list(ISADevice *dev,
     assert(dev);
     assert(piolist && !piolist->owner);
 
-    if (!isabus) {
-        return -ENODEV;
-    }
-
     /* START is how we should treat DEV, regardless of the actual
        contents of the portio array.  This is how the old code
        actually handled e.g. the FDC device.  */
-- 
2.38.1

[PATCH v2 17/18] hw/isa: Un-inline isa_bus_from_device()

[Philippe Mathieu-Daudé]

No point in inlining isa_bus_from_device() which is only
used at device realization time.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c     | 5 +++++
 include/hw/isa/isa.h | 6 +-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 719f2e96f2..f44817b88b 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -167,6 +167,11 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp)
     return qdev_realize_and_unref(&dev->parent_obj, &bus->parent_obj, errp);
 }
 
+ISABus *isa_bus_from_device(ISADevice *dev)
+{
+    return ISA_BUS(qdev_get_parent_bus(DEVICE(dev)));
+}
+
 ISADevice *isa_vga_init(ISABus *bus)
 {
     vga_interface_created = true;
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 1084d68ead..c9954a7d99 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -96,6 +96,7 @@ ISADevice *isa_vga_init(ISABus *bus);
 
 /*  isa_get_irq() is deprecated, please use isa_bus_get_irq() instead. */
 qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum);
+ISABus *isa_bus_from_device(ISADevice *dev);
 
 /**
  * isa_register_ioport: Install an I/O port region on the ISA bus.
@@ -133,9 +134,4 @@ int isa_register_portio_list(ISADevice *dev,
                              const MemoryRegionPortio *portio,
                              void *opaque, const char *name);
 
-static inline ISABus *isa_bus_from_device(ISADevice *d)
-{
-    return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
-}
-
 #endif
-- 
2.38.1

Re: [PATCH v2 17/18] hw/isa: Un-inline isa_bus_from_device()

[Bernhard Beschow]

Am 15. Februar 2023 16:16:40 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>No point in inlining isa_bus_from_device() which is only
>used at device realization time.
>
>Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/isa/isa-bus.c     | 5 +++++
> include/hw/isa/isa.h | 6 +-----
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
>index 719f2e96f2..f44817b88b 100644
>--- a/hw/isa/isa-bus.c
>+++ b/hw/isa/isa-bus.c
>@@ -167,6 +167,11 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp)
>     return qdev_realize_and_unref(&dev->parent_obj, &bus->parent_obj, errp);
> }
> 
>+ISABus *isa_bus_from_device(ISADevice *dev)
>+{
>+    return ISA_BUS(qdev_get_parent_bus(DEVICE(dev)));
>+}
>+
> ISADevice *isa_vga_init(ISABus *bus)
> {
>     vga_interface_created = true;
>diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
>index 1084d68ead..c9954a7d99 100644
>--- a/include/hw/isa/isa.h
>+++ b/include/hw/isa/isa.h
>@@ -96,6 +96,7 @@ ISADevice *isa_vga_init(ISABus *bus);
> 
> /*  isa_get_irq() is deprecated, please use isa_bus_get_irq() instead. */
> qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum);
>+ISABus *isa_bus_from_device(ISADevice *dev);
> 
> /**
>  * isa_register_ioport: Install an I/O port region on the ISA bus.
>@@ -133,9 +134,4 @@ int isa_register_portio_list(ISADevice *dev,
>                              const MemoryRegionPortio *portio,
>                              void *opaque, const char *name);
> 
>-static inline ISABus *isa_bus_from_device(ISADevice *d)
>-{
>-    return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
>-}
>-
> #endif

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

[PATCH v2 18/18] hw/isa: Remove empty ISADeviceClass structure

[Philippe Mathieu-Daudé]

ISADeviceClass is an empty class and just increase code
complexity. Remove it, directly embedding DeviceClass in
classes expanding TYPE_ISA_DEVICE.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c                  | 1 -
 hw/rtc/m48t59-isa.c               | 2 +-
 include/hw/isa/i8259_internal.h   | 2 +-
 include/hw/isa/isa.h              | 6 +-----
 include/hw/isa/superio.h          | 2 +-
 include/hw/timer/i8254_internal.h | 2 +-
 6 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index f44817b88b..1276f31826 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -221,7 +221,6 @@ static const TypeInfo isa_device_type_info = {
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(ISADevice),
     .abstract = true,
-    .class_size = sizeof(ISADeviceClass),
     .class_init = isa_device_class_init,
 };
 
diff --git a/hw/rtc/m48t59-isa.c b/hw/rtc/m48t59-isa.c
index e61f7ec370..5bb46f2383 100644
--- a/hw/rtc/m48t59-isa.c
+++ b/hw/rtc/m48t59-isa.c
@@ -47,7 +47,7 @@ struct M48txxISAState {
 };
 
 struct M48txxISADeviceClass {
-    ISADeviceClass parent_class;
+    DeviceClass parent_class;
     M48txxInfo info;
 };
 
diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
index d272d879fb..155b098452 100644
--- a/include/hw/isa/i8259_internal.h
+++ b/include/hw/isa/i8259_internal.h
@@ -35,7 +35,7 @@
 OBJECT_DECLARE_TYPE(PICCommonState, PICCommonClass, PIC_COMMON)
 
 struct PICCommonClass {
-    ISADeviceClass parent_class;
+    DeviceClass parent_class;
 
     void (*pre_save)(PICCommonState *s);
     void (*post_load)(PICCommonState *s);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index c9954a7d99..411d12330b 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -11,7 +11,7 @@
 #define ISA_NUM_IRQS 16
 
 #define TYPE_ISA_DEVICE "isa-device"
-OBJECT_DECLARE_TYPE(ISADevice, ISADeviceClass, ISA_DEVICE)
+OBJECT_DECLARE_SIMPLE_TYPE(ISADevice, ISA_DEVICE)
 
 #define TYPE_ISA_BUS "ISA"
 OBJECT_DECLARE_SIMPLE_TYPE(ISABus, ISA_BUS)
@@ -48,10 +48,6 @@ struct IsaDmaClass {
                              void *opaque);
 };
 
-struct ISADeviceClass {
-    DeviceClass parent_class;
-};
-
 struct ISABus {
     /*< private >*/
     BusState parent_obj;
diff --git a/include/hw/isa/superio.h b/include/hw/isa/superio.h
index b9f5c19155..0dc45104d4 100644
--- a/include/hw/isa/superio.h
+++ b/include/hw/isa/superio.h
@@ -44,7 +44,7 @@ typedef struct ISASuperIOFuncs {
 
 struct ISASuperIOClass {
     /*< private >*/
-    ISADeviceClass parent_class;
+    DeviceClass parent_class;
     /*< public >*/
     DeviceRealize parent_realize;
 
diff --git a/include/hw/timer/i8254_internal.h b/include/hw/timer/i8254_internal.h
index a9a600d941..1761deb4cf 100644
--- a/include/hw/timer/i8254_internal.h
+++ b/include/hw/timer/i8254_internal.h
@@ -58,7 +58,7 @@ struct PITCommonState {
 };
 
 struct PITCommonClass {
-    ISADeviceClass parent_class;
+    DeviceClass parent_class;
 
     void (*set_channel_gate)(PITCommonState *s, PITChannelState *sc, int val);
     void (*get_channel_info)(PITCommonState *s, PITChannelState *sc,
-- 
2.38.1

Re: [PATCH v2 18/18] hw/isa: Remove empty ISADeviceClass structure

[Bernhard Beschow]

Am 15. Februar 2023 16:16:41 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>ISADeviceClass is an empty class and just increase code
>complexity. Remove it, directly embedding DeviceClass in
>classes expanding TYPE_ISA_DEVICE.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/isa/isa-bus.c                  | 1 -
> hw/rtc/m48t59-isa.c               | 2 +-
> include/hw/isa/i8259_internal.h   | 2 +-
> include/hw/isa/isa.h              | 6 +-----
> include/hw/isa/superio.h          | 2 +-
> include/hw/timer/i8254_internal.h | 2 +-
> 6 files changed, 5 insertions(+), 10 deletions(-)
>
>diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
>index f44817b88b..1276f31826 100644
>--- a/hw/isa/isa-bus.c
>+++ b/hw/isa/isa-bus.c
>@@ -221,7 +221,6 @@ static const TypeInfo isa_device_type_info = {
>     .parent = TYPE_DEVICE,
>     .instance_size = sizeof(ISADevice),
>     .abstract = true,
>-    .class_size = sizeof(ISADeviceClass),
>     .class_init = isa_device_class_init,
> };
> 
>diff --git a/hw/rtc/m48t59-isa.c b/hw/rtc/m48t59-isa.c
>index e61f7ec370..5bb46f2383 100644
>--- a/hw/rtc/m48t59-isa.c
>+++ b/hw/rtc/m48t59-isa.c
>@@ -47,7 +47,7 @@ struct M48txxISAState {
> };
> 
> struct M48txxISADeviceClass {
>-    ISADeviceClass parent_class;
>+    DeviceClass parent_class;
>     M48txxInfo info;
> };
> 
>diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
>index d272d879fb..155b098452 100644
>--- a/include/hw/isa/i8259_internal.h
>+++ b/include/hw/isa/i8259_internal.h
>@@ -35,7 +35,7 @@
> OBJECT_DECLARE_TYPE(PICCommonState, PICCommonClass, PIC_COMMON)
> 
> struct PICCommonClass {
>-    ISADeviceClass parent_class;
>+    DeviceClass parent_class;
> 
>     void (*pre_save)(PICCommonState *s);
>     void (*post_load)(PICCommonState *s);
>diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
>index c9954a7d99..411d12330b 100644
>--- a/include/hw/isa/isa.h
>+++ b/include/hw/isa/isa.h
>@@ -11,7 +11,7 @@
> #define ISA_NUM_IRQS 16
> 
> #define TYPE_ISA_DEVICE "isa-device"
>-OBJECT_DECLARE_TYPE(ISADevice, ISADeviceClass, ISA_DEVICE)
>+OBJECT_DECLARE_SIMPLE_TYPE(ISADevice, ISA_DEVICE)
> 
> #define TYPE_ISA_BUS "ISA"
> OBJECT_DECLARE_SIMPLE_TYPE(ISABus, ISA_BUS)
>@@ -48,10 +48,6 @@ struct IsaDmaClass {
>                              void *opaque);
> };
> 
>-struct ISADeviceClass {
>-    DeviceClass parent_class;
>-};
>-
> struct ISABus {
>     /*< private >*/
>     BusState parent_obj;
>diff --git a/include/hw/isa/superio.h b/include/hw/isa/superio.h
>index b9f5c19155..0dc45104d4 100644
>--- a/include/hw/isa/superio.h
>+++ b/include/hw/isa/superio.h
>@@ -44,7 +44,7 @@ typedef struct ISASuperIOFuncs {
> 
> struct ISASuperIOClass {
>     /*< private >*/
>-    ISADeviceClass parent_class;
>+    DeviceClass parent_class;
>     /*< public >*/
>     DeviceRealize parent_realize;
> 
>diff --git a/include/hw/timer/i8254_internal.h b/include/hw/timer/i8254_internal.h
>index a9a600d941..1761deb4cf 100644
>--- a/include/hw/timer/i8254_internal.h
>+++ b/include/hw/timer/i8254_internal.h
>@@ -58,7 +58,7 @@ struct PITCommonState {
> };
> 
> struct PITCommonClass {
>-    ISADeviceClass parent_class;
>+    DeviceClass parent_class;
> 
>     void (*set_channel_gate)(PITCommonState *s, PITChannelState *sc, int val);
>     void (*get_channel_info)(PITCommonState *s, PITChannelState *sc,

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

[PATCH v2 06.5/18] hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function

[Philippe Mathieu-Daudé]

In order to allow Frankenstein uses such plugging a PIIX3
IDE function on a ICH9 chipset (which already exposes AHCI
ports...) as:

  $ qemu-system-x86_64 -M q35 -device piix3-ide

add a kludge to automatically wires the IDE IRQs on an ISA
bus exposed by a PCI-to-ISA bridge (usually function #0).
Restrict this kludge to the PIIX3.

Reported-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9d876dd4a7..50975a16b3 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -170,6 +170,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
 
+    if (!d->irq[0] && !d->irq[1] && DEVICE_GET_CLASS(d)->user_creatable) {
+        /* kludge specific to TYPE_PIIX3_IDE */
+        Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, NULL);
+
+        if (!isabus) {
+            error_setg(errp, "Unable to find a unique ISA bus");
+            return;
+        }
+        d->irq[0] = isa_bus_get_irq(ISA_BUS(isabus), 14);
+        d->irq[1] = isa_bus_get_irq(ISA_BUS(isabus), 15);
+    }
     for (unsigned i = 0; i < ARRAY_SIZE(d->irq); i++) {
         if (!pci_piix_init_bus(d, i, errp)) {
             return;
@@ -202,6 +213,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
+    /*
+     * This function is part of a Super I/O chip and shouldn't be user
+     * creatable. However QEMU accepts impossible hardware setups such
+     * plugging a PIIX IDE function on a ICH ISA bridge.
+     * Keep this Frankenstein (ab)use working.
+     */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo piix3_ide_info = {
@@ -225,6 +243,8 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
+    /* Reason: Part of a Super I/O chip */
+    dc->user_creatable = false;
 }
 
 static const TypeInfo piix4_ide_info = {
-- 
2.38.1

Re: [PATCH v2 06.5/18] hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function

[Gerd Hoffmann]

On Mon, Feb 20, 2023 at 09:00:44AM +0100, Philippe Mathieu-Daudé wrote:
> In order to allow Frankenstein uses such plugging a PIIX3
> IDE function on a ICH9 chipset (which already exposes AHCI
> ports...) as:
> 
>   $ qemu-system-x86_64 -M q35 -device piix3-ide
> 
> add a kludge to automatically wires the IDE IRQs on an ISA
> bus exposed by a PCI-to-ISA bridge (usually function #0).
> Restrict this kludge to the PIIX3.

Well.  On physical hardware you have a config switch in the bios
setup which turns off sata and enables ide instead (i.e. the
chipset implements both and can be configured to expose the one
or the other).

If we want support ide for q35 we should IMHO do something simliar
instead of making piix-ide user-pluggable.  We already have -machine
q35,sata={on,off}.  We could extend that somehow, by adding
ide={on,off}, or by using storage={sata,ide,off} instead.

This has been discussed now and then in the past and the usual
conclusion was that there is little reason to implement that given
that you can just use the 'pc' machine type.  For guests that old
that they can't handle sata storage this is usually the better fit
anyway ...

take care,
  Gerd

Re: [PATCH v2 06.5/18] hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function

[Philippe Mathieu-Daudé]

On 20/2/23 10:10, Gerd Hoffmann wrote:
> On Mon, Feb 20, 2023 at 09:00:44AM +0100, Philippe Mathieu-Daudé wrote:
>> In order to allow Frankenstein uses such plugging a PIIX3
>> IDE function on a ICH9 chipset (which already exposes AHCI
>> ports...) as:
>>
>>    $ qemu-system-x86_64 -M q35 -device piix3-ide
>>
>> add a kludge to automatically wires the IDE IRQs on an ISA
>> bus exposed by a PCI-to-ISA bridge (usually function #0).
>> Restrict this kludge to the PIIX3.
> 
> Well.  On physical hardware you have a config switch in the bios
> setup which turns off sata and enables ide instead (i.e. the
> chipset implements both and can be configured to expose the one
> or the other).
> 
> If we want support ide for q35 we should IMHO do something simliar
> instead of making piix-ide user-pluggable.  We already have -machine
> q35,sata={on,off}.  We could extend that somehow, by adding
> ide={on,off}, or by using storage={sata,ide,off} instead.
> 
> This has been discussed now and then in the past and the usual
> conclusion was that there is little reason to implement that given
> that you can just use the 'pc' machine type.  For guests that old
> that they can't handle sata storage this is usually the better fit
> anyway ...

I think we might not using the same words, but agree on the goal :)

Since this has been discussed in the past, I suppose some users
want this config available. Why are they using this convoluted
config? Could it be due to lack of good documentation?

Do we really need a storage={sata,ide,off} flag? I don't see its
value. Help cloud users to have a sane config?

(old) boards exist with both IDE/SATA and we might want to emulate
some of them, but IMO such boards should be well modeled (Either
in C or later in declarative language) but not automagically created
from CLI.

IOW:

- using PIIX on Q35 (or any machine exposing a PCI bus) is
   acceptable, but the chipset should be instantiated as a whole.
   So to be clear I'm not against "-M q35 -device piix3", we should
   keep that working.

- we shouldn't try to maintain such Frankenstein corner cases which
   force us to add complex hacks, hard to remove, which limit us
   in implementing new features and cost us a lot of maintenance /
   testing time.

So I propose we deprecate this config so we can keep going forward.

(More generally I'd like to not allow instantiating part of chipset).

Re: [PATCH v2 06.5/18] hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function

[Philippe Mathieu-Daudé]

On 20/2/23 10:52, Philippe Mathieu-Daudé wrote:
> On 20/2/23 10:10, Gerd Hoffmann wrote:
>> On Mon, Feb 20, 2023 at 09:00:44AM +0100, Philippe Mathieu-Daudé wrote:
>>> In order to allow Frankenstein uses such plugging a PIIX3
>>> IDE function on a ICH9 chipset (which already exposes AHCI
>>> ports...) as:
>>>
>>>    $ qemu-system-x86_64 -M q35 -device piix3-ide
>>>
>>> add a kludge to automatically wires the IDE IRQs on an ISA
>>> bus exposed by a PCI-to-ISA bridge (usually function #0).
>>> Restrict this kludge to the PIIX3.
>>
>> Well.  On physical hardware you have a config switch in the bios
>> setup which turns off sata and enables ide instead (i.e. the
>> chipset implements both and can be configured to expose the one
>> or the other).
>>
>> If we want support ide for q35 we should IMHO do something simliar
>> instead of making piix-ide user-pluggable.  We already have -machine
>> q35,sata={on,off}.  We could extend that somehow, by adding
>> ide={on,off}, or by using storage={sata,ide,off} instead.
>>
>> This has been discussed now and then in the past and the usual
>> conclusion was that there is little reason to implement that given
>> that you can just use the 'pc' machine type.  For guests that old
>> that they can't handle sata storage this is usually the better fit
>> anyway ...
> 
> I think we might not using the same words, but agree on the goal :)
> 
> Since this has been discussed in the past, I suppose some users
> want this config available. Why are they using this convoluted
> config? Could it be due to lack of good documentation?
> 
> Do we really need a storage={sata,ide,off} flag? I don't see its
> value. Help cloud users to have a sane config?
> 
> (old) boards exist with both IDE/SATA and we might want to emulate
> some of them, but IMO such boards should be well modeled (Either
> in C or later in declarative language) but not automagically created
> from CLI.
> 
> IOW:
> 
> - using PIIX on Q35 (or any machine exposing a PCI bus) is
>    acceptable, but the chipset should be instantiated as a whole.
>    So to be clear I'm not against "-M q35 -device piix3", we should
>    keep that working.
> 
> - we shouldn't try to maintain such Frankenstein corner cases which
>    force us to add complex hacks, hard to remove, which limit us
>    in implementing new features and cost us a lot of maintenance /
>    testing time.
> 
> So I propose we deprecate this config so we can keep going forward.
> 
> (More generally I'd like to not allow instantiating part of chipset).

So there is a design problem here.

- ICH expose ISA bridge (fn0) with ISA IRQs.
- internal ICH SATA fn wires the IRQs 14/15

if we plug a cripple piix3-ide function which lookup for fn0 (ISA
bridge) to wire its IRQs 14/15, we end up having 2 devices able
to raise/lower IRQs while in the BQL iothread.
IOW, one device raise an IRQ, while the other lower the same IRQ...
thus the 1st device IRQ is acked from HW and missed from guest SW.

Daniel tested such config (2 drives used concurrently, one on IDE
and one on SATA) and reported "lost interrupt" from dmesg.

I haven't investigated using a SplitIrq object, but this doesn't
match real hw.

If user want to suffer from poor quality, we should find a different
(valid) config to add IDE drives to Q35 machine. Or maybe it is
a documentation problem, and we should redirect the users to the
best config.

Ref about similar problems:
https://lore.kernel.org/qemu-devel/b8d457d1-40b1-adb5-a2ac-98070f62ac1e@eik.bme.hu/
https://lore.kernel.org/qemu-devel/Y%2FSu8eB2nIO0INOX@redhat.com/

PD: For the remote-pci it is different because it is based on
PCIe which serializes MSIX IRQs, so no way to overwrite one.