All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton
@ 2021-05-18 21:55 Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls Philippe Mathieu-Daudé
                   ` (10 more replies)
  0 siblings, 11 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

Hi,

This series implements the suggestions from Markus analysis:
https://www.mail-archive.com/qemu-block@nongnu.org/msg84090.html
on the ISA bus.

There is still work to do (remove global get_system_io?) but
this is enough to convert a crash to an error message, and
the changes are simple, so posting now as RFC.

TBC...

Philippe Mathieu-Daudé (11):
  hw/isa: Explode pci_create_simple() calls
  hw/ide: Add PCIIDEState::isa_bus link
  hw/ide/piix: Set the ISA-bus QOM link
  hw/ide/via: Set the ISA-bus QOM link
  hw/isa: Extract isa_bus_get_irq() from isa_get_irq()
  hw/ide: Replace isa_get_irq() by isa_bus_get_irq()
  hw/isa: Simplify isa_get_irq()
  hw/isa: Extract bus part from isa_register_portio_list()
  hw/ide: Let ide_init_ioport() take an ISA bus argument instead of
    device
  hw/isa: Remove use of global isa bus
  hw/isa: Rename isabus singleton as 'g_isabus'

 include/hw/ide/internal.h |  3 +-
 include/hw/ide/pci.h      |  1 +
 include/hw/isa/isa.h      |  5 ++++
 hw/i386/pc_piix.c         |  7 +++--
 hw/ide/ioport.c           | 11 +++----
 hw/ide/isa.c              |  3 +-
 hw/ide/piix.c             | 22 +++++++++++---
 hw/ide/via.c              | 18 ++++++++++--
 hw/isa/isa-bus.c          | 61 +++++++++++++++++++++++++++++----------
 hw/isa/piix4.c            | 13 ++++++---
 hw/mips/fuloong2e.c       |  7 ++++-
 hw/ppc/pegasos2.c         |  7 ++++-
 12 files changed, 122 insertions(+), 36 deletions(-)

-- 
2.26.3




^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-21  7:09   ` Markus Armbruster
  2021-05-18 21:55 ` [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link Philippe Mathieu-Daudé
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

To be able to set a property on the ISA-IDE bridges objects
before they are realized, explode the pci_create_simple()
calls as pci_new() + pci_realize_and_unref().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc_piix.c   | 5 +++--
 hw/isa/piix4.c      | 3 ++-
 hw/mips/fuloong2e.c | 3 ++-
 hw/ppc/pegasos2.c   | 3 ++-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 30b8bd6ea92..fb606c14768 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -244,8 +244,9 @@ static void pc_init1(MachineState *machine,
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
 
-        dev = pci_create_simple(pci_bus, piix3_devfn + 1,
-                                xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+        dev = pci_new(piix3_devfn + 1,
+                      xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+        pci_realize_and_unref(dev, pci_bus, &error_abort);
         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");
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0fe7b69bc4c..d60f161ecf4 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -261,7 +261,8 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
         *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
     }
 
-    pci = pci_create_simple(pci_bus, devfn + 1, "piix4-ide");
+    pci = pci_new(devfn + 1, "piix4-ide");
+    pci_realize_and_unref(pci, pci_bus, &error_abort);
     pci_ide_create_devs(pci);
 
     pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index c1b8066a13b..40e9a645e1b 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -206,7 +206,8 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
                                           TYPE_VT82C686B_ISA);
     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
 
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
+    dev = pci_new(PCI_DEVFN(slot, 1), "via-ide");
+    pci_realize_and_unref(dev, pci_bus, &error_abort);
     pci_ide_create_devs(dev);
 
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 0bfd0928aa5..8486a2eb8c6 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -108,7 +108,8 @@ static void pegasos2_init(MachineState *machine)
                           qdev_get_gpio_in_named(mv, "gpp", 31));
 
     /* VT8231 function 1: IDE Controller */
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
+    dev = pci_new(PCI_DEVFN(12, 1), "via-ide");
+    pci_realize_and_unref(dev, pci_bus, &error_abort);
     pci_ide_create_devs(dev);
 
     /* VT8231 function 2-3: USB Ports */
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 23:05   ` BALATON Zoltan
  2021-05-18 21:55 ` [RFC PATCH 03/11] hw/ide/piix: Set the ISA-bus QOM link Philippe Mathieu-Daudé
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

IDE bus depends on ISA bus for IRQ/DMA.

Add an ISABus reference in PCIIDEState, and add link properties
to it in the PIIX and VIA objects (which inherit PCI_IDE).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/ide/pci.h |  1 +
 hw/ide/piix.c        | 11 ++++++++++-
 hw/ide/via.c         | 10 +++++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c422..e790722ed14 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -47,6 +47,7 @@ struct PCIIDEState {
     PCIDevice parent_obj;
     /*< public >*/
 
+    ISABus *isa_bus;
     IDEBus bus[2];
     BMDMAState bmdma[2];
     uint32_t secondary; /* used only for cmd646 */
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b9860e35a5c..48da68da37f 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -30,8 +30,9 @@
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/dma.h"
-
+#include "qapi/error.h"
 #include "hw/ide/pci.h"
+#include "hw/isa/isa.h"
 #include "trace.h"
 
 static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
@@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
     }
 }
 
+static Property piix_ide_properties[] = {
+    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
+                     TYPE_ISA_BUS, ISABus *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
@@ -221,6 +228,7 @@ 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;
+    device_class_set_props(dc, piix_ide_properties);
 }
 
 static const TypeInfo piix3_ide_info = {
@@ -249,6 +257,7 @@ 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;
+    device_class_set_props(dc, piix_ide_properties);
 }
 
 static const TypeInfo piix4_ide_info = {
diff --git a/hw/ide/via.c b/hw/ide/via.c
index be09912b334..65fdca6dcf4 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -28,8 +28,9 @@
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
 #include "sysemu/dma.h"
-
+#include "hw/isa/isa.h"
 #include "hw/ide/pci.h"
 #include "trace.h"
 
@@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev)
     }
 }
 
+static Property via_ide_properties[] = {
+    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
+                     TYPE_ISA_BUS, ISABus *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void via_ide_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     k->revision = 0x06;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    device_class_set_props(dc, via_ide_properties);
 }
 
 static const TypeInfo via_ide_info = {
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 03/11] hw/ide/piix: Set the ISA-bus QOM link
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 04/11] hw/ide/via: " Philippe Mathieu-Daudé
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

Set the ISA Bus link property on the PIIX IDE object from the
two unique users, the PC machine and the PIIX4 function.

Add a check in pci_piix_ide_realize() to be sure this property
is set.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc_piix.c |  2 ++
 hw/ide/piix.c     |  5 +++++
 hw/isa/piix4.c    | 10 +++++++---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fb606c14768..d799c8004df 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -246,6 +246,8 @@ static void pc_init1(MachineState *machine,
 
         dev = pci_new(piix3_devfn + 1,
                       xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+        object_property_set_link(OBJECT(dev), "isa-bus",
+                                 OBJECT(isa_bus), &error_abort);
         pci_realize_and_unref(dev, pci_bus, &error_abort);
         pci_ide_create_devs(dev);
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 48da68da37f..3aef9b1e21c 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -152,6 +152,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     PCIIDEState *d = PCI_IDE(dev);
     uint8_t *pci_conf = dev->config;
 
+    if (!d->isa_bus) {
+        error_setg(errp, "piix-ide: 'isa-bus' link not set");
+        return;
+    }
+
     pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
 
     bmdma_setup_bar(d);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index d60f161ecf4..ef11c2d1f8d 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -248,20 +248,24 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
+DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_busp, I2CBus **smbus)
 {
     PCIDevice *pci;
     DeviceState *dev;
+    ISABus *isa_bus;
     int devfn = PCI_DEVFN(10, 0);
 
     pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
                                           TYPE_PIIX4_PCI_DEVICE);
     dev = DEVICE(pci);
-    if (isa_bus) {
-        *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    if (isa_busp) {
+        *isa_busp = isa_bus;
     }
 
     pci = pci_new(devfn + 1, "piix4-ide");
+    object_property_set_link(OBJECT(pci), "isa-bus",
+                             OBJECT(isa_bus), &error_abort);
     pci_realize_and_unref(pci, pci_bus, &error_abort);
     pci_ide_create_devs(pci);
 
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 04/11] hw/ide/via: Set the ISA-bus QOM link
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 03/11] hw/ide/piix: Set the ISA-bus QOM link Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 05/11] hw/isa: Extract isa_bus_get_irq() from isa_get_irq() Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

Set the ISA Bus link property on the VIA IDE object from the
two unique users, the Fuloong and Pegasos machines.

Add a check in via_ide_realize() to be sure this property is set.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/ide/via.c        | 5 +++++
 hw/mips/fuloong2e.c | 4 ++++
 hw/ppc/pegasos2.c   | 4 ++++
 3 files changed, 13 insertions(+)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 65fdca6dcf4..654e15edfed 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -165,6 +165,11 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     uint8_t *pci_conf = dev->config;
     int i;
 
+    if (!d->isa_bus) {
+        error_setg(errp, "via-ide: 'isa-bus' link not set");
+        return;
+    }
+
     pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
     dev->wmask[PCI_INTERRUPT_LINE] = 0;
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 40e9a645e1b..7e644a701bc 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -201,12 +201,16 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
                                        I2CBus **i2c_bus)
 {
     PCIDevice *dev;
+    BusState *isa_bus;
 
     dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
                                           TYPE_VT82C686B_ISA);
+    isa_bus = qdev_get_child_bus(DEVICE(dev), "isa.0");
     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
 
     dev = pci_new(PCI_DEVFN(slot, 1), "via-ide");
+    object_property_set_link(OBJECT(dev), "isa-bus",
+                             OBJECT(isa_bus), &error_abort);
     pci_realize_and_unref(dev, pci_bus, &error_abort);
     pci_ide_create_devs(dev);
 
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 8486a2eb8c6..ed6ddc3569b 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -57,6 +57,7 @@ static void pegasos2_init(MachineState *machine)
     PCIBus *pci_bus;
     PCIDevice *dev;
     I2CBus *i2c_bus;
+    BusState *isa_bus;
     const char *fwname = machine->firmware ?: PROM_FILENAME;
     char *filename;
     int sz;
@@ -104,11 +105,14 @@ static void pegasos2_init(MachineState *machine)
     /* VT8231 function 0: PCI-to-ISA Bridge */
     dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
                                           TYPE_VT8231_ISA);
+    isa_bus = qdev_get_child_bus(DEVICE(dev), "isa.0");
     qdev_connect_gpio_out(DEVICE(dev), 0,
                           qdev_get_gpio_in_named(mv, "gpp", 31));
 
     /* VT8231 function 1: IDE Controller */
     dev = pci_new(PCI_DEVFN(12, 1), "via-ide");
+    object_property_set_link(OBJECT(dev), "isa-bus",
+                             OBJECT(isa_bus), &error_abort);
     pci_realize_and_unref(dev, pci_bus, &error_abort);
     pci_ide_create_devs(dev);
 
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 05/11] hw/isa: Extract isa_bus_get_irq() from isa_get_irq()
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 04/11] hw/ide/via: " Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 06/11] hw/ide: Replace isa_get_irq() by isa_bus_get_irq() Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

isa_get_irq() takes an ISADevice argument mostly to resolve
the ISA bus. Extract the bus logic to isa_bus_get_irq().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/isa/isa.h | 1 +
 hw/isa/isa-bus.c     | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index ddaae89a853..fd8b84d8007 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -91,6 +91,7 @@ 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_bus_get_irq(ISABus *bus, unsigned isairq);
 qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, unsigned isairq);
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 7820068e6e1..b946e6dc478 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -75,6 +75,12 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
     bus->irqs = irqs;
 }
 
+qemu_irq isa_bus_get_irq(ISABus *bus, unsigned isairq)
+{
+    assert(isairq < ISA_NUM_IRQS);
+    return bus->irqs[isairq];
+}
+
 /*
  * isa_get_irq() returns the corresponding qemu_irq entry for the i8259.
  *
@@ -84,8 +90,7 @@ 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_init_irq(ISADevice *dev, qemu_irq *p, unsigned isairq)
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 06/11] hw/ide: Replace isa_get_irq() by isa_bus_get_irq()
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 05/11] hw/isa: Extract isa_bus_get_irq() from isa_get_irq() Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 07/11] hw/isa: Simplify isa_get_irq() Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

Both PIIX/VIA objects inherit PCI_IDE, thus have a pointer to an
ISA bus. Pass this bus argument to isa_bus_get_irq() instead of
calling isa_get_irq() with a NULL device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/ide/piix.c | 2 +-
 hw/ide/via.c  | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 3aef9b1e21c..debbc0023dc 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -139,7 +139,7 @@ static void pci_piix_init_ports(PCIIDEState *d) {
         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
         ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
                         port_info[i].iobase2);
-        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+        ide_init2(&d->bus[i], isa_bus_get_irq(d->isa_bus, port_info[i].isairq));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
         d->bmdma[i].bus = &d->bus[i];
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 654e15edfed..53545aac474 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -106,6 +106,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
 static void via_ide_set_irq(void *opaque, int n, int level)
 {
     PCIDevice *d = PCI_DEVICE(opaque);
+    PCIIDEState *id = PCI_IDE(d);
 
     if (level) {
         d->config[0x70 + n * 8] |= 0x80;
@@ -113,7 +114,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
         d->config[0x70 + n * 8] &= ~0x80;
     }
 
-    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
+    qemu_set_irq(isa_bus_get_irq(id->isa_bus, 14 + n), level);
 }
 
 static void via_ide_reset(DeviceState *dev)
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 07/11] hw/isa: Simplify isa_get_irq()
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 06/11] hw/ide: Replace isa_get_irq() by isa_bus_get_irq() Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 08/11] hw/isa: Extract bus part from isa_register_portio_list() Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

Previous commit removed the calls to isa_get_irq() passing a NULL
ISADevice. Simplify the assertion, removing the use on the global
isabus object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/isa/isa-bus.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index b946e6dc478..65a26ac6c2c 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -89,7 +89,11 @@ qemu_irq isa_bus_get_irq(ISABus *bus, unsigned isairq)
  */
 qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
 {
-    assert(!dev || ISA_BUS(qdev_get_parent_bus(DEVICE(dev))) == isabus);
+    ISABus *isabus;
+
+    assert(dev);
+    isabus = isa_bus_from_device(dev);
+
     return isa_bus_get_irq(isabus, isairq);
 }
 
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 08/11] hw/isa: Extract bus part from isa_register_portio_list()
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 07/11] hw/isa: Simplify isa_get_irq() Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 09/11] hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

isa_register_portio_list() takes an ISADevice argument mostly
to resolve the ISA bus. Extract the bus logic to a new function:
isa_bus_register_portio_list().

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

diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index fd8b84d8007..ce31eef8858 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -139,6 +139,10 @@ void isa_register_portio_list(ISADevice *dev,
                               uint16_t start,
                               const MemoryRegionPortio *portio,
                               void *opaque, const char *name);
+void isa_bus_register_portio_list(ISABus *isabus, Object *owner,
+                                  PortioList *piolist, uint16_t start,
+                                  const MemoryRegionPortio *portio,
+                                  void *opaque, const char *name);
 
 static inline ISABus *isa_bus_from_device(ISADevice *d)
 {
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 65a26ac6c2c..c79d7e338b0 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -140,20 +140,29 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
     isa_init_ioport(dev, start);
 }
 
+void isa_bus_register_portio_list(ISABus *isabus, Object *owner,
+                                  PortioList *piolist, uint16_t start,
+                                  const MemoryRegionPortio *portio,
+                                  void *opaque, const char *name)
+{
+    assert(piolist && !piolist->owner);
+
+    portio_list_init(piolist, owner, portio, opaque, name);
+    portio_list_add(piolist, isabus->address_space_io, start);
+}
+
 void isa_register_portio_list(ISADevice *dev,
                               PortioList *piolist, uint16_t start,
                               const MemoryRegionPortio *pio_start,
                               void *opaque, const char *name)
 {
-    assert(piolist && !piolist->owner);
-
     /* 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.  */
     isa_init_ioport(dev, start);
 
-    portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
-    portio_list_add(piolist, isabus->address_space_io, start);
+    isa_bus_register_portio_list(isabus, OBJECT(dev), piolist, start,
+                                 pio_start, opaque, name);
 }
 
 static void isa_device_init(Object *obj)
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 09/11] hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (7 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 08/11] hw/isa: Extract bus part from isa_register_portio_list() Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 10/11] hw/isa: Remove use of global isa bus Philippe Mathieu-Daudé
  2021-05-18 21:55 ` [RFC PATCH 11/11] hw/isa: Rename isabus singleton as 'g_isabus' Philippe Mathieu-Daudé
  10 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

Both callers to ide_init_ioport() have access to the ISA bus of the
device, so can pass it directly. This allows ide_init_ioport() to
directly call isa_bus_register_portio_list().

Note, now the callers become the owner of the PortioList.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/ide/internal.h |  3 ++-
 hw/ide/ioport.c           | 11 ++++++-----
 hw/ide/isa.c              |  3 ++-
 hw/ide/piix.c             |  4 ++--
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 2d09162eeb7..141f53006a9 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -624,7 +624,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    int chs_trans, Error **errp);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_exit(IDEState *s);
-void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void ide_init_ioport(IDEBus *bus, ISABus *isabus, Object *owner,
+                     int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index b613ff3bbaf..be2309459e1 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+void ide_init_ioport(IDEBus *bus, ISABus *isabus, Object *owner,
+                     int iobase, int iobase2)
 {
     /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
        bridge has been setup properly to always register with ISA.  */
-    isa_register_portio_list(dev, &bus->portio_list,
-                             iobase, ide_portio_list, bus, "ide");
+    isa_bus_register_portio_list(isabus, owner, &bus->portio_list,
+                                iobase, ide_portio_list, bus, "ide");
 
     if (iobase2) {
-        isa_register_portio_list(dev, &bus->portio2_list,
-                                 iobase2, ide_portio2_list, bus, "ide");
+        isa_bus_register_portio_list(isabus, owner, &bus->portio2_list,
+                                     iobase2, ide_portio2_list, bus, "ide");
     }
 }
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 6bc19de2265..e7cf6714c8f 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -74,7 +74,8 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
     ISAIDEState *s = ISA_IDE(dev);
 
     ide_bus_new(&s->bus, sizeof(s->bus), dev, 0, 2);
-    ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
+    ide_init_ioport(&s->bus, isa_bus_from_device(isadev), OBJECT(dev),
+                    s->iobase, s->iobase2);
     isa_init_irq(isadev, &s->irq, s->isairq);
     ide_init2(&s->bus, s->irq);
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index debbc0023dc..0d6966fc7cb 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -137,8 +137,8 @@ static void pci_piix_init_ports(PCIIDEState *d) {
 
     for (i = 0; i < 2; i++) {
         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                        port_info[i].iobase2);
+        ide_init_ioport(&d->bus[i], d->isa_bus, OBJECT(d),
+                        port_info[i].iobase, port_info[i].iobase2);
         ide_init2(&d->bus[i], isa_bus_get_irq(d->isa_bus, port_info[i].isairq));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 10/11] hw/isa: Remove use of global isa bus
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (8 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 09/11] hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-19 16:11   ` Stefan Hajnoczi
  2021-05-18 21:55 ` [RFC PATCH 11/11] hw/isa: Rename isabus singleton as 'g_isabus' Philippe Mathieu-Daudé
  10 siblings, 1 reply; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

In the previous commit we removed all call to these functions
passing a NULL ISADevice argument. We can simplify and remove
the use of the global isabus object.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/isa/isa-bus.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index c79d7e338b0..a19e3688c28 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -136,6 +136,10 @@ static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 
 void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
+    ISABus *isabus;
+
+    assert(dev);
+    isabus = isa_bus_from_device(dev);
     memory_region_add_subregion(isabus->address_space_io, start, io);
     isa_init_ioport(dev, start);
 }
@@ -156,6 +160,11 @@ void isa_register_portio_list(ISADevice *dev,
                               const MemoryRegionPortio *pio_start,
                               void *opaque, const char *name)
 {
+    ISABus *isabus;
+
+    assert(dev);
+    isabus = isa_bus_from_device(dev);
+
     /* 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.  */
@@ -302,18 +311,20 @@ 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;
-    }
+    ISABus *isabus;
+
+    assert(dev);
+    isabus = isa_bus_from_device(dev);
 
     return isabus->address_space;
 }
 
 MemoryRegion *isa_address_space_io(ISADevice *dev)
 {
-    if (dev) {
-        return isa_bus_from_device(dev)->address_space_io;
-    }
+    ISABus *isabus;
+
+    assert(dev);
+    isabus = isa_bus_from_device(dev);
 
     return isabus->address_space_io;
 }
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [RFC PATCH 11/11] hw/isa: Rename isabus singleton as 'g_isabus'
  2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
                   ` (9 preceding siblings ...)
  2021-05-18 21:55 ` [RFC PATCH 10/11] hw/isa: Remove use of global isa bus Philippe Mathieu-Daudé
@ 2021-05-18 21:55 ` Philippe Mathieu-Daudé
  2021-05-19 16:18   ` Stefan Hajnoczi
  10 siblings, 1 reply; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-18 21:55 UTC (permalink / raw)
  To: John Snow, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

To make explicit the isabus singleton isn't used anywhere else,
move it's static declaration locally to isa_bus_new() and rename
it as 'g_isabus'.

Unfortunately we provide the get_system_io() call which expose
an unique I/O bus to a machine, and the ISA bus rely on this I/O
bus, so we can not remove the ISA bus singleton until we remove
the get_system_io() API.

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

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index a19e3688c28..422eb9615f4 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -26,8 +26,6 @@
 #include "sysemu/sysemu.h"
 #include "hw/isa/isa.h"
 
-static ISABus *isabus;
-
 static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *isabus_get_fw_dev_path(DeviceState *dev);
 
@@ -55,7 +53,10 @@ static const TypeInfo isa_bus_info = {
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
                     MemoryRegion *address_space_io, Error **errp)
 {
-    if (isabus) {
+    static ISABus *g_isabus;
+    ISABus *isabus;
+
+    if (g_isabus) {
         error_setg(errp, "Can't create a second ISA bus");
         return NULL;
     }
@@ -67,6 +68,7 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
     isabus = ISA_BUS(qbus_create(TYPE_ISA_BUS, dev, NULL));
     isabus->address_space = address_space;
     isabus->address_space_io = address_space_io;
+    g_isabus = isabus;
     return isabus;
 }
 
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-18 21:55 ` [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link Philippe Mathieu-Daudé
@ 2021-05-18 23:05   ` BALATON Zoltan
  2021-05-19 21:49     ` John Snow
  2021-05-20  7:41     ` Mark Cave-Ayland
  0 siblings, 2 replies; 23+ messages in thread
From: BALATON Zoltan @ 2021-05-18 23:05 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, qemu-devel, Markus Armbruster, Stefan Hajnoczi,
	John Snow

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

On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote:
> IDE bus depends on ISA bus for IRQ/DMA.
>
> Add an ISABus reference in PCIIDEState, and add link properties
> to it in the PIIX and VIA objects (which inherit PCI_IDE).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> include/hw/ide/pci.h |  1 +
> hw/ide/piix.c        | 11 ++++++++++-
> hw/ide/via.c         | 10 +++++++++-
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c422..e790722ed14 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -47,6 +47,7 @@ struct PCIIDEState {
>     PCIDevice parent_obj;
>     /*< public >*/
>
> +    ISABus *isa_bus;

I'm not sure that this belongs here. Previously we managed to remove 
device specific fields from this structure so it's now really just holds 
stuff related to PCI IDE (except the remaining "secondary" field specific 
to CMD646). PCI IDE normaly has nothing to do with ISA except for those 
south bridges that have IDE with legacy mode. So this ISABus reference 
should be in those south bridges instead. But that may need a new subclass 
just for this so putting it here is just avoiding boilerplate of declaring 
new subclasses in piix and via-ide. I can sympathise with that but I'd 
still prefer to keep it off here but I wonder if there's a way to do that 
without subclassing and storing an ISABus ref? If I understand correctly 
this ISABus ref is just needed to get appropriate ISA irqs. But could we 
just store a ref to those irqs directly so we don't need to keep the ref 
to the ISA bus? There's already a qemu_irq in BMDMAState but I'm not sure 
how those are set and if you could store an isa irq there to simplify 
this. I don't know the details and could not detangle it by a brief look 
so not sure it can be done but conceptually it feels better to keep PCI 
IDE separate from ISA and let it raise either PCI irq or ISA irq as 
needed. For that a ref to the irq should be enough and that can either 
come from a PCI bus (which is normaly expected for PCI IDE) or an ISA 
bridge for legacy modes. Hope it makes sense and you get what I'm trying 
to say. (Longer term we may want to make it changeable also after the 
device is created to allow switching between legacy and PCI mode but so 
far we could get away without emulating that so it's not a requirement 
just something to consider when you're changing this. The real problem 
that prevents switching modes is not irq I think but ioports and that ISA 
devices are not configurable after creating them but that would need 
QOM'ifying ISA emulation which probably does not worth the effort unless 
we come across some guest that needs this.)

Regards,
BALATON Zoltan

>     IDEBus bus[2];
>     BMDMAState bmdma[2];
>     uint32_t secondary; /* used only for cmd646 */
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index b9860e35a5c..48da68da37f 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -30,8 +30,9 @@
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/dma.h"
> -
> +#include "qapi/error.h"
> #include "hw/ide/pci.h"
> +#include "hw/isa/isa.h"
> #include "trace.h"
>
> static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
> @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>     }
> }
>
> +static Property piix_ide_properties[] = {
> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
> +                     TYPE_ISA_BUS, ISABus *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> static void piix3_ide_class_init(ObjectClass *klass, void *data)
> {
> @@ -221,6 +228,7 @@ 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;
> +    device_class_set_props(dc, piix_ide_properties);
> }
>
> static const TypeInfo piix3_ide_info = {
> @@ -249,6 +257,7 @@ 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;
> +    device_class_set_props(dc, piix_ide_properties);
> }
>
> static const TypeInfo piix4_ide_info = {
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index be09912b334..65fdca6dcf4 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -28,8 +28,9 @@
> #include "hw/pci/pci.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> +#include "qapi/error.h"
> #include "sysemu/dma.h"
> -
> +#include "hw/isa/isa.h"
> #include "hw/ide/pci.h"
> #include "trace.h"
>
> @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev)
>     }
> }
>
> +static Property via_ide_properties[] = {
> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
> +                     TYPE_ISA_BUS, ISABus *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void via_ide_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>     k->revision = 0x06;
>     k->class_id = PCI_CLASS_STORAGE_IDE;
>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +    device_class_set_props(dc, via_ide_properties);
> }
>
> static const TypeInfo via_ide_info = {
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 10/11] hw/isa: Remove use of global isa bus
  2021-05-18 21:55 ` [RFC PATCH 10/11] hw/isa: Remove use of global isa bus Philippe Mathieu-Daudé
@ 2021-05-19 16:11   ` Stefan Hajnoczi
  0 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2021-05-19 16:11 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, qemu-devel, Markus Armbruster, John Snow

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

On Tue, May 18, 2021 at 11:55:44PM +0200, Philippe Mathieu-Daudé wrote:
> In the previous commit we removed all call to these functions
> passing a NULL ISADevice argument. We can simplify and remove
> the use of the global isabus object.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/isa/isa-bus.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 11/11] hw/isa: Rename isabus singleton as 'g_isabus'
  2021-05-18 21:55 ` [RFC PATCH 11/11] hw/isa: Rename isabus singleton as 'g_isabus' Philippe Mathieu-Daudé
@ 2021-05-19 16:18   ` Stefan Hajnoczi
  0 siblings, 0 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2021-05-19 16:18 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, qemu-devel, Markus Armbruster, John Snow

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

On Tue, May 18, 2021 at 11:55:45PM +0200, Philippe Mathieu-Daudé wrote:
> @@ -55,7 +53,10 @@ static const TypeInfo isa_bus_info = {
>  ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
>                      MemoryRegion *address_space_io, Error **errp)
>  {
> -    if (isabus) {
> +    static ISABus *g_isabus;

QEMU doesn't use Hungarian notation, g_isabus isn't a global variable so
the prefix is confusing, and g_ conflicts with GLib's namespace so the
rename is odd. I suggest keeping the name unchanged and making the
commit message "remove the global isabus variable".

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-18 23:05   ` BALATON Zoltan
@ 2021-05-19 21:49     ` John Snow
  2021-05-20  0:46       ` BALATON Zoltan
  2021-05-20  7:41     ` Mark Cave-Ayland
  1 sibling, 1 reply; 23+ messages in thread
From: John Snow @ 2021-05-19 21:49 UTC (permalink / raw)
  To: BALATON Zoltan, Philippe Mathieu-Daudé
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, qemu-devel, Markus Armbruster, Stefan Hajnoczi

On 5/18/21 7:05 PM, BALATON Zoltan wrote:
> On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote:
>> IDE bus depends on ISA bus for IRQ/DMA.
>>
>> Add an ISABus reference in PCIIDEState, and add link properties
>> to it in the PIIX and VIA objects (which inherit PCI_IDE).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> include/hw/ide/pci.h |  1 +
>> hw/ide/piix.c        | 11 ++++++++++-
>> hw/ide/via.c         | 10 +++++++++-
>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index d8384e1c422..e790722ed14 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -47,6 +47,7 @@ struct PCIIDEState {
>>     PCIDevice parent_obj;
>>     /*< public >*/
>>
>> +    ISABus *isa_bus;
> 
> I'm not sure that this belongs here. Previously we managed to remove 
> device specific fields from this structure so it's now really just holds 
> stuff related to PCI IDE (except the remaining "secondary" field 
> specific to CMD646). PCI IDE normaly has nothing to do with ISA except 
> for those south bridges that have IDE with legacy mode. So this ISABus 
> reference should be in those south bridges instead. But that may need a 

by "those south bridges" I assume you are referring to the integrated 
PIIX and VIA controller implementations.

> new subclass just for this so putting it here is just avoiding 
> boilerplate of declaring new subclasses in piix and via-ide. I can 
> sympathise with that but I'd still prefer to keep it off here but I 
> wonder if there's a way to do that without subclassing and storing an 
> ISABus ref? If I understand correctly this ISABus ref is just needed to 
> get appropriate ISA irqs. But could we just store a ref to those irqs 

It looks like it's just the IRQs, yeah.

> directly so we don't need to keep the ref to the ISA bus? There's 

I think the idea actually is to formalize the dependency of these models 
on the ISA bus instead of hacking / faking one. I think we DO want the 
dependency.

> already a qemu_irq in BMDMAState but I'm not sure how those are set and 
> if you could store an isa irq there to simplify this. I don't know the 
> details and could not detangle it by a brief look so not sure it can be 
> done but conceptually it feels better to keep PCI IDE separate from ISA 
> and let it raise either PCI irq or ISA irq as needed. For that a ref to 
> the irq should be enough and that can either come from a PCI bus (which 
> is normaly expected for PCI IDE) or an ISA bridge for legacy modes. Hope 
> it makes sense and you get what I'm trying to say. (Longer term we may 
> want to make it changeable also after the device is created to allow 
> switching between legacy and PCI mode but so far we could get away 
> without emulating that so it's not a requirement just something to 
> consider when you're changing this. The real problem that prevents 
> switching modes is not irq I think but ioports and that ISA devices are 
> not configurable after creating them but that would need QOM'ifying ISA 
> emulation which probably does not worth the effort unless we come across 
> some guest that needs this.)
> 
> Regards,
> BALATON Zoltan
> 

I assume the idea here is that PCIIDE does not technically need "ISA" to 
provide ioport access to the ATA drives, so taxonomically it's odd for 
the generic/abstract PCIIDE device to require an ISA bus.

Am I understanding correctly?

>>     IDEBus bus[2];
>>     BMDMAState bmdma[2];
>>     uint32_t secondary; /* used only for cmd646 */
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index b9860e35a5c..48da68da37f 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -30,8 +30,9 @@
>> #include "sysemu/block-backend.h"
>> #include "sysemu/blockdev.h"
>> #include "sysemu/dma.h"
>> -
>> +#include "qapi/error.h"
>> #include "hw/ide/pci.h"
>> +#include "hw/isa/isa.h"
>> #include "trace.h"
>>
>> static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
>> @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>     }
>> }
>>
>> +static Property piix_ide_properties[] = {
>> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
>> +                     TYPE_ISA_BUS, ISABus *),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>> static void piix3_ide_class_init(ObjectClass *klass, void *data)
>> {
>> @@ -221,6 +228,7 @@ 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;
>> +    device_class_set_props(dc, piix_ide_properties);
>> }
>>
>> static const TypeInfo piix3_ide_info = {
>> @@ -249,6 +257,7 @@ 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;
>> +    device_class_set_props(dc, piix_ide_properties);
>> }
>>
>> static const TypeInfo piix4_ide_info = {
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index be09912b334..65fdca6dcf4 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -28,8 +28,9 @@
>> #include "hw/pci/pci.h"
>> #include "migration/vmstate.h"
>> #include "qemu/module.h"
>> +#include "qapi/error.h"
>> #include "sysemu/dma.h"
>> -
>> +#include "hw/isa/isa.h"
>> #include "hw/ide/pci.h"
>> #include "trace.h"
>>
>> @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev)
>>     }
>> }
>>
>> +static Property via_ide_properties[] = {
>> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
>> +                     TYPE_ISA_BUS, ISABus *),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> static void via_ide_class_init(ObjectClass *klass, void *data)
>> {
>>     DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, 
>> void *data)
>>     k->revision = 0x06;
>>     k->class_id = PCI_CLASS_STORAGE_IDE;
>>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>> +    device_class_set_props(dc, via_ide_properties);
>> }
>>
>> static const TypeInfo via_ide_info = {
>>



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-19 21:49     ` John Snow
@ 2021-05-20  0:46       ` BALATON Zoltan
  2021-05-20  8:35         ` Stefan Hajnoczi
  0 siblings, 1 reply; 23+ messages in thread
From: BALATON Zoltan @ 2021-05-20  0:46 UTC (permalink / raw)
  To: John Snow
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, qemu-devel, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

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

On Wed, 19 May 2021, John Snow wrote:
> On 5/18/21 7:05 PM, BALATON Zoltan wrote:
>> On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote:
>>> IDE bus depends on ISA bus for IRQ/DMA.
>>> 
>>> Add an ISABus reference in PCIIDEState, and add link properties
>>> to it in the PIIX and VIA objects (which inherit PCI_IDE).
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> include/hw/ide/pci.h |  1 +
>>> hw/ide/piix.c        | 11 ++++++++++-
>>> hw/ide/via.c         | 10 +++++++++-
>>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>> index d8384e1c422..e790722ed14 100644
>>> --- a/include/hw/ide/pci.h
>>> +++ b/include/hw/ide/pci.h
>>> @@ -47,6 +47,7 @@ struct PCIIDEState {
>>>     PCIDevice parent_obj;
>>>     /*< public >*/
>>> 
>>> +    ISABus *isa_bus;
>> 
>> I'm not sure that this belongs here. Previously we managed to remove device 
>> specific fields from this structure so it's now really just holds stuff 
>> related to PCI IDE (except the remaining "secondary" field specific to 
>> CMD646). PCI IDE normaly has nothing to do with ISA except for those south 
>> bridges that have IDE with legacy mode. So this ISABus reference should be 
>> in those south bridges instead. But that may need a 
>
> by "those south bridges" I assume you are referring to the integrated PIIX 
> and VIA controller implementations.

Yes, those are that also have an ISA bridge so the IDE in those can use 
either ISA or PCI IRQs but we probably only emulate one mode. At least 
that's the case for via-ide which we have gone into great detail before 
and concluded we can't cleanly switch between legacy ISA or PCI mode and 
the pegasos2 needs hard coded ISA interrupts even when in PCI mode so we 
only emulate that.

Apart from that this PCI IDE is also used by CMD646 and sii3112 that are 
pure PCI IDE devices without any ISA dependency so that's why I think we 
should not need this ISABus here to keep this implementing PCI IDE and 
only keep ISA in the south bridge models.

>> new subclass just for this so putting it here is just avoiding boilerplate 
>> of declaring new subclasses in piix and via-ide. I can sympathise with that 
>> but I'd still prefer to keep it off here but I wonder if there's a way to 
>> do that without subclassing and storing an ISABus ref? If I understand 
>> correctly this ISABus ref is just needed to get appropriate ISA irqs. But 
>> could we just store a ref to those irqs 
>
> It looks like it's just the IRQs, yeah.
>
>> directly so we don't need to keep the ref to the ISA bus? There's 
>
> I think the idea actually is to formalize the dependency of these models on 
> the ISA bus instead of hacking / faking one. I think we DO want the 
> dependency.

Right, but only piix and via depend on ISA so the dependency should be in 
those not in PCI IDE in my opinion. But I don't mind too much so if it 
would be too difficult to put it elsewhere I don't mind introducing this 
ISABus field but we should at least look if it could be avoided first.

>> already a qemu_irq in BMDMAState but I'm not sure how those are set and if 
>> you could store an isa irq there to simplify this. I don't know the details 
>> and could not detangle it by a brief look so not sure it can be done but 
>> conceptually it feels better to keep PCI IDE separate from ISA and let it 
>> raise either PCI irq or ISA irq as needed. For that a ref to the irq should 
>> be enough and that can either come from a PCI bus (which is normaly 
>> expected for PCI IDE) or an ISA bridge for legacy modes. Hope it makes 
>> sense and you get what I'm trying to say. (Longer term we may want to make 
>> it changeable also after the device is created to allow switching between 
>> legacy and PCI mode but so far we could get away without emulating that so 
>> it's not a requirement just something to consider when you're changing 
>> this. The real problem that prevents switching modes is not irq I think but 
>> ioports and that ISA devices are not configurable after creating them but 
>> that would need QOM'ifying ISA emulation which probably does not worth the 
>> effort unless we come across some guest that needs this.)
>> 
>> Regards,
>> BALATON Zoltan
>> 
>
> I assume the idea here is that PCIIDE does not technically need "ISA" to 
> provide ioport access to the ATA drives, so taxonomically it's odd for the 
> generic/abstract PCIIDE device to require an ISA bus.
>
> Am I understanding correctly?

I'm not sure I understand all of the IDE emulation but in my view PCI IDE 
should be independent of ISA so instead of adding a reference to an ISA 
bus to PCIIDEState maybe it's enough to set the irqs used by PCI IDE to 
the appropriate irq to raise which could be an ISA interrupt for the south 
bridges in legacy mode or a PCI irq for PCI cards and that way we don't 
need a dependency on ISABus in PCI IDE. But I'm not sure how IDE 
interrupts are set so don't know if that would work so it's just an idea 
to avoid introducing ISA into PCI IDE where it does not seem to belong.

A simpler way keeping the current code may be to subclass PCI IDE in piix 
and via and put the ISABus ref in those subclasses but that's more boiler 
plate and the result may not be simpler so while conceptually may be 
cleaner the code may be longer and harder to understand that way. So 
cleaning up the interrupt handling could make it simpler and also avoid 
the subclasses but that needs more work to detangle how IDE interrupts are 
emulated and add some clean way to set them if that's not yet available. 
But I don't completely understand what the qemu_irqs are in BMDMAState and 
if those could be connected to an ISA interrupt or some more changes would 
be needed.

Regards,
BALATON Zoltan

>>>     IDEBus bus[2];
>>>     BMDMAState bmdma[2];
>>>     uint32_t secondary; /* used only for cmd646 */
>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>> index b9860e35a5c..48da68da37f 100644
>>> --- a/hw/ide/piix.c
>>> +++ b/hw/ide/piix.c
>>> @@ -30,8 +30,9 @@
>>> #include "sysemu/block-backend.h"
>>> #include "sysemu/blockdev.h"
>>> #include "sysemu/dma.h"
>>> -
>>> +#include "qapi/error.h"
>>> #include "hw/ide/pci.h"
>>> +#include "hw/isa/isa.h"
>>> #include "trace.h"
>>> 
>>> static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
>>> @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>     }
>>> }
>>> 
>>> +static Property piix_ide_properties[] = {
>>> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
>>> +                     TYPE_ISA_BUS, ISABus *),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>>> static void piix3_ide_class_init(ObjectClass *klass, void *data)
>>> {
>>> @@ -221,6 +228,7 @@ 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;
>>> +    device_class_set_props(dc, piix_ide_properties);
>>> }
>>> 
>>> static const TypeInfo piix3_ide_info = {
>>> @@ -249,6 +257,7 @@ 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;
>>> +    device_class_set_props(dc, piix_ide_properties);
>>> }
>>> 
>>> static const TypeInfo piix4_ide_info = {
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index be09912b334..65fdca6dcf4 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -28,8 +28,9 @@
>>> #include "hw/pci/pci.h"
>>> #include "migration/vmstate.h"
>>> #include "qemu/module.h"
>>> +#include "qapi/error.h"
>>> #include "sysemu/dma.h"
>>> -
>>> +#include "hw/isa/isa.h"
>>> #include "hw/ide/pci.h"
>>> #include "trace.h"
>>> 
>>> @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev)
>>>     }
>>> }
>>> 
>>> +static Property via_ide_properties[] = {
>>> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
>>> +                     TYPE_ISA_BUS, ISABus *),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> static void via_ide_class_init(ObjectClass *klass, void *data)
>>> {
>>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, 
>>> void *data)
>>>     k->revision = 0x06;
>>>     k->class_id = PCI_CLASS_STORAGE_IDE;
>>>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>> +    device_class_set_props(dc, via_ide_properties);
>>> }
>>> 
>>> static const TypeInfo via_ide_info = {
>>> 
>
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-18 23:05   ` BALATON Zoltan
  2021-05-19 21:49     ` John Snow
@ 2021-05-20  7:41     ` Mark Cave-Ayland
  2021-05-20  8:29       ` Mark Cave-Ayland
  1 sibling, 1 reply; 23+ messages in thread
From: Mark Cave-Ayland @ 2021-05-20  7:41 UTC (permalink / raw)
  To: BALATON Zoltan, Philippe Mathieu-Daudé
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	qemu-devel, Markus Armbruster, Stefan Hajnoczi, John Snow

On 19/05/2021 00:05, BALATON Zoltan wrote:

> On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote:
>> IDE bus depends on ISA bus for IRQ/DMA.
>>
>> Add an ISABus reference in PCIIDEState, and add link properties
>> to it in the PIIX and VIA objects (which inherit PCI_IDE).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> include/hw/ide/pci.h |  1 +
>> hw/ide/piix.c        | 11 ++++++++++-
>> hw/ide/via.c         | 10 +++++++++-
>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index d8384e1c422..e790722ed14 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -47,6 +47,7 @@ struct PCIIDEState {
>>     PCIDevice parent_obj;
>>     /*< public >*/
>>
>> +    ISABus *isa_bus;
> 
> I'm not sure that this belongs here. Previously we managed to remove device specific 
> fields from this structure so it's now really just holds stuff related to PCI IDE 
> (except the remaining "secondary" field specific to CMD646). PCI IDE normaly has 
> nothing to do with ISA except for those south bridges that have IDE with legacy mode. 
> So this ISABus reference should be in those south bridges instead. But that may need 
> a new subclass just for this so putting it here is just avoiding boilerplate of 
> declaring new subclasses in piix and via-ide. I can sympathise with that but I'd 
> still prefer to keep it off here but I wonder if there's a way to do that without 
> subclassing and storing an ISABus ref? If I understand correctly this ISABus ref is 
> just needed to get appropriate ISA irqs. But could we just store a ref to those irqs 
> directly so we don't need to keep the ref to the ISA bus? There's already a qemu_irq 
> in BMDMAState but I'm not sure how those are set and if you could store an isa irq 
> there to simplify this. I don't know the details and could not detangle it by a brief 
> look so not sure it can be done but conceptually it feels better to keep PCI IDE 
> separate from ISA and let it raise either PCI irq or ISA irq as needed. For that a 
> ref to the irq should be enough and that can either come from a PCI bus (which is 
> normaly expected for PCI IDE) or an ISA bridge for legacy modes. Hope it makes sense 
> and you get what I'm trying to say. (Longer term we may want to make it changeable 
> also after the device is created to allow switching between legacy and PCI mode but 
> so far we could get away without emulating that so it's not a requirement just 
> something to consider when you're changing this. The real problem that prevents 
> switching modes is not irq I think but ioports and that ISA devices are not 
> configurable after creating them but that would need QOM'ifying ISA emulation which 
> probably does not worth the effort unless we come across some guest that needs this.)

Right. I've had a quick look over the patchset and the IRQ changes look good: the 
part I'm not keen on is adding the property links for the ISABus directly to these 
devices, since as Zoltan correctly points out this is handled by a PCI-ISA bridge in 
the PCI host bridge and not the PCI device itself.

One of the better diagrams to explain how this wires together is on the QEMU wiki at 
https://wiki.qemu.org/Features/Q35. Bear in mind that the PCI-ISA bridge is optional 
and not all PCI bridges have them.

I spent a bit of time trying to figure how we know which devices have a PCI-ISA 
bridge and then realised that it is those devices with a PCI class id of 
PCI_CLASS_BRIDGE_ISA. If you grep for this then it is possible to see that there are 
a couple of places that check for whether a PCI device is a PCI-ISA bridge by 
checking the PCI class id for PCI_CLASS_BRIDGE_ISA instead of using a QOM type.

This gives me the following idea:

1) Create an abstract TYPE_PCI_ISA_BRIDGE QOM type with parent PCI_DEVICE and change 
the existing devices with PCI_CLASS_BRIDGE_ISA so that they have TYPE_PCI_ISA_BRIDGE 
as a parent

2) In the PCI_ISA_BRIDGE realize function add the following common code to 
instantiate the ISABus and remove it from the individual PCI_CLASS_BRIDGE_ISA devices:

static void pci_isa_bridge_realize(DeviceState *dev)
{
     PCIISABridge *s = PCI_ISA_BRIDGE(dev);

     s->isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
                              pci_address_space_io(dev), errp);

     /* FIXME: this should probably use errp */
     if (!s->isa_bus) {
         return;
     }
}

You may also be able to do something similar with the 8259 IRQ initialisation 
although I haven't really looked at that.

3) Add a new pci_isa_bridge_get_isabus(PCIDevice *d) function that the devices such 
as via-ide can use to obtain a reference to the ISABus from their own PCIDevice. It 
should hopefully be quite simple like this:

ISABus *pci_isa_bridge_get_isabus(PCIDevice *d)
{
     PCIISABridge *s = PCI_ISA_BRIDGE(d);

     return s->isa_bus;
}


I think this is the best solution since it avoids either 1) having to embed ISABus 
unconditionally into PCIHostState and therefore bringing in all ISA devices for all 
PCI builds and 2) it avoids having to manually link the ISABus directly for the 
affected devices. It also allows for a future cleanup where PCI-ISA bridges can be 
detected by checking QOM type rather than PCI class id directly.

Once that is done the follow on work could be to investigate how TYPE_PCI_ISA_BRIDGE 
devices can use the ISABus to handle the switch between legacy mode and PCI mode, but 
that's certainly a job for another day.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-20  7:41     ` Mark Cave-Ayland
@ 2021-05-20  8:29       ` Mark Cave-Ayland
  0 siblings, 0 replies; 23+ messages in thread
From: Mark Cave-Ayland @ 2021-05-20  8:29 UTC (permalink / raw)
  To: BALATON Zoltan, Philippe Mathieu-Daudé
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	qemu-devel, Markus Armbruster, Stefan Hajnoczi, John Snow

On 20/05/2021 08:41, Mark Cave-Ayland wrote:

> 3) Add a new pci_isa_bridge_get_isabus(PCIDevice *d) function that the devices such 
> as via-ide can use to obtain a reference to the ISABus from their own PCIDevice. It 
> should hopefully be quite simple like this:
> 
> ISABus *pci_isa_bridge_get_isabus(PCIDevice *d)
> {
>     PCIISABridge *s = PCI_ISA_BRIDGE(d);
> 
>     return s->isa_bus;
> }

Oops - have just realised that PCIDevice in this case is the PCI/ISA device and not 
the bridge itself. This means there is a bit more work to do, perhaps something like:

    ISABus *pci_device_get_isabus(PCIDevice *d)
    {
         PCIBus *bus = pci_get_bus(d);
         PCIDeviceClass *k;
         int devfn;

         for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
             PCIDevice *pd = bus->devices[devfn];
             PCIDeviceClass *k = PCI_DEVICE_GET_CLASS(d);
             PCIISABridge *s;
             if (object_dynamic_cast(OBJECT(pd), TYPE_PCI_ISA_BRIDGE)) {
                 s = PCI_ISA_BRIDGE(pd);
                 return s->isa_bus;
             }
         }

         return NULL;
    }

Given that a PCI-ISA bridge effectively manages the bottom part of the single IO 
address space then I believe there can only be one PCI-ISA bridge per PCI host 
bridge, and therefore bus.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-20  0:46       ` BALATON Zoltan
@ 2021-05-20  8:35         ` Stefan Hajnoczi
  2021-05-20  8:56           ` Mark Cave-Ayland
  0 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2021-05-20  8:35 UTC (permalink / raw)
  To: BALATON Zoltan
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	John Snow, Mark Cave-Ayland, qemu-devel, Markus Armbruster,
	Philippe Mathieu-Daudé

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

On Thu, May 20, 2021 at 02:46:47AM +0200, BALATON Zoltan wrote:
> On Wed, 19 May 2021, John Snow wrote:
> > On 5/18/21 7:05 PM, BALATON Zoltan wrote:
> > > On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote:
> > > > IDE bus depends on ISA bus for IRQ/DMA.
> > > > 
> > > > Add an ISABus reference in PCIIDEState, and add link properties
> > > > to it in the PIIX and VIA objects (which inherit PCI_IDE).
> > > > 
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > ---
> > > > include/hw/ide/pci.h |  1 +
> > > > hw/ide/piix.c        | 11 ++++++++++-
> > > > hw/ide/via.c         | 10 +++++++++-
> > > > 3 files changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> > > > index d8384e1c422..e790722ed14 100644
> > > > --- a/include/hw/ide/pci.h
> > > > +++ b/include/hw/ide/pci.h
> > > > @@ -47,6 +47,7 @@ struct PCIIDEState {
> > > >     PCIDevice parent_obj;
> > > >     /*< public >*/
> > > > 
> > > > +    ISABus *isa_bus;
> > > 
> > > I'm not sure that this belongs here. Previously we managed to remove
> > > device specific fields from this structure so it's now really just
> > > holds stuff related to PCI IDE (except the remaining "secondary"
> > > field specific to CMD646). PCI IDE normaly has nothing to do with
> > > ISA except for those south bridges that have IDE with legacy mode.
> > > So this ISABus reference should be in those south bridges instead.
> > > But that may need a
> > 
> > by "those south bridges" I assume you are referring to the integrated
> > PIIX and VIA controller implementations.
> 
> Yes, those are that also have an ISA bridge so the IDE in those can use
> either ISA or PCI IRQs but we probably only emulate one mode. At least
> that's the case for via-ide which we have gone into great detail before and
> concluded we can't cleanly switch between legacy ISA or PCI mode and the
> pegasos2 needs hard coded ISA interrupts even when in PCI mode so we only
> emulate that.
> 
> Apart from that this PCI IDE is also used by CMD646 and sii3112 that are
> pure PCI IDE devices without any ISA dependency so that's why I think we
> should not need this ISABus here to keep this implementing PCI IDE and only
> keep ISA in the south bridge models.
> 
> > > new subclass just for this so putting it here is just avoiding
> > > boilerplate of declaring new subclasses in piix and via-ide. I can
> > > sympathise with that but I'd still prefer to keep it off here but I
> > > wonder if there's a way to do that without subclassing and storing
> > > an ISABus ref? If I understand correctly this ISABus ref is just
> > > needed to get appropriate ISA irqs. But could we just store a ref to
> > > those irqs
> > 
> > It looks like it's just the IRQs, yeah.
> > 
> > > directly so we don't need to keep the ref to the ISA bus? There's
> > 
> > I think the idea actually is to formalize the dependency of these models
> > on the ISA bus instead of hacking / faking one. I think we DO want the
> > dependency.
> 
> Right, but only piix and via depend on ISA so the dependency should be in
> those not in PCI IDE in my opinion. But I don't mind too much so if it would
> be too difficult to put it elsewhere I don't mind introducing this ISABus
> field but we should at least look if it could be avoided first.
> 
> > > already a qemu_irq in BMDMAState but I'm not sure how those are set
> > > and if you could store an isa irq there to simplify this. I don't
> > > know the details and could not detangle it by a brief look so not
> > > sure it can be done but conceptually it feels better to keep PCI IDE
> > > separate from ISA and let it raise either PCI irq or ISA irq as
> > > needed. For that a ref to the irq should be enough and that can
> > > either come from a PCI bus (which is normaly expected for PCI IDE)
> > > or an ISA bridge for legacy modes. Hope it makes sense and you get
> > > what I'm trying to say. (Longer term we may want to make it
> > > changeable also after the device is created to allow switching
> > > between legacy and PCI mode but so far we could get away without
> > > emulating that so it's not a requirement just something to consider
> > > when you're changing this. The real problem that prevents switching
> > > modes is not irq I think but ioports and that ISA devices are not
> > > configurable after creating them but that would need QOM'ifying ISA
> > > emulation which probably does not worth the effort unless we come
> > > across some guest that needs this.)
> > > 
> > > Regards,
> > > BALATON Zoltan
> > > 
> > 
> > I assume the idea here is that PCIIDE does not technically need "ISA" to
> > provide ioport access to the ATA drives, so taxonomically it's odd for
> > the generic/abstract PCIIDE device to require an ISA bus.
> > 
> > Am I understanding correctly?
> 
> I'm not sure I understand all of the IDE emulation but in my view PCI IDE
> should be independent of ISA so instead of adding a reference to an ISA bus
> to PCIIDEState maybe it's enough to set the irqs used by PCI IDE to the
> appropriate irq to raise which could be an ISA interrupt for the south
> bridges in legacy mode or a PCI irq for PCI cards and that way we don't need
> a dependency on ISABus in PCI IDE. But I'm not sure how IDE interrupts are
> set so don't know if that would work so it's just an idea to avoid
> introducing ISA into PCI IDE where it does not seem to belong.
> 
> A simpler way keeping the current code may be to subclass PCI IDE in piix
> and via and put the ISABus ref in those subclasses but that's more boiler
> plate and the result may not be simpler so while conceptually may be cleaner
> the code may be longer and harder to understand that way. So cleaning up the
> interrupt handling could make it simpler and also avoid the subclasses but
> that needs more work to detangle how IDE interrupts are emulated and add
> some clean way to set them if that's not yet available. But I don't
> completely understand what the qemu_irqs are in BMDMAState and if those
> could be connected to an ISA interrupt or some more changes would be needed.

I realized I don't really understand how ISA IDE and PCI IDE interact in
PIIX3:

- ISA IDE has well-known PIO registers that are always present?

- PCI IDE has the same registers, but the BAR must be mapped and PCI IO
  space access must be enabled?

- ISA IDE has a hardcoded ISA irq number?

- PCI IDE has a normal PCI irq that is routed like any legacy PCI INTx
  irq?

- What combinations of ISA enabled/disabled and PCI enabled/disabled
  need to be supported?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-20  8:35         ` Stefan Hajnoczi
@ 2021-05-20  8:56           ` Mark Cave-Ayland
  2021-05-20 12:18             ` BALATON Zoltan
  0 siblings, 1 reply; 23+ messages in thread
From: Mark Cave-Ayland @ 2021-05-20  8:56 UTC (permalink / raw)
  To: Stefan Hajnoczi, BALATON Zoltan
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	John Snow, qemu-devel, Markus Armbruster,
	Philippe Mathieu-Daudé

On 20/05/2021 09:35, Stefan Hajnoczi wrote:

> I realized I don't really understand how ISA IDE and PCI IDE interact in
> PIIX3:
> 
> - ISA IDE has well-known PIO registers that are always present?
> 
> - PCI IDE has the same registers, but the BAR must be mapped and PCI IO
>    space access must be enabled?
> 
> - ISA IDE has a hardcoded ISA irq number?
> 
> - PCI IDE has a normal PCI irq that is routed like any legacy PCI INTx
>    irq?
> 
> - What combinations of ISA enabled/disabled and PCI enabled/disabled
>    need to be supported?

Yeah a lot of this discussion happened several months back in the Pegasos threads, 
but here is my understanding:

- Older legacy PCI devices such as IDE controllers connected via a host containing a 
PCI-ISA bridge can be switched by the guest OS into PCI legacy (also known as 
compatibility mode) via a PCI config space register so that IO space accesses, IRQs 
(and possible DMA?) are done via the ISA bus

- QEMU handles the IO memory accesses fine, since in these cases isa_bus_new() is 
given the IO space by pci_address_space_io(dev) so IO space access generally "just works"

- Currently it is the responsibility of these older PCI devices to determine how they 
have been configured and either use e.g. pci_set_irq() or qemu_raise_irq() on the ISA 
IRQ for interrupts

- Generally ISA IRQs are fixed as per the old AT-style PCs so IDE would be 14/15

My thoughts above were about how to allow a PCIDevice to locate its ISABus if it is 
connected to a bus with a PCI-ISA bridge to potentially allow access to ISA IRQs and 
DMA if configured in PCI legacy mode.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
  2021-05-20  8:56           ` Mark Cave-Ayland
@ 2021-05-20 12:18             ` BALATON Zoltan
  0 siblings, 0 replies; 23+ messages in thread
From: BALATON Zoltan @ 2021-05-20 12:18 UTC (permalink / raw)
  To: Mark Cave-Ayland
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	John Snow, qemu-devel, Markus Armbruster, Stefan Hajnoczi,
	Philippe Mathieu-Daudé

On Thu, 20 May 2021, Mark Cave-Ayland wrote:
> On 20/05/2021 09:35, Stefan Hajnoczi wrote:
>> I realized I don't really understand how ISA IDE and PCI IDE interact in
>> PIIX3:

You're not alone with that. :-)

>> - ISA IDE has well-known PIO registers that are always present?
>> 
>> - PCI IDE has the same registers, but the BAR must be mapped and PCI IO
>>    space access must be enabled?
>> 
>> - ISA IDE has a hardcoded ISA irq number?
>> 
>> - PCI IDE has a normal PCI irq that is routed like any legacy PCI INTx
>>    irq?
>> 
>> - What combinations of ISA enabled/disabled and PCI enabled/disabled
>>    need to be supported?
>
> Yeah a lot of this discussion happened several months back in the Pegasos 
> threads, but here is my understanding:
>
> - Older legacy PCI devices such as IDE controllers connected via a host 
> containing a PCI-ISA bridge can be switched by the guest OS into PCI legacy 
> (also known as compatibility mode) via a PCI config space register so that IO 
> space accesses, IRQs (and possible DMA?) are done via the ISA bus

Maybe you can look at the VIA VT82C686B and VT8231 docs that have some 
info on how this works for these integrated "super south bridges" (superio 
+ PCI bridge). The concept for PIIX may be similar but registers may be 
different. There are at least two modes: a legacy mode that uses normal 
ISA IDE ioports and IRQs so that older drivers work without change and 
some native mode that may be full PCI mode with BARs and PCI irq-s or some 
strange non-100& native mode (as Linux calls it) on some systems such as 
pegasos2 where in this mode port addresses can be set indepently but IRQs 
are still hard coded to use ISA IRQs regardless of what the documented IRQ 
reg is set to. (I'm still not sure how this is implemented in hardware but 
that's how guests expect it to work and this caused some problem with 
implementing this as another machine using via-ide (the MIPS fuloong2e) 
has either legacy or a real native mode with IRQ also set by a register 
(it's still not a PCI IRQ I think as an ISA IRQ is selected by this 
register but instead of the legacy IRQ 14+15 in this mode it's using a 
single interrupt for both channels set by a reg e.g. 9 while normal PCI 
interrupts may be connected somewhere else). On pegasos2 where setting 
this IRQ reg does not change the IRQ 14+15 mapping, there native mode only 
changes ports to use configured port numbers instead of the legacy 1f0-170 
ones but keeping the legacy ISA IRQs. We have to model this otherwise 
guests don't work because they often expect things to work a certain way 
without checking.

Maybe the IDE in these integrated south bridges are not really PCI IDE but 
in native mode behaves more or less like a real PCI IDE card so we just 
reusing the QEMU PCI IDE model to emulate them but we also need to emulate 
the quirks of their native mode in some cases. Currently we likely only 
emulate one of the possible modes that work with the guests and not fully 
emulate all modes due to ISA model not being QOM that can be added or 
removed on demand so if we set it up in the beginning then we're stuck 
with legacy mode as we can't really disable the legacy io ports any more 
to switch to native mode without hacking into ISA emulation. (A similar 
problem was also found with other superio devices in the VIA south bridge, 
such as serial, parallel, FDC, that also have configurable io ports but we 
can't emulate that as ISA superio devices can only be created with port 
addresses but these addresses cannot be set later. Fortunately guests set 
it up once at startup and usually don't change the default so if we put it 
there it works.)

> - QEMU handles the IO memory accesses fine, since in these cases 
> isa_bus_new() is given the IO space by pci_address_space_io(dev) so IO space 
> access generally "just works"
>
> - Currently it is the responsibility of these older PCI devices to determine 
> how they have been configured and either use e.g. pci_set_irq() or 
> qemu_raise_irq() on the ISA IRQ for interrupts

This is probably OK considering that these IDE device can be in different 
modes and probably the only part that knows which mode it's in is the 
device itself so it has to determine what IRQ to use. But as in the 
via-ide case the modes (and thus IRQs used) can be different based on 
which south bridge or machine it's used in so maybe it should be the 
higher level object (south bridge or machine) which instantiates via-ide 
that decides which irq to use. So I wonder if it would be possible to 
remove the decision of using pci_set_irq or using an ISA irq from via-ide 
and only pass it a qemu_irq that it can raise without caring where it's 
connected and the south bridge or machine that creates via-ide could then 
pass it an appropriate irq (PCI or ISA based on how it's configured). This 
seems the simplest way but due to the current entanglement of IRQ handling 
in the different models it's not clear to me how to implement this or if 
it's possible at all.

> - Generally ISA IRQs are fixed as per the old AT-style PCs so IDE would be 
> 14/15
>
> My thoughts above were about how to allow a PCIDevice to locate its ISABus if 
> it is connected to a bus with a PCI-ISA bridge to potentially allow access to 
> ISA IRQs and DMA if configured in PCI legacy mode.

In my opinion a PCI device should have no knowledge about ISA at all, it's 
probably the south bridge that uses this PCI IDE device that should 
connect it to ISA as that's the one that knows about ISA bus or ISA IRQs. 
I'm a bit concerned about the performance of your proposed 
pci_device_get_isabus() function that walks the PCI bus to get an ISA bus 
to get an ISA IRQ. Do you really want to do that every time an IRQ is 
raised which can be quite frequent? I think it would be better to find the 
IRQ once when the PCI IDE device is set up in it's parent object that also 
already has a reference to ISA bus and just pass the IRQ to the IDE device 
removing the need for it to know about ISA. I think that's a better 
direction to go but don't know how to get there.

As a side note, in my understanding the main problem with fully emulating 
these south bridges is that ISA emulation predates everything and it's not 
fully QOM-ified so a lot of it still uses global vars and legacy init 
functions that allow creating these ISA devices but not changing them 
afterwards in any way. This prevents cleanly modelling south bridges that 
can switch between legacy and PCI mode for IDE for example because we can 
create legacy IDE ports but then we cannot switch tnem off to use BARs 
instead when the mode change so without ovethauling the ISA emulation we 
can only emulate one mode or the other. I've stumbled upon this for 
via-ide and VT8231 serial and decided I don't want to try cleaning up ISA 
as it's a basic device class that could break a lot of things so I did not 
feel like wanting to attempt that. This patch set from Philippe tries to 
go a bit further in that direction but maybe not all the way for the same 
reason that it's a big task with a lot of potential breakage. So I'm OK 
with leaving it as it is now as it works well enough or make small clean 
ups as possible without breaking too many things. If this introduces an 
ISABus link in PCI IDE I can live with that knowing that it's to avoid 
more extensive changes or adding new subclasses but if there's a simpler 
way by passing IRQs directly and it could be done that seems to be a 
cleaner way to me.

Regards,
BALATON Zoltan


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls
  2021-05-18 21:55 ` [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls Philippe Mathieu-Daudé
@ 2021-05-21  7:09   ` Markus Armbruster
  0 siblings, 0 replies; 23+ messages in thread
From: Markus Armbruster @ 2021-05-21  7:09 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Elena Ufimtseva, John G Johnson, Thomas Huth, Jagannathan Raman,
	Mark Cave-Ayland, qemu-devel, Stefan Hajnoczi, John Snow

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> To be able to set a property on the ISA-IDE bridges objects
> before they are realized, explode the pci_create_simple()
> calls as pci_new() + pci_realize_and_unref().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/pc_piix.c   | 5 +++--
>  hw/isa/piix4.c      | 3 ++-
>  hw/mips/fuloong2e.c | 3 ++-
>  hw/ppc/pegasos2.c   | 3 ++-
>  4 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 30b8bd6ea92..fb606c14768 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -244,8 +244,9 @@ static void pc_init1(MachineState *machine,
>      if (pcmc->pci_enabled) {
>          PCIDevice *dev;
>  
> -        dev = pci_create_simple(pci_bus, piix3_devfn + 1,
> -                                xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
> +        dev = pci_new(piix3_devfn + 1,
> +                      xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
> +        pci_realize_and_unref(dev, pci_bus, &error_abort);
>          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");

This replaces pci_create_simple()'s &error_fatal by &error_abort.
Intentional?  If yes, the commit message should briefly explain why
errors are not expected to happen.

Same for the other hunks.

[...]



^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2021-05-21  7:10 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls Philippe Mathieu-Daudé
2021-05-21  7:09   ` Markus Armbruster
2021-05-18 21:55 ` [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link Philippe Mathieu-Daudé
2021-05-18 23:05   ` BALATON Zoltan
2021-05-19 21:49     ` John Snow
2021-05-20  0:46       ` BALATON Zoltan
2021-05-20  8:35         ` Stefan Hajnoczi
2021-05-20  8:56           ` Mark Cave-Ayland
2021-05-20 12:18             ` BALATON Zoltan
2021-05-20  7:41     ` Mark Cave-Ayland
2021-05-20  8:29       ` Mark Cave-Ayland
2021-05-18 21:55 ` [RFC PATCH 03/11] hw/ide/piix: Set the ISA-bus QOM link Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 04/11] hw/ide/via: " Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 05/11] hw/isa: Extract isa_bus_get_irq() from isa_get_irq() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 06/11] hw/ide: Replace isa_get_irq() by isa_bus_get_irq() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 07/11] hw/isa: Simplify isa_get_irq() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 08/11] hw/isa: Extract bus part from isa_register_portio_list() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 09/11] hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 10/11] hw/isa: Remove use of global isa bus Philippe Mathieu-Daudé
2021-05-19 16:11   ` Stefan Hajnoczi
2021-05-18 21:55 ` [RFC PATCH 11/11] hw/isa: Rename isabus singleton as 'g_isabus' Philippe Mathieu-Daudé
2021-05-19 16:18   ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.