Re: [PATCH RESEND 1/2] PCI: Extend isolated function probing to s390

[Bjorn Helgaas <helgaas@kernel.org>]

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@linux.ibm.com>
> > 
> > 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