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
