From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45848) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxvxI-0002be-Bj for qemu-devel@nongnu.org; Thu, 28 May 2015 07:26:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxvxF-0004D0-4K for qemu-devel@nongnu.org; Thu, 28 May 2015 07:26:32 -0400 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:55000) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxvxE-0004Cu-Oo for qemu-devel@nongnu.org; Thu, 28 May 2015 07:26:29 -0400 From: Don Slutz Message-ID: <5566FB3E.3080002@one.verizon.com> Date: Thu, 28 May 2015 07:25:50 -0400 MIME-Version: 1.0 References: <1432802797-21950-1-git-send-email-dslutz@verizon.com> <20150528093048.GA8125@redhat.com> In-Reply-To: <20150528093048.GA8125@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="LtWe03OSsf0APDakrBde5obLBfaKxW496" Subject: Re: [Qemu-devel] [PATCH 1/1] Fix device listener interface for PCI to PCI bridges List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Paul Durrant , "qemu-devel@nongnu.org" , Stefano Stabellini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --LtWe03OSsf0APDakrBde5obLBfaKxW496 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 05/28/15 05:30, Michael S. Tsirkin wrote: > On Thu, May 28, 2015 at 04:46:37AM -0400, Don Slutz wrote: >> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a >> PCI device has a static address. This is not true for PCI devices >> that are on the secondary bus of a PCI to PCI bridge. >> >> BIOS and/or guest OS can change the secondary bus number to a new >> value at any time. >> >> When a PCI to PCI bridge bridge is reset, the secondary bus number >> is set to zero. Normally the BIOS will set it to 255 during PCI bus >> scanning so that only the PCI devices on the root can be accessed >> via bus 0. Later it is set to a number between 1 and 254. >> >> Adjust xen_map_pcidev() to not register with Xen for secondary bus >> numbers 0 and 255. >> >> Extend the device listener interface to be called when ever the >> secondary bus number is set to a usable value. This includes >> a call on unrealize if the secondary bus number was valid. >> >> Signed-off-by: Don Slutz >> --- >> >> Note: Right now hvmloader in Xen does not handle PCI to PCI bridges >> and so SeaBIOS does not have access to PCI device(s) on secondary >> buses. How ever domU can setup the secondary bus(es) and this patch >> will restore access to these PCI devices. >> >> hw/core/qdev.c | 10 ++++++++++ >> hw/pci/pci_bridge.c | 30 ++++++++++++++++++++++++++++++ >> include/hw/qdev-core.h | 2 ++ >> include/hw/xen/xen_common.h | 31 +++++++++++++++++++++++++------ >> trace-events | 1 + >> 5 files changed, 68 insertions(+), 6 deletions(-) >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index b0f0f84..6a514ee 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -239,6 +239,16 @@ void device_listener_unregister(DeviceListener *l= istener) >> QTAILQ_REMOVE(&device_listeners, listener, link); >> } >> =20 >> +void device_listener_realize(DeviceState *dev) >> +{ >> + DEVICE_LISTENER_CALL(realize, Forward, dev); >> +} >> + >> +void device_listener_unrealize(DeviceState *dev) >> +{ >> + DEVICE_LISTENER_CALL(unrealize, Forward, dev); >> +} >> + >> static void device_realize(DeviceState *dev, Error **errp) >> { >> DeviceClass *dc =3D DEVICE_GET_CLASS(dev); >=20 >=20 > This looks wrong. Devices not accessible to config cycles are still > accessible e.g. to memory or IO. It's not the same as unrealize. >=20 > You need some other API that makes sense, > probably pci specific. >=20 If I am understanding you correctly, you would like: void device_listener_change_pci_bus_num(PCIDevice *d, uint8_t oldbus) { DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus); } >=20 >=20 >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c >> index 40c97b1..042680d 100644 >> --- a/hw/pci/pci_bridge.c >> +++ b/hw/pci/pci_bridge.c >> @@ -241,6 +241,18 @@ void pci_bridge_update_mappings(PCIBridge *br) >> pci_bridge_region_cleanup(br, w); >> } >> =20 >> +static void pci_bridge_realize_sub(PCIBus *b, PCIDevice *d, >> + void *opaque) >> +{ >> + device_listener_realize(DEVICE(d)); >> +} >> + >> +static void pci_bridge_unrealize_sub(PCIBus *b, PCIDevice *d, >> + void *opaque) >> +{ >> + device_listener_unrealize(DEVICE(d)); >> +} >> + >> /* default write_config function for PCI-to-PCI bridge */ >> void pci_bridge_write_config(PCIDevice *d, >> uint32_t address, uint32_t val, int len)= >> @@ -248,6 +260,8 @@ void pci_bridge_write_config(PCIDevice *d, >> PCIBridge *s =3D PCI_BRIDGE(d); >> uint16_t oldctl =3D pci_get_word(d->config + PCI_BRIDGE_CONTROL);= >> uint16_t newctl; >> + uint8_t oldbus =3D pci_get_byte(d->config + PCI_SECONDARY_BUS); >> + uint8_t newbus; >> =20 >> pci_default_write_config(d, address, val, len); >> =20 >> @@ -265,6 +279,22 @@ void pci_bridge_write_config(PCIDevice *d, >> pci_bridge_update_mappings(s); >> } >> =20 >> + newbus =3D pci_get_byte(d->config + PCI_SECONDARY_BUS); >> + if (newbus !=3D oldbus) { >> + PCIBus *sec_bus =3D pci_bridge_get_sec_bus(s); >> + >> + if (oldbus && oldbus !=3D 255) { >> + pci_set_byte(d->config + PCI_SECONDARY_BUS, oldbus); >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >> + pci_bridge_unrealize_sub, NULL); >> + pci_set_byte(d->config + PCI_SECONDARY_BUS, newbus); >> + } >> + if (newbus && newbus !=3D 255) { >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >> + pci_bridge_realize_sub, NULL); >> + } >> + } >> + >=20 >=20 > This is relying on undocumented assumptions and how specific firmware > works. There's nothing special about bus number 255, and 0 is not very > special either (except it happens to be the reset value). >=20 Ok, using the above it would change to (almost): if (newbus !=3D oldbus) { pci_for_each_device(pci_bridge_get_sec_bus(s), pci_bus_num(sec_bus), device_listener_change_pci_bus_num, oldbus); } Would it be better to have: void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void *opaque) { uint8_t oldbus =3D (uint8_t)opaque; DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus); } So that the above works, or to add a function to convert args? > To know whether device is accessible to config cycles, you > really need to scan the hierarchy from root downwards. >=20 Yes, that is correct. However what I am doing here is not changing how QEMU checks if the device is accessible, but changing what pci config Xen sends to QEMU. If the change to PCI_SECONDARY_BUS hides this PCI to PCI bridge, that is not an issue. -Don Slutz --LtWe03OSsf0APDakrBde5obLBfaKxW496 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.14 (GNU/Linux) iQIcBAEBAgAGBQJVZvtfAAoJEHkofi8zcwBkV8sP/17C/Wa6GDZ/BoeE4iBYBvCa CUg4ebCvbsXyn9XmehGv6/bbUfNcUqB8g6PgvdfPNQ30fXT6V/PY9ge3ncxo26wU gKi8qEDY6RSjUgNrKIIuj6Pnfv5cKAjkrgHahdNW6ZWzgb7pqhZ4vbWh99/aqItH KJ5WRFZTsYr/48A8sKVQDhNuZE+w4tPhxDTPTbFq8KsEt9BFo4CcfJTrdTd0dP0F yqTo5CR32OaRaoXQLp/yANAb4AflVsU+2iHKFbrGsXj+4dBxCvG0RkHE6gTFVmNw FjJD3uojE0kpwh/mVPWH93C/qOJo7kjdycfnZEImJ+BVqZ+t62PFujSUmRdLuYXh fe+nEgXtk8G2SzPIUNAw0E7en8/SEEgm9DEx0KqJzdziMnGTeUNmbwoqsE1Qa0Rk /eH9UmF85kWeTwg9qyvUFw4IOIr0LzjFTf4KsH7RWBJYP5kF3VbPBorL2odBi902 4j2WIGmvGD4VRqdJBTLsB4YcKmI9jSaaQHP1keeGkKVY4aCkb6P+5PLpRoobPy9x 3mcAdM0xzx6IgKTg24fna5OMbJPWQMa5vi+cx8ICDhbqfOStaR8zK9Qh85gKyAS/ o4eMXAhAFJszFbKRz05pb2Tg6JOalKeXV8TvlfTgyxChfK9+eWHWxeDBPz15kLwR sLXn4411PLG6P47Y2CeL =aIPm -----END PGP SIGNATURE----- --LtWe03OSsf0APDakrBde5obLBfaKxW496--