* [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
@ 2012-05-21 13:13 Jan Kiszka
2012-05-21 13:15 ` [Qemu-devel] [PATCH 2/2] pci: Add INTx routing notifier Jan Kiszka
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Jan Kiszka @ 2012-05-21 13:13 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: Alex Williamson, Marcelo Tosatti, Avi Kivity
Add a PCI IRQ path discovery function that walks from a given device to
the host bridge, returning the IRQ number that is reported to the
attached interrupt controller. For this purpose, another PCI bridge
callback function is introduced: map_host_irq. It is so far only
implemented by the PIIX3, other host bridges can be added later on as
required.
Will be used for KVM PCI device assignment.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/alpha_typhoon.c | 2 +-
hw/apb_pci.c | 2 +-
hw/bonito.c | 2 +-
hw/grackle_pci.c | 1 +
hw/gt64xxx.c | 2 +-
hw/pci.c | 23 ++++++++++++++++++++---
hw/pci.h | 7 +++++--
hw/pci_internals.h | 1 +
hw/piix_pci.c | 15 ++++++++++++---
hw/ppc4xx_pci.c | 2 +-
hw/ppce500_pci.c | 2 +-
hw/prep_pci.c | 2 +-
hw/sh_pci.c | 2 +-
hw/spapr_pci.c | 2 +-
hw/unin_pci.c | 4 ++--
hw/versatile_pci.c | 2 +-
16 files changed, 51 insertions(+), 20 deletions(-)
diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
index 872e112..fc2e4b3 100644
--- a/hw/alpha_typhoon.c
+++ b/hw/alpha_typhoon.c
@@ -764,7 +764,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
&s->pchip.reg_io);
b = pci_register_bus(&s->host.busdev.qdev, "pci",
- typhoon_set_irq, sys_map_irq, s,
+ typhoon_set_irq, sys_map_irq, NULL, s,
&s->pchip.reg_mem, addr_space_io, 0, 64);
s->host.bus = b;
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 7e28808..819bf1d 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -366,7 +366,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
memory_region_add_subregion(get_system_memory(), mem_base, &d->pci_mmio);
d->bus = pci_register_bus(&d->busdev.qdev, "pci",
- pci_apb_set_irq, pci_pbm_map_irq, d,
+ pci_apb_set_irq, pci_pbm_map_irq, NULL, d,
&d->pci_mmio,
get_system_io(),
0, 32);
diff --git a/hw/bonito.c b/hw/bonito.c
index 77786f8..7ce5993 100644
--- a/hw/bonito.c
+++ b/hw/bonito.c
@@ -750,7 +750,7 @@ PCIBus *bonito_init(qemu_irq *pic)
dev = qdev_create(NULL, "Bonito-pcihost");
pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
- pci_bonito_map_irq, pic, get_system_memory(),
+ pci_bonito_map_irq, NULL, pic, get_system_memory(),
get_system_io(),
0x28, 32);
pcihost->bus = b;
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index 81ff3a3..f47d9fe 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -85,6 +85,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
pci_grackle_set_irq,
pci_grackle_map_irq,
+ NULL,
pic,
&d->pci_mmio,
address_space_io,
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index a2d0e5a..a97bbf0 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -1093,7 +1093,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
d = FROM_SYSBUS(GT64120State, s);
d->pci.bus = pci_register_bus(&d->busdev.qdev, "pci",
gt64120_pci_set_irq, gt64120_pci_map_irq,
- pic,
+ NULL, pic,
get_system_memory(),
get_system_io(),
PCI_DEVFN(18, 0), 4);
diff --git a/hw/pci.c b/hw/pci.c
index 439f3ce..df4d93e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -298,10 +298,11 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
}
void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
- void *irq_opaque, int nirq)
+ pci_map_host_irq_fn map_host_irq, void *irq_opaque, int nirq)
{
bus->set_irq = set_irq;
bus->map_irq = map_irq;
+ bus->map_host_irq = map_host_irq;
bus->irq_opaque = irq_opaque;
bus->nirq = nirq;
bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
@@ -316,7 +317,7 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
PCIBus *pci_register_bus(DeviceState *parent, const char *name,
pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
- void *irq_opaque,
+ pci_map_host_irq_fn map_host_irq, void *irq_opaque,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
uint8_t devfn_min, int nirq)
@@ -325,7 +326,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
bus = pci_bus_new(parent, name, address_space_mem,
address_space_io, devfn_min);
- pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
+ pci_bus_irqs(bus, set_irq, map_irq, map_host_irq, irq_opaque, nirq);
return bus;
}
@@ -1067,6 +1068,22 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
pci_change_irq_level(pci_dev, irq_num, change);
}
+int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
+{
+ PCIBus *bus;
+
+ for (;;) {
+ bus = pci_dev->bus;
+ irq_num = bus->map_irq(pci_dev, irq_num);
+ if (bus->map_host_irq) {
+ break;
+ }
+ pci_dev = bus->parent_dev;
+ assert(pci_dev);
+ }
+ return bus->map_host_irq(bus->irq_opaque, irq_num);
+}
+
/***********************************************************/
/* monitor info on PCI */
diff --git a/hw/pci.h b/hw/pci.h
index c3cacce..29bc8bf 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -276,6 +276,7 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
+typedef int (*pci_map_host_irq_fn)(void *opaque, int irq_num);
typedef enum {
PCI_HOTPLUG_DISABLED,
@@ -295,15 +296,17 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
MemoryRegion *address_space_io,
uint8_t devfn_min);
void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
- void *irq_opaque, int nirq);
+ pci_map_host_irq_fn map_host_irq, void *irq_opaque,
+ int nirq);
int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
PCIBus *pci_register_bus(DeviceState *parent, const char *name,
pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
- void *irq_opaque,
+ pci_map_host_irq_fn map_host_irq, void *irq_opaque,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
uint8_t devfn_min, int nirq);
+int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
void pci_device_reset(PCIDevice *dev);
void pci_bus_reset(PCIBus *bus);
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
index 96690b7..a92353e 100644
--- a/hw/pci_internals.h
+++ b/hw/pci_internals.h
@@ -19,6 +19,7 @@ struct PCIBus {
uint8_t devfn_min;
pci_set_irq_fn set_irq;
pci_map_irq_fn map_irq;
+ pci_map_host_irq_fn map_host_irq;
pci_hotplug_fn hotplug;
DeviceState *hotplug_qdev;
void *irq_opaque;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 09e84f5..cfea97c 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -89,6 +89,7 @@ struct PCII440FXState {
#define I440FX_SMRAM 0x72
static void piix3_set_irq(void *opaque, int pirq, int level);
+static int piix3_map_host_irq(void *opaque, int pci_intx);
static void piix3_write_config_xen(PCIDevice *dev,
uint32_t address, uint32_t val, int len);
@@ -308,13 +309,13 @@ static PCIBus *i440fx_common_init(const char *device_name,
if (xen_enabled()) {
piix3 = DO_UPCAST(PIIX3State, dev,
pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
- pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+ pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq, NULL,
piix3, XEN_PIIX_NUM_PIRQS);
} else {
piix3 = DO_UPCAST(PIIX3State, dev,
pci_create_simple_multifunction(b, -1, true, "PIIX3"));
- pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
- PIIX_NUM_PIRQS);
+ pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3_map_host_irq,
+ piix3, PIIX_NUM_PIRQS);
}
piix3->pic = pic;
*isa_bus = DO_UPCAST(ISABus, qbus,
@@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
piix3_set_irq_level(piix3, pirq, level);
}
+static int piix3_map_host_irq(void *opaque, int pci_intx)
+{
+ PIIX3State *piix3 = opaque;
+ int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
+
+ return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
+}
+
/* irq routing is changed. so rebuild bitmap */
static void piix3_update_irq_levels(PIIX3State *piix3)
{
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 203c3cd..224c4a0 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -343,7 +343,7 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
}
b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, ppc4xx_pci_set_irq,
- ppc4xx_pci_map_irq, s->irq, get_system_memory(),
+ ppc4xx_pci_map_irq, NULL, s->irq, get_system_memory(),
get_system_io(), 0, 4);
s->pci_state.bus = b;
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 0f60b24..dd924ae 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -318,7 +318,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
}
b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
- mpc85xx_pci_map_irq, s->irq, address_space_mem,
+ mpc85xx_pci_map_irq, NULL, s->irq, address_space_mem,
address_space_io, PCI_DEVFN(0x11, 0), 4);
s->pci_state.bus = b;
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 38dbff4..9d7bec7 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -108,7 +108,7 @@ static int raven_pcihost_init(SysBusDevice *dev)
}
bus = pci_register_bus(&h->busdev.qdev, NULL,
- prep_set_irq, prep_map_irq, s->irq,
+ prep_set_irq, prep_map_irq, NULL, s->irq,
address_space_mem, address_space_io, 0, 4);
h->bus = bus;
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index 0cfac46..1cea12b 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -120,7 +120,7 @@ static int sh_pci_device_init(SysBusDevice *dev)
sysbus_init_irq(dev, &s->irq[i]);
}
s->bus = pci_register_bus(&s->busdev.qdev, "pci",
- sh_pci_set_irq, sh_pci_map_irq,
+ sh_pci_set_irq, sh_pci_map_irq, NULL,
s->irq,
get_system_memory(),
get_system_io(),
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 25b400a..4a43062 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -306,7 +306,7 @@ static int spapr_phb_init(SysBusDevice *s)
bus = pci_register_bus(&phb->busdev.qdev,
phb->busname ? phb->busname : phb->dtbusname,
- pci_spapr_set_irq, pci_spapr_map_irq, phb,
+ pci_spapr_set_irq, pci_spapr_map_irq, NULL, phb,
&phb->memspace, &phb->iospace,
PCI_DEVFN(0, 0), PCI_NUM_PINS);
phb->host_state.bus = bus;
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 409bcd4..056e3bc 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -227,7 +227,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
d->host_state.bus = pci_register_bus(dev, "pci",
pci_unin_set_irq, pci_unin_map_irq,
- pic,
+ NULL, pic,
&d->pci_mmio,
address_space_io,
PCI_DEVFN(11, 0), 4);
@@ -293,7 +293,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
d->host_state.bus = pci_register_bus(dev, "pci",
pci_unin_set_irq, pci_unin_map_irq,
- pic,
+ NULL, pic,
&d->pci_mmio,
address_space_io,
PCI_DEVFN(11, 0), 4);
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index ae53a8b..90c400e 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -68,7 +68,7 @@ static int pci_vpb_init(SysBusDevice *dev)
sysbus_init_irq(dev, &s->irq[i]);
}
bus = pci_register_bus(&dev->qdev, "pci",
- pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
+ pci_vpb_set_irq, pci_vpb_map_irq, NULL, s->irq,
get_system_memory(), get_system_io(),
PCI_DEVFN(11, 0), 4);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 2/2] pci: Add INTx routing notifier
2012-05-21 13:13 [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Jan Kiszka
@ 2012-05-21 13:15 ` Jan Kiszka
2012-05-21 14:13 ` Alex Williamson
2012-05-21 14:10 ` [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Alex Williamson
` (3 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Jan Kiszka @ 2012-05-21 13:15 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: Alex Williamson, Marcelo Tosatti, Avi Kivity
This per-device notifier shall be triggered by any interrupt router
along the path of a device's legacy interrupt signal on routing changes.
For simplicity reasons and as this is a slow path anyway, no further
details on the routing changes are provided. Instead, the callback is
expected to use pci_device_get_host_irq to check the effect of the
change.
Will be used by KVM PCI device assignment.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/pci.c | 19 +++++++++++++++++++
hw/pci.h | 7 +++++++
hw/pci_bridge.c | 8 ++++++++
hw/piix_pci.c | 2 ++
4 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/hw/pci.c b/hw/pci.c
index df4d93e..27ecc37 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1084,6 +1084,25 @@ int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
return bus->map_host_irq(bus->irq_opaque, irq_num);
}
+void pci_bus_fire_intx_routing_notifier(PCIBus *bus)
+{
+ PCIDevice *dev;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
+ dev = bus->devices[i];
+ if (dev && dev->intx_routing_notifier) {
+ dev->intx_routing_notifier(dev);
+ }
+ }
+}
+
+void pci_device_set_intx_routing_notifier(PCIDevice *pci_dev,
+ INTxRoutingNotifier notifier)
+{
+ pci_dev->intx_routing_notifier = notifier;
+}
+
/***********************************************************/
/* monitor info on PCI */
diff --git a/hw/pci.h b/hw/pci.h
index 29bc8bf..eaf7695 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -173,6 +173,7 @@ typedef struct PCIDeviceClass {
const char *romfile;
} PCIDeviceClass;
+typedef void (*INTxRoutingNotifier)(PCIDevice *dev);
typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector,
MSIMessage msg);
typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector);
@@ -248,6 +249,9 @@ struct PCIDevice {
MemoryRegion rom;
uint32_t rom_bar;
+ /* INTx routing notifier */
+ INTxRoutingNotifier intx_routing_notifier;
+
/* MSI-X notifiers */
MSIVectorUseNotifier msix_vector_use_notifier;
MSIVectorReleaseNotifier msix_vector_release_notifier;
@@ -307,6 +311,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
MemoryRegion *address_space_io,
uint8_t devfn_min, int nirq);
int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
+void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
+void pci_device_set_intx_routing_notifier(PCIDevice *pci_dev,
+ INTxRoutingNotifier notifier);
void pci_device_reset(PCIDevice *dev);
void pci_bus_reset(PCIBus *bus);
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index e0832b4..2490b2f 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -292,6 +292,13 @@ void pci_bridge_reset(DeviceState *qdev)
pci_set_word(conf + PCI_BRIDGE_CONTROL, 0);
}
+static void pci_bridge_intx_routing_update(PCIDevice *dev)
+{
+ PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
+
+ pci_bus_fire_intx_routing_notifier(&br->sec_bus);
+}
+
/* default qdev initialization function for PCI-to-PCI bridge */
int pci_bridge_initfn(PCIDevice *dev)
{
@@ -327,6 +334,7 @@ int pci_bridge_initfn(PCIDevice *dev)
sec_bus->address_space_io = &br->address_space_io;
memory_region_init(&br->address_space_io, "pci_bridge_io", 65536);
pci_bridge_region_init(br);
+ pci_device_set_intx_routing_notifier(dev, pci_bridge_intx_routing_update);
QLIST_INIT(&sec_bus->child);
QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
return 0;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index cfea97c..b0d1325 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -414,6 +414,8 @@ static void piix3_write_config(PCIDevice *dev,
if (ranges_overlap(address, len, PIIX_PIRQC, 4)) {
PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
int pic_irq;
+
+ pci_bus_fire_intx_routing_notifier(piix3->dev.bus);
piix3_update_irq_levels(piix3);
for (pic_irq = 0; pic_irq < PIIX_NUM_PIC_IRQS; pic_irq++) {
piix3_set_irq_pic(piix3, pic_irq);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 13:13 [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Jan Kiszka
2012-05-21 13:15 ` [Qemu-devel] [PATCH 2/2] pci: Add INTx routing notifier Jan Kiszka
@ 2012-05-21 14:10 ` Alex Williamson
2012-05-21 14:36 ` Avi Kivity
` (2 subsequent siblings)
4 siblings, 0 replies; 26+ messages in thread
From: Alex Williamson @ 2012-05-21 14:10 UTC (permalink / raw)
To: Jan Kiszka
Cc: Alexey Kardashevskiy, Avi Kivity, Marcelo Tosatti, qemu-devel,
Michael S. Tsirkin
On Mon, 2012-05-21 at 10:13 -0300, Jan Kiszka wrote:
> Add a PCI IRQ path discovery function that walks from a given device to
> the host bridge, returning the IRQ number that is reported to the
> attached interrupt controller. For this purpose, another PCI bridge
> callback function is introduced: map_host_irq. It is so far only
> implemented by the PIIX3, other host bridges can be added later on as
> required.
>
> Will be used for KVM PCI device assignment.
And VFIO. Not so different from the get_irq function I added in the
VFIO tree.
Acked-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> hw/alpha_typhoon.c | 2 +-
> hw/apb_pci.c | 2 +-
> hw/bonito.c | 2 +-
> hw/grackle_pci.c | 1 +
> hw/gt64xxx.c | 2 +-
> hw/pci.c | 23 ++++++++++++++++++++---
> hw/pci.h | 7 +++++--
> hw/pci_internals.h | 1 +
> hw/piix_pci.c | 15 ++++++++++++---
> hw/ppc4xx_pci.c | 2 +-
> hw/ppce500_pci.c | 2 +-
> hw/prep_pci.c | 2 +-
> hw/sh_pci.c | 2 +-
> hw/spapr_pci.c | 2 +-
> hw/unin_pci.c | 4 ++--
> hw/versatile_pci.c | 2 +-
> 16 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
> index 872e112..fc2e4b3 100644
> --- a/hw/alpha_typhoon.c
> +++ b/hw/alpha_typhoon.c
> @@ -764,7 +764,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
> &s->pchip.reg_io);
>
> b = pci_register_bus(&s->host.busdev.qdev, "pci",
> - typhoon_set_irq, sys_map_irq, s,
> + typhoon_set_irq, sys_map_irq, NULL, s,
> &s->pchip.reg_mem, addr_space_io, 0, 64);
> s->host.bus = b;
>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 7e28808..819bf1d 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -366,7 +366,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
> memory_region_add_subregion(get_system_memory(), mem_base, &d->pci_mmio);
>
> d->bus = pci_register_bus(&d->busdev.qdev, "pci",
> - pci_apb_set_irq, pci_pbm_map_irq, d,
> + pci_apb_set_irq, pci_pbm_map_irq, NULL, d,
> &d->pci_mmio,
> get_system_io(),
> 0, 32);
> diff --git a/hw/bonito.c b/hw/bonito.c
> index 77786f8..7ce5993 100644
> --- a/hw/bonito.c
> +++ b/hw/bonito.c
> @@ -750,7 +750,7 @@ PCIBus *bonito_init(qemu_irq *pic)
> dev = qdev_create(NULL, "Bonito-pcihost");
> pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
> b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
> - pci_bonito_map_irq, pic, get_system_memory(),
> + pci_bonito_map_irq, NULL, pic, get_system_memory(),
> get_system_io(),
> 0x28, 32);
> pcihost->bus = b;
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 81ff3a3..f47d9fe 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -85,6 +85,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> pci_grackle_set_irq,
> pci_grackle_map_irq,
> + NULL,
> pic,
> &d->pci_mmio,
> address_space_io,
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index a2d0e5a..a97bbf0 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1093,7 +1093,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
> d = FROM_SYSBUS(GT64120State, s);
> d->pci.bus = pci_register_bus(&d->busdev.qdev, "pci",
> gt64120_pci_set_irq, gt64120_pci_map_irq,
> - pic,
> + NULL, pic,
> get_system_memory(),
> get_system_io(),
> PCI_DEVFN(18, 0), 4);
> diff --git a/hw/pci.c b/hw/pci.c
> index 439f3ce..df4d93e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -298,10 +298,11 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> }
>
> void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque, int nirq)
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque, int nirq)
> {
> bus->set_irq = set_irq;
> bus->map_irq = map_irq;
> + bus->map_host_irq = map_host_irq;
> bus->irq_opaque = irq_opaque;
> bus->nirq = nirq;
> bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> @@ -316,7 +317,7 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
>
> PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque,
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> MemoryRegion *address_space_mem,
> MemoryRegion *address_space_io,
> uint8_t devfn_min, int nirq)
> @@ -325,7 +326,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>
> bus = pci_bus_new(parent, name, address_space_mem,
> address_space_io, devfn_min);
> - pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
> + pci_bus_irqs(bus, set_irq, map_irq, map_host_irq, irq_opaque, nirq);
> return bus;
> }
>
> @@ -1067,6 +1068,22 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
> pci_change_irq_level(pci_dev, irq_num, change);
> }
>
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
> +{
> + PCIBus *bus;
> +
> + for (;;) {
> + bus = pci_dev->bus;
> + irq_num = bus->map_irq(pci_dev, irq_num);
> + if (bus->map_host_irq) {
> + break;
> + }
> + pci_dev = bus->parent_dev;
> + assert(pci_dev);
> + }
> + return bus->map_host_irq(bus->irq_opaque, irq_num);
> +}
> +
> /***********************************************************/
> /* monitor info on PCI */
>
> diff --git a/hw/pci.h b/hw/pci.h
> index c3cacce..29bc8bf 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -276,6 +276,7 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
>
> typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> +typedef int (*pci_map_host_irq_fn)(void *opaque, int irq_num);
>
> typedef enum {
> PCI_HOTPLUG_DISABLED,
> @@ -295,15 +296,17 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> MemoryRegion *address_space_io,
> uint8_t devfn_min);
> void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque, int nirq);
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> + int nirq);
> int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
> PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque,
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> MemoryRegion *address_space_mem,
> MemoryRegion *address_space_io,
> uint8_t devfn_min, int nirq);
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
> void pci_device_reset(PCIDevice *dev);
> void pci_bus_reset(PCIBus *bus);
>
> diff --git a/hw/pci_internals.h b/hw/pci_internals.h
> index 96690b7..a92353e 100644
> --- a/hw/pci_internals.h
> +++ b/hw/pci_internals.h
> @@ -19,6 +19,7 @@ struct PCIBus {
> uint8_t devfn_min;
> pci_set_irq_fn set_irq;
> pci_map_irq_fn map_irq;
> + pci_map_host_irq_fn map_host_irq;
> pci_hotplug_fn hotplug;
> DeviceState *hotplug_qdev;
> void *irq_opaque;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 09e84f5..cfea97c 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -89,6 +89,7 @@ struct PCII440FXState {
> #define I440FX_SMRAM 0x72
>
> static void piix3_set_irq(void *opaque, int pirq, int level);
> +static int piix3_map_host_irq(void *opaque, int pci_intx);
> static void piix3_write_config_xen(PCIDevice *dev,
> uint32_t address, uint32_t val, int len);
>
> @@ -308,13 +309,13 @@ static PCIBus *i440fx_common_init(const char *device_name,
> if (xen_enabled()) {
> piix3 = DO_UPCAST(PIIX3State, dev,
> pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
> - pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> + pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq, NULL,
> piix3, XEN_PIIX_NUM_PIRQS);
> } else {
> piix3 = DO_UPCAST(PIIX3State, dev,
> pci_create_simple_multifunction(b, -1, true, "PIIX3"));
> - pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
> - PIIX_NUM_PIRQS);
> + pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3_map_host_irq,
> + piix3, PIIX_NUM_PIRQS);
> }
> piix3->pic = pic;
> *isa_bus = DO_UPCAST(ISABus, qbus,
> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
> piix3_set_irq_level(piix3, pirq, level);
> }
>
> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> +{
> + PIIX3State *piix3 = opaque;
> + int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> +
> + return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> +}
> +
> /* irq routing is changed. so rebuild bitmap */
> static void piix3_update_irq_levels(PIIX3State *piix3)
> {
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 203c3cd..224c4a0 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -343,7 +343,7 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
> }
>
> b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, ppc4xx_pci_set_irq,
> - ppc4xx_pci_map_irq, s->irq, get_system_memory(),
> + ppc4xx_pci_map_irq, NULL, s->irq, get_system_memory(),
> get_system_io(), 0, 4);
> s->pci_state.bus = b;
>
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 0f60b24..dd924ae 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -318,7 +318,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> }
>
> b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
> - mpc85xx_pci_map_irq, s->irq, address_space_mem,
> + mpc85xx_pci_map_irq, NULL, s->irq, address_space_mem,
> address_space_io, PCI_DEVFN(0x11, 0), 4);
> s->pci_state.bus = b;
>
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 38dbff4..9d7bec7 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -108,7 +108,7 @@ static int raven_pcihost_init(SysBusDevice *dev)
> }
>
> bus = pci_register_bus(&h->busdev.qdev, NULL,
> - prep_set_irq, prep_map_irq, s->irq,
> + prep_set_irq, prep_map_irq, NULL, s->irq,
> address_space_mem, address_space_io, 0, 4);
> h->bus = bus;
>
> diff --git a/hw/sh_pci.c b/hw/sh_pci.c
> index 0cfac46..1cea12b 100644
> --- a/hw/sh_pci.c
> +++ b/hw/sh_pci.c
> @@ -120,7 +120,7 @@ static int sh_pci_device_init(SysBusDevice *dev)
> sysbus_init_irq(dev, &s->irq[i]);
> }
> s->bus = pci_register_bus(&s->busdev.qdev, "pci",
> - sh_pci_set_irq, sh_pci_map_irq,
> + sh_pci_set_irq, sh_pci_map_irq, NULL,
> s->irq,
> get_system_memory(),
> get_system_io(),
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 25b400a..4a43062 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -306,7 +306,7 @@ static int spapr_phb_init(SysBusDevice *s)
>
> bus = pci_register_bus(&phb->busdev.qdev,
> phb->busname ? phb->busname : phb->dtbusname,
> - pci_spapr_set_irq, pci_spapr_map_irq, phb,
> + pci_spapr_set_irq, pci_spapr_map_irq, NULL, phb,
> &phb->memspace, &phb->iospace,
> PCI_DEVFN(0, 0), PCI_NUM_PINS);
> phb->host_state.bus = bus;
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 409bcd4..056e3bc 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -227,7 +227,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
>
> d->host_state.bus = pci_register_bus(dev, "pci",
> pci_unin_set_irq, pci_unin_map_irq,
> - pic,
> + NULL, pic,
> &d->pci_mmio,
> address_space_io,
> PCI_DEVFN(11, 0), 4);
> @@ -293,7 +293,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
>
> d->host_state.bus = pci_register_bus(dev, "pci",
> pci_unin_set_irq, pci_unin_map_irq,
> - pic,
> + NULL, pic,
> &d->pci_mmio,
> address_space_io,
> PCI_DEVFN(11, 0), 4);
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index ae53a8b..90c400e 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -68,7 +68,7 @@ static int pci_vpb_init(SysBusDevice *dev)
> sysbus_init_irq(dev, &s->irq[i]);
> }
> bus = pci_register_bus(&dev->qdev, "pci",
> - pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
> + pci_vpb_set_irq, pci_vpb_map_irq, NULL, s->irq,
> get_system_memory(), get_system_io(),
> PCI_DEVFN(11, 0), 4);
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pci: Add INTx routing notifier
2012-05-21 13:15 ` [Qemu-devel] [PATCH 2/2] pci: Add INTx routing notifier Jan Kiszka
@ 2012-05-21 14:13 ` Alex Williamson
0 siblings, 0 replies; 26+ messages in thread
From: Alex Williamson @ 2012-05-21 14:13 UTC (permalink / raw)
To: Jan Kiszka
Cc: Alexey Kardashevskiy, Avi Kivity, Marcelo Tosatti, qemu-devel,
Michael S. Tsirkin
On Mon, 2012-05-21 at 10:15 -0300, Jan Kiszka wrote:
> This per-device notifier shall be triggered by any interrupt router
> along the path of a device's legacy interrupt signal on routing changes.
> For simplicity reasons and as this is a slow path anyway, no further
> details on the routing changes are provided. Instead, the callback is
> expected to use pci_device_get_host_irq to check the effect of the
> change.
>
> Will be used by KVM PCI device assignment.
I think we could do this without inventing our out notifier mechanism,
Notifier + NotifierList, but it's better than what we've got now. VFIO
will use this too.
Acked-by: Alex Williamson <alex.williamson@redhat.com>
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> hw/pci.c | 19 +++++++++++++++++++
> hw/pci.h | 7 +++++++
> hw/pci_bridge.c | 8 ++++++++
> hw/piix_pci.c | 2 ++
> 4 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index df4d93e..27ecc37 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1084,6 +1084,25 @@ int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
> return bus->map_host_irq(bus->irq_opaque, irq_num);
> }
>
> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus)
> +{
> + PCIDevice *dev;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> + dev = bus->devices[i];
> + if (dev && dev->intx_routing_notifier) {
> + dev->intx_routing_notifier(dev);
> + }
> + }
> +}
> +
> +void pci_device_set_intx_routing_notifier(PCIDevice *pci_dev,
> + INTxRoutingNotifier notifier)
> +{
> + pci_dev->intx_routing_notifier = notifier;
> +}
> +
> /***********************************************************/
> /* monitor info on PCI */
>
> diff --git a/hw/pci.h b/hw/pci.h
> index 29bc8bf..eaf7695 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -173,6 +173,7 @@ typedef struct PCIDeviceClass {
> const char *romfile;
> } PCIDeviceClass;
>
> +typedef void (*INTxRoutingNotifier)(PCIDevice *dev);
> typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector,
> MSIMessage msg);
> typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector);
> @@ -248,6 +249,9 @@ struct PCIDevice {
> MemoryRegion rom;
> uint32_t rom_bar;
>
> + /* INTx routing notifier */
> + INTxRoutingNotifier intx_routing_notifier;
> +
> /* MSI-X notifiers */
> MSIVectorUseNotifier msix_vector_use_notifier;
> MSIVectorReleaseNotifier msix_vector_release_notifier;
> @@ -307,6 +311,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> MemoryRegion *address_space_io,
> uint8_t devfn_min, int nirq);
> int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
> +void pci_device_set_intx_routing_notifier(PCIDevice *pci_dev,
> + INTxRoutingNotifier notifier);
> void pci_device_reset(PCIDevice *dev);
> void pci_bus_reset(PCIBus *bus);
>
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index e0832b4..2490b2f 100644
> --- a/hw/pci_bridge.c
> +++ b/hw/pci_bridge.c
> @@ -292,6 +292,13 @@ void pci_bridge_reset(DeviceState *qdev)
> pci_set_word(conf + PCI_BRIDGE_CONTROL, 0);
> }
>
> +static void pci_bridge_intx_routing_update(PCIDevice *dev)
> +{
> + PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
> +
> + pci_bus_fire_intx_routing_notifier(&br->sec_bus);
> +}
> +
> /* default qdev initialization function for PCI-to-PCI bridge */
> int pci_bridge_initfn(PCIDevice *dev)
> {
> @@ -327,6 +334,7 @@ int pci_bridge_initfn(PCIDevice *dev)
> sec_bus->address_space_io = &br->address_space_io;
> memory_region_init(&br->address_space_io, "pci_bridge_io", 65536);
> pci_bridge_region_init(br);
> + pci_device_set_intx_routing_notifier(dev, pci_bridge_intx_routing_update);
> QLIST_INIT(&sec_bus->child);
> QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> return 0;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index cfea97c..b0d1325 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -414,6 +414,8 @@ static void piix3_write_config(PCIDevice *dev,
> if (ranges_overlap(address, len, PIIX_PIRQC, 4)) {
> PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
> int pic_irq;
> +
> + pci_bus_fire_intx_routing_notifier(piix3->dev.bus);
> piix3_update_irq_levels(piix3);
> for (pic_irq = 0; pic_irq < PIIX_NUM_PIC_IRQS; pic_irq++) {
> piix3_set_irq_pic(piix3, pic_irq);
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 13:13 [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Jan Kiszka
2012-05-21 13:15 ` [Qemu-devel] [PATCH 2/2] pci: Add INTx routing notifier Jan Kiszka
2012-05-21 14:10 ` [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Alex Williamson
@ 2012-05-21 14:36 ` Avi Kivity
2012-05-21 14:47 ` Jan Kiszka
2012-05-21 17:34 ` Michael S. Tsirkin
2012-05-21 19:05 ` Michael S. Tsirkin
4 siblings, 1 reply; 26+ messages in thread
From: Avi Kivity @ 2012-05-21 14:36 UTC (permalink / raw)
To: Jan Kiszka
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Michael S. Tsirkin
On 05/21/2012 04:13 PM, Jan Kiszka wrote:
> Add a PCI IRQ path discovery function that walks from a given device to
> the host bridge, returning the IRQ number that is reported to the
> attached interrupt controller. For this purpose, another PCI bridge
> callback function is introduced: map_host_irq. It is so far only
> implemented by the PIIX3, other host bridges can be added later on as
> required.
>
> Will be used for KVM PCI device assignment.
This is similar to the memory API, which converts a memory region
hierarchy to a flat list and fires notifiers whenever it changes.
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
> +{
> + PCIBus *bus;
> +
> + for (;;) {
> + bus = pci_dev->bus;
> + irq_num = bus->map_irq(pci_dev, irq_num);
> + if (bus->map_host_irq) {
> + break;
> + }
> + pci_dev = bus->parent_dev;
> + assert(pci_dev);
> + }
> + return bus->map_host_irq(bus->irq_opaque, irq_num);
> +}
> +
My personal preference is to avoid infinite loops with breaks, I'd write
this as a do/while (without the assert). Or maybe supply all buses with
a default map_host_irq that recurses back into
pci_device_get_host_irq(). But this is not an objection to the patch.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 14:36 ` Avi Kivity
@ 2012-05-21 14:47 ` Jan Kiszka
0 siblings, 0 replies; 26+ messages in thread
From: Jan Kiszka @ 2012-05-21 14:47 UTC (permalink / raw)
To: Avi Kivity
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Michael S. Tsirkin
On 2012-05-21 11:36, Avi Kivity wrote:
> On 05/21/2012 04:13 PM, Jan Kiszka wrote:
>> Add a PCI IRQ path discovery function that walks from a given device to
>> the host bridge, returning the IRQ number that is reported to the
>> attached interrupt controller. For this purpose, another PCI bridge
>> callback function is introduced: map_host_irq. It is so far only
>> implemented by the PIIX3, other host bridges can be added later on as
>> required.
>>
>> Will be used for KVM PCI device assignment.
>
> This is similar to the memory API, which converts a memory region
> hierarchy to a flat list and fires notifiers whenever it changes.
In fact, this should become a generic thing one day, independent of PCI.
But that's an exercise to be done while reworking the IRQ layer of QEMU
(e.g. to support delivery feedback...).
>
>> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
>> +{
>> + PCIBus *bus;
>> +
>> + for (;;) {
>> + bus = pci_dev->bus;
>> + irq_num = bus->map_irq(pci_dev, irq_num);
>> + if (bus->map_host_irq) {
>> + break;
>> + }
>> + pci_dev = bus->parent_dev;
>> + assert(pci_dev);
>> + }
>> + return bus->map_host_irq(bus->irq_opaque, irq_num);
>> +}
>> +
>
> My personal preference is to avoid infinite loops with breaks, I'd write
> this as a do/while (without the assert). Or maybe supply all buses with
> a default map_host_irq that recurses back into
> pci_device_get_host_irq(). But this is not an objection to the patch.
I've modeled it after pci_change_irq_level. I would suggest to change
both later on.
BTW, the assert catches host bridges that do not yet implement the
required mapping service.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 13:13 [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Jan Kiszka
` (2 preceding siblings ...)
2012-05-21 14:36 ` Avi Kivity
@ 2012-05-21 17:34 ` Michael S. Tsirkin
2012-05-21 18:58 ` Jan Kiszka
2012-05-21 19:05 ` Michael S. Tsirkin
4 siblings, 1 reply; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-05-21 17:34 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> Add a PCI IRQ path discovery function that walks from a given device to
> the host bridge, returning the IRQ number that is reported to the
> attached interrupt controller. For this purpose, another PCI bridge
> callback function is introduced: map_host_irq. It is so far only
> implemented by the PIIX3, other host bridges can be added later on as
> required.
>
> Will be used for KVM PCI device assignment.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
interrupt injection is data path even for emulated devices.
So instead of special casing device assignment I would like to see all
devices converted to an API that caches irqs.
This will likely mean that we can maintain the final
irq as part of the pci device structure, and
this api will simply return it.
> ---
> hw/alpha_typhoon.c | 2 +-
> hw/apb_pci.c | 2 +-
> hw/bonito.c | 2 +-
> hw/grackle_pci.c | 1 +
> hw/gt64xxx.c | 2 +-
> hw/pci.c | 23 ++++++++++++++++++++---
> hw/pci.h | 7 +++++--
> hw/pci_internals.h | 1 +
> hw/piix_pci.c | 15 ++++++++++++---
> hw/ppc4xx_pci.c | 2 +-
> hw/ppce500_pci.c | 2 +-
> hw/prep_pci.c | 2 +-
> hw/sh_pci.c | 2 +-
> hw/spapr_pci.c | 2 +-
> hw/unin_pci.c | 4 ++--
> hw/versatile_pci.c | 2 +-
> 16 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
> index 872e112..fc2e4b3 100644
> --- a/hw/alpha_typhoon.c
> +++ b/hw/alpha_typhoon.c
> @@ -764,7 +764,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
> &s->pchip.reg_io);
>
> b = pci_register_bus(&s->host.busdev.qdev, "pci",
> - typhoon_set_irq, sys_map_irq, s,
> + typhoon_set_irq, sys_map_irq, NULL, s,
> &s->pchip.reg_mem, addr_space_io, 0, 64);
> s->host.bus = b;
>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 7e28808..819bf1d 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -366,7 +366,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
> memory_region_add_subregion(get_system_memory(), mem_base, &d->pci_mmio);
>
> d->bus = pci_register_bus(&d->busdev.qdev, "pci",
> - pci_apb_set_irq, pci_pbm_map_irq, d,
> + pci_apb_set_irq, pci_pbm_map_irq, NULL, d,
> &d->pci_mmio,
> get_system_io(),
> 0, 32);
> diff --git a/hw/bonito.c b/hw/bonito.c
> index 77786f8..7ce5993 100644
> --- a/hw/bonito.c
> +++ b/hw/bonito.c
> @@ -750,7 +750,7 @@ PCIBus *bonito_init(qemu_irq *pic)
> dev = qdev_create(NULL, "Bonito-pcihost");
> pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
> b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
> - pci_bonito_map_irq, pic, get_system_memory(),
> + pci_bonito_map_irq, NULL, pic, get_system_memory(),
> get_system_io(),
> 0x28, 32);
> pcihost->bus = b;
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 81ff3a3..f47d9fe 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -85,6 +85,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> pci_grackle_set_irq,
> pci_grackle_map_irq,
> + NULL,
> pic,
> &d->pci_mmio,
> address_space_io,
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index a2d0e5a..a97bbf0 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1093,7 +1093,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
> d = FROM_SYSBUS(GT64120State, s);
> d->pci.bus = pci_register_bus(&d->busdev.qdev, "pci",
> gt64120_pci_set_irq, gt64120_pci_map_irq,
> - pic,
> + NULL, pic,
> get_system_memory(),
> get_system_io(),
> PCI_DEVFN(18, 0), 4);
> diff --git a/hw/pci.c b/hw/pci.c
> index 439f3ce..df4d93e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -298,10 +298,11 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> }
>
> void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque, int nirq)
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque, int nirq)
> {
> bus->set_irq = set_irq;
> bus->map_irq = map_irq;
> + bus->map_host_irq = map_host_irq;
> bus->irq_opaque = irq_opaque;
> bus->nirq = nirq;
> bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> @@ -316,7 +317,7 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
>
> PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque,
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> MemoryRegion *address_space_mem,
> MemoryRegion *address_space_io,
> uint8_t devfn_min, int nirq)
> @@ -325,7 +326,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>
> bus = pci_bus_new(parent, name, address_space_mem,
> address_space_io, devfn_min);
> - pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
> + pci_bus_irqs(bus, set_irq, map_irq, map_host_irq, irq_opaque, nirq);
> return bus;
> }
>
> @@ -1067,6 +1068,22 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
> pci_change_irq_level(pci_dev, irq_num, change);
> }
>
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
> +{
> + PCIBus *bus;
> +
> + for (;;) {
> + bus = pci_dev->bus;
> + irq_num = bus->map_irq(pci_dev, irq_num);
> + if (bus->map_host_irq) {
> + break;
> + }
> + pci_dev = bus->parent_dev;
> + assert(pci_dev);
> + }
> + return bus->map_host_irq(bus->irq_opaque, irq_num);
> +}
> +
> /***********************************************************/
> /* monitor info on PCI */
>
> diff --git a/hw/pci.h b/hw/pci.h
> index c3cacce..29bc8bf 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -276,6 +276,7 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
>
> typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> +typedef int (*pci_map_host_irq_fn)(void *opaque, int irq_num);
>
> typedef enum {
> PCI_HOTPLUG_DISABLED,
> @@ -295,15 +296,17 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> MemoryRegion *address_space_io,
> uint8_t devfn_min);
> void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque, int nirq);
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> + int nirq);
> int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
> PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> - void *irq_opaque,
> + pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> MemoryRegion *address_space_mem,
> MemoryRegion *address_space_io,
> uint8_t devfn_min, int nirq);
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
> void pci_device_reset(PCIDevice *dev);
> void pci_bus_reset(PCIBus *bus);
>
> diff --git a/hw/pci_internals.h b/hw/pci_internals.h
> index 96690b7..a92353e 100644
> --- a/hw/pci_internals.h
> +++ b/hw/pci_internals.h
> @@ -19,6 +19,7 @@ struct PCIBus {
> uint8_t devfn_min;
> pci_set_irq_fn set_irq;
> pci_map_irq_fn map_irq;
> + pci_map_host_irq_fn map_host_irq;
> pci_hotplug_fn hotplug;
> DeviceState *hotplug_qdev;
> void *irq_opaque;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 09e84f5..cfea97c 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -89,6 +89,7 @@ struct PCII440FXState {
> #define I440FX_SMRAM 0x72
>
> static void piix3_set_irq(void *opaque, int pirq, int level);
> +static int piix3_map_host_irq(void *opaque, int pci_intx);
> static void piix3_write_config_xen(PCIDevice *dev,
> uint32_t address, uint32_t val, int len);
>
> @@ -308,13 +309,13 @@ static PCIBus *i440fx_common_init(const char *device_name,
> if (xen_enabled()) {
> piix3 = DO_UPCAST(PIIX3State, dev,
> pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
> - pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> + pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq, NULL,
> piix3, XEN_PIIX_NUM_PIRQS);
> } else {
> piix3 = DO_UPCAST(PIIX3State, dev,
> pci_create_simple_multifunction(b, -1, true, "PIIX3"));
> - pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
> - PIIX_NUM_PIRQS);
> + pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3_map_host_irq,
> + piix3, PIIX_NUM_PIRQS);
> }
> piix3->pic = pic;
> *isa_bus = DO_UPCAST(ISABus, qbus,
> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
> piix3_set_irq_level(piix3, pirq, level);
> }
>
> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> +{
> + PIIX3State *piix3 = opaque;
> + int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> +
> + return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> +}
> +
> /* irq routing is changed. so rebuild bitmap */
> static void piix3_update_irq_levels(PIIX3State *piix3)
> {
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 203c3cd..224c4a0 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -343,7 +343,7 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
> }
>
> b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, ppc4xx_pci_set_irq,
> - ppc4xx_pci_map_irq, s->irq, get_system_memory(),
> + ppc4xx_pci_map_irq, NULL, s->irq, get_system_memory(),
> get_system_io(), 0, 4);
> s->pci_state.bus = b;
>
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 0f60b24..dd924ae 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -318,7 +318,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> }
>
> b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
> - mpc85xx_pci_map_irq, s->irq, address_space_mem,
> + mpc85xx_pci_map_irq, NULL, s->irq, address_space_mem,
> address_space_io, PCI_DEVFN(0x11, 0), 4);
> s->pci_state.bus = b;
>
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 38dbff4..9d7bec7 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -108,7 +108,7 @@ static int raven_pcihost_init(SysBusDevice *dev)
> }
>
> bus = pci_register_bus(&h->busdev.qdev, NULL,
> - prep_set_irq, prep_map_irq, s->irq,
> + prep_set_irq, prep_map_irq, NULL, s->irq,
> address_space_mem, address_space_io, 0, 4);
> h->bus = bus;
>
> diff --git a/hw/sh_pci.c b/hw/sh_pci.c
> index 0cfac46..1cea12b 100644
> --- a/hw/sh_pci.c
> +++ b/hw/sh_pci.c
> @@ -120,7 +120,7 @@ static int sh_pci_device_init(SysBusDevice *dev)
> sysbus_init_irq(dev, &s->irq[i]);
> }
> s->bus = pci_register_bus(&s->busdev.qdev, "pci",
> - sh_pci_set_irq, sh_pci_map_irq,
> + sh_pci_set_irq, sh_pci_map_irq, NULL,
> s->irq,
> get_system_memory(),
> get_system_io(),
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 25b400a..4a43062 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -306,7 +306,7 @@ static int spapr_phb_init(SysBusDevice *s)
>
> bus = pci_register_bus(&phb->busdev.qdev,
> phb->busname ? phb->busname : phb->dtbusname,
> - pci_spapr_set_irq, pci_spapr_map_irq, phb,
> + pci_spapr_set_irq, pci_spapr_map_irq, NULL, phb,
> &phb->memspace, &phb->iospace,
> PCI_DEVFN(0, 0), PCI_NUM_PINS);
> phb->host_state.bus = bus;
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 409bcd4..056e3bc 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -227,7 +227,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
>
> d->host_state.bus = pci_register_bus(dev, "pci",
> pci_unin_set_irq, pci_unin_map_irq,
> - pic,
> + NULL, pic,
> &d->pci_mmio,
> address_space_io,
> PCI_DEVFN(11, 0), 4);
> @@ -293,7 +293,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
>
> d->host_state.bus = pci_register_bus(dev, "pci",
> pci_unin_set_irq, pci_unin_map_irq,
> - pic,
> + NULL, pic,
> &d->pci_mmio,
> address_space_io,
> PCI_DEVFN(11, 0), 4);
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index ae53a8b..90c400e 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -68,7 +68,7 @@ static int pci_vpb_init(SysBusDevice *dev)
> sysbus_init_irq(dev, &s->irq[i]);
> }
> bus = pci_register_bus(&dev->qdev, "pci",
> - pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
> + pci_vpb_set_irq, pci_vpb_map_irq, NULL, s->irq,
> get_system_memory(), get_system_io(),
> PCI_DEVFN(11, 0), 4);
>
> --
> 1.7.3.4
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 17:34 ` Michael S. Tsirkin
@ 2012-05-21 18:58 ` Jan Kiszka
2012-05-21 21:04 ` Michael S. Tsirkin
0 siblings, 1 reply; 26+ messages in thread
From: Jan Kiszka @ 2012-05-21 18:58 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-05-21 14:34, Michael S. Tsirkin wrote:
> On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
>> Add a PCI IRQ path discovery function that walks from a given device to
>> the host bridge, returning the IRQ number that is reported to the
>> attached interrupt controller. For this purpose, another PCI bridge
>> callback function is introduced: map_host_irq. It is so far only
>> implemented by the PIIX3, other host bridges can be added later on as
>> required.
>>
>> Will be used for KVM PCI device assignment.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>
> interrupt injection is data path even for emulated devices.
> So instead of special casing device assignment I would like to see all
> devices converted to an API that caches irqs.
>
> This will likely mean that we can maintain the final
> irq as part of the pci device structure, and
> this api will simply return it.
Yep, I definitely agree. It's just that such a design has to please even
more users than PCI devices, thus will likely take longer to settle than
the device assignment effort. Therefore I decided to rush forward with
an intermediate approach first.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 13:13 [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Jan Kiszka
` (3 preceding siblings ...)
2012-05-21 17:34 ` Michael S. Tsirkin
@ 2012-05-21 19:05 ` Michael S. Tsirkin
2012-05-21 20:35 ` Jan Kiszka
4 siblings, 1 reply; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-05-21 19:05 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
> piix3_set_irq_level(piix3, pirq, level);
> }
>
> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> +{
> + PIIX3State *piix3 = opaque;
> + int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> +
> + return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> +}
> +
> /* irq routing is changed. so rebuild bitmap */
> static void piix3_update_irq_levels(PIIX3State *piix3)
> {
So, instead of special API just for assignment,
I would like to see map_irq in piix being reworked
to take dev config into account.
I think piix is almost unique in this but need to check,
if not fix other host buses that are programmable.
PCI bridges are all fixed routing.
Then we can drop set_irq callback.
Finally all devices can cache the irq#,
and piix would scan devices behind it
and update the irq.
Assignment then just needs a notifier, since
it owns the device just a pointer in device is
enough.
Could you look at doing this please?
If no I can give it a stub.
--
MST
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 19:05 ` Michael S. Tsirkin
@ 2012-05-21 20:35 ` Jan Kiszka
2012-05-21 21:03 ` Michael S. Tsirkin
0 siblings, 1 reply; 26+ messages in thread
From: Jan Kiszka @ 2012-05-21 20:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-05-21 16:05, Michael S. Tsirkin wrote:
> On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
>> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
>> piix3_set_irq_level(piix3, pirq, level);
>> }
>>
>> +static int piix3_map_host_irq(void *opaque, int pci_intx)
>> +{
>> + PIIX3State *piix3 = opaque;
>> + int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
>> +
>> + return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
>> +}
>> +
>> /* irq routing is changed. so rebuild bitmap */
>> static void piix3_update_irq_levels(PIIX3State *piix3)
>> {
>
>
> So, instead of special API just for assignment,
> I would like to see map_irq in piix being reworked
> to take dev config into account.
> I think piix is almost unique in this but need to check,
Maybe it is the only host bridge with routing that we have in QEMU right
now, but it is surely not unique (e.g. all later Intel chipsets support
this).
> if not fix other host buses that are programmable.
> PCI bridges are all fixed routing.
>
> Then we can drop set_irq callback.
set_irq may do more than IRQ routing. It may also flip polarity (see
bonito.c). I guess we need to analyze the requirements of all in-tree
host bridges first.
>
> Finally all devices can cache the irq#,
> and piix would scan devices behind it
> and update the irq.
>
> Assignment then just needs a notifier, since
> it owns the device just a pointer in device is
> enough.
I cannot completely follow your ideas here yet. Do you want to pass the
mapping information along the notifier, or why "just ... a notifier"?
>
> Could you look at doing this please?
> If no I can give it a stub.
>
Unless we can converge over a final API quickly, I would suggest to base
these refactorings on top of what I propose here. We will have to touch
all host bridges more invasively for this anyway. If you can explain to
me how simple the refactoring can be (but I'm a bit skeptical ;) ), I
could do this, otherwise I would prefer to focus on the device
assignment topic which provide some more nuts.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 20:35 ` Jan Kiszka
@ 2012-05-21 21:03 ` Michael S. Tsirkin
0 siblings, 0 replies; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-05-21 21:03 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Mon, May 21, 2012 at 05:35:34PM -0300, Jan Kiszka wrote:
> On 2012-05-21 16:05, Michael S. Tsirkin wrote:
> > On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> >> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
> >> piix3_set_irq_level(piix3, pirq, level);
> >> }
> >>
> >> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> >> +{
> >> + PIIX3State *piix3 = opaque;
> >> + int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> >> +
> >> + return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> >> +}
> >> +
> >> /* irq routing is changed. so rebuild bitmap */
> >> static void piix3_update_irq_levels(PIIX3State *piix3)
> >> {
> >
> >
> > So, instead of special API just for assignment,
> > I would like to see map_irq in piix being reworked
> > to take dev config into account.
> > I think piix is almost unique in this but need to check,
>
> Maybe it is the only host bridge with routing that we have in QEMU right
> now, but it is surely not unique (e.g. all later Intel chipsets support
> this).
Yes. APIs for this should be in place. Just saying
instead of failing we can easily just make it work
if there are no remappings.
> > if not fix other host buses that are programmable.
> > PCI bridges are all fixed routing.
> >
> > Then we can drop set_irq callback.
>
> set_irq may do more than IRQ routing. It may also flip polarity (see
> bonito.c).
In that case host_map_irq is not a good API?
Need to investigate.
> I guess we need to analyze the requirements of all in-tree
> host bridges first.
Yes.
> >
> > Finally all devices can cache the irq#,
> > and piix would scan devices behind it
> > and update the irq.
> >
> > Assignment then just needs a notifier, since
> > it owns the device just a pointer in device is
> > enough.
>
> I cannot completely follow your ideas here yet. Do you want to pass the
> mapping information along the notifier, or why "just ... a notifier"?
Each device would resolve IRQs that it uses.
> >
> > Could you look at doing this please?
> > If no I can give it a stub.
> >
>
> Unless we can converge over a final API quickly, I would suggest to base
> these refactorings on top of what I propose here. We will have to touch
> all host bridges more invasively for this anyway. If you can explain to
> me how simple the refactoring can be (but I'm a bit skeptical ;) ), I
> could do this, otherwise I would prefer to focus on the device
> assignment topic which provide some more nuts.
>
> Jan
Yes it's simple. I'll post in a couple of days (lots of
meetings tomorrow). I think you'll
see it's easier and cleaner.
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-05-21 18:58 ` Jan Kiszka
@ 2012-05-21 21:04 ` Michael S. Tsirkin
0 siblings, 0 replies; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-05-21 21:04 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Mon, May 21, 2012 at 03:58:57PM -0300, Jan Kiszka wrote:
> On 2012-05-21 14:34, Michael S. Tsirkin wrote:
> > On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> >> Add a PCI IRQ path discovery function that walks from a given device to
> >> the host bridge, returning the IRQ number that is reported to the
> >> attached interrupt controller. For this purpose, another PCI bridge
> >> callback function is introduced: map_host_irq. It is so far only
> >> implemented by the PIIX3, other host bridges can be added later on as
> >> required.
> >>
> >> Will be used for KVM PCI device assignment.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >
> > interrupt injection is data path even for emulated devices.
> > So instead of special casing device assignment I would like to see all
> > devices converted to an API that caches irqs.
> >
> > This will likely mean that we can maintain the final
> > irq as part of the pci device structure, and
> > this api will simply return it.
>
> Yep, I definitely agree. It's just that such a design has to please even
> more users than PCI devices, thus will likely take longer to settle than
> the device assignment effort. Therefore I decided to rush forward with
> an intermediate approach first.
>
> Jan
I think it's easy, will try to do it soon.
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
[not found] ` <20120530203119.GH1551@redhat.com>
@ 2012-06-01 12:52 ` Jan Kiszka
2012-06-01 13:27 ` Michael S. Tsirkin
2012-06-01 13:28 ` Michael S. Tsirkin
0 siblings, 2 replies; 26+ messages in thread
From: Jan Kiszka @ 2012-06-01 12:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-05-30 22:31, Michael S. Tsirkin wrote:
>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
>>> irq_count instead of the pic_levels bitmap.
>>
>> Just that this affects generic PCI code, not only PIIX-specific things.
>
> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
>
>> And that we need to save/restore some irq_count field according to the
>> old semantics.
>
> Well, it's a bug: this is redundant info we should not have exposed it.
>
> Anyway, let's make the rest work properly and cleanly first, add a FIXME
> for now, then we'll find a hack making it work for migration.
It remains non-trivial: I got your patch working (a minor init issue),
but yet without changing the number of IRQs for PIIX3, so keeping the
irq_count semantics for this host bridge.
Now I'm facing three possibilities of how to proceed:
1. Give up on the (currently broken) feature to write a vmstate for
older QEMU versions.
This will allow to declare the irq_count field in vmstate_pcibus
unused, and we would have to restore it on vmload step-wise via the
PCI devices. It would also allow to change its semantics for PIIX3,
mapping directly to PIC IRQs.
2. Keep writing a legacy irq_count field.
This will require quite a few new APIs so that host bridges that
want to change their nirq can still generate a compatible irq_count
vmstate field. Namely:
- A function to set up vmstate_irq_count and define a callback that
the core will invoke to prepare the vmstate_irq_count before
vmsave.
- A function to obtain the IRQ mapping /without/ the final host
bridge step. This is required so that the callback above can
calculate the old state like in the PIIX3 case.
3. Keep irq_count and nirq as is, introduce additional map_host_irq.
This is simpler than 2 and more compatible than 1. It would also
allow to introduce the polarity and masking information more
smoothly as we won't have to add it to existing map_irq callbacks
then.
Any other suggestions?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 12:52 ` Jan Kiszka
@ 2012-06-01 13:27 ` Michael S. Tsirkin
2012-06-01 13:57 ` Jan Kiszka
2012-06-01 13:28 ` Michael S. Tsirkin
1 sibling, 1 reply; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 13:27 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
> >>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
> >>> irq_count instead of the pic_levels bitmap.
> >>
> >> Just that this affects generic PCI code, not only PIIX-specific things.
> >
> > Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
> >
> >> And that we need to save/restore some irq_count field according to the
> >> old semantics.
> >
> > Well, it's a bug: this is redundant info we should not have exposed it.
> >
> > Anyway, let's make the rest work properly and cleanly first, add a FIXME
> > for now, then we'll find a hack making it work for migration.
>
> It remains non-trivial: I got your patch working (a minor init issue),
> but yet without changing the number of IRQs for PIIX3, so keeping the
> irq_count semantics for this host bridge.
>
> Now I'm facing three possibilities of how to proceed:
They all look OK I think :) Some comments below.
> 1. Give up on the (currently broken) feature to write a vmstate for
> older QEMU versions.
>
> This will allow to declare the irq_count field in vmstate_pcibus
> unused, and we would have to restore it on vmload step-wise via the
> PCI devices. It would also allow to change its semantics for PIIX3,
> mapping directly to PIC IRQs.
I think that's okay too simply because these things are usually
easy to fix after the fact when the rest of the issues are addressed.
> 2. Keep writing a legacy irq_count field.
>
> This will require quite a few new APIs so that host bridges that
> want to change their nirq can still generate a compatible irq_count
> vmstate field. Namely:
> - A function to set up vmstate_irq_count and define a callback that
> the core will invoke to prepare the vmstate_irq_count before
> vmsave.
> - A function to obtain the IRQ mapping /without/ the final host
> bridge step. This is required so that the callback above can
> calculate the old state like in the PIIX3 case.
Does this really need to be so complex? It seems that we just need
pci_get_irq_count(bus, irq) which can use the existing map_irq API, no?
Then invoke that before save.
> 3. Keep irq_count and nirq as is, introduce additional map_host_irq.
>
> This is simpler than 2 and more compatible than 1. It would also
> allow to introduce the polarity and masking information more
> smoothly as we won't have to add it to existing map_irq callbacks
> then.
So what does it map, and to what?
Maybe we can make the name imply that somehow.
> Any other suggestions?
>
> Jan
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 12:52 ` Jan Kiszka
2012-06-01 13:27 ` Michael S. Tsirkin
@ 2012-06-01 13:28 ` Michael S. Tsirkin
2012-06-01 13:43 ` Jan Kiszka
1 sibling, 1 reply; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 13:28 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
> >>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
> >>> irq_count instead of the pic_levels bitmap.
> >>
> >> Just that this affects generic PCI code, not only PIIX-specific things.
> >
> > Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
> >
> >> And that we need to save/restore some irq_count field according to the
> >> old semantics.
> >
> > Well, it's a bug: this is redundant info we should not have exposed it.
> >
> > Anyway, let's make the rest work properly and cleanly first, add a FIXME
> > for now, then we'll find a hack making it work for migration.
>
> It remains non-trivial: I got your patch working (a minor init issue),
> but yet without changing the number of IRQs for PIIX3, so keeping the
> irq_count semantics for this host bridge.
BTW can you post the fixed version please in case
others want to play with it?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 13:28 ` Michael S. Tsirkin
@ 2012-06-01 13:43 ` Jan Kiszka
0 siblings, 0 replies; 26+ messages in thread
From: Jan Kiszka @ 2012-06-01 13:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-06-01 15:28, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
>> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
>>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
>>>>> irq_count instead of the pic_levels bitmap.
>>>>
>>>> Just that this affects generic PCI code, not only PIIX-specific things.
>>>
>>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
>>>
>>>> And that we need to save/restore some irq_count field according to the
>>>> old semantics.
>>>
>>> Well, it's a bug: this is redundant info we should not have exposed it.
>>>
>>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
>>> for now, then we'll find a hack making it work for migration.
>>
>> It remains non-trivial: I got your patch working (a minor init issue),
>> but yet without changing the number of IRQs for PIIX3, so keeping the
>> irq_count semantics for this host bridge.
>
> BTW can you post the fixed version please in case
> others want to play with it?
Pushed a snapshot to git://git.kiszka.org/qemu.git queues/pci. That
version survived simple tests.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 13:27 ` Michael S. Tsirkin
@ 2012-06-01 13:57 ` Jan Kiszka
2012-06-01 14:34 ` Michael S. Tsirkin
0 siblings, 1 reply; 26+ messages in thread
From: Jan Kiszka @ 2012-06-01 13:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-06-01 15:27, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
>> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
>>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
>>>>> irq_count instead of the pic_levels bitmap.
>>>>
>>>> Just that this affects generic PCI code, not only PIIX-specific things.
>>>
>>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
>>>
>>>> And that we need to save/restore some irq_count field according to the
>>>> old semantics.
>>>
>>> Well, it's a bug: this is redundant info we should not have exposed it.
>>>
>>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
>>> for now, then we'll find a hack making it work for migration.
>>
>> It remains non-trivial: I got your patch working (a minor init issue),
>> but yet without changing the number of IRQs for PIIX3, so keeping the
>> irq_count semantics for this host bridge.
>>
>> Now I'm facing three possibilities of how to proceed:
>
> They all look OK I think :) Some comments below.
>
>> 1. Give up on the (currently broken) feature to write a vmstate for
>> older QEMU versions.
>>
>> This will allow to declare the irq_count field in vmstate_pcibus
>> unused, and we would have to restore it on vmload step-wise via the
>> PCI devices. It would also allow to change its semantics for PIIX3,
>> mapping directly to PIC IRQs.
>
> I think that's okay too simply because these things are usually
> easy to fix after the fact when the rest of the issues are addressed.
Don't get what you mean with "fixed". If we fix the vmstate generation
in making it backward-compatible again, we enter option 2. Option 1 is
explicitly about giving this up.
>
>> 2. Keep writing a legacy irq_count field.
>>
>> This will require quite a few new APIs so that host bridges that
>> want to change their nirq can still generate a compatible irq_count
>> vmstate field. Namely:
>> - A function to set up vmstate_irq_count and define a callback that
>> the core will invoke to prepare the vmstate_irq_count before
>> vmsave.
>> - A function to obtain the IRQ mapping /without/ the final host
>> bridge step. This is required so that the callback above can
>> calculate the old state like in the PIIX3 case.
>
> Does this really need to be so complex? It seems that we just need
> pci_get_irq_count(bus, irq) which can use the existing map_irq API, no?
> Then invoke that before save.
No, because the new map_irq of the PIIX3 bridge will also include the
host bridge routing (or masking) according to the PIRQx routoing
registers of the PIIX3. Moreover, the fixup of the written legacy
irq_count state has to happen in the PCI layer, which therefore has to
query the host bridge for fixup information, not the other way around
(because the PCI bus vmstate is separate from the PIIX3 host bridge).
>
>> 3. Keep irq_count and nirq as is, introduce additional map_host_irq.
>>
>> This is simpler than 2 and more compatible than 1. It would also
>> allow to introduce the polarity and masking information more
>> smoothly as we won't have to add it to existing map_irq callbacks
>> then.
>
> So what does it map, and to what?
PCI bus IRQ to *host* IRQ. It is supposed to explore the routing of the
host bridge between the root bus and the host's interrupt controller
(i.e. the step that is currently missing the cached chain).
> Maybe we can make the name imply that somehow.
Better suggestions for this handler and maybe also the existing map_irq
are welcome to make the difference clearer.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 13:57 ` Jan Kiszka
@ 2012-06-01 14:34 ` Michael S. Tsirkin
2012-06-01 15:15 ` Jan Kiszka
2012-06-01 15:26 ` Michael S. Tsirkin
0 siblings, 2 replies; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 14:34 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Fri, Jun 01, 2012 at 03:57:01PM +0200, Jan Kiszka wrote:
> On 2012-06-01 15:27, Michael S. Tsirkin wrote:
> > On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
> >> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
> >>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
> >>>>> irq_count instead of the pic_levels bitmap.
> >>>>
> >>>> Just that this affects generic PCI code, not only PIIX-specific things.
> >>>
> >>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
> >>>
> >>>> And that we need to save/restore some irq_count field according to the
> >>>> old semantics.
> >>>
> >>> Well, it's a bug: this is redundant info we should not have exposed it.
> >>>
> >>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
> >>> for now, then we'll find a hack making it work for migration.
> >>
> >> It remains non-trivial: I got your patch working (a minor init issue),
> >> but yet without changing the number of IRQs for PIIX3, so keeping the
> >> irq_count semantics for this host bridge.
> >>
> >> Now I'm facing three possibilities of how to proceed:
> >
> > They all look OK I think :) Some comments below.
> >
> >> 1. Give up on the (currently broken) feature to write a vmstate for
> >> older QEMU versions.
> >>
> >> This will allow to declare the irq_count field in vmstate_pcibus
> >> unused, and we would have to restore it on vmload step-wise via the
> >> PCI devices. It would also allow to change its semantics for PIIX3,
> >> mapping directly to PIC IRQs.
> >
> > I think that's okay too simply because these things are usually
> > easy to fix after the fact when the rest of the issues are addressed.
>
> Don't get what you mean with "fixed". If we fix the vmstate generation
> in making it backward-compatible again, we enter option 2. Option 1 is
> explicitly about giving this up.
What I really mean is I think I see how 2 can be added without much
pain. So let's focus on 1 for now and worst case we break migration.
> >
> >> 2. Keep writing a legacy irq_count field.
> >>
> >> This will require quite a few new APIs so that host bridges that
> >> want to change their nirq can still generate a compatible irq_count
> >> vmstate field. Namely:
> >> - A function to set up vmstate_irq_count and define a callback that
> >> the core will invoke to prepare the vmstate_irq_count before
> >> vmsave.
> >> - A function to obtain the IRQ mapping /without/ the final host
> >> bridge step. This is required so that the callback above can
> >> calculate the old state like in the PIIX3 case.
> >
> > Does this really need to be so complex? It seems that we just need
> > pci_get_irq_count(bus, irq) which can use the existing map_irq API, no?
> > Then invoke that before save.
>
> No, because the new map_irq of the PIIX3 bridge will also include the
> host bridge routing (or masking) according to the PIRQx routoing
> registers of the PIIX3. Moreover, the fixup of the written legacy
> irq_count state has to happen in the PCI layer, which therefore has to
> query the host bridge for fixup information, not the other way around
> (because the PCI bus vmstate is separate from the PIIX3 host bridge).
>
> >
> >> 3. Keep irq_count and nirq as is, introduce additional map_host_irq.
> >>
> >> This is simpler than 2 and more compatible than 1. It would also
> >> allow to introduce the polarity and masking information more
> >> smoothly as we won't have to add it to existing map_irq callbacks
> >> then.
> >
> > So what does it map, and to what?
>
> PCI bus IRQ to *host* IRQ. It is supposed to explore the routing of the
> host bridge between the root bus and the host's interrupt controller
> (i.e. the step that is currently missing the cached chain).
>
> > Maybe we can make the name imply that somehow.
>
> Better suggestions for this handler and maybe also the existing map_irq
> are welcome to make the difference clearer.
>
> Jan
So I won't object to adding a new API but if we do
it properly this won't help compatibility :(
Let's formulate what these do exactly, this will
also help us come up with sensible names.
1. The difference is that pci bridges route interrupt pins. So it gets
interrupt pin on device and returns interrupt pin on connector. All
attributes are standard PCI. We should remove all mentions of "irq"
really.
2. The pci root (yes it's a host bridge but let's
not use the term host if we can) routes
an interrupt pin on device to a host irq. It can also
do more things like invert polarity.
So yes we can add 2 to piix but we really should
remove 1 from it.
Wrt names - do you object to long names?
How about route_interrupt_pin for 1
and route_interrupt_pin_to_irq for 2?
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 14:34 ` Michael S. Tsirkin
@ 2012-06-01 15:15 ` Jan Kiszka
2012-06-01 15:28 ` Michael S. Tsirkin
2012-06-01 15:30 ` Michael S. Tsirkin
2012-06-01 15:26 ` Michael S. Tsirkin
1 sibling, 2 replies; 26+ messages in thread
From: Jan Kiszka @ 2012-06-01 15:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-06-01 16:34, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2012 at 03:57:01PM +0200, Jan Kiszka wrote:
>> On 2012-06-01 15:27, Michael S. Tsirkin wrote:
>>> On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
>>>> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
>>>>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
>>>>>>> irq_count instead of the pic_levels bitmap.
>>>>>>
>>>>>> Just that this affects generic PCI code, not only PIIX-specific things.
>>>>>
>>>>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
>>>>>
>>>>>> And that we need to save/restore some irq_count field according to the
>>>>>> old semantics.
>>>>>
>>>>> Well, it's a bug: this is redundant info we should not have exposed it.
>>>>>
>>>>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
>>>>> for now, then we'll find a hack making it work for migration.
>>>>
>>>> It remains non-trivial: I got your patch working (a minor init issue),
>>>> but yet without changing the number of IRQs for PIIX3, so keeping the
>>>> irq_count semantics for this host bridge.
>>>>
>>>> Now I'm facing three possibilities of how to proceed:
>>>
>>> They all look OK I think :) Some comments below.
>>>
>>>> 1. Give up on the (currently broken) feature to write a vmstate for
>>>> older QEMU versions.
>>>>
>>>> This will allow to declare the irq_count field in vmstate_pcibus
>>>> unused, and we would have to restore it on vmload step-wise via the
>>>> PCI devices. It would also allow to change its semantics for PIIX3,
>>>> mapping directly to PIC IRQs.
>>>
>>> I think that's okay too simply because these things are usually
>>> easy to fix after the fact when the rest of the issues are addressed.
>>
>> Don't get what you mean with "fixed". If we fix the vmstate generation
>> in making it backward-compatible again, we enter option 2. Option 1 is
>> explicitly about giving this up.
>
> What I really mean is I think I see how 2 can be added without much
> pain. So let's focus on 1 for now and worst case we break migration.
I'd like to avoid planing for this worst case as long as there are also
statements [1] that this is not acceptable for QEMU in general. It
doesn't to create a beautiful architecture initially about which we
already know that it will become more complex than alternatives in the end.
>
>>>
>>>> 2. Keep writing a legacy irq_count field.
>>>>
>>>> This will require quite a few new APIs so that host bridges that
>>>> want to change their nirq can still generate a compatible irq_count
>>>> vmstate field. Namely:
>>>> - A function to set up vmstate_irq_count and define a callback that
>>>> the core will invoke to prepare the vmstate_irq_count before
>>>> vmsave.
>>>> - A function to obtain the IRQ mapping /without/ the final host
>>>> bridge step. This is required so that the callback above can
>>>> calculate the old state like in the PIIX3 case.
>>>
>>> Does this really need to be so complex? It seems that we just need
>>> pci_get_irq_count(bus, irq) which can use the existing map_irq API, no?
>>> Then invoke that before save.
>>
>> No, because the new map_irq of the PIIX3 bridge will also include the
>> host bridge routing (or masking) according to the PIRQx routoing
>> registers of the PIIX3. Moreover, the fixup of the written legacy
>> irq_count state has to happen in the PCI layer, which therefore has to
>> query the host bridge for fixup information, not the other way around
>> (because the PCI bus vmstate is separate from the PIIX3 host bridge).
>>
>>>
>>>> 3. Keep irq_count and nirq as is, introduce additional map_host_irq.
>>>>
>>>> This is simpler than 2 and more compatible than 1. It would also
>>>> allow to introduce the polarity and masking information more
>>>> smoothly as we won't have to add it to existing map_irq callbacks
>>>> then.
>>>
>>> So what does it map, and to what?
>>
>> PCI bus IRQ to *host* IRQ. It is supposed to explore the routing of the
>> host bridge between the root bus and the host's interrupt controller
>> (i.e. the step that is currently missing the cached chain).
>>
>>> Maybe we can make the name imply that somehow.
>>
>> Better suggestions for this handler and maybe also the existing map_irq
>> are welcome to make the difference clearer.
>>
>> Jan
>
> So I won't object to adding a new API but if we do
> it properly this won't help compatibility :(
It will as this API does not touch the parts that affect the vmstate
(ie. semantics of irq_count won't change).
>
> Let's formulate what these do exactly, this will
> also help us come up with sensible names.
>
> 1. The difference is that pci bridges route interrupt pins. So it gets
> interrupt pin on device and returns interrupt pin on connector. All
> attributes are standard PCI. We should remove all mentions of "irq"
> really.
>
>
> 2. The pci root (yes it's a host bridge but let's
> not use the term host if we can) routes
> an interrupt pin on device to a host irq. It can also
> do more things like invert polarity.
>
> So yes we can add 2 to piix but we really should
> remove 1 from it.
>
> Wrt names - do you object to long names?
> How about route_interrupt_pin for 1
> and route_interrupt_pin_to_irq for 2?
I'm fine with this.
Jan
[1] http://permalink.gmane.org/gmane.comp.emulators.qemu/153357
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 14:34 ` Michael S. Tsirkin
2012-06-01 15:15 ` Jan Kiszka
@ 2012-06-01 15:26 ` Michael S. Tsirkin
1 sibling, 0 replies; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 15:26 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Fri, Jun 01, 2012 at 05:34:14PM +0300, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2012 at 03:57:01PM +0200, Jan Kiszka wrote:
> > On 2012-06-01 15:27, Michael S. Tsirkin wrote:
> > > On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
> > >> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
> > >>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
> > >>>>> irq_count instead of the pic_levels bitmap.
> > >>>>
> > >>>> Just that this affects generic PCI code, not only PIIX-specific things.
> > >>>
> > >>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
> > >>>
> > >>>> And that we need to save/restore some irq_count field according to the
> > >>>> old semantics.
> > >>>
> > >>> Well, it's a bug: this is redundant info we should not have exposed it.
> > >>>
> > >>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
> > >>> for now, then we'll find a hack making it work for migration.
> > >>
> > >> It remains non-trivial: I got your patch working (a minor init issue),
> > >> but yet without changing the number of IRQs for PIIX3, so keeping the
> > >> irq_count semantics for this host bridge.
> > >>
> > >> Now I'm facing three possibilities of how to proceed:
> > >
> > > They all look OK I think :) Some comments below.
> > >
> > >> 1. Give up on the (currently broken) feature to write a vmstate for
> > >> older QEMU versions.
> > >>
> > >> This will allow to declare the irq_count field in vmstate_pcibus
> > >> unused, and we would have to restore it on vmload step-wise via the
> > >> PCI devices. It would also allow to change its semantics for PIIX3,
> > >> mapping directly to PIC IRQs.
> > >
> > > I think that's okay too simply because these things are usually
> > > easy to fix after the fact when the rest of the issues are addressed.
> >
> > Don't get what you mean with "fixed". If we fix the vmstate generation
> > in making it backward-compatible again, we enter option 2. Option 1 is
> > explicitly about giving this up.
>
> What I really mean is I think I see how 2 can be added without much
> pain. So let's focus on 1 for now and worst case we break migration.
>
> > >
> > >> 2. Keep writing a legacy irq_count field.
> > >>
> > >> This will require quite a few new APIs so that host bridges that
> > >> want to change their nirq can still generate a compatible irq_count
> > >> vmstate field. Namely:
> > >> - A function to set up vmstate_irq_count and define a callback that
> > >> the core will invoke to prepare the vmstate_irq_count before
> > >> vmsave.
> > >> - A function to obtain the IRQ mapping /without/ the final host
> > >> bridge step. This is required so that the callback above can
> > >> calculate the old state like in the PIIX3 case.
> > >
> > > Does this really need to be so complex? It seems that we just need
> > > pci_get_irq_count(bus, irq) which can use the existing map_irq API, no?
> > > Then invoke that before save.
> >
> > No, because the new map_irq of the PIIX3 bridge will also include the
> > host bridge routing (or masking) according to the PIRQx routoing
> > registers of the PIIX3. Moreover, the fixup of the written legacy
> > irq_count state has to happen in the PCI layer, which therefore has to
> > query the host bridge for fixup information, not the other way around
> > (because the PCI bus vmstate is separate from the PIIX3 host bridge).
> >
> > >
> > >> 3. Keep irq_count and nirq as is, introduce additional map_host_irq.
> > >>
> > >> This is simpler than 2 and more compatible than 1. It would also
> > >> allow to introduce the polarity and masking information more
> > >> smoothly as we won't have to add it to existing map_irq callbacks
> > >> then.
> > >
> > > So what does it map, and to what?
> >
> > PCI bus IRQ to *host* IRQ. It is supposed to explore the routing of the
> > host bridge between the root bus and the host's interrupt controller
> > (i.e. the step that is currently missing the cached chain).
> >
> > > Maybe we can make the name imply that somehow.
> >
> > Better suggestions for this handler and maybe also the existing map_irq
> > are welcome to make the difference clearer.
> >
> > Jan
>
> So I won't object to adding a new API but if we do
> it properly this won't help compatibility :(
>
> Let's formulate what these do exactly, this will
> also help us come up with sensible names.
>
> 1. The difference is that pci bridges route interrupt pins. So it gets
> interrupt pin on device and returns interrupt pin on connector. All
> attributes are standard PCI. We should remove all mentions of "irq"
> really.
>
>
> 2. The pci root (yes it's a host bridge but let's
> not use the term host if we can) routes
> an interrupt pin on device to a host irq. It can also
> do more things like invert polarity.
>
> So yes we can add 2 to piix but we really should
> remove 1 from it.
>
> Wrt names - do you object to long names?
> How about route_interrupt_pin for 1
> and route_interrupt_pin_to_irq for 2?
A small clarification.
What I tried to say above is that pci bus does not
route irqs. pci bridge routes irqs: either host or
pci to pci bridge.
But as long as we thought the functionality is roughly
the same it made sense to say that bus has this
function simply because both bridge types have a bus.
But if not then the clean thing is a callback in p2p bridge
and another one in a host bridge. Having both these
and one in the bus looks weird.
> > --
> > Siemens AG, Corporate Technology, CT T DE IT 1
> > Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 15:15 ` Jan Kiszka
@ 2012-06-01 15:28 ` Michael S. Tsirkin
2012-06-01 15:54 ` Jan Kiszka
2012-06-01 15:30 ` Michael S. Tsirkin
1 sibling, 1 reply; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 15:28 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Fri, Jun 01, 2012 at 05:15:42PM +0200, Jan Kiszka wrote:
> On 2012-06-01 16:34, Michael S. Tsirkin wrote:
> > On Fri, Jun 01, 2012 at 03:57:01PM +0200, Jan Kiszka wrote:
> >> On 2012-06-01 15:27, Michael S. Tsirkin wrote:
> >>> On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
> >>>> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
> >>>>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
> >>>>>>> irq_count instead of the pic_levels bitmap.
> >>>>>>
> >>>>>> Just that this affects generic PCI code, not only PIIX-specific things.
> >>>>>
> >>>>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
> >>>>>
> >>>>>> And that we need to save/restore some irq_count field according to the
> >>>>>> old semantics.
> >>>>>
> >>>>> Well, it's a bug: this is redundant info we should not have exposed it.
> >>>>>
> >>>>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
> >>>>> for now, then we'll find a hack making it work for migration.
> >>>>
> >>>> It remains non-trivial: I got your patch working (a minor init issue),
> >>>> but yet without changing the number of IRQs for PIIX3, so keeping the
> >>>> irq_count semantics for this host bridge.
> >>>>
> >>>> Now I'm facing three possibilities of how to proceed:
> >>>
> >>> They all look OK I think :) Some comments below.
> >>>
> >>>> 1. Give up on the (currently broken) feature to write a vmstate for
> >>>> older QEMU versions.
> >>>>
> >>>> This will allow to declare the irq_count field in vmstate_pcibus
> >>>> unused, and we would have to restore it on vmload step-wise via the
> >>>> PCI devices. It would also allow to change its semantics for PIIX3,
> >>>> mapping directly to PIC IRQs.
> >>>
> >>> I think that's okay too simply because these things are usually
> >>> easy to fix after the fact when the rest of the issues are addressed.
> >>
> >> Don't get what you mean with "fixed". If we fix the vmstate generation
> >> in making it backward-compatible again, we enter option 2. Option 1 is
> >> explicitly about giving this up.
> >
> > What I really mean is I think I see how 2 can be added without much
> > pain. So let's focus on 1 for now and worst case we break migration.
>
> I'd like to avoid planing for this worst case as long as there are also
> statements [1] that this is not acceptable for QEMU in general. It
> doesn't to create a beautiful architecture initially about which we
> already know that it will become more complex than alternatives in the end.
>
> >
> >>>
> >>>> 2. Keep writing a legacy irq_count field.
> >>>>
> >>>> This will require quite a few new APIs so that host bridges that
> >>>> want to change their nirq can still generate a compatible irq_count
> >>>> vmstate field. Namely:
> >>>> - A function to set up vmstate_irq_count and define a callback that
> >>>> the core will invoke to prepare the vmstate_irq_count before
> >>>> vmsave.
> >>>> - A function to obtain the IRQ mapping /without/ the final host
> >>>> bridge step. This is required so that the callback above can
> >>>> calculate the old state like in the PIIX3 case.
> >>>
> >>> Does this really need to be so complex? It seems that we just need
> >>> pci_get_irq_count(bus, irq) which can use the existing map_irq API, no?
> >>> Then invoke that before save.
> >>
> >> No, because the new map_irq of the PIIX3 bridge will also include the
> >> host bridge routing (or masking) according to the PIRQx routoing
> >> registers of the PIIX3. Moreover, the fixup of the written legacy
> >> irq_count state has to happen in the PCI layer, which therefore has to
> >> query the host bridge for fixup information, not the other way around
> >> (because the PCI bus vmstate is separate from the PIIX3 host bridge).
> >>
> >>>
> >>>> 3. Keep irq_count and nirq as is, introduce additional map_host_irq.
> >>>>
> >>>> This is simpler than 2 and more compatible than 1. It would also
> >>>> allow to introduce the polarity and masking information more
> >>>> smoothly as we won't have to add it to existing map_irq callbacks
> >>>> then.
> >>>
> >>> So what does it map, and to what?
> >>
> >> PCI bus IRQ to *host* IRQ. It is supposed to explore the routing of the
> >> host bridge between the root bus and the host's interrupt controller
> >> (i.e. the step that is currently missing the cached chain).
> >>
> >>> Maybe we can make the name imply that somehow.
> >>
> >> Better suggestions for this handler and maybe also the existing map_irq
> >> are welcome to make the difference clearer.
> >>
> >> Jan
> >
> > So I won't object to adding a new API but if we do
> > it properly this won't help compatibility :(
>
> It will as this API does not touch the parts that affect the vmstate
> (ie. semantics of irq_count won't change).
Yes but irq_count in vmstate is a bug. IMO even if we do
not change anything we should ignore irq_count on
load and recalculate it from what the devices supply.
> >
> > Let's formulate what these do exactly, this will
> > also help us come up with sensible names.
> >
> > 1. The difference is that pci bridges route interrupt pins. So it gets
> > interrupt pin on device and returns interrupt pin on connector. All
> > attributes are standard PCI. We should remove all mentions of "irq"
> > really.
> >
> >
> > 2. The pci root (yes it's a host bridge but let's
> > not use the term host if we can) routes
> > an interrupt pin on device to a host irq. It can also
> > do more things like invert polarity.
> >
> > So yes we can add 2 to piix but we really should
> > remove 1 from it.
> >
> > Wrt names - do you object to long names?
> > How about route_interrupt_pin for 1
> > and route_interrupt_pin_to_irq for 2?
>
> I'm fine with this.
>
> Jan
>
> [1] http://permalink.gmane.org/gmane.comp.emulators.qemu/153357
>
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 15:15 ` Jan Kiszka
2012-06-01 15:28 ` Michael S. Tsirkin
@ 2012-06-01 15:30 ` Michael S. Tsirkin
2012-06-01 15:59 ` Jan Kiszka
1 sibling, 1 reply; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 15:30 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Fri, Jun 01, 2012 at 05:15:42PM +0200, Jan Kiszka wrote:
> On 2012-06-01 16:34, Michael S. Tsirkin wrote:
> > On Fri, Jun 01, 2012 at 03:57:01PM +0200, Jan Kiszka wrote:
> >> On 2012-06-01 15:27, Michael S. Tsirkin wrote:
> >>> On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
> >>>> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
> >>>>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
> >>>>>>> irq_count instead of the pic_levels bitmap.
> >>>>>>
> >>>>>> Just that this affects generic PCI code, not only PIIX-specific things.
> >>>>>
> >>>>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
> >>>>>
> >>>>>> And that we need to save/restore some irq_count field according to the
> >>>>>> old semantics.
> >>>>>
> >>>>> Well, it's a bug: this is redundant info we should not have exposed it.
> >>>>>
> >>>>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
> >>>>> for now, then we'll find a hack making it work for migration.
> >>>>
> >>>> It remains non-trivial: I got your patch working (a minor init issue),
> >>>> but yet without changing the number of IRQs for PIIX3, so keeping the
> >>>> irq_count semantics for this host bridge.
> >>>>
> >>>> Now I'm facing three possibilities of how to proceed:
> >>>
> >>> They all look OK I think :) Some comments below.
> >>>
> >>>> 1. Give up on the (currently broken) feature to write a vmstate for
> >>>> older QEMU versions.
> >>>>
> >>>> This will allow to declare the irq_count field in vmstate_pcibus
> >>>> unused, and we would have to restore it on vmload step-wise via the
> >>>> PCI devices. It would also allow to change its semantics for PIIX3,
> >>>> mapping directly to PIC IRQs.
> >>>
> >>> I think that's okay too simply because these things are usually
> >>> easy to fix after the fact when the rest of the issues are addressed.
> >>
> >> Don't get what you mean with "fixed". If we fix the vmstate generation
> >> in making it backward-compatible again, we enter option 2. Option 1 is
> >> explicitly about giving this up.
> >
> > What I really mean is I think I see how 2 can be added without much
> > pain. So let's focus on 1 for now and worst case we break migration.
>
> I'd like to avoid planing for this worst case as long as there are also
> statements [1] that this is not acceptable for QEMU in general. It
> doesn't to create a beautiful architecture initially about which we
> already know that it will become more complex than alternatives in the end.
1 and 2 are same really except 2 adds a hack for compatibility, no?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 15:28 ` Michael S. Tsirkin
@ 2012-06-01 15:54 ` Jan Kiszka
2012-06-01 16:05 ` Michael S. Tsirkin
0 siblings, 1 reply; 26+ messages in thread
From: Jan Kiszka @ 2012-06-01 15:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-06-01 17:28, Michael S. Tsirkin wrote:
>>> So I won't object to adding a new API but if we do
>>> it properly this won't help compatibility :(
>>
>> It will as this API does not touch the parts that affect the vmstate
>> (ie. semantics of irq_count won't change).
>
> Yes but irq_count in vmstate is a bug. IMO even if we do
> not change anything we should ignore irq_count on
> load and recalculate it from what the devices supply.
I don't disagree. But this will only allow keeping backward migration
support if we preserve the semantics of current map_irq somewhere - to
keep the chance of calculating the legacy values for vmsave as well.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 15:30 ` Michael S. Tsirkin
@ 2012-06-01 15:59 ` Jan Kiszka
0 siblings, 0 replies; 26+ messages in thread
From: Jan Kiszka @ 2012-06-01 15:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-06-01 17:30, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2012 at 05:15:42PM +0200, Jan Kiszka wrote:
>> On 2012-06-01 16:34, Michael S. Tsirkin wrote:
>>> On Fri, Jun 01, 2012 at 03:57:01PM +0200, Jan Kiszka wrote:
>>>> On 2012-06-01 15:27, Michael S. Tsirkin wrote:
>>>>> On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
>>>>>> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
>>>>>>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
>>>>>>>>> irq_count instead of the pic_levels bitmap.
>>>>>>>>
>>>>>>>> Just that this affects generic PCI code, not only PIIX-specific things.
>>>>>>>
>>>>>>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
>>>>>>>
>>>>>>>> And that we need to save/restore some irq_count field according to the
>>>>>>>> old semantics.
>>>>>>>
>>>>>>> Well, it's a bug: this is redundant info we should not have exposed it.
>>>>>>>
>>>>>>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
>>>>>>> for now, then we'll find a hack making it work for migration.
>>>>>>
>>>>>> It remains non-trivial: I got your patch working (a minor init issue),
>>>>>> but yet without changing the number of IRQs for PIIX3, so keeping the
>>>>>> irq_count semantics for this host bridge.
>>>>>>
>>>>>> Now I'm facing three possibilities of how to proceed:
>>>>>
>>>>> They all look OK I think :) Some comments below.
>>>>>
>>>>>> 1. Give up on the (currently broken) feature to write a vmstate for
>>>>>> older QEMU versions.
>>>>>>
>>>>>> This will allow to declare the irq_count field in vmstate_pcibus
>>>>>> unused, and we would have to restore it on vmload step-wise via the
>>>>>> PCI devices. It would also allow to change its semantics for PIIX3,
>>>>>> mapping directly to PIC IRQs.
>>>>>
>>>>> I think that's okay too simply because these things are usually
>>>>> easy to fix after the fact when the rest of the issues are addressed.
>>>>
>>>> Don't get what you mean with "fixed". If we fix the vmstate generation
>>>> in making it backward-compatible again, we enter option 2. Option 1 is
>>>> explicitly about giving this up.
>>>
>>> What I really mean is I think I see how 2 can be added without much
>>> pain. So let's focus on 1 for now and worst case we break migration.
>>
>> I'd like to avoid planing for this worst case as long as there are also
>> statements [1] that this is not acceptable for QEMU in general. It
>> doesn't to create a beautiful architecture initially about which we
>> already know that it will become more complex than alternatives in the end.
>
> 1 and 2 are same really except 2 adds a hack for compatibility, no?
Yes, 2 would allow us to maintain irq_count in different ways as well,
specifically using it calculate shared output IRQ lines before reporting
them to the PIC generically. This approach has both a charming and an
ugly face.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 15:54 ` Jan Kiszka
@ 2012-06-01 16:05 ` Michael S. Tsirkin
2012-06-01 16:17 ` Jan Kiszka
0 siblings, 1 reply; 26+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 16:05 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On Fri, Jun 01, 2012 at 05:54:15PM +0200, Jan Kiszka wrote:
> On 2012-06-01 17:28, Michael S. Tsirkin wrote:
> >>> So I won't object to adding a new API but if we do
> >>> it properly this won't help compatibility :(
> >>
> >> It will as this API does not touch the parts that affect the vmstate
> >> (ie. semantics of irq_count won't change).
> >
> > Yes but irq_count in vmstate is a bug. IMO even if we do
> > not change anything we should ignore irq_count on
> > load and recalculate it from what the devices supply.
>
> I don't disagree. But this will only allow keeping backward migration
> support if we preserve the semantics of current map_irq somewhere - to
> keep the chance of calculating the legacy values for vmsave as well.
>
> Jan
We don't need to preserve it, right? We can calculate it before
savevm.
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq
2012-06-01 16:05 ` Michael S. Tsirkin
@ 2012-06-01 16:17 ` Jan Kiszka
0 siblings, 0 replies; 26+ messages in thread
From: Jan Kiszka @ 2012-06-01 16:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Marcelo Tosatti, qemu-devel, Avi Kivity
On 2012-06-01 18:05, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2012 at 05:54:15PM +0200, Jan Kiszka wrote:
>> On 2012-06-01 17:28, Michael S. Tsirkin wrote:
>>>>> So I won't object to adding a new API but if we do
>>>>> it properly this won't help compatibility :(
>>>>
>>>> It will as this API does not touch the parts that affect the vmstate
>>>> (ie. semantics of irq_count won't change).
>>>
>>> Yes but irq_count in vmstate is a bug. IMO even if we do
>>> not change anything we should ignore irq_count on
>>> load and recalculate it from what the devices supply.
>>
>> I don't disagree. But this will only allow keeping backward migration
>> support if we preserve the semantics of current map_irq somewhere - to
>> keep the chance of calculating the legacy values for vmsave as well.
>>
>> Jan
>
> We don't need to preserve it, right? We can calculate it before
> savevm.
We can't calculate it without something like the additional
infrastructure I listed.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2012-06-01 16:17 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-21 13:13 [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Jan Kiszka
2012-05-21 13:15 ` [Qemu-devel] [PATCH 2/2] pci: Add INTx routing notifier Jan Kiszka
2012-05-21 14:13 ` Alex Williamson
2012-05-21 14:10 ` [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq Alex Williamson
2012-05-21 14:36 ` Avi Kivity
2012-05-21 14:47 ` Jan Kiszka
2012-05-21 17:34 ` Michael S. Tsirkin
2012-05-21 18:58 ` Jan Kiszka
2012-05-21 21:04 ` Michael S. Tsirkin
2012-05-21 19:05 ` Michael S. Tsirkin
2012-05-21 20:35 ` Jan Kiszka
2012-05-21 21:03 ` Michael S. Tsirkin
[not found] <4FC65910.4030908@siemens.com>
[not found] ` <20120530174125.GC32721@redhat.com>
[not found] ` <4FC65E19.6090203@siemens.com>
[not found] ` <4FC65F70.4040501@siemens.com>
[not found] ` <20120530182356.GD32721@redhat.com>
[not found] ` <20120530182913.GE32721@redhat.com>
[not found] ` <20120530185150.GA1546@redhat.com>
[not found] ` <4FC66FB1.9050306@siemens.com>
[not found] ` <20120530193034.GE1551@redhat.com>
[not found] ` <4FC681B4.3030807@web.de>
[not found] ` <20120530203119.GH1551@redhat.com>
2012-06-01 12:52 ` Jan Kiszka
2012-06-01 13:27 ` Michael S. Tsirkin
2012-06-01 13:57 ` Jan Kiszka
2012-06-01 14:34 ` Michael S. Tsirkin
2012-06-01 15:15 ` Jan Kiszka
2012-06-01 15:28 ` Michael S. Tsirkin
2012-06-01 15:54 ` Jan Kiszka
2012-06-01 16:05 ` Michael S. Tsirkin
2012-06-01 16:17 ` Jan Kiszka
2012-06-01 15:30 ` Michael S. Tsirkin
2012-06-01 15:59 ` Jan Kiszka
2012-06-01 15:26 ` Michael S. Tsirkin
2012-06-01 13:28 ` Michael S. Tsirkin
2012-06-01 13:43 ` Jan Kiszka
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).