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'll do some more investigation and testing and report back. I do agree that there seems to be some potential for cleanup here.
Attachment:
signature.asc
Description: This is a digitally signed message part
