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. > > > 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'? 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. Is it enough to know if BARs are correctly set for 64-bit? If it's not, how can I check? > > Rob Thanks Rob! Leonardo Brás
