From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SdgA2-0006Xz-Gb for qemu-devel@nongnu.org; Sun, 10 Jun 2012 07:18:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SdgA0-0002as-BC for qemu-devel@nongnu.org; Sun, 10 Jun 2012 07:18:22 -0400 Received: from mout.web.de ([212.227.17.12]:61340) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SdgA0-0002ai-1L for qemu-devel@nongnu.org; Sun, 10 Jun 2012 07:18:20 -0400 Message-ID: <4FD48277.60704@web.de> Date: Sun, 10 Jun 2012 13:18:15 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <73f236cab517fc5b2c2ba332e9efe2acbe727151.1338799936.git.jan.kiszka@siemens.com> <20120610094840.GC6250@redhat.com> <4FD47156.3020200@web.de> <20120610103352.GE6250@redhat.com> <4FD47A75.4020706@web.de> <20120610111149.GJ6250@redhat.com> In-Reply-To: <20120610111149.GJ6250@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig609A0B8F60AF0D44AAA7990E" Subject: Re: [Qemu-devel] [PATCH 06/13] pci: Add INTx routing notifier List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , qemu-devel This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig609A0B8F60AF0D44AAA7990E Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 >>> >>>>> >>>>>> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, >>>>>> + INTxRoutingNotifier not= ifier) >>>>>> +{ >>>>>> + dev->intx_routing_notifier =3D 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 th= e >>>> output of the host bridge. >>>>> >>>>>> /***********************************************************/ >>>>>> /* monitor info on PCI */ >>>>>> =20 >>>>>> 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; >>>>>> =20 >>>>>> +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; >>>>>> =20 >>>>>> + /* 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. >=20 > 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. >=20 >>> >>>>> >>>>>> +void pci_device_set_intx_routing_notifier(PCIDevice *dev, >>>>>> + INTxRoutingNotifier not= ifier); >>>>>> void pci_device_reset(PCIDevice *dev); >>>>>> void pci_bus_reset(PCIBus *bus); >>>>>> =20 >>>>>> 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); >>>>>> } >>>>>> =20 >>>>>> +static void pci_bridge_intx_routing_update(PCIDevice *dev) >>>>>> +{ >>>>>> + PCIBridge *br =3D 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 >> >=20 > I don't think it's ever more complex than a tree. >=20 For sure, and this is what the recursive invocation addresses. Jan --------------enig609A0B8F60AF0D44AAA7990E Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk/UgncACgkQitSsb3rl5xQPjQCdEysqZXNPF8X/NRnKd5PsugpX 5vAAn3atWIqIw3IkUkWHmnm4ySRG3ZMO =sB7K -----END PGP SIGNATURE----- --------------enig609A0B8F60AF0D44AAA7990E--