| Message ID | 73f236cab517fc5b2c2ba332e9efe2acbe727151.1338799936.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 04, 2012 at 10:52:14AM +0200, 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. pci_device_get_host_irq isn't in the cards anymore, no? > Will be used by KVM PCI device assignment and VFIO. > > 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 8878a11..5b99f4b 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1101,6 +1101,25 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin) > return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]); > } > > +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 *dev, > + INTxRoutingNotifier notifier) > +{ > + dev->intx_routing_notifier = notifier; > +} > + > /***********************************************************/ > /* monitor info on PCI */ > > diff --git a/hw/pci.h b/hw/pci.h > index bbba01e..e7237cf 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -182,6 +182,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); > @@ -261,6 +262,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; > @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > MemoryRegion *address_space_io, > uint8_t devfn_min, int nirq); > PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); > +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); > +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 > --- a/hw/pci_bridge.c > +++ b/hw/pci_bridge.c > @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) > pci_bridge_reset_reg(dev); > } > > +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) > { > @@ -333,6 +340,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 347177f..8fd21f3 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -422,6 +422,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
On 2012-06-07 15:14, Michael S. Tsirkin wrote: > On Mon, Jun 04, 2012 at 10:52:14AM +0200, 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. > > pci_device_get_host_irq isn't in the cards anymore, no? Yep, must be pci_device_route_intx_to_irq. Will fix if this series requires more than dropping patch 2. Otherwise, I would ask you to replace it on merge. Thanks, Jan
On Mon, Jun 04, 2012 at 10:52:14AM +0200, 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 and VFIO. > > 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 8878a11..5b99f4b 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1101,6 +1101,25 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin) > return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]); > } > > +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); > + } > + } > +} > + No documentation and it's not obvious when do you need to do this. It would seem from the name that it should be called after you change interrupt routing at the specific bus? From commit log it would seem that even irq changes should invoke this. So why isn't this notifier at the host bridge then? > +void pci_device_set_intx_routing_notifier(PCIDevice *dev, > + INTxRoutingNotifier notifier) > +{ > + dev->intx_routing_notifier = notifier; > +} > + No documentation, and it's not obvious. Why is this getting PCIDevice? Does this notify users about updates to this device? Updates below this device? Above this device? > /***********************************************************/ > /* monitor info on PCI */ > > diff --git a/hw/pci.h b/hw/pci.h > index bbba01e..e7237cf 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -182,6 +182,7 @@ typedef struct PCIDeviceClass { > const char *romfile; > } PCIDeviceClass; > > +typedef void (*INTxRoutingNotifier)(PCIDevice *dev); Let's call it PCIINTx.... please > typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector, > MSIMessage msg); > typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector); > @@ -261,6 +262,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; > @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > MemoryRegion *address_space_io, > uint8_t devfn_min, int nirq); > PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); > +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); Well true it fires the notifier but what it does conceptually is update intx routing. > +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 > --- a/hw/pci_bridge.c > +++ b/hw/pci_bridge.c > @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) > pci_bridge_reset_reg(dev); > } > > +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) > { > @@ -333,6 +340,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 347177f..8fd21f3 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -422,6 +422,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
On 2012-06-10 11:48, Michael S. Tsirkin wrote: > On Mon, Jun 04, 2012 at 10:52:14AM +0200, 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 and VFIO. >> >> 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 8878a11..5b99f4b 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -1101,6 +1101,25 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin) >> return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]); >> } >> >> +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); >> + } >> + } >> +} >> + > > No documentation and it's not obvious when do you need > to do this. Yes, will add some lines. > It would seem from the name that it should be called after you change > interrupt routing at the specific bus? Correct. > > From commit log it would seem that even irq changes should > invoke this. So why isn't this notifier at the host bridge then? Can't follow, where does the commit log imply this? It is only about routing changes, not IRQ level changes. > >> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, >> + INTxRoutingNotifier notifier) >> +{ >> + dev->intx_routing_notifier = notifier; >> +} >> + > > No documentation, and it's not obvious. > Why is this getting PCIDevice? > Does this notify users about updates to this device? > Updates below this device? > Above this device? It informs about changes on the route of the device interrupts to the output of the host bridge. > >> /***********************************************************/ >> /* monitor info on PCI */ >> >> diff --git a/hw/pci.h b/hw/pci.h >> index bbba01e..e7237cf 100644 >> --- a/hw/pci.h >> +++ b/hw/pci.h >> @@ -182,6 +182,7 @@ typedef struct PCIDeviceClass { >> const char *romfile; >> } PCIDeviceClass; >> >> +typedef void (*INTxRoutingNotifier)(PCIDevice *dev); > > Let's call it PCIINTx.... please OK. > >> typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector, >> MSIMessage msg); >> typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector); >> @@ -261,6 +262,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; >> @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, >> MemoryRegion *address_space_io, >> uint8_t devfn_min, int nirq); >> PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); >> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); > > Well true it fires the notifier but what it does conceptually > is update intx routing. Nope, it informs about updates _after_ they happened. > >> +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 >> --- a/hw/pci_bridge.c >> +++ b/hw/pci_bridge.c >> @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) >> pci_bridge_reset_reg(dev); >> } >> >> +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) >> { >> @@ -333,6 +340,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 347177f..8fd21f3 100644 >> --- a/hw/piix_pci.c >> +++ b/hw/piix_pci.c >> @@ -422,6 +422,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 > > Thanks, Jan
On Sun, Jun 10, 2012 at 12:05:10PM +0200, Jan Kiszka wrote: > On 2012-06-10 11:48, Michael S. Tsirkin wrote: > > On Mon, Jun 04, 2012 at 10:52:14AM +0200, 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 and VFIO. > >> > >> 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 8878a11..5b99f4b 100644 > >> --- a/hw/pci.c > >> +++ b/hw/pci.c > >> @@ -1101,6 +1101,25 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin) > >> return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]); > >> } > >> > >> +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); > >> + } > >> + } > >> +} > >> + > > > > No documentation and it's not obvious when do you need > > to do this. > > Yes, will add some lines. Also, who calls this? Apparently it's invoked from pci_bridge_intx_routing_update? > > It would seem from the name that it should be called after you change > > interrupt routing at the specific bus? > > Correct. > > > > > From commit log it would seem that even irq changes should > > invoke this. So why isn't this notifier at the host bridge then? > > Can't follow, where does the commit log imply this? It is only about > routing changes, not IRQ level changes. Not sure - it says use pci_device_get_host_irq so the implication is users cache the result of pci_device_get_host_irq? > > > >> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, > >> + INTxRoutingNotifier notifier) > >> +{ > >> + dev->intx_routing_notifier = notifier; > >> +} > >> + > > > > No documentation, and it's not obvious. > > Why is this getting PCIDevice? > > Does this notify users about updates to this device? > > Updates below this device? > > Above this device? > > It informs about changes on the route of the device interrupts to the > output of the host bridge. > > > >> /***********************************************************/ > >> /* monitor info on PCI */ > >> > >> diff --git a/hw/pci.h b/hw/pci.h > >> index bbba01e..e7237cf 100644 > >> --- a/hw/pci.h > >> +++ b/hw/pci.h > >> @@ -182,6 +182,7 @@ typedef struct PCIDeviceClass { > >> const char *romfile; > >> } PCIDeviceClass; > >> > >> +typedef void (*INTxRoutingNotifier)(PCIDevice *dev); > > > > Let's call it PCIINTx.... please > > OK. > > > > >> typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector, > >> MSIMessage msg); > >> typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector); > >> @@ -261,6 +262,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; > >> @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > >> MemoryRegion *address_space_io, > >> uint8_t devfn_min, int nirq); > >> PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); > >> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); > > > > Well true it fires the notifier but what it does conceptually > > is update intx routing. > > Nope, it informs about updates _after_ they happened. Don't we need to update the cached pin if this happens? If yes I would this a better API would both update the cache and then trigger a notifier. And the notifier can then be cache change notifier, and the "fire" function would instead be "update_cache". > > > >> +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 > >> --- a/hw/pci_bridge.c > >> +++ b/hw/pci_bridge.c > >> @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) > >> pci_bridge_reset_reg(dev); > >> } > >> > >> +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); > >> +} > >> + I'd prefer a version that uses a simple loop, not recursion. For example it is not clear at this point for which devices is it OK to set the notifier and which assume the notifier recurses to children. > >> /* default qdev initialization function for PCI-to-PCI bridge */ > >> int pci_bridge_initfn(PCIDevice *dev) > >> { > >> @@ -333,6 +340,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 347177f..8fd21f3 100644 > >> --- a/hw/piix_pci.c > >> +++ b/hw/piix_pci.c > >> @@ -422,6 +422,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 > > > > > > Thanks, > Jan >
On 2012-06-10 12:33, Michael S. Tsirkin wrote: > On Sun, Jun 10, 2012 at 12:05:10PM +0200, Jan Kiszka wrote: >> On 2012-06-10 11:48, Michael S. Tsirkin wrote: >>> On Mon, Jun 04, 2012 at 10:52:14AM +0200, 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 and VFIO. >>>> >>>> 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 8878a11..5b99f4b 100644 >>>> --- a/hw/pci.c >>>> +++ b/hw/pci.c >>>> @@ -1101,6 +1101,25 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin) >>>> return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]); >>>> } >>>> >>>> +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); >>>> + } >>>> + } >>>> +} >>>> + >>> >>> No documentation and it's not obvious when do you need >>> to do this. >> >> Yes, will add some lines. > > Also, who calls this? Apparently it's invoked from > pci_bridge_intx_routing_update? That's to forward the change reported from the parent bus. In fact, PCI does not allow pin routing changes once the device is plugged. The only change can come from the host bridge's configuration. So there are two types of users: - the host bridge after internal configuration changes - a PCI-PCI bridge to forward the notification to its children > > >>> It would seem from the name that it should be called after you change >>> interrupt routing at the specific bus? >> >> Correct. >> >>> >>> From commit log it would seem that even irq changes should >>> invoke this. So why isn't this notifier at the host bridge then? >> >> Can't follow, where does the commit log imply this? It is only about >> routing changes, not IRQ level changes. > > Not sure - it says use pci_device_get_host_irq > so the implication is users cache the result of > pci_device_get_host_irq? That's the old name, I've sent v2 where the commitlog was fixed. > >>> >>>> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, >>>> + INTxRoutingNotifier notifier) >>>> +{ >>>> + dev->intx_routing_notifier = notifier; >>>> +} >>>> + >>> >>> No documentation, and it's not obvious. >>> Why is this getting PCIDevice? >>> Does this notify users about updates to this device? >>> Updates below this device? >>> Above this device? >> >> It informs about changes on the route of the device interrupts to the >> output of the host bridge. >>> >>>> /***********************************************************/ >>>> /* monitor info on PCI */ >>>> >>>> diff --git a/hw/pci.h b/hw/pci.h >>>> index bbba01e..e7237cf 100644 >>>> --- a/hw/pci.h >>>> +++ b/hw/pci.h >>>> @@ -182,6 +182,7 @@ typedef struct PCIDeviceClass { >>>> const char *romfile; >>>> } PCIDeviceClass; >>>> >>>> +typedef void (*INTxRoutingNotifier)(PCIDevice *dev); >>> >>> Let's call it PCIINTx.... please >> >> OK. >> >>> >>>> typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector, >>>> MSIMessage msg); >>>> typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector); >>>> @@ -261,6 +262,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; >>>> @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, >>>> MemoryRegion *address_space_io, >>>> uint8_t devfn_min, int nirq); >>>> PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); >>>> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); >>> >>> Well true it fires the notifier but what it does conceptually >>> is update intx routing. >> >> Nope, it informs about updates _after_ they happened. > > Don't we need to update the cached pin if this happens? > If yes I would this a better API would both update the cache > and then trigger a notifier. > And the notifier can then be cache change notifier, > and the "fire" function would instead be "update_cache". See above, the cached part of the route is static anyway. What changes is the host bridge configuration. > >>> >>>> +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 >>>> --- a/hw/pci_bridge.c >>>> +++ b/hw/pci_bridge.c >>>> @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) >>>> pci_bridge_reset_reg(dev); >>>> } >>>> >>>> +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); >>>> +} >>>> + > > I'd prefer a version that uses a simple loop, > not recursion. For example it is not clear > at this point for which devices is it OK to set > the notifier and which assume the notifier > recurses to children. The notification must be forwarded to any secondary bus because any device below can have a notifier registered. And I think recursion is the cleaner approach for this as we can have complex topologies. Jan
On Sun, Jun 10, 2012 at 12:44:05PM +0200, Jan Kiszka wrote: > On 2012-06-10 12:33, Michael S. Tsirkin wrote: > > On Sun, Jun 10, 2012 at 12:05:10PM +0200, Jan Kiszka wrote: > >> On 2012-06-10 11:48, Michael S. Tsirkin wrote: > >>> On Mon, Jun 04, 2012 at 10:52:14AM +0200, 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 and VFIO. > >>>> > >>>> 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 8878a11..5b99f4b 100644 > >>>> --- a/hw/pci.c > >>>> +++ b/hw/pci.c > >>>> @@ -1101,6 +1101,25 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin) > >>>> return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]); > >>>> } > >>>> > >>>> +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); > >>>> + } > >>>> + } > >>>> +} > >>>> + > >>> > >>> No documentation and it's not obvious when do you need > >>> to do this. > >> > >> Yes, will add some lines. > > > > Also, who calls this? Apparently it's invoked from > > pci_bridge_intx_routing_update? > > That's to forward the change reported from the parent bus. In fact, PCI > does not allow pin routing changes once the device is plugged. The only > change can come from the host bridge's configuration. > > So there are two types of users: > - the host bridge after internal configuration changes > - a PCI-PCI bridge to forward the notification to its children > > > > > > >>> It would seem from the name that it should be called after you change > >>> interrupt routing at the specific bus? > >> > >> Correct. > >> > >>> > >>> From commit log it would seem that even irq changes should > >>> invoke this. So why isn't this notifier at the host bridge then? > >> > >> Can't follow, where does the commit log imply this? It is only about > >> routing changes, not IRQ level changes. > > > > Not sure - it says use pci_device_get_host_irq > > so the implication is users cache the result of > > pci_device_get_host_irq? > > That's the old name, I've sent v2 where the commitlog was fixed. Yes but still. If users cache the irq they need to get notified about *that*. Not about intx routing. > > > >>> > >>>> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, > >>>> + INTxRoutingNotifier notifier) > >>>> +{ > >>>> + dev->intx_routing_notifier = notifier; > >>>> +} > >>>> + > >>> > >>> No documentation, and it's not obvious. > >>> Why is this getting PCIDevice? > >>> Does this notify users about updates to this device? > >>> Updates below this device? > >>> Above this device? > >> > >> It informs about changes on the route of the device interrupts to the > >> output of the host bridge. > >>> > >>>> /***********************************************************/ > >>>> /* monitor info on PCI */ > >>>> > >>>> diff --git a/hw/pci.h b/hw/pci.h > >>>> index bbba01e..e7237cf 100644 > >>>> --- a/hw/pci.h > >>>> +++ b/hw/pci.h > >>>> @@ -182,6 +182,7 @@ typedef struct PCIDeviceClass { > >>>> const char *romfile; > >>>> } PCIDeviceClass; > >>>> > >>>> +typedef void (*INTxRoutingNotifier)(PCIDevice *dev); > >>> > >>> Let's call it PCIINTx.... please > >> > >> OK. > >> > >>> > >>>> typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector, > >>>> MSIMessage msg); > >>>> typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector); > >>>> @@ -261,6 +262,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; > >>>> @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > >>>> MemoryRegion *address_space_io, > >>>> uint8_t devfn_min, int nirq); > >>>> PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); > >>>> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); > >>> > >>> Well true it fires the notifier but what it does conceptually > >>> is update intx routing. > >> > >> Nope, it informs about updates _after_ they happened. > > > > Don't we need to update the cached pin if this happens? > > If yes I would this a better API would both update the cache > > and then trigger a notifier. > > And the notifier can then be cache change notifier, > > and the "fire" function would instead be "update_cache". > > See above, the cached part of the route is static anyway. What changes > is the host bridge configuration. You are saying it is only the intx to irq routing that can change? So maybe "pci_bus_update_intx_to_irq_routing"? > > > >>> > >>>> +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 > >>>> --- a/hw/pci_bridge.c > >>>> +++ b/hw/pci_bridge.c > >>>> @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) > >>>> pci_bridge_reset_reg(dev); > >>>> } > >>>> > >>>> +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); > >>>> +} > >>>> + > > > > I'd prefer a version that uses a simple loop, > > not recursion. For example it is not clear > > at this point for which devices is it OK to set > > the notifier and which assume the notifier > > recurses to children. > > The notification must be forwarded to any secondary bus because any > device below can have a notifier registered. And I think recursion is > the cleaner approach for this as we can have complex topologies. > > Jan > I don't think it's ever more complex than a tree.
On 2012-06-10 13:11, Michael S. Tsirkin wrote: >>>>> From commit log it would seem that even irq changes should >>>>> invoke this. So why isn't this notifier at the host bridge then? >>>> >>>> Can't follow, where does the commit log imply this? It is only about >>>> routing changes, not IRQ level changes. >>> >>> Not sure - it says use pci_device_get_host_irq >>> so the implication is users cache the result of >>> pci_device_get_host_irq? >> >> That's the old name, I've sent v2 where the commitlog was fixed. > > Yes but still. If users cache the irq they need to get > notified about *that*. Not about intx routing. The user caches the route of a device INTx to the host bridge output (precisely what pci_device_route_inx_to_irq returns), and for changes of that result it gets a notification this way. Nothing else. > >>> >>>>> >>>>>> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, >>>>>> + INTxRoutingNotifier notifier) >>>>>> +{ >>>>>> + dev->intx_routing_notifier = notifier; >>>>>> +} >>>>>> + >>>>> >>>>> No documentation, and it's not obvious. >>>>> Why is this getting PCIDevice? >>>>> Does this notify users about updates to this device? >>>>> Updates below this device? >>>>> Above this device? >>>> >>>> It informs about changes on the route of the device interrupts to the >>>> output of the host bridge. >>>>> >>>>>> /***********************************************************/ >>>>>> /* monitor info on PCI */ >>>>>> >>>>>> diff --git a/hw/pci.h b/hw/pci.h >>>>>> index bbba01e..e7237cf 100644 >>>>>> --- a/hw/pci.h >>>>>> +++ b/hw/pci.h >>>>>> @@ -182,6 +182,7 @@ typedef struct PCIDeviceClass { >>>>>> const char *romfile; >>>>>> } PCIDeviceClass; >>>>>> >>>>>> +typedef void (*INTxRoutingNotifier)(PCIDevice *dev); >>>>> >>>>> Let's call it PCIINTx.... please >>>> >>>> OK. >>>> >>>>> >>>>>> typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector, >>>>>> MSIMessage msg); >>>>>> typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector); >>>>>> @@ -261,6 +262,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; >>>>>> @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, >>>>>> MemoryRegion *address_space_io, >>>>>> uint8_t devfn_min, int nirq); >>>>>> PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); >>>>>> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); >>>>> >>>>> Well true it fires the notifier but what it does conceptually >>>>> is update intx routing. >>>> >>>> Nope, it informs about updates _after_ they happened. >>> >>> Don't we need to update the cached pin if this happens? >>> If yes I would this a better API would both update the cache >>> and then trigger a notifier. >>> And the notifier can then be cache change notifier, >>> and the "fire" function would instead be "update_cache". >> >> See above, the cached part of the route is static anyway. What changes >> is the host bridge configuration. > > You are saying it is only the intx to irq routing that > can change? > So maybe "pci_bus_update_intx_to_irq_routing"? Again, this function does not _update_ anything. It informs about a host-bridge-specific update, i.e. something that happened outside the generic code beforehand. > >>> >>>>> >>>>>> +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 >>>>>> --- a/hw/pci_bridge.c >>>>>> +++ b/hw/pci_bridge.c >>>>>> @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) >>>>>> pci_bridge_reset_reg(dev); >>>>>> } >>>>>> >>>>>> +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); >>>>>> +} >>>>>> + >>> >>> I'd prefer a version that uses a simple loop, >>> not recursion. For example it is not clear >>> at this point for which devices is it OK to set >>> the notifier and which assume the notifier >>> recurses to children. >> >> The notification must be forwarded to any secondary bus because any >> device below can have a notifier registered. And I think recursion is >> the cleaner approach for this as we can have complex topologies. >> >> Jan >> > > I don't think it's ever more complex than a tree. > For sure, and this is what the recursive invocation addresses. Jan
On Sun, Jun 10, 2012 at 01:18:15PM +0200, Jan Kiszka wrote: > On 2012-06-10 13:11, Michael S. Tsirkin wrote: > >>>>> From commit log it would seem that even irq changes should > >>>>> invoke this. So why isn't this notifier at the host bridge then? > >>>> > >>>> Can't follow, where does the commit log imply this? It is only about > >>>> routing changes, not IRQ level changes. > >>> > >>> Not sure - it says use pci_device_get_host_irq > >>> so the implication is users cache the result of > >>> pci_device_get_host_irq? > >> > >> That's the old name, I've sent v2 where the commitlog was fixed. > > > > Yes but still. If users cache the irq they need to get > > notified about *that*. Not about intx routing. > > The user caches the route of a device INTx to the host bridge output > (precisely what pci_device_route_inx_to_irq returns), and for changes of > that result it gets a notification this way. Nothing else. > > > > >>> > >>>>> > >>>>>> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, > >>>>>> + INTxRoutingNotifier notifier) > >>>>>> +{ > >>>>>> + dev->intx_routing_notifier = notifier; > >>>>>> +} > >>>>>> + > >>>>> > >>>>> No documentation, and it's not obvious. > >>>>> Why is this getting PCIDevice? > >>>>> Does this notify users about updates to this device? > >>>>> Updates below this device? > >>>>> Above this device? > >>>> > >>>> It informs about changes on the route of the device interrupts to the > >>>> output of the host bridge. > >>>>> > >>>>>> /***********************************************************/ > >>>>>> /* monitor info on PCI */ > >>>>>> > >>>>>> diff --git a/hw/pci.h b/hw/pci.h > >>>>>> index bbba01e..e7237cf 100644 > >>>>>> --- a/hw/pci.h > >>>>>> +++ b/hw/pci.h > >>>>>> @@ -182,6 +182,7 @@ typedef struct PCIDeviceClass { > >>>>>> const char *romfile; > >>>>>> } PCIDeviceClass; > >>>>>> > >>>>>> +typedef void (*INTxRoutingNotifier)(PCIDevice *dev); > >>>>> > >>>>> Let's call it PCIINTx.... please > >>>> > >>>> OK. > >>>> > >>>>> > >>>>>> typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector, > >>>>>> MSIMessage msg); > >>>>>> typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector); > >>>>>> @@ -261,6 +262,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; > >>>>>> @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > >>>>>> MemoryRegion *address_space_io, > >>>>>> uint8_t devfn_min, int nirq); > >>>>>> PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); > >>>>>> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); > >>>>> > >>>>> Well true it fires the notifier but what it does conceptually > >>>>> is update intx routing. > >>>> > >>>> Nope, it informs about updates _after_ they happened. > >>> > >>> Don't we need to update the cached pin if this happens? > >>> If yes I would this a better API would both update the cache > >>> and then trigger a notifier. > >>> And the notifier can then be cache change notifier, > >>> and the "fire" function would instead be "update_cache". > >> > >> See above, the cached part of the route is static anyway. What changes > >> is the host bridge configuration. > > > > You are saying it is only the intx to irq routing that > > can change? > > So maybe "pci_bus_update_intx_to_irq_routing"? > > Again, this function does not _update_ anything. It informs about a > host-bridge-specific update, i.e. something that happened outside the > generic code beforehand. > > > > >>> > >>>>> > >>>>>> +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 > >>>>>> --- a/hw/pci_bridge.c > >>>>>> +++ b/hw/pci_bridge.c > >>>>>> @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) > >>>>>> pci_bridge_reset_reg(dev); > >>>>>> } > >>>>>> > >>>>>> +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); > >>>>>> +} > >>>>>> + > >>> > >>> I'd prefer a version that uses a simple loop, > >>> not recursion. For example it is not clear > >>> at this point for which devices is it OK to set > >>> the notifier and which assume the notifier > >>> recurses to children. > >> > >> The notification must be forwarded to any secondary bus because any > >> device below can have a notifier registered. And I think recursion is > >> the cleaner approach for this as we can have complex topologies. > >> > >> Jan > >> > > > > I don't think it's ever more complex than a tree. > > > > For sure, and this is what the recursive invocation addresses. > > Jan > It's OK to use recursion but when done through a callback like this it's unreadable. Also, you need to setup you cache after intx cache has been initialized, and you provide no clean way to do that. One way to fix all this is call the notifier for devices, if set, from pci_set_bus_intx_routing. Then assume that intx to irq translations can be cached even though they aren't now. So you will need to invoke pci_set_bus_intx_routing on intx to irq mapping changes, and that fires the notifier for free.
On 2012-06-10 13:39, Michael S. Tsirkin wrote: > It's OK to use recursion but when done through a callback > like this it's unreadable. Isn't the alternative poking into foreign bridge device states for their secondary buses? > Also, you need to setup you cache after intx cache has been > initialized, and you provide no clean way to do that. Once a PCI device is registered, the INTx route can be queried. So the device user will call pci_device_route_intx_to_irq once (e.g. in the device init function which is invoked afterward) to fill its cache and receive a notification if an update is needed. I do not see why, and specifically how you could query the route earlier or register a callback. > > One way to fix all this is call the notifier for devices, if set, from > pci_set_bus_intx_routing. > Then assume that intx to irq translations can be cached > even though they aren't now. So you will need to invoke > pci_set_bus_intx_routing on intx to irq mapping changes, > and that fires the notifier for free. pci_set_bus_intx_routing is really only for the initial setup of the static INTx pin routes. And this happens on pci_bus_irqs/pci_register_bus, ie. triggered by the host bridge. By that time, there can't be any notifier listeners - as there are no devices yet. Jan
On Sun, Jun 10, 2012 at 02:09:20PM +0200, Jan Kiszka wrote: > On 2012-06-10 13:39, Michael S. Tsirkin wrote: > > It's OK to use recursion but when done through a callback > > like this it's unreadable. > > Isn't the alternative poking into foreign bridge device states for their > secondary buses? pci_set_bus_intx_routing does this already. > > Also, you need to setup you cache after intx cache has been > > initialized, and you provide no clean way to do that. > > Once a PCI device is registered, the INTx route can be queried. So the > device user will call pci_device_route_intx_to_irq once (e.g. in the > device init function which is invoked afterward) to fill its cache and > receive a notification if an update is needed. I do not see why, and > specifically how you could query the route earlier or register a callback. Before pci_bus_irqs is called. Why is another question. > > > > One way to fix all this is call the notifier for devices, if set, from > > pci_set_bus_intx_routing. > > Then assume that intx to irq translations can be cached > > even though they aren't now. So you will need to invoke > > pci_set_bus_intx_routing on intx to irq mapping changes, > > and that fires the notifier for free. > > pci_set_bus_intx_routing is really only for the initial setup of the > static INTx pin routes. And this happens on > pci_bus_irqs/pci_register_bus, ie. triggered by the host bridge. By that > time, there can't be any notifier listeners - as there are no devices yet. > > Jan > What I am saying is we'll cache the final IRQ at some point. Pretend it's already that way so callers are ready for this.
On Sun, Jun 10, 2012 at 01:18:15PM +0200, Jan Kiszka wrote: > >>>>>> @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > >>>>>> MemoryRegion *address_space_io, > >>>>>> uint8_t devfn_min, int nirq); > >>>>>> PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); > >>>>>> +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); > >>>>> > >>>>> Well true it fires the notifier but what it does conceptually > >>>>> is update intx routing. > >>>> > >>>> Nope, it informs about updates _after_ they happened. > >>> > >>> Don't we need to update the cached pin if this happens? > >>> If yes I would this a better API would both update the cache > >>> and then trigger a notifier. > >>> And the notifier can then be cache change notifier, > >>> and the "fire" function would instead be "update_cache". > >> > >> See above, the cached part of the route is static anyway. What changes > >> is the host bridge configuration. > > > > You are saying it is only the intx to irq routing that > > can change? > > So maybe "pci_bus_update_intx_to_irq_routing"? > > Again, this function does not _update_ anything. It informs about a > host-bridge-specific update, i.e. something that happened outside the > generic code beforehand. Yes it does what it says but it's an ugly interface all the same. What does a host device emulation care about notifiers? It's an implementation detail. The *real* reason is that someone caches intx to irq routing and you need to update that cache. So just say so.
On 2012-06-10 14:16, Michael S. Tsirkin wrote: > On Sun, Jun 10, 2012 at 02:09:20PM +0200, Jan Kiszka wrote: >> On 2012-06-10 13:39, Michael S. Tsirkin wrote: >>> It's OK to use recursion but when done through a callback >>> like this it's unreadable. >> >> Isn't the alternative poking into foreign bridge device states for their >> secondary buses? > > pci_set_bus_intx_routing does this already. True. OK, I can do the recursion in pci_bus_fire_intx_routing_notifier directly instead of pushing this into the bridge. > >>> Also, you need to setup you cache after intx cache has been >>> initialized, and you provide no clean way to do that. >> >> Once a PCI device is registered, the INTx route can be queried. So the >> device user will call pci_device_route_intx_to_irq once (e.g. in the >> device init function which is invoked afterward) to fill its cache and >> receive a notification if an update is needed. I do not see why, and >> specifically how you could query the route earlier or register a callback. > > Before pci_bus_irqs is called. > Why is another question. > >>> >>> One way to fix all this is call the notifier for devices, if set, from >>> pci_set_bus_intx_routing. >>> Then assume that intx to irq translations can be cached >>> even though they aren't now. So you will need to invoke >>> pci_set_bus_intx_routing on intx to irq mapping changes, >>> and that fires the notifier for free. >> >> pci_set_bus_intx_routing is really only for the initial setup of the >> static INTx pin routes. And this happens on >> pci_bus_irqs/pci_register_bus, ie. triggered by the host bridge. By that >> time, there can't be any notifier listeners - as there are no devices yet. >> >> Jan >> > > What I am saying is we'll cache the final IRQ at some point. > Pretend it's already that way so callers are ready for this. This wouldn't change the picture very much: Before the host bridge is fully initialized, there is no valid route available. But before that, there is also no device attached to it. So the invocation pattern wouldn't change. What would change is the semantic of the interface to the host bridge. So what about this: provide a public pci_root_bus_intx_routing_updated which so far just calls the internal-use-only pci_bus_fire_intx_routing_notifier? Jan
On Sun, Jun 10, 2012 at 02:33:03PM +0200, Jan Kiszka wrote: > On 2012-06-10 14:16, Michael S. Tsirkin wrote: > > On Sun, Jun 10, 2012 at 02:09:20PM +0200, Jan Kiszka wrote: > >> On 2012-06-10 13:39, Michael S. Tsirkin wrote: > >>> It's OK to use recursion but when done through a callback > >>> like this it's unreadable. > >> > >> Isn't the alternative poking into foreign bridge device states for their > >> secondary buses? > > > > pci_set_bus_intx_routing does this already. > > True. OK, I can do the recursion in pci_bus_fire_intx_routing_notifier > directly instead of pushing this into the bridge. > > > > >>> Also, you need to setup you cache after intx cache has been > >>> initialized, and you provide no clean way to do that. > >> > >> Once a PCI device is registered, the INTx route can be queried. So the > >> device user will call pci_device_route_intx_to_irq once (e.g. in the > >> device init function which is invoked afterward) to fill its cache and > >> receive a notification if an update is needed. I do not see why, and > >> specifically how you could query the route earlier or register a callback. > > > > Before pci_bus_irqs is called. > > Why is another question. > > > >>> > >>> One way to fix all this is call the notifier for devices, if set, from > >>> pci_set_bus_intx_routing. > >>> Then assume that intx to irq translations can be cached > >>> even though they aren't now. So you will need to invoke > >>> pci_set_bus_intx_routing on intx to irq mapping changes, > >>> and that fires the notifier for free. > >> > >> pci_set_bus_intx_routing is really only for the initial setup of the > >> static INTx pin routes. And this happens on > >> pci_bus_irqs/pci_register_bus, ie. triggered by the host bridge. By that > >> time, there can't be any notifier listeners - as there are no devices yet. > >> > >> Jan > >> > > > > What I am saying is we'll cache the final IRQ at some point. > > Pretend it's already that way so callers are ready for this. > > This wouldn't change the picture very much: Before the host bridge is > fully initialized, there is no valid route available. But before that, > there is also no device attached to it. So the invocation pattern > wouldn't change. > > What would change is the semantic of the interface to the host bridge. > So what about this: provide a public pci_root_bus_intx_routing_updated > which so far just calls the internal-use-only > pci_bus_fire_intx_routing_notifier? > > Jan > I think a better name is pci_bus_update_intx_irq_cache or something like that. And I really think it's better to recalculate the intx routing there as well, so that if some bus implements a dynamic route_intx it just needs to call this after update.
On 2012-06-10 14:42, Michael S. Tsirkin wrote: > On Sun, Jun 10, 2012 at 02:33:03PM +0200, Jan Kiszka wrote: >> On 2012-06-10 14:16, Michael S. Tsirkin wrote: >>> On Sun, Jun 10, 2012 at 02:09:20PM +0200, Jan Kiszka wrote: >>>> On 2012-06-10 13:39, Michael S. Tsirkin wrote: >>>>> It's OK to use recursion but when done through a callback >>>>> like this it's unreadable. >>>> >>>> Isn't the alternative poking into foreign bridge device states for their >>>> secondary buses? >>> >>> pci_set_bus_intx_routing does this already. >> >> True. OK, I can do the recursion in pci_bus_fire_intx_routing_notifier >> directly instead of pushing this into the bridge. >> >>> >>>>> Also, you need to setup you cache after intx cache has been >>>>> initialized, and you provide no clean way to do that. >>>> >>>> Once a PCI device is registered, the INTx route can be queried. So the >>>> device user will call pci_device_route_intx_to_irq once (e.g. in the >>>> device init function which is invoked afterward) to fill its cache and >>>> receive a notification if an update is needed. I do not see why, and >>>> specifically how you could query the route earlier or register a callback. >>> >>> Before pci_bus_irqs is called. >>> Why is another question. >>> >>>>> >>>>> One way to fix all this is call the notifier for devices, if set, from >>>>> pci_set_bus_intx_routing. >>>>> Then assume that intx to irq translations can be cached >>>>> even though they aren't now. So you will need to invoke >>>>> pci_set_bus_intx_routing on intx to irq mapping changes, >>>>> and that fires the notifier for free. >>>> >>>> pci_set_bus_intx_routing is really only for the initial setup of the >>>> static INTx pin routes. And this happens on >>>> pci_bus_irqs/pci_register_bus, ie. triggered by the host bridge. By that >>>> time, there can't be any notifier listeners - as there are no devices yet. >>>> >>>> Jan >>>> >>> >>> What I am saying is we'll cache the final IRQ at some point. >>> Pretend it's already that way so callers are ready for this. >> >> This wouldn't change the picture very much: Before the host bridge is >> fully initialized, there is no valid route available. But before that, >> there is also no device attached to it. So the invocation pattern >> wouldn't change. >> >> What would change is the semantic of the interface to the host bridge. >> So what about this: provide a public pci_root_bus_intx_routing_updated >> which so far just calls the internal-use-only >> pci_bus_fire_intx_routing_notifier? >> >> Jan >> > > I think a better name is pci_bus_update_intx_irq_cache > or something like that. At least "update cache" is not a better description as it implies in the function name the required steps of the implementation. In fact, this function /may/ update a cache and will fire notifiers. But that's nothing the care should worry about. > > And I really think it's better to recalculate the > intx routing there as well, so that if some bus > implements a dynamic route_intx it just needs to > call this after update. I thought dynamic routing is disallowed according to the spec? If not, I agree. Jan
On Sun, Jun 10, 2012 at 02:47:40PM +0200, Jan Kiszka wrote: > On 2012-06-10 14:42, Michael S. Tsirkin wrote: > > On Sun, Jun 10, 2012 at 02:33:03PM +0200, Jan Kiszka wrote: > >> On 2012-06-10 14:16, Michael S. Tsirkin wrote: > >>> On Sun, Jun 10, 2012 at 02:09:20PM +0200, Jan Kiszka wrote: > >>>> On 2012-06-10 13:39, Michael S. Tsirkin wrote: > >>>>> It's OK to use recursion but when done through a callback > >>>>> like this it's unreadable. > >>>> > >>>> Isn't the alternative poking into foreign bridge device states for their > >>>> secondary buses? > >>> > >>> pci_set_bus_intx_routing does this already. > >> > >> True. OK, I can do the recursion in pci_bus_fire_intx_routing_notifier > >> directly instead of pushing this into the bridge. > >> > >>> > >>>>> Also, you need to setup you cache after intx cache has been > >>>>> initialized, and you provide no clean way to do that. > >>>> > >>>> Once a PCI device is registered, the INTx route can be queried. So the > >>>> device user will call pci_device_route_intx_to_irq once (e.g. in the > >>>> device init function which is invoked afterward) to fill its cache and > >>>> receive a notification if an update is needed. I do not see why, and > >>>> specifically how you could query the route earlier or register a callback. > >>> > >>> Before pci_bus_irqs is called. > >>> Why is another question. > >>> > >>>>> > >>>>> One way to fix all this is call the notifier for devices, if set, from > >>>>> pci_set_bus_intx_routing. > >>>>> Then assume that intx to irq translations can be cached > >>>>> even though they aren't now. So you will need to invoke > >>>>> pci_set_bus_intx_routing on intx to irq mapping changes, > >>>>> and that fires the notifier for free. > >>>> > >>>> pci_set_bus_intx_routing is really only for the initial setup of the > >>>> static INTx pin routes. And this happens on > >>>> pci_bus_irqs/pci_register_bus, ie. triggered by the host bridge. By that > >>>> time, there can't be any notifier listeners - as there are no devices yet. > >>>> > >>>> Jan > >>>> > >>> > >>> What I am saying is we'll cache the final IRQ at some point. > >>> Pretend it's already that way so callers are ready for this. > >> > >> This wouldn't change the picture very much: Before the host bridge is > >> fully initialized, there is no valid route available. But before that, > >> there is also no device attached to it. So the invocation pattern > >> wouldn't change. > >> > >> What would change is the semantic of the interface to the host bridge. > >> So what about this: provide a public pci_root_bus_intx_routing_updated > >> which so far just calls the internal-use-only > >> pci_bus_fire_intx_routing_notifier? > >> > >> Jan > >> > > > > I think a better name is pci_bus_update_intx_irq_cache > > or something like that. > > At least "update cache" is not a better description as it implies in the > function name the required steps of the implementation. That's only reasonable: saying what a function does limits the implementation at some level. If you don't you end up with no way to understand it or check for correctness unless you find and read all callers as well. > In fact, this > function /may/ update a cache and will fire notifiers. notifiers *if any*. And the only reason to have a notifier is if you cache the routing. > But that's nothing the care should worry about. IMO function name should say what it does, not how it does it (fire notifiers) or when it should be called (routing updated). > > > > And I really think it's better to recalculate the > > intx routing there as well, so that if some bus > > implements a dynamic route_intx it just needs to > > call this after update. > > I thought dynamic routing is disallowed according to the spec? If not, I > agree. > > Jan > I think it is in pci to pci bridges, but might not be in pci host bridges. It's not a hard requirement though, but nice to have.
diff --git a/hw/pci.c b/hw/pci.c index 8878a11..5b99f4b 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1101,6 +1101,25 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin) return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]); } +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 *dev, + INTxRoutingNotifier notifier) +{ + dev->intx_routing_notifier = notifier; +} + /***********************************************************/ /* monitor info on PCI */ diff --git a/hw/pci.h b/hw/pci.h index bbba01e..e7237cf 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -182,6 +182,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); @@ -261,6 +262,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; @@ -318,6 +322,9 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, MemoryRegion *address_space_io, uint8_t devfn_min, int nirq); PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin); +void pci_bus_fire_intx_routing_notifier(PCIBus *bus); +void pci_device_set_intx_routing_notifier(PCIDevice *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 7d13a85..9ace0b7 100644 --- a/hw/pci_bridge.c +++ b/hw/pci_bridge.c @@ -298,6 +298,13 @@ void pci_bridge_reset(DeviceState *qdev) pci_bridge_reset_reg(dev); } +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) { @@ -333,6 +340,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 347177f..8fd21f3 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -422,6 +422,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);