* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).