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

[Jan Kiszka]

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

signature.asc
Description: OpenPGP digital signature