[PATCH v6 0/5] PCI: Rework pci_scan_slot() and isolated PCI functions

[PATCH v6 0/5] PCI: Rework pci_scan_slot() and isolated PCI functions

[Niklas Schnelle]

Hi Bjorn,

In an earlier version[0], I sought to apply the existing jailhouse special case
for isolated PCI functions to s390. As Bjorn noted in[1] there appears to be
some potential for cleaning things up and removing duplication.

This series attempts to do this cleanup (Patches 1 through 3) followed by enabling
isolated PCI functions for s390 (Patches 4 and 5).

Testing:
- On s390 with SR-IOV and a ConnectX NIC with PF 1 but not PF 0 passed throug
  i.e. the isolated function case. Also of course with just VFs and an NVMe.
- On x86_64 on a desktop system where ARI is disabled and with an SR-IOV NIC
  with non-contiguous VFs as well as the usual other PCI devices.

Thanks,
Niklas

[0] https://lore.kernel.org/linux-pci/20220404095346.2324666-1-schnelle@linux.ibm.com/
[1] https://lore.kernel.org/linux-pci/20220408224514.GA353445@bhelgaas/

Changes v5 -> v6:
- Added a patch (2) which separates the ARI case into its own function
- Some whitespace changes to remove unnecesssary empty lines
Changes v4 -> v5:
- Remove unintended whitespace change in patch 1
Changes v3 -> v4:
- Use a do {} while loop in pci_scan_slot() as it is simpler (Bjorn)
- Explicitly check "fn == 0" as it is not a pointer or bool (Bjorn)
- Keep the "!dev" check in the ARI branch of next_fn() (Bjorn)
- Moved the "fn == 0 && !dev" condition out of next_fn() into pci_scan_slot().
  This allows us to keep the "!dev" case in the ARI branch and means there are
  no new conditions in next_fn() making it easier to verify that its behavior
  is equivalent to the existing code.
- Guard the assignment of dev->multifunction with "fn > 0"
  instead of "nr > 0". This matches the existing logic more closely and works
  for the jailhouse case which unconditionally sets dev->multifunction for
  "fn > 0". This also means fn == 0 is the single "first iteration" test.
- Remove some unneeded whitespace in patch 2

Changes v2 -> v3:
- Removed now unused nr_devs variable (kernel test robot)

Niklas Schnelle (5):
  PCI: Clean up pci_scan_slot()
  PCI: Split out next_ari_fn() from next_fn()
  PCI: Move jailhouse's isolated function handling to pci_scan_slot()
  PCI: Extend isolated function probing to s390
  s390/pci: allow zPCI zbus without a function zero

 arch/s390/pci/pci_bus.c    | 82 +++++++++---------------------------
 drivers/pci/probe.c        | 86 ++++++++++++++++++--------------------
 include/linux/hypervisor.h |  8 ++++
 3 files changed, 68 insertions(+), 108 deletions(-)

-- 
2.32.0

[PATCH v6 1/5] PCI: Clean up pci_scan_slot()

[Niklas Schnelle]

While determining the next PCI function is factored out of
pci_scan_slot() into next_fn() the former still handles the first
function as a special case. This duplicates the code from the scan loop.

Furthermore the non ARI branch of next_fn() is generally hard to
understand and especially the check for multifunction devices is hidden
in the handling of NULL devices for non-contiguous multifunction. It
also signals that no further functions need to be scanned by returning
0 via wraparound and this is a valid function number.

Improve upon this by transforming the conditions in next_fn() to be
easier to understand.

By changing next_fn() to return -ENODEV instead of 0 when there is no
next function we can then handle the initial function inside the loop
and deduplicate the shared handling. This also makes it more explicit
that only function 0 must exist.

No functional change is intended.

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/pci/probe.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..b05d0ed83a24 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2579,8 +2579,7 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
 }
 EXPORT_SYMBOL(pci_scan_single_device);
 
-static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
-			    unsigned int fn)
+static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
 {
 	int pos;
 	u16 cap = 0;
@@ -2588,24 +2587,26 @@ static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
 
 	if (pci_ari_enabled(bus)) {
 		if (!dev)
-			return 0;
+			return -ENODEV;
 		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
 		if (!pos)
-			return 0;
+			return -ENODEV;
 
 		pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
 		next_fn = PCI_ARI_CAP_NFN(cap);
 		if (next_fn <= fn)
-			return 0;	/* protect against malformed list */
+			return -ENODEV;	/* protect against malformed list */
 
 		return next_fn;
 	}
 
-	/* dev may be NULL for non-contiguous multifunction devices */
-	if (!dev || dev->multifunction)
-		return (fn + 1) % 8;
+	if (fn >= 7)
+		return -ENODEV;
+	/* only multifunction devices may have more functions */
+	if (dev && !dev->multifunction)
+		return -ENODEV;
 
-	return 0;
+	return fn + 1;
 }
 
 static int only_one_child(struct pci_bus *bus)
@@ -2643,26 +2644,25 @@ static int only_one_child(struct pci_bus *bus)
  */
 int pci_scan_slot(struct pci_bus *bus, int devfn)
 {
-	unsigned int fn, nr = 0;
 	struct pci_dev *dev;
+	int fn = 0, nr = 0;
 
 	if (only_one_child(bus) && (devfn > 0))
 		return 0; /* Already scanned the entire slot */
 
-	dev = pci_scan_single_device(bus, devfn);
-	if (!dev)
-		return 0;
-	if (!pci_dev_is_added(dev))
-		nr++;
-
-	for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) {
+	do {
 		dev = pci_scan_single_device(bus, devfn + fn);
 		if (dev) {
 			if (!pci_dev_is_added(dev))
 				nr++;
-			dev->multifunction = 1;
+			if (fn > 0)
+				dev->multifunction = 1;
+		} else if (fn == 0) {
+			/* function 0 is required */
+			break;
 		}
-	}
+		fn = next_fn(bus, dev, fn);
+	} while (fn >= 0);
 
 	/* Only one slot has PCIe device */
 	if (bus->self && nr)
-- 
2.32.0

[PATCH v6 2/5] PCI: Split out next_ari_fn() from next_fn()

[Niklas Schnelle]

In commit b1bd58e448f2 ("PCI: Consolidate "next-function" functions")
the next_fn() function subsumed the traditional and ARI based next
function determination. This got rid of some needlessly complex function
pointer handling but also reduced the separation between these very
different methods of finding the next function. With the next_fn()
cleaned up a bit we can re-introduce this separation by moving out the
ARI handling while sticking with direct function calls.

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/pci/probe.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b05d0ed83a24..2c737dce757e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2579,26 +2579,30 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
 }
 EXPORT_SYMBOL(pci_scan_single_device);
 
-static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
+static int next_ari_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
 {
 	int pos;
 	u16 cap = 0;
 	unsigned int next_fn;
 
-	if (pci_ari_enabled(bus)) {
-		if (!dev)
-			return -ENODEV;
-		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
-		if (!pos)
-			return -ENODEV;
+	if (!dev)
+		return -ENODEV;
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
+	if (!pos)
+		return -ENODEV;
+
+	pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
+	next_fn = PCI_ARI_CAP_NFN(cap);
+	if (next_fn <= fn)
+		return -ENODEV;	/* protect against malformed list */
 
-		pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
-		next_fn = PCI_ARI_CAP_NFN(cap);
-		if (next_fn <= fn)
-			return -ENODEV;	/* protect against malformed list */
+	return next_fn;
+}
 
-		return next_fn;
-	}
+static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
+{
+	if (pci_ari_enabled(bus))
+		return next_ari_fn(bus, dev, fn);
 
 	if (fn >= 7)
 		return -ENODEV;
-- 
2.32.0

[PATCH v6 3/5] PCI: Move jailhouse's isolated function handling to pci_scan_slot()

[Niklas Schnelle]

The special case of the jailhouse hypervisor passing through individual
PCI functions handles scanning for PCI functions even if function 0 does
not exist. Currently this is done with an extra loop duplicating the one
in pci_scan_slot(). By incorporating the check for jailhouse_paravirt()
into pci_scan_slot() we can instead do this as part of the normal
slot scan. Note that with the assignment of dev->multifunction gated by
fn > 0 we set dev->multifunction unconditionally for all functions if
function 0 is missing just as in the existing jailhouse loop.

The only functional change is that we now call
pcie_aspm_init_link_state() for these functions but this already
happened if function 0 was passed through and should not be a problem.

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Link: https://lore.kernel.org/linux-pci/20220408224514.GA353445@bhelgaas/
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/pci/probe.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2c737dce757e..a18e07e6a7df 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2662,8 +2662,13 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
 			if (fn > 0)
 				dev->multifunction = 1;
 		} else if (fn == 0) {
-			/* function 0 is required */
-			break;
+			/*
+			 * function 0 is required unless we are running on
+			 * a hypervisor which passes through individual PCI
+			 * functions.
+			 */
+			if (!jailhouse_paravirt())
+				break;
 		}
 		fn = next_fn(bus, dev, fn);
 	} while (fn >= 0);
@@ -2862,29 +2867,14 @@ 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) {
-		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;
-			}
-		}
-	}
+	for (devfn = 0; devfn < 256; devfn += 8)
+		pci_scan_slot(bus, devfn);
 
 	/* Reserve buses for SR-IOV capability */
 	used_buses = pci_iov_bus_range(bus);
-- 
2.32.0

[PATCH v6 4/5] 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.

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/pci/probe.c        | 2 +-
 include/linux/hypervisor.h | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a18e07e6a7df..156dd13594b8 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2667,7 +2667,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
 			 * a hypervisor which passes through individual PCI
 			 * functions.
 			 */
-			if (!jailhouse_paravirt())
+			if (!hypervisor_isolated_pci_functions())
 				break;
 		}
 		fn = next_fn(bus, dev, fn);
diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
index fc08b433c856..33b1c0482aac 100644
--- a/include/linux/hypervisor.h
+++ b/include/linux/hypervisor.h
@@ -32,4 +32,12 @@ 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 v6 5/5] 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 v6 1/5] PCI: Clean up pci_scan_slot()

[Pierre Morel]

On 6/28/22 16:30, Niklas Schnelle wrote:
> While determining the next PCI function is factored out of
> pci_scan_slot() into next_fn() the former still handles the first
> function as a special case. This duplicates the code from the scan loop.
> 
> Furthermore the non ARI branch of next_fn() is generally hard to
> understand and especially the check for multifunction devices is hidden
> in the handling of NULL devices for non-contiguous multifunction. It
> also signals that no further functions need to be scanned by returning
> 0 via wraparound and this is a valid function number.
> 
> Improve upon this by transforming the conditions in next_fn() to be
> easier to understand.
> 
> By changing next_fn() to return -ENODEV instead of 0 when there is no
> next function we can then handle the initial function inside the loop
> and deduplicate the shared handling. This also makes it more explicit
> that only function 0 must exist.
> 
> No functional change is intended.
> 
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>   drivers/pci/probe.c | 38 +++++++++++++++++++-------------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..b05d0ed83a24 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2579,8 +2579,7 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
>   }
>   EXPORT_SYMBOL(pci_scan_single_device);
>   
> -static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
> -			    unsigned int fn)
> +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
>   {
>   	int pos;
>   	u16 cap = 0;
> @@ -2588,24 +2587,26 @@ static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
>   
>   	if (pci_ari_enabled(bus)) {
>   		if (!dev)
> -			return 0;
> +			return -ENODEV;
>   		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
>   		if (!pos)
> -			return 0;
> +			return -ENODEV;
>   
>   		pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
>   		next_fn = PCI_ARI_CAP_NFN(cap);
>   		if (next_fn <= fn)
> -			return 0;	/* protect against malformed list */
> +			return -ENODEV;	/* protect against malformed list */
>   
>   		return next_fn;
>   	}
>   
> -	/* dev may be NULL for non-contiguous multifunction devices */
> -	if (!dev || dev->multifunction)
> -		return (fn + 1) % 8;
> +	if (fn >= 7)
> +		return -ENODEV;
> +	/* only multifunction devices may have more functions */
> +	if (dev && !dev->multifunction)
> +		return -ENODEV;
>   
> -	return 0;
> +	return fn + 1;

No more % 8 ?
Even it disapear later shouldn't we keep it ?



>   }
>   
>   static int only_one_child(struct pci_bus *bus)
> @@ -2643,26 +2644,25 @@ static int only_one_child(struct pci_bus *bus)
>    */
>   int pci_scan_slot(struct pci_bus *bus, int devfn)
>   {
> -	unsigned int fn, nr = 0;
>   	struct pci_dev *dev;
> +	int fn = 0, nr = 0;
>   
>   	if (only_one_child(bus) && (devfn > 0))
>   		return 0; /* Already scanned the entire slot */
>   
> -	dev = pci_scan_single_device(bus, devfn);
> -	if (!dev)
> -		return 0;
> -	if (!pci_dev_is_added(dev))
> -		nr++;
> -
> -	for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) {
> +	do {
>   		dev = pci_scan_single_device(bus, devfn + fn);
>   		if (dev) {
>   			if (!pci_dev_is_added(dev))
>   				nr++;
> -			dev->multifunction = 1;
> +			if (fn > 0)
> +				dev->multifunction = 1;
> +		} else if (fn == 0) {
> +			/* function 0 is required */
> +			break;
>   		}
> -	}
> +		fn = next_fn(bus, dev, fn);
> +	} while (fn >= 0);
>   
>   	/* Only one slot has PCIe device */
>   	if (bus->self && nr)
> 

Otherwise LGTM


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v6 2/5] PCI: Split out next_ari_fn() from next_fn()

[Pierre Morel]

On 6/28/22 16:30, Niklas Schnelle wrote:
> In commit b1bd58e448f2 ("PCI: Consolidate "next-function" functions")
> the next_fn() function subsumed the traditional and ARI based next
> function determination. This got rid of some needlessly complex function
> pointer handling but also reduced the separation between these very
> different methods of finding the next function. With the next_fn()
> cleaned up a bit we can re-introduce this separation by moving out the
> ARI handling while sticking with direct function calls.
> 
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>   drivers/pci/probe.c | 30 +++++++++++++++++-------------
>   1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b05d0ed83a24..2c737dce757e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2579,26 +2579,30 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
>   }
>   EXPORT_SYMBOL(pci_scan_single_device);
>   
> -static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
> +static int next_ari_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
>   {
>   	int pos;
>   	u16 cap = 0;
>   	unsigned int next_fn;
>   
> -	if (pci_ari_enabled(bus)) {
> -		if (!dev)
> -			return -ENODEV;
> -		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
> -		if (!pos)
> -			return -ENODEV;
> +	if (!dev)
> +		return -ENODEV;
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
> +	if (!pos)
> +		return -ENODEV;
> +
> +	pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
> +	next_fn = PCI_ARI_CAP_NFN(cap);
> +	if (next_fn <= fn)
> +		return -ENODEV;	/* protect against malformed list */
>   
> -		pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
> -		next_fn = PCI_ARI_CAP_NFN(cap);
> -		if (next_fn <= fn)
> -			return -ENODEV;	/* protect against malformed list */
> +	return next_fn;
> +}
>   
> -		return next_fn;
> -	}
> +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
> +{
> +	if (pci_ari_enabled(bus))
> +		return next_ari_fn(bus, dev, fn);
>   
>   	if (fn >= 7)
>   		return -ENODEV;
> 

Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v6 4/5] PCI: Extend isolated function probing to s390

[Pierre Morel]

On 6/28/22 16:30, 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.
> 
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>   drivers/pci/probe.c        | 2 +-
>   include/linux/hypervisor.h | 8 ++++++++
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index a18e07e6a7df..156dd13594b8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2667,7 +2667,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
>   			 * a hypervisor which passes through individual PCI
>   			 * functions.
>   			 */
> -			if (!jailhouse_paravirt())
> +			if (!hypervisor_isolated_pci_functions())
>   				break;
>   		}
>   		fn = next_fn(bus, dev, fn);
> diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
> index fc08b433c856..33b1c0482aac 100644
> --- a/include/linux/hypervisor.h
> +++ b/include/linux/hypervisor.h
> @@ -32,4 +32,12 @@ 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();

I would spare the else,

Another remark, shouldn't it be the last patch?

otherwise LGTM

Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>


> +}
> +
>   #endif /* __LINUX_HYPEVISOR_H */
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v6 3/5] PCI: Move jailhouse's isolated function handling to pci_scan_slot()

[Pierre Morel]

On 6/28/22 16:30, Niklas Schnelle wrote:
> The special case of the jailhouse hypervisor passing through individual
> PCI functions handles scanning for PCI functions even if function 0 does
> not exist. Currently this is done with an extra loop duplicating the one
> in pci_scan_slot(). By incorporating the check for jailhouse_paravirt()
> into pci_scan_slot() we can instead do this as part of the normal
> slot scan. Note that with the assignment of dev->multifunction gated by
> fn > 0 we set dev->multifunction unconditionally for all functions if
> function 0 is missing just as in the existing jailhouse loop.
> 
> The only functional change is that we now call
> pcie_aspm_init_link_state() for these functions but this already
> happened if function 0 was passed through and should not be a problem.
> 
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Link: https://lore.kernel.org/linux-pci/20220408224514.GA353445@bhelgaas/
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>   drivers/pci/probe.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2c737dce757e..a18e07e6a7df 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2662,8 +2662,13 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
>   			if (fn > 0)
>   				dev->multifunction = 1;
>   		} else if (fn == 0) {
> -			/* function 0 is required */
> -			break;
> +			/*
> +			 * function 0 is required unless we are running on
> +			 * a hypervisor which passes through individual PCI
> +			 * functions.
> +			 */
> +			if (!jailhouse_paravirt())
> +				break;
>   		}
>   		fn = next_fn(bus, dev, fn);
>   	} while (fn >= 0);
> @@ -2862,29 +2867,14 @@ 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) {
> -		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;
> -			}
> -		}
> -	}
> +	for (devfn = 0; devfn < 256; devfn += 8)
> +		pci_scan_slot(bus, devfn);
>   
>   	/* Reserve buses for SR-IOV capability */
>   	used_buses = pci_iov_bus_range(bus);
> 


Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v6 5/5] s390/pci: allow zPCI zbus without a function zero

[Pierre Morel]

On 6/28/22 16:31, Niklas Schnelle wrote:
> 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;
>   	}
> +

Unnecessary CR

>   	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;
> 


Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v6 1/5] PCI: Clean up pci_scan_slot()

[Niklas Schnelle]

On Thu, 2022-06-30 at 14:40 +0200, Pierre Morel wrote:
> 
> On 6/28/22 16:30, Niklas Schnelle wrote:
> > While determining the next PCI function is factored out of
> > pci_scan_slot() into next_fn() the former still handles the first
> > function as a special case. This duplicates the code from the scan loop.
> > 
> > Furthermore the non ARI branch of next_fn() is generally hard to
> > understand and especially the check for multifunction devices is hidden
> > in the handling of NULL devices for non-contiguous multifunction. It
> > also signals that no further functions need to be scanned by returning
> > 0 via wraparound and this is a valid function number.
> > 
> > Improve upon this by transforming the conditions in next_fn() to be
> > easier to understand.
> > 
> > By changing next_fn() to return -ENODEV instead of 0 when there is no
> > next function we can then handle the initial function inside the loop
> > and deduplicate the shared handling. This also makes it more explicit
> > that only function 0 must exist.
> > 
> > No functional change is intended.
> > 
> > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >   drivers/pci/probe.c | 38 +++++++++++++++++++-------------------
> >   1 file changed, 19 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 17a969942d37..b05d0ed83a24 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2579,8 +2579,7 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
> >   }
> >   EXPORT_SYMBOL(pci_scan_single_device);
> >   
> > -static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
> > -			    unsigned int fn)
> > +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
> >   {
> >   	int pos;
> >   	u16 cap = 0;
> > @@ -2588,24 +2587,26 @@ static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
> >   
> >   	if (pci_ari_enabled(bus)) {
> >   		if (!dev)
> > -			return 0;
> > +			return -ENODEV;
> >   		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
> >   		if (!pos)
> > -			return 0;
> > +			return -ENODEV;
> >   
> >   		pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
> >   		next_fn = PCI_ARI_CAP_NFN(cap);
> >   		if (next_fn <= fn)
> > -			return 0;	/* protect against malformed list */
> > +			return -ENODEV;	/* protect against malformed list */
> >   
> >   		return next_fn;
> >   	}
> >   
> > -	/* dev may be NULL for non-contiguous multifunction devices */
> > -	if (!dev || dev->multifunction)
> > -		return (fn + 1) % 8;
> > +	if (fn >= 7)
> > +		return -ENODEV;
> > +	/* only multifunction devices may have more functions */
> > +	if (dev && !dev->multifunction)
> > +		return -ENODEV;
> >   
> > -	return 0;
> > +	return fn + 1;
> 
> No more % 8 ?
> Even it disapear later shouldn't we keep it ?

The "% 8" became unnecessary due to the explicit "if (fn >= 7)"
above.
The original "% 8" did what I referred to in the commit message with
"It [the function] also signals that no further functions need to be
scanned by returning 0 via wraparound and this is a valid function
number.". Instead we now explicitly return -ENODEV in this case.

> 
> 
> 
> >   }
> >   
> >   static int only_one_child(struct pci_bus *bus)
> > @@ -2643,26 +2644,25 @@ static int only_one_child(struct pci_bus *bus)
> >    */
> >   int pci_scan_slot(struct pci_bus *bus, int devfn)
> >   {
> > -	unsigned int fn, nr = 0;
> >   	struct pci_dev *dev;
> > +	int fn = 0, nr = 0;
> >   
> >   	if (only_one_child(bus) && (devfn > 0))
> >   		return 0; /* Already scanned the entire slot */
> >   
> > -	dev = pci_scan_single_device(bus, devfn);
> > -	if (!dev)
> > -		return 0;
> > -	if (!pci_dev_is_added(dev))
> > -		nr++;
> > -
> > -	for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) {
> > +	do {
> >   		dev = pci_scan_single_device(bus, devfn + fn);
> >   		if (dev) {
> >   			if (!pci_dev_is_added(dev))
> >   				nr++;
> > -			dev->multifunction = 1;
> > +			if (fn > 0)
> > +				dev->multifunction = 1;
> > +		} else if (fn == 0) {
> > +			/* function 0 is required */
> > +			break;
> >   		}
> > -	}
> > +		fn = next_fn(bus, dev, fn);
> > +	} while (fn >= 0);
> >   
> >   	/* Only one slot has PCIe device */
> >   	if (bus->self && nr)
> > 
> 
> Otherwise LGTM
> 

Thanks for taking a look!

Re: [PATCH v6 1/5] PCI: Clean up pci_scan_slot()

[Pierre Morel]

On 6/30/22 15:48, Niklas Schnelle wrote:
> On Thu, 2022-06-30 at 14:40 +0200, Pierre Morel wrote:
>>
>> On 6/28/22 16:30, Niklas Schnelle wrote:
>>> While determining the next PCI function is factored out of
>>> pci_scan_slot() into next_fn() the former still handles the first
>>> function as a special case. This duplicates the code from the scan loop.
>>>
>>> Furthermore the non ARI branch of next_fn() is generally hard to
>>> understand and especially the check for multifunction devices is hidden
>>> in the handling of NULL devices for non-contiguous multifunction. It
>>> also signals that no further functions need to be scanned by returning
>>> 0 via wraparound and this is a valid function number.
>>>
>>> Improve upon this by transforming the conditions in next_fn() to be
>>> easier to understand.
>>>
>>> By changing next_fn() to return -ENODEV instead of 0 when there is no
>>> next function we can then handle the initial function inside the loop
>>> and deduplicate the shared handling. This also makes it more explicit
>>> that only function 0 must exist.
>>>
>>> No functional change is intended.
>>>
>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>>> ---
>>>    drivers/pci/probe.c | 38 +++++++++++++++++++-------------------
>>>    1 file changed, 19 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 17a969942d37..b05d0ed83a24 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -2579,8 +2579,7 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
>>>    }
>>>    EXPORT_SYMBOL(pci_scan_single_device);
>>>    
>>> -static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
>>> -			    unsigned int fn)
>>> +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
>>>    {
>>>    	int pos;
>>>    	u16 cap = 0;
>>> @@ -2588,24 +2587,26 @@ static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
>>>    
>>>    	if (pci_ari_enabled(bus)) {
>>>    		if (!dev)
>>> -			return 0;
>>> +			return -ENODEV;
>>>    		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
>>>    		if (!pos)
>>> -			return 0;
>>> +			return -ENODEV;
>>>    
>>>    		pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
>>>    		next_fn = PCI_ARI_CAP_NFN(cap);
>>>    		if (next_fn <= fn)
>>> -			return 0;	/* protect against malformed list */
>>> +			return -ENODEV;	/* protect against malformed list */
>>>    
>>>    		return next_fn;
>>>    	}
>>>    
>>> -	/* dev may be NULL for non-contiguous multifunction devices */
>>> -	if (!dev || dev->multifunction)
>>> -		return (fn + 1) % 8;
>>> +	if (fn >= 7)
>>> +		return -ENODEV;
>>> +	/* only multifunction devices may have more functions */
>>> +	if (dev && !dev->multifunction)
>>> +		return -ENODEV;
>>>    
>>> -	return 0;
>>> +	return fn + 1;
>>
>> No more % 8 ?
>> Even it disapear later shouldn't we keep it ?
> 
> The "% 8" became unnecessary due to the explicit "if (fn >= 7)"
> above.
> The original "% 8" did what I referred to in the commit message with
> "It [the function] also signals that no further functions need to be
> scanned by returning 0 via wraparound and this is a valid function
> number.". Instead we now explicitly return -ENODEV in this case.

Yes it goes with it.
With this code next_fn returns -ENODEV for fn = 8 instead of previously 
returning 1. (If I am right)

With the previous code, did we assume that next_fn is never called with 
fn > 7?
I guess yes as we test pci_ari_enabled first and without ARI we do not 
have more than 7 more functions. is it right?

For what I think this new code seems better as it does not make the 
assumption that it get called with fn < 8.

> 
>>
>>
>>
>>>    }
>>>    
>>>    static int only_one_child(struct pci_bus *bus)
>>> @@ -2643,26 +2644,25 @@ static int only_one_child(struct pci_bus *bus)
>>>     */
>>>    int pci_scan_slot(struct pci_bus *bus, int devfn)
>>>    {
>>> -	unsigned int fn, nr = 0;
>>>    	struct pci_dev *dev;
>>> +	int fn = 0, nr = 0;
>>>    
>>>    	if (only_one_child(bus) && (devfn > 0))
>>>    		return 0; /* Already scanned the entire slot */
>>>    
>>> -	dev = pci_scan_single_device(bus, devfn);
>>> -	if (!dev)
>>> -		return 0;
>>> -	if (!pci_dev_is_added(dev))
>>> -		nr++;
>>> -
>>> -	for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) {
>>> +	do {
>>>    		dev = pci_scan_single_device(bus, devfn + fn);
>>>    		if (dev) {
>>>    			if (!pci_dev_is_added(dev))
>>>    				nr++;
>>> -			dev->multifunction = 1;
>>> +			if (fn > 0)
>>> +				dev->multifunction = 1;
>>> +		} else if (fn == 0) {
>>> +			/* function 0 is required */
>>> +			break;
>>>    		}
>>> -	}
>>> +		fn = next_fn(bus, dev, fn);
>>> +	} while (fn >= 0);
>>>    
>>>    	/* Only one slot has PCIe device */
>>>    	if (bus->self && nr)
>>>
>>
>> Otherwise LGTM
>>
> 
> Thanks for taking a look!
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v6 4/5] PCI: Extend isolated function probing to s390

[Niklas Schnelle]

On Thu, 2022-06-30 at 14:45 +0200, Pierre Morel wrote:
> 
> On 6/28/22 16:30, 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.
> > 
> > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >   drivers/pci/probe.c        | 2 +-
> >   include/linux/hypervisor.h | 8 ++++++++
> >   2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index a18e07e6a7df..156dd13594b8 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2667,7 +2667,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
> >   			 * a hypervisor which passes through individual PCI
> >   			 * functions.
> >   			 */
> > -			if (!jailhouse_paravirt())
> > +			if (!hypervisor_isolated_pci_functions())
> >   				break;
> >   		}
> >   		fn = next_fn(bus, dev, fn);
> > diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
> > index fc08b433c856..33b1c0482aac 100644
> > --- a/include/linux/hypervisor.h
> > +++ b/include/linux/hypervisor.h
> > @@ -32,4 +32,12 @@ 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();
> 
> I would spare the else,

I don't have a preference for either style so sure.

> 
> Another remark, shouldn't it be the last patch?

Either way should work. Without the last patch we don't try to probe
and without this patch the probing wouldn't find the function. I think
I'll keep the order to keep the PCI subsystem changes together and
because I feel trying to probe without that working is worse than not
probing.

> 
> otherwise LGTM
> 
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>

Thanks for taking a look!

> 
> 
> > +}
> > +
> >   #endif /* __LINUX_HYPEVISOR_H */
> > 

Re: [PATCH v6 1/5] PCI: Clean up pci_scan_slot()

[Niklas Schnelle]

On Thu, 2022-06-30 at 16:50 +0200, Pierre Morel wrote:
> > > 

> 
> On 6/30/22 15:48, Niklas Schnelle wrote:
> > On Thu, 2022-06-30 at 14:40 +0200, Pierre Morel wrote:
> > > On 6/28/22 16:30, Niklas Schnelle wrote:
> > > > While determining the next PCI function is factored out of
> > > > pci_scan_slot() into next_fn() the former still handles the first
> > > > function as a special case. This duplicates the code from the scan loop.
> > > > 
> > > > Furthermore the non ARI branch of next_fn() is generally hard to
> > > > understand and especially the check for multifunction devices is hidden
> > > > in the handling of NULL devices for non-contiguous multifunction. It
> > > > also signals that no further functions need to be scanned by returning
> > > > 0 via wraparound and this is a valid function number.
> > > > 
> > > > Improve upon this by transforming the conditions in next_fn() to be
> > > > easier to understand.
> > > > 
> > > > By changing next_fn() to return -ENODEV instead of 0 when there is no
> > > > next function we can then handle the initial function inside the loop
> > > > and deduplicate the shared handling. This also makes it more explicit
> > > > that only function 0 must exist.
> > > > 
> > > > No functional change is intended.
> > > > 
> > > > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > > ---
> > > >    drivers/pci/probe.c | 38 +++++++++++++++++++-------------------
> > > >    1 file changed, 19 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 17a969942d37..b05d0ed83a24 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -2579,8 +2579,7 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
> > > >    }
> > > >    EXPORT_SYMBOL(pci_scan_single_device);
> > > >    
> > > > -static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
> > > > -			    unsigned int fn)
> > > > +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
> > > >    {
> > > >    	int pos;
> > > >    	u16 cap = 0;
> > > > @@ -2588,24 +2587,26 @@ static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
> > > >    
> > > >    	if (pci_ari_enabled(bus)) {
> > > >    		if (!dev)
> > > > -			return 0;
> > > > +			return -ENODEV;
> > > >    		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
> > > >    		if (!pos)
> > > > -			return 0;
> > > > +			return -ENODEV;
> > > >    
> > > >    		pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap);
> > > >    		next_fn = PCI_ARI_CAP_NFN(cap);
> > > >    		if (next_fn <= fn)
> > > > -			return 0;	/* protect against malformed list */
> > > > +			return -ENODEV;	/* protect against malformed list */
> > > >    
> > > >    		return next_fn;
> > > >    	}
> > > >    
> > > > -	/* dev may be NULL for non-contiguous multifunction devices */
> > > > -	if (!dev || dev->multifunction)
> > > > -		return (fn + 1) % 8;
> > > > +	if (fn >= 7)
> > > > +		return -ENODEV;
> > > > +	/* only multifunction devices may have more functions */
> > > > +	if (dev && !dev->multifunction)
> > > > +		return -ENODEV;
> > > >    
> > > > -	return 0;
> > > > +	return fn + 1;
> > > 
> > > No more % 8 ?
> > > Even it disapear later shouldn't we keep it ?
> > 
> > The "% 8" became unnecessary due to the explicit "if (fn >= 7)"
> > above.
> > The original "% 8" did what I referred to in the commit message with
> > "It [the function] also signals that no further functions need to be
> > scanned by returning 0 via wraparound and this is a valid function
> > number.". Instead we now explicitly return -ENODEV in this case.
> 
> Yes it goes with it.
> With this code next_fn returns -ENODEV for fn = 8 instead of previously 
> returning 1. (If I am right)
> 
> With the previous code, did we assume that next_fn is never called with 
> fn > 7?
> I guess yes as we test pci_ari_enabled first and without ARI we do not 
> have more than 7 more functions. is it right?
> 
> For what I think this new code seems better as it does not make the 
> assumption that it get called with fn < 8.
> 

The fn value in this case iterates through the least significant 3 bits
of the geographical PCI address so yes this limits it to 7 functions.
My main qualm with the old code was that returning 0 for the end is
ambiguous because that is also a valid devfn.

Re: [PATCH v6 0/5] PCI: Rework pci_scan_slot() and isolated PCI functions

[Bjorn Helgaas]

On Tue, Jun 28, 2022 at 04:30:55PM +0200, Niklas Schnelle wrote:
> Hi Bjorn,
> 
> In an earlier version[0], I sought to apply the existing jailhouse special case
> for isolated PCI functions to s390. As Bjorn noted in[1] there appears to be
> some potential for cleaning things up and removing duplication.
> 
> This series attempts to do this cleanup (Patches 1 through 3) followed by enabling
> isolated PCI functions for s390 (Patches 4 and 5).
> 
> Testing:
> - On s390 with SR-IOV and a ConnectX NIC with PF 1 but not PF 0 passed throug
>   i.e. the isolated function case. Also of course with just VFs and an NVMe.
> - On x86_64 on a desktop system where ARI is disabled and with an SR-IOV NIC
>   with non-contiguous VFs as well as the usual other PCI devices.
> 
> Thanks,
> Niklas
> 
> [0] https://lore.kernel.org/linux-pci/20220404095346.2324666-1-schnelle@linux.ibm.com/
> [1] https://lore.kernel.org/linux-pci/20220408224514.GA353445@bhelgaas/
> 
> Changes v5 -> v6:
> - Added a patch (2) which separates the ARI case into its own function
> - Some whitespace changes to remove unnecesssary empty lines
> Changes v4 -> v5:
> - Remove unintended whitespace change in patch 1
> Changes v3 -> v4:
> - Use a do {} while loop in pci_scan_slot() as it is simpler (Bjorn)
> - Explicitly check "fn == 0" as it is not a pointer or bool (Bjorn)
> - Keep the "!dev" check in the ARI branch of next_fn() (Bjorn)
> - Moved the "fn == 0 && !dev" condition out of next_fn() into pci_scan_slot().
>   This allows us to keep the "!dev" case in the ARI branch and means there are
>   no new conditions in next_fn() making it easier to verify that its behavior
>   is equivalent to the existing code.
> - Guard the assignment of dev->multifunction with "fn > 0"
>   instead of "nr > 0". This matches the existing logic more closely and works
>   for the jailhouse case which unconditionally sets dev->multifunction for
>   "fn > 0". This also means fn == 0 is the single "first iteration" test.
> - Remove some unneeded whitespace in patch 2
> 
> Changes v2 -> v3:
> - Removed now unused nr_devs variable (kernel test robot)
> 
> Niklas Schnelle (5):
>   PCI: Clean up pci_scan_slot()
>   PCI: Split out next_ari_fn() from next_fn()
>   PCI: Move jailhouse's isolated function handling to pci_scan_slot()
>   PCI: Extend isolated function probing to s390
>   s390/pci: allow zPCI zbus without a function zero
> 
>  arch/s390/pci/pci_bus.c    | 82 +++++++++---------------------------
>  drivers/pci/probe.c        | 86 ++++++++++++++++++--------------------
>  include/linux/hypervisor.h |  8 ++++
>  3 files changed, 68 insertions(+), 108 deletions(-)

Applied to pci/enumeration for v5.20, thanks!

Re: [PATCH v6 4/5] PCI: Extend isolated function probing to s390

[Bjorn Helgaas]

On Tue, Jun 28, 2022 at 04:30:59PM +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.
> 
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/pci/probe.c        | 2 +-
>  include/linux/hypervisor.h | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index a18e07e6a7df..156dd13594b8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2667,7 +2667,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
>  			 * a hypervisor which passes through individual PCI
>  			 * functions.
>  			 */
> -			if (!jailhouse_paravirt())
> +			if (!hypervisor_isolated_pci_functions())
>  				break;
>  		}
>  		fn = next_fn(bus, dev, fn);
> diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
> index fc08b433c856..33b1c0482aac 100644
> --- a/include/linux/hypervisor.h
> +++ b/include/linux/hypervisor.h
> @@ -32,4 +32,12 @@ 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();

It looks kind of wasteful that jailhouse_paravirt() searches the DT
for "jailhouse,cell" several times when I think that's an unchanging
property.

Obviously you didn't add that in this series, and s390 avoids that
cost anyway.  But the jailhouse folks might consider optimizing it
somehow.

> +}
> +
>  #endif /* __LINUX_HYPEVISOR_H */
> -- 
> 2.32.0
>