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

[Michael S. Tsirkin]

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.

-- 
MST