Re: [Qemu-devel] [PATCH 06/13] pci: Add INTx routing notifier

["Michael S. Tsirkin" <mst@redhat.com>]

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.

-- 
MST