On Mon, 2022-04-11 at 14:23 -0500, Bjorn Helgaas wrote: > On Mon, Apr 11, 2022 at 10:43:56AM +0200, Niklas Schnelle wrote: > > On Fri, 2022-04-08 at 17:45 -0500, Bjorn Helgaas wrote: > > > On Mon, Apr 04, 2022 at 11:53:45AM +0200, Niklas Schnelle wrote: > > > > Like the jailhouse hypervisor s390's PCI architecture allows passing > > > > isolated PCI functions to an OS instance. As of now this is was not > > > > utilized even with multi-function support as the s390 PCI code makes > > > > sure that only virtual PCI busses including a function with devfn 0 are > > > > presented to the PCI subsystem. A subsequent change will remove this > > > > restriction. > > > > > > > > Allow probing such functions by replacing the existing check for > > > > jailhouse_paravirt() with a new hypervisor_isolated_pci_functions() > > > > helper. > > > > > > > > Signed-off-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx> > > > > > > I'm OK with the idea of generalizing this Jailhouse test, but I wonder > > > if this check should be in pci_scan_slot() rather than in > > > pci_scan_child_bus_extend(). > > > > > > I think the idea is that pci_scan_slot() should find all the functions > > > of a device (a.k.a. "slot"), so it's a little weird to have a loop > > > calling pci_scan_single_device() for each function in both places. > > > > Yeah, I agree. > > > Currently we never call pcie_aspm_init_link_state() for these > > > Jailhouse or s390 functions. Maybe that's OK (and I think > > > pci_scan_slot() is the wrong place to initialize ASPM anyway) but if > > > we could move the Jailhouse/s390 checking to pci_scan_slot(), it would > > > at least remove the inconsistency. > > > > > > I'm thinking something along the lines of the patch below. I'm sure > > > Jan considered this originally, so maybe there's some reason this > > > won't work. > > > > One thing I already noticed is that I think next_fn() may need to be > > changed. If pci_ari_enabled(bus) is true, then it immediately returns 0 > > on dev == NULL while if it is false there is an extra check for non- > > contiguous multifunction devices. Even then I think on jailhouse() dev- > > > multifunction might not be set at that point. This is in contrast to > > s390 where we set dev->multifunction based on information provided by > > the platform before scanning the bus. So I'll have to be careful not to > > create a state where this works on s390 but might not work for > > jailhouse. > > > > I also do wonder what the role of the PCI_SCAN_ALL_PCIE_DEVS flag > > should be here. At least the comment in only_one_child() sounds a lot > > like that flag kind of indicates the same thing. > > I looked at PCI_SCAN_ALL_PCIE_DEVS, too, but I think that's for a > slightly different situation; see > https://git.kernel.org/linus/284f5f9dbac1 > > Bjorn Thanks for the link. I did some more investigating and testing. I think I understand it now but I have to say I did struggle a little with the whole pci_scan_slot()/next_fn() logic. The most interesting to me is how I think the following check in next_fn() actually has multiple uses that weren't clear to me on first glance: /* dev may be NULL for non-contiguous multifunction devices */ if (!dev || dev->multifunction) return (fn + 1) % 8; First it does cover the case mentioned in the comment where a multifunction device has some functions missing making it non- contiguous. As I understand it this case is also triggered on s390 when we "powered off" some of the VFs of a SR-IOV device where both the PFs and VFs are under the control of Linux but enumeration is done by firmware and that can hide some of the VFs. Secondly assuming we have a call of pci_scan_slot(bus, 0) and no ARI this check also makes sure that the next_fn(bus, dev, 0) call that initializes fn in the loop returns 0 unless dev->multifunction is set so we only enter the loop and look for more functions if the devfn 0 device is multifunction. I found this a bit non obvious. Also I personally don't like that next_fn() returns 0 which is a valid fn to indicate no more functions and that the handling of the first function duplicates the code in the loop for the other functions. The first point also means that your proposal of allowing dev == NULL for the first function if jailhouse_paravirt() respectively hypervisor_isolated_pci_functions() is set indeed works as far as I can tell from the s390 case. That made me wonder though. If we instead unconditionally allow the first function to be missing (dev == NULL) then we don't even need the extra chek for isolated PCI functions. I'm assuming though we can't do that as we would then waste time scanning all function of empty downstream ports? It's a bit of a larger cleanup but I think I will send a patch to propose changing things around such that the first function is handled in the loop of pci_scan_slot() too and that next_fn() returns -ENODEV if there are no more functions. That will also make things more readable (to me) and commented the cases where we know that we're done looking. Then a second patch can pull the jailhouse/s390 special case into pci_scan_slot(). Thanks, Niklas
Attachment:
signature.asc
Description: This is a digitally signed message part
