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.william...@redhat.com> > >>>> Signed-off-by: Jan Kiszka <jan.kis...@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. -- MST