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
