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 <m...@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.smir...@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.mayd...@linaro.org>
> >> > Cc: Jason Wang <jasow...@redhat.com>
> >> > Cc: Philippe Mathieu-Daudé <f4...@amsat.org>
> >> > Cc: Marcel Apfelbaum <marcel.apfelb...@zoho.com>
> >> > Cc: Michael S. Tsirkin <m...@redhat.com>
> >> > Cc: qemu-devel@nongnu.org
> >> > Cc: qemu-...@nongnu.org
> >> > Cc: yurov...@gmail.com
> >> > Signed-off-by: Andrey Smirnov <andrew.smir...@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.
--
MST
