| Message ID | 20220404095346.2324666-1-schnelle@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND,1/2] PCI: Extend isolated function probing to s390 | expand |
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. 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. Bjorn > --- > drivers/pci/probe.c | 4 ++-- > include/linux/hypervisor.h | 9 +++++++++ > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 17a969942d37..e8fd89a1f984 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2869,11 +2869,11 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > nr_devs = pci_scan_slot(bus, devfn); > > /* > - * The Jailhouse hypervisor may pass individual functions of a > + * Some hypervisors may pass individual functions of a > * multi-function device to a guest without passing function 0. > * Look for them as well. > */ > - if (jailhouse_paravirt() && nr_devs == 0) { > + if (hypervisor_isolated_pci_functions() && nr_devs == 0) { > for (fn = 1; fn < 8; fn++) { > dev = pci_scan_single_device(bus, devfn + fn); > if (dev) > diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h > index fc08b433c856..52abd459f9a3 100644 > --- a/include/linux/hypervisor.h > +++ b/include/linux/hypervisor.h > @@ -32,4 +32,13 @@ static inline bool jailhouse_paravirt(void) > > #endif /* !CONFIG_X86 */ > > +static inline bool hypervisor_isolated_pci_functions(void) > +{ > + if (IS_ENABLED(CONFIG_S390)) > + return true; > + else > + return jailhouse_paravirt(); > +} > + > + > #endif /* __LINUX_HYPEVISOR_H */ > -- > 2.32.0 > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 17a969942d37..83e4885e0698 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2650,9 +2650,9 @@ int pci_scan_slot(struct pci_bus *bus, int devfn) return 0; /* Already scanned the entire slot */ dev = pci_scan_single_device(bus, devfn); - if (!dev) + if (!dev && !jailhouse_paravirt()) return 0; - if (!pci_dev_is_added(dev)) + if (dev && !pci_dev_is_added(dev)) nr++; for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) { @@ -2858,30 +2858,16 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, { unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; unsigned int start = bus->busn_res.start; - unsigned int devfn, fn, cmax, max = start; + unsigned int devfn, cmax, max = start; struct pci_dev *dev; int nr_devs; dev_dbg(&bus->dev, "scanning bus\n"); /* Go find them, Rover! */ - for (devfn = 0; devfn < 256; devfn += 8) { + for (devfn = 0; devfn < 256; devfn += 8) nr_devs = pci_scan_slot(bus, devfn); - /* - * The Jailhouse hypervisor may pass individual functions of a - * multi-function device to a guest without passing function 0. - * Look for them as well. - */ - if (jailhouse_paravirt() && nr_devs == 0) { - for (fn = 1; fn < 8; fn++) { - dev = pci_scan_single_device(bus, devfn + fn); - if (dev) - dev->multifunction = 1; - } - } - } - /* Reserve buses for SR-IOV capability */ used_buses = pci_iov_bus_range(bus); max += used_buses;
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.
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
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@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 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
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 17a969942d37..e8fd89a1f984 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2869,11 +2869,11 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, nr_devs = pci_scan_slot(bus, devfn); /* - * The Jailhouse hypervisor may pass individual functions of a + * Some hypervisors may pass individual functions of a * multi-function device to a guest without passing function 0. * Look for them as well. */ - if (jailhouse_paravirt() && nr_devs == 0) { + if (hypervisor_isolated_pci_functions() && nr_devs == 0) { for (fn = 1; fn < 8; fn++) { dev = pci_scan_single_device(bus, devfn + fn); if (dev) diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h index fc08b433c856..52abd459f9a3 100644 --- a/include/linux/hypervisor.h +++ b/include/linux/hypervisor.h @@ -32,4 +32,13 @@ static inline bool jailhouse_paravirt(void) #endif /* !CONFIG_X86 */ +static inline bool hypervisor_isolated_pci_functions(void) +{ + if (IS_ENABLED(CONFIG_S390)) + return true; + else + return jailhouse_paravirt(); +} + + #endif /* __LINUX_HYPEVISOR_H */
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> --- drivers/pci/probe.c | 4 ++-- include/linux/hypervisor.h | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-)