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?
> > 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
>
