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

[Niklas Schnelle <schnelle@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

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'll do some more investigation and testing and report back. I do agree
that there seems to be some potential for cleanup here.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]