From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ejqL9-00032R-Ey for qemu-devel@nongnu.org; Thu, 08 Feb 2018 12:50:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ejqL8-0007Zu-1e for qemu-devel@nongnu.org; Thu, 08 Feb 2018 12:50:31 -0500 Date: Thu, 8 Feb 2018 19:50:21 +0200 From: "Michael S. Tsirkin" Message-ID: <20180208194731-mutt-send-email-mst@kernel.org> References: <20180207042438.15422-1-andrew.smirnov@gmail.com> <20180207042438.15422-10-andrew.smirnov@gmail.com> <20180208193033-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrey Smirnov Cc: Peter Maydell , qemu-arm , Jason Wang , Philippe =?iso-8859-1?Q?Mathieu-Daud=E9?= , Marcel Apfelbaum , QEMU Developers , Andrey Yurovsky On Thu, Feb 08, 2018 at 09:43:53AM -0800, Andrey Smirnov wrote: > On Thu, Feb 8, 2018 at 9:34 AM, Michael S. Tsirkin wro= te: > > On Thu, Feb 08, 2018 at 05:20:53PM +0000, Peter Maydell wrote: > >> On 7 February 2018 at 04:24, Andrey Smirnov wrote: > >> > Use pci_config_size (as opposed to PCI_CONFIG_SPACE_SIZE) in > >> > pci_data_read() and pci_data_write(), so this function would work = for > >> > both classic PCI and PCIe use-cases. > >> > > >> > Cc: Peter Maydell > >> > Cc: Jason Wang > >> > Cc: Philippe Mathieu-Daud=E9 > >> > Cc: Marcel Apfelbaum > >> > Cc: Michael S. Tsirkin > >> > Cc: qemu-devel@nongnu.org > >> > Cc: qemu-arm@nongnu.org > >> > Cc: yurovsky@gmail.com > >> > Signed-off-by: Andrey Smirnov > >> > --- > >> > hw/pci/pci_host.c | 13 +++++++++---- > >> > 1 file changed, 9 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > >> > index 5eaa935cb5..ea52ea07cd 100644 > >> > --- a/hw/pci/pci_host.c > >> > +++ b/hw/pci/pci_host.c > >> > @@ -89,30 +89,35 @@ uint32_t pci_host_config_read_common(PCIDevice= *pci_dev, uint32_t addr, > >> > void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int l= en) > >> > { > >> > PCIDevice *pci_dev =3D pci_dev_find_by_addr(s, addr); > >> > - uint32_t config_addr =3D addr & (PCI_CONFIG_SPACE_SIZE - 1); > >> > + uint32_t config_addr; > >> > > >> > if (!pci_dev) { > >> > return; > >> > } > >> > > >> > + config_addr =3D addr & (pci_config_size(pci_dev) - 1); > >> > + > >> > PCI_DPRINTF("%s: %s: addr=3D%02" PRIx32 " val=3D%08" PRIx32 "= len=3D%d\n", > >> > __func__, pci_dev->name, config_addr, val, len); > >> > - pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG= _SPACE_SIZE, > >> > + pci_host_config_write_common(pci_dev, config_addr, > >> > + pci_config_size(pci_dev), > >> > val, len); > >> > } > >> > > >> > uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) > >> > { > >> > PCIDevice *pci_dev =3D pci_dev_find_by_addr(s, addr); > >> > - uint32_t config_addr =3D addr & (PCI_CONFIG_SPACE_SIZE - 1); > >> > + uint32_t config_addr; > >> > uint32_t val; > >> > > >> > if (!pci_dev) { > >> > return ~0x0; > >> > } > >> > > >> > + config_addr =3D addr & (pci_config_size(pci_dev) - 1); > >> > + > >> > val =3D pci_host_config_read_common(pci_dev, config_addr, > >> > - PCI_CONFIG_SPACE_SIZE, len)= ; > >> > + pci_config_size(pci_dev), l= en); > >> > PCI_DPRINTF("%s: %s: addr=3D%02"PRIx32" val=3D%08"PRIx32" len= =3D%d\n", > >> > __func__, pci_dev->name, config_addr, val, len); > >> > > >> > >> I've just discovered that this breaks the e1000e-test: > >> > >> $ (cd build/clang; QTEST_QEMU_BINARY=3Dx86_64-softmmu/qemu-system-x8= 6_64 > >> ./tests/e1000e-test) > >> /x86_64/e1000e/init: ** > >> ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/tests/e1000e-test.= c:118:e1000e_device_find: > >> 'e1000e_dev' should not be NULL > >> > >> Any idea what's going on here? > >> >=20 > Peter: >=20 > Don't really have a good idea what's going on, but I am guessing this > change broke affected pci_host_data_* methods in negative way. >=20 > >> thanks > >> -- PMM > > > > I'm yet to review these patches, but IIRC pci_data_read and friends > > aren't designed with pcie in mind - they implement the classic pci > > config space. > > > > I'd like to see more justification about how these are now going > > to be used. > > >=20 > Michael: >=20 > My only justification for this change was suggestion from Marcel to > use pci_data_* functions, instead of calling pci_host_config_*_common > explicitly in designware.c introduced in next patch in the series. My > v4 patches incorrectly limited config space size to 256, so such > replacement didn't require this patch, but switching pci_data_* to use > pci_config_size() seemed harmless enough. I am more than happy to got > back to using pci_host_config_*_common in designware.c and dropping > this patch. >=20 > Thanks, > Andrey Smirnov Aren't these implementing MMCFG? In that case I think you should reuse pcie_host_mmcfg_update, like q35 does. This would be the second user, so we might need to extend that interface somewhat. Let me know if that's a good fit. --=20 MST