Re: [Qemu-devel] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors
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 wrote: > > On Thu, Feb 08, 2018 at 05:20:53PM +, 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é > >> > Cc: Marcel Apfelbaum > >> > Cc: Michael S. Tsirkin > >> > Cc: qemu-devel@nongnu.org > >> > Cc: qemu-...@nongnu.org > >> > Cc: yurov...@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 len) > >> > { > >> > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > >> > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > >> > +uint32_t config_addr; > >> > > >> > if (!pci_dev) { > >> > return; > >> > } > >> > > >> > +config_addr = addr & (pci_config_size(pci_dev) - 1); > >> > + > >> > PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%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 = pci_dev_find_by_addr(s, addr); > >> > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > >> > +uint32_t config_addr; > >> > uint32_t val; > >> > > >> > if (!pci_dev) { > >> > return ~0x0; > >> > } > >> > > >> > +config_addr = addr & (pci_config_size(pci_dev) - 1); > >> > + > >> > val = 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=%02"PRIx32" val=%08"PRIx32" len=%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=x86_64-softmmu/qemu-system-x86_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? > >> > > Peter: > > 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. > > >> 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. > > > > Michael: > > 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. > > 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. -- MST
Re: [Qemu-devel] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors
On 8 February 2018 at 17:43, Andrey Smirnov wrote: > 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. OK. I'll keep patches 1-8, 11 and 13 in target-arm, and drop 9, 10, 12 and 14 for the moment. thanks -- PMM
Re: [Qemu-devel] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors
On Thu, Feb 8, 2018 at 9:34 AM, Michael S. Tsirkin wrote: > On Thu, Feb 08, 2018 at 05:20:53PM +, 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é >> > Cc: Marcel Apfelbaum >> > Cc: Michael S. Tsirkin >> > Cc: qemu-devel@nongnu.org >> > Cc: qemu-...@nongnu.org >> > Cc: yurov...@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 len) >> > { >> > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); >> > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); >> > +uint32_t config_addr; >> > >> > if (!pci_dev) { >> > return; >> > } >> > >> > +config_addr = addr & (pci_config_size(pci_dev) - 1); >> > + >> > PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%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 = pci_dev_find_by_addr(s, addr); >> > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); >> > +uint32_t config_addr; >> > uint32_t val; >> > >> > if (!pci_dev) { >> > return ~0x0; >> > } >> > >> > +config_addr = addr & (pci_config_size(pci_dev) - 1); >> > + >> > val = 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=%02"PRIx32" val=%08"PRIx32" len=%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=x86_64-softmmu/qemu-system-x86_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? >> Peter: 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. >> 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. > Michael: 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. Thanks, Andrey Smirnov
Re: [Qemu-devel] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors
On Thu, Feb 08, 2018 at 05:20:53PM +, 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é > > Cc: Marcel Apfelbaum > > Cc: Michael S. Tsirkin > > Cc: qemu-devel@nongnu.org > > Cc: qemu-...@nongnu.org > > Cc: yurov...@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 len) > > { > > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > > +uint32_t config_addr; > > > > if (!pci_dev) { > > return; > > } > > > > +config_addr = addr & (pci_config_size(pci_dev) - 1); > > + > > PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%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 = pci_dev_find_by_addr(s, addr); > > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > > +uint32_t config_addr; > > uint32_t val; > > > > if (!pci_dev) { > > return ~0x0; > > } > > > > +config_addr = addr & (pci_config_size(pci_dev) - 1); > > + > > val = 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=%02"PRIx32" val=%08"PRIx32" len=%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=x86_64-softmmu/qemu-system-x86_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? > > 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. -- MST
Re: [Qemu-devel] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors
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é > Cc: Marcel Apfelbaum > Cc: Michael S. Tsirkin > Cc: qemu-devel@nongnu.org > Cc: qemu-...@nongnu.org > Cc: yurov...@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 len) > { > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > +uint32_t config_addr; > > if (!pci_dev) { > return; > } > > +config_addr = addr & (pci_config_size(pci_dev) - 1); > + > PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%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 = pci_dev_find_by_addr(s, addr); > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > +uint32_t config_addr; > uint32_t val; > > if (!pci_dev) { > return ~0x0; > } > > +config_addr = addr & (pci_config_size(pci_dev) - 1); > + > val = 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=%02"PRIx32" val=%08"PRIx32" len=%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=x86_64-softmmu/qemu-system-x86_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? thanks -- PMM
Re: [Qemu-devel] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors
On 02/07/2018 01:24 AM, 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é > Cc: Marcel Apfelbaum > Cc: Michael S. Tsirkin > Cc: qemu-devel@nongnu.org > Cc: qemu-...@nongnu.org > Cc: yurov...@gmail.com > Signed-off-by: Andrey Smirnov Reviewed-by: Philippe Mathieu-Daudé > --- > 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 len) > { > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > +uint32_t config_addr; > > if (!pci_dev) { > return; > } > > +config_addr = addr & (pci_config_size(pci_dev) - 1); > + > PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%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 = pci_dev_find_by_addr(s, addr); > -uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > +uint32_t config_addr; > uint32_t val; > > if (!pci_dev) { > return ~0x0; > } > > +config_addr = addr & (pci_config_size(pci_dev) - 1); > + > val = 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=%02"PRIx32" val=%08"PRIx32" len=%d\n", > __func__, pci_dev->name, config_addr, val, len); > >