From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ejq5s-0005o4-2s for qemu-devel@nongnu.org; Thu, 08 Feb 2018 12:34:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ejq5q-0003g2-Tl for qemu-devel@nongnu.org; Thu, 08 Feb 2018 12:34:44 -0500 Date: Thu, 8 Feb 2018 19:34:20 +0200 From: "Michael S. Tsirkin" Message-ID: <20180208193033-mutt-send-email-mst@kernel.org> References: <20180207042438.15422-1-andrew.smirnov@gmail.com> <20180207042438.15422-10-andrew.smirnov@gmail.com> 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: Peter Maydell Cc: Andrey Smirnov , qemu-arm , Jason Wang , Philippe =?iso-8859-1?Q?Mathieu-Daud=E9?= , Marcel Apfelbaum , QEMU Developers , Andrey Yurovsky 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 *p= ci_dev, uint32_t addr, > > void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, 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; > > > > 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 " le= n=3D%d\n", > > __func__, pci_dev->name, config_addr, val, len); > > - pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SP= ACE_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), len)= ; > > PCI_DPRINTF("%s: %s: addr=3D%02"PRIx32" val=3D%08"PRIx32" len=3D= %d\n", > > __func__, pci_dev->name, config_addr, val, len); > > >=20 > I've just discovered that this breaks the e1000e-test: >=20 > $ (cd build/clang; QTEST_QEMU_BINARY=3Dx86_64-softmmu/qemu-system-x86_6= 4 > ./tests/e1000e-test) > /x86_64/e1000e/init: ** > ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/tests/e1000e-test.c:1= 18:e1000e_device_find: > 'e1000e_dev' should not be NULL >=20 > Any idea what's going on here? >=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 MST