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

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

[Niklas Schnelle]

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(-)

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

[PATCH RESEND 2/2] s390/pci: allow zPCI zbus without a function zero

[Niklas Schnelle]

Currently the zPCI code block PCI bus creation and probing of a zPCI
zbus unless there is a PCI function with devfn 0. This is always the
case for the PCI functions with hidden RID but may keep PCI functions
from a multi-function PCI device with RID information invisible until
the function 0 becomes visible. Worse as a PCI bus is necessary to even
present a PCI hotplug slot even that remains invisible.

With the probing of these so called isolated PCI functions enabled for
s390 in common code this restriction is no longer necessary. On network
cards with multiple ports and a PF per port this also allows using each
port on its own while still providing the physical PCI topology
information in the devfn needed to associate VFs with their parent PF.

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/pci/pci_bus.c | 82 ++++++++++-------------------------------
 1 file changed, 20 insertions(+), 62 deletions(-)

diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
index 5d77acbd1c87..6a8da1b742ae 100644
--- a/arch/s390/pci/pci_bus.c
+++ b/arch/s390/pci/pci_bus.c
@@ -145,9 +145,6 @@ int zpci_bus_scan_bus(struct zpci_bus *zbus)
 	struct zpci_dev *zdev;
 	int devfn, rc, ret = 0;
 
-	if (!zbus->function[0])
-		return 0;
-
 	for (devfn = 0; devfn < ZPCI_FUNCTIONS_PER_BUS; devfn++) {
 		zdev = zbus->function[devfn];
 		if (zdev && zdev->state == ZPCI_FN_STATE_CONFIGURED) {
@@ -184,26 +181,26 @@ void zpci_bus_scan_busses(void)
 
 /* zpci_bus_create_pci_bus - Create the PCI bus associated with this zbus
  * @zbus: the zbus holding the zdevices
- * @f0: function 0 of the bus
+ * @fr: PCI root function that will determine the bus's domain, and bus speeed
  * @ops: the pci operations
  *
- * Function zero is taken as a parameter as this is used to determine the
- * domain, multifunction property and maximum bus speed of the entire bus.
+ * The PCI function @fr determines the domain (its UID), multifunction property
+ * and maximum bus speed of the entire bus.
  *
  * Return: 0 on success, an error code otherwise
  */
-static int zpci_bus_create_pci_bus(struct zpci_bus *zbus, struct zpci_dev *f0, struct pci_ops *ops)
+static int zpci_bus_create_pci_bus(struct zpci_bus *zbus, struct zpci_dev *fr, struct pci_ops *ops)
 {
 	struct pci_bus *bus;
 	int domain;
 
-	domain = zpci_alloc_domain((u16)f0->uid);
+	domain = zpci_alloc_domain((u16)fr->uid);
 	if (domain < 0)
 		return domain;
 
 	zbus->domain_nr = domain;
-	zbus->multifunction = f0->rid_available;
-	zbus->max_bus_speed = f0->max_bus_speed;
+	zbus->multifunction = fr->rid_available;
+	zbus->max_bus_speed = fr->max_bus_speed;
 
 	/*
 	 * Note that the zbus->resources are taken over and zbus->resources
@@ -303,47 +300,6 @@ void pcibios_bus_add_device(struct pci_dev *pdev)
 	}
 }
 
-/* zpci_bus_create_hotplug_slots - Add hotplug slot(s) for device added to bus
- * @zdev: the zPCI device that was newly added
- *
- * Add the hotplug slot(s) for the newly added PCI function. Normally this is
- * simply the slot for the function itself. If however we are adding the
- * function 0 on a zbus, it might be that we already registered functions on
- * that zbus but could not create their hotplug slots yet so add those now too.
- *
- * Return: 0 on success, an error code otherwise
- */
-static int zpci_bus_create_hotplug_slots(struct zpci_dev *zdev)
-{
-	struct zpci_bus *zbus = zdev->zbus;
-	int devfn, rc = 0;
-
-	rc = zpci_init_slot(zdev);
-	if (rc)
-		return rc;
-	zdev->has_hp_slot = 1;
-
-	if (zdev->devfn == 0 && zbus->multifunction) {
-		/* Now that function 0 is there we can finally create the
-		 * hotplug slots for those functions with devfn != 0 that have
-		 * been parked in zbus->function[] waiting for us to be able to
-		 * create the PCI bus.
-		 */
-		for  (devfn = 1; devfn < ZPCI_FUNCTIONS_PER_BUS; devfn++) {
-			zdev = zbus->function[devfn];
-			if (zdev && !zdev->has_hp_slot) {
-				rc = zpci_init_slot(zdev);
-				if (rc)
-					return rc;
-				zdev->has_hp_slot = 1;
-			}
-		}
-
-	}
-
-	return rc;
-}
-
 static int zpci_bus_add_device(struct zpci_bus *zbus, struct zpci_dev *zdev)
 {
 	int rc = -EINVAL;
@@ -352,21 +308,19 @@ static int zpci_bus_add_device(struct zpci_bus *zbus, struct zpci_dev *zdev)
 		pr_err("devfn %04x is already assigned\n", zdev->devfn);
 		return rc;
 	}
+
 	zdev->zbus = zbus;
 	zbus->function[zdev->devfn] = zdev;
 	zpci_nb_devices++;
 
-	if (zbus->bus) {
-		if (zbus->multifunction && !zdev->rid_available) {
-			WARN_ONCE(1, "rid_available not set for multifunction\n");
-			goto error;
-		}
-
-		zpci_bus_create_hotplug_slots(zdev);
-	} else {
-		/* Hotplug slot will be created once function 0 appears */
-		zbus->multifunction = 1;
+	if (zbus->multifunction && !zdev->rid_available) {
+		WARN_ONCE(1, "rid_available not set for multifunction\n");
+		goto error;
 	}
+	rc = zpci_init_slot(zdev);
+	if (rc)
+		goto error;
+	zdev->has_hp_slot = 1;
 
 	return 0;
 
@@ -400,7 +354,11 @@ int zpci_bus_device_register(struct zpci_dev *zdev, struct pci_ops *ops)
 			return -ENOMEM;
 	}
 
-	if (zdev->devfn == 0) {
+	if (!zbus->bus) {
+		/* The UID of the first PCI function registered with a zpci_bus
+		 * is used as the domain number for that bus. Currently there
+		 * is exactly one zpci_bus per domain.
+		 */
 		rc = zpci_bus_create_pci_bus(zbus, zdev, ops);
 		if (rc)
 			goto error;
-- 
2.32.0

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

[Bjorn Helgaas]

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;

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

[Niklas Schnelle]

[-- 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 --]

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

[Bjorn Helgaas]

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

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

[Niklas Schnelle]

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

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

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