Re: [PATCH 1/1] of/pci: Add IORESOURCE_MEM_64 to resource flags for 64-bit memory addresses

[Rob Herring]

On Mon, Apr 19, 2021 at 7:35 PM Leonardo Bras <leobras.c@xxxxxxxxx> wrote:
>
> On Mon, 2021-04-19 at 10:44 -0500, Rob Herring wrote:
> > On Fri, Apr 16, 2021 at 3:58 PM Leonardo Bras <leobras.c@xxxxxxxxx> wrote:
> > >
> > > Hello Rob, thanks for this feedback!
> > >
> > > On Thu, 2021-04-15 at 13:59 -0500, Rob Herring wrote:
> > > > +PPC and PCI lists
> > > >
> > > > On Thu, Apr 15, 2021 at 1:01 PM Leonardo Bras <leobras.c@xxxxxxxxx> wrote:
> > > > >
> > > > > Many other resource flag parsers already add this flag when the input
> > > > > has bits 24 & 25 set, so update this one to do the same.
> > > >
> > > > Many others? Looks like sparc and powerpc to me.
> > > >
> > >
> > > s390 also does that, but it look like it comes from a device-tree.
> >
> > I'm only looking at DT based platforms, and s390 doesn't use DT.
>
> Correct.
> Sorry, I somehow write above the opposite of what I was thinking.
>
> >
> > > > Those would be the
> > > > ones I worry about breaking. Sparc doesn't use of/address.c so it's
> > > > fine. Powerpc version of the flags code was only fixed in 2019, so I
> > > > don't think powerpc will care either.
> > >
> > > In powerpc I reach this function with this stack, while configuring a
> > > virtio-net device for a qemu/KVM pseries guest:
> > >
> > > pci_process_bridge_OF_ranges+0xac/0x2d4
> > > pSeries_discover_phbs+0xc4/0x158
> > > discover_phbs+0x40/0x60
> > > do_one_initcall+0x60/0x2d0
> > > kernel_init_freeable+0x308/0x3a8
> > > kernel_init+0x2c/0x168
> > > ret_from_kernel_thread+0x5c/0x70
> > >
> > > For this, both MMIO32 and MMIO64 resources will have flags 0x200.
> >
> > Oh good, powerpc has 2 possible flags parsing functions. So in the
> > above path, do we need to set PCI_BASE_ADDRESS_MEM_TYPE_64?
> >
> > Does pci_parse_of_flags() get called in your case?
> >
>
> It's called in some cases, but not for the device I am debugging
> (virtio-net pci@800000020000000).
>
> For the above device, here is an expanded stack trace:
>
> of_bus_pci_get_flags() (from parser->bus->get_flags())
> of_pci_range_parser_one() (from macro for_each_of_pci_range)
> pci_process_bridge_OF_ranges+0xac/0x2d4
> pSeries_discover_phbs+0xc4/0x158
> discover_phbs+0x40/0x60
> do_one_initcall+0x60/0x2d0
> kernel_init_freeable+0x308/0x3a8
> kernel_init+0x2c/0x168
> ret_from_kernel_thread+0x5c/0x70
>
> For other devices, I could also see the following stack trace:
> ## device ethernet@8
>
> pci_parse_of_flags()
> of_create_pci_dev+0x7f0/0xa40
> __of_scan_bus+0x248/0x320
> pcibios_scan_phb+0x370/0x3b0
> pcibios_init+0x8c/0x12c
> do_one_initcall+0x60/0x2d0
> kernel_init_freeable+0x308/0x3a8
> kernel_init+0x2c/0x168
> ret_from_kernel_thread+0x5c/0x70
>
> Devices that get parsed with of_bus_pci_get_flags() appears first at
> dmesg (around 0.015s in my test), while devices that get parsed by
> pci_parse_of_flags() appears later (0.025s in my test).
>
> I am not really used to this code, but having the term "discover phbs"
> in the first trace and the term "scan phb" in the second, makes me
> wonder if the first trace is seen on devices that are seen/described in
> the device-tree and the second trace is seen in devices not present in
> the device-tree and found scanning pci bus.

That was my guess as well. I think on pSeries that most PCI devices
are in the DT whereas on Arm and other flattened DT (non OpenFirmware)
platforms PCI devices are not in DT. Of course, for virtio devices,
they would not be in DT in either case.

> > > > I noticed both sparc and powerpc set PCI_BASE_ADDRESS_MEM_TYPE_64 in
> > > > the flags. AFAICT, that's not set anywhere outside of arch code. So
> > > > never for riscv, arm and arm64 at least. That leads me to
> > > > pci_std_update_resource() which is where the PCI code sets BARs and
> > > > just copies the flags in PCI_BASE_ADDRESS_MEM_MASK ignoring
> > > > IORESOURCE_* flags. So it seems like 64-bit is still not handled and
> > > > neither is prefetch.
> > > >
> > >
> > > I am not sure if you mean here:
> > > a) it's ok to add IORESOURCE_MEM_64 here, because it does not affect
> > > anything else, or
> > > b) it should be using PCI_BASE_ADDRESS_MEM_TYPE_64
> > > (or IORESOURCE_MEM_64 | PCI_BASE_ADDRESS_MEM_TYPE_64) instead, since
> > > it's how it's added in powerpc/sparc, and else there is no point.
> >
> > I'm wondering if a) is incomplete and PCI_BASE_ADDRESS_MEM_TYPE_64
> > also needs to be set. The question is ultimately are BARs getting set
> > correctly for 64-bit? It looks to me like they aren't.
>
> I am not used to these terms, does BAR means 'Base Address Register'?

Yes. Standard PCI thing.

> If so, those are the addresses stored in pci->phb->mem_resources[i] and
> pci->phb->mem_offset[i], printed from enable_ddw() (which takes place a
> lot after discovering the device (0.17s in my run)).
>
> resource #1 pci@800000020000000: start=0x200080000000
> end=0x2000ffffffff flags=0x200 desc=0x0 offset=0x200000000000
> resource #2 pci@800000020000000: start=0x210000000000
> end=0x21ffffffffff flags=0x200 desc=0x0 offset=0x0
>
> The message above was printed without this patch.
> With the patch, the flags for memory resource #2 gets ORed with
> 0x00100000.

Right, as expected.

> Is it enough to know if BARs are correctly set for 64-bit?

No, because AFAICT, bit 2 in the BAR would not be set.

> If it's not, how can I check?

Can you try 'lspci -vv' and look at the 'Region X:' lines which will
say 32 or 64-bit. I *think* that should reflect what actually got
written into the BARs.

Rob