| Message ID | 20180207042438.15422-10-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <peter.maydell@linaro.org> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > Cc: Marcel Apfelbaum <marcel.apfelbaum@zoho.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: qemu-devel@nongnu.org > Cc: qemu-arm@nongnu.org > Cc: yurovsky@gmail.com > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > 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); > >
On 7 February 2018 at 04:24, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > Cc: Marcel Apfelbaum <marcel.apfelbaum@zoho.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: qemu-devel@nongnu.org > Cc: qemu-arm@nongnu.org > Cc: yurovsky@gmail.com > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > 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
On Thu, Feb 08, 2018 at 05:20:53PM +0000, Peter Maydell wrote: > On 7 February 2018 at 04:24, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org> > > Cc: Jason Wang <jasowang@redhat.com> > > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Cc: Marcel Apfelbaum <marcel.apfelbaum@zoho.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: qemu-devel@nongnu.org > > Cc: qemu-arm@nongnu.org > > Cc: yurovsky@gmail.com > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > --- > > 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.
On Thu, Feb 8, 2018 at 9:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Feb 08, 2018 at 05:20:53PM +0000, Peter Maydell wrote: >> On 7 February 2018 at 04:24, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org> >> > Cc: Jason Wang <jasowang@redhat.com> >> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> >> > Cc: Marcel Apfelbaum <marcel.apfelbaum@zoho.com> >> > Cc: Michael S. Tsirkin <mst@redhat.com> >> > Cc: qemu-devel@nongnu.org >> > Cc: qemu-arm@nongnu.org >> > Cc: yurovsky@gmail.com >> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> > --- >> > 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
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 <mst@redhat.com> wrote: > > On Thu, Feb 08, 2018 at 05:20:53PM +0000, Peter Maydell wrote: > >> On 7 February 2018 at 04:24, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org> > >> > Cc: Jason Wang <jasowang@redhat.com> > >> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> > Cc: Marcel Apfelbaum <marcel.apfelbaum@zoho.com> > >> > Cc: Michael S. Tsirkin <mst@redhat.com> > >> > Cc: qemu-devel@nongnu.org > >> > Cc: qemu-arm@nongnu.org > >> > Cc: yurovsky@gmail.com > >> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > >> > --- > >> > 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.
On 8 February 2018 at 17:43, Andrey Smirnov <andrew.smirnov@gmail.com> 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
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);
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 <peter.maydell@linaro.org> Cc: Jason Wang <jasowang@redhat.com> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> Cc: Marcel Apfelbaum <marcel.apfelbaum@zoho.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: qemu-devel@nongnu.org Cc: qemu-arm@nongnu.org Cc: yurovsky@gmail.com Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- hw/pci/pci_host.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)