Re: [Qemu-arm] [PATCH v5 09/14] pci: Use pci_config_size in pci_data_* accessors

[Michael S. Tsirkin]

On Thu, Feb 08, 2018 at 05:20:53PM +0000, Peter Maydell wrote:
> On 7 February 2018 at 04:24, Andrey Smirnov <address@hidden> 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 <address@hidden>
> > Cc: Jason Wang <address@hidden>
> > Cc: Philippe Mathieu-Daudé <address@hidden>
> > Cc: Marcel Apfelbaum <address@hidden>
> > Cc: Michael S. Tsirkin <address@hidden>
> > Cc: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > Signed-off-by: Andrey Smirnov <address@hidden>
> > ---
> >  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