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

[Andrey Smirnov]

On Thu, Feb 8, 2018 at 9:34 AM, Michael S. Tsirkin <address@hidden> wrote:
> 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?
>>

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