sriov enablement on s390

Quoted 21 lines · Bjorn Helgaas

[+cc Arnd, powerpc folks]
Quoted 15 lines · Sebastian Ott
Hello Bjorn,
On s390 we currently handle SRIOV within firmware. Which means that the PF is under firmware control and not visible to operating systems. SRIOV enablement happens within firmware and VFs are passed through to logical partitions.
I'm working on a new mode were the PF is under operating system control (including SRIOV enablement). However we still need firmware support to access the VFs. The way this is supposed to work is that when firmware traps the SRIOV enablement it will present machine checks to the logical partition that triggered the SRIOV enablement and provide the VFs via hotplug events.
Quoted 2 lines
The problem I'm faced with is that the VF detection code in sriov_enable leads to unusable functions in s390.

Quoted 8 lines

We're moving away from the weak function implementation style. Can you take a look at Arnd's work here, which uses pci_host_bridge callbacks instead?
https://lkml.kernel.org/r/[email protected]
I cc'd some powerpc folks because they also have a fair amount of arch-specific SR-IOV code that might one day move in this direction.

Rebased to Arnd's pci-probe-rework branch.

Sebastian Ott (2):
pci: provide add_vfs/del_vfs callbacks
s390/pci: handle function enumeration after sriov enablement

 arch/s390/pci/pci.c | 11 +++++++++++
 drivers/pci/iov.c   | 51 +++++++++++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  2 ++
 3 files changed, 52 insertions(+), 12 deletions(-)

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-09-13 12:41 UTC #4 lore raw

PATCH 1/2 pci: provide add_vfs/del_vfs callbacks

Provide callbacks that can be used by PCI host bridge implementations to override the behavior of the generic vf detection and device creation code.

---
 drivers/pci/iov.c   | 51 +++++++++++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  2 ++
 2 files changed, 41 insertions(+), 12 deletions(-)

drivers/pci/iov.c +39 ~~-12~~

index 0f04ae648cf1..b2ddfe30c5d8 100644
@@ -251,6 +251,41 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
 	return 0;
 }
 
+static int pcibios_sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
+{
+	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
+	unsigned int i;
+	int rc;
+
+	if (bridge->add_vfs)
+		return bridge->add_vfs(dev, num_vfs);
+
+	for (i = 0; i < num_vfs; i++) {
+		rc = pci_iov_add_virtfn(dev, i);
+		if (rc)
+			goto failed;
+	}
+	return 0;
+failed:
+	while (i--)
+		pci_iov_remove_virtfn(dev, i);
+
+	return rc;
+}
+
+static void pcibios_sriov_del_vfs(struct pci_dev *dev)
+{
+	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
+	struct pci_sriov *iov = dev->sriov;
+	int i;
+
+	if (bridge->del_vfs)
+		return bridge->del_vfs(dev);
+
+	for (i = 0; i < iov->num_VFs; i++)
+		pci_iov_remove_virtfn(dev, i);
+}
+
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 {
 	int rc;

@@ -336,21 +371,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	msleep(100);
 	pci_cfg_access_unlock(dev);
 
-	for (i = 0; i < initial; i++) {
-		rc = pci_iov_add_virtfn(dev, i);
-		if (rc)
-			goto failed;
-	}
+	rc = pcibios_sriov_add_vfs(dev, initial);
+	if (rc)
+		goto err_pcibios;
 
 	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
 	iov->num_VFs = nr_virtfn;
 
 	return 0;
 
-failed:
-	while (i--)
-		pci_iov_remove_virtfn(dev, i);
-
 err_pcibios:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

@@ -369,14 +398,12 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 
 static void sriov_disable(struct pci_dev *dev)
 {
-	int i;
 	struct pci_sriov *iov = dev->sriov;
 
 	if (!iov->num_VFs)
 		return;
 
-	for (i = 0; i < iov->num_VFs; i++)
-		pci_iov_remove_virtfn(dev, i);
+	pcibios_sriov_del_vfs(dev);
 
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

include/linux/pci.h +2 -0

index 680b6bcd0b97..bf99ae98ecb5 100644
@@ -475,6 +475,8 @@ struct pci_host_bridge {
 	int (*free_irq)(struct pci_dev *);
 	void (*add_bus)(struct pci_bus *);
 	void (*remove_bus)(struct pci_bus *);
+	int (*add_vfs)(struct pci_dev *dev, u16 num_vfs);
+	void (*del_vfs)(struct pci_dev *dev);
 	void		*release_data;
 	struct msi_controller *msi;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-09-13 12:41 UTC #5 lore raw

PATCH 2/2 s390/pci: handle function enumeration after sriov enablement

Implement add_vfs|del_vfs callbacks as empty functions. VF creation will be triggered by the hotplug code.

---
 arch/s390/pci/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

arch/s390/pci/pci.c +11 -0

index 9381d5d98156..2ba2cbfaa091 100644
@@ -785,6 +785,15 @@ static void zpci_remove_bus(struct pci_bus *bus)
 	kfree(zdev);
 }
 
+static int zpci_add_vfs(struct pci_dev *pdev, u16 num_vfs)
+{
+	return 0;
+}
+
+static void zpci_del_vfs(struct pci_dev *pdev)
+{
+}
+
 static struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 		struct pci_ops *ops, void *sysdata, struct list_head *resources)
 {

@@ -801,6 +810,8 @@ static struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 	bridge->busnr = bus;
 	bridge->ops = ops;
 	bridge->remove_bus = zpci_remove_bus;
+	bridge->add_vfs = zpci_add_vfs;
+	bridge->del_vfs = zpci_del_vfs;
 
 	error = pci_scan_root_bus_bridge(bridge);
 	if (error < 0)

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-10-10 12:55 UTC #6 lore raw

[1] https://lore.kernel.org/linux-pci/[email protected]

Hello Bjorn,

Quoted 14 lines · Bjorn Helgaas

Quoted 13 lines · Sebastian Ott
On s390 we currently handle SRIOV within firmware. Which means that the PF is under firmware control and not visible to operating systems. SRIOV enablement happens within firmware and VFs are passed through to logical partitions.
I'm working on a new mode were the PF is under operating system control (including SRIOV enablement). However we still need firmware support to access the VFs. The way this is supposed to work is that when firmware traps the SRIOV enablement it will present machine checks to the logical partition that triggered the SRIOV enablement and provide the VFs via hotplug events.

Quoted 8 lines

Quoted 2 lines
The problem I'm faced with is that the VF detection code in sriov_enable leads to unusable functions in s390.
We're moving away from the weak function implementation style. Can you take a look at Arnd's work here, which uses pci_host_bridge callbacks instead?
https://lkml.kernel.org/r/[email protected]

What's the status of Arnd's patches - will they go upstream in the next couple of versions? What about my patches that I rebased on Arnd's branch will they be considered?

Regards,
Sebastian

Bjorn Helgaas

2018-10-10 16:26 UTC #7 lore raw

Quoted 20 lines · Sebastian Ott

Hello Bjorn,
Quoted 17 lines · Bjorn Helgaas
Quoted 15 lines · Sebastian Ott
On s390 we currently handle SRIOV within firmware. Which means that the PF is under firmware control and not visible to operating systems. SRIOV enablement happens within firmware and VFs are passed through to logical partitions.
I'm working on a new mode were the PF is under operating system control (including SRIOV enablement). However we still need firmware support to access the VFs. The way this is supposed to work is that when firmware traps the SRIOV enablement it will present machine checks to the logical partition that triggered the SRIOV enablement and provide the VFs via hotplug events.
The problem I'm faced with is that the VF detection code in sriov_enable leads to unusable functions in s390.

Quoted 8 lines

Quoted 5 lines
We're moving away from the weak function implementation style. Can you take a look at Arnd's work here, which uses pci_host_bridge callbacks instead?
https://lkml.kernel.org/r/[email protected]
What's the status of Arnd's patches - will they go upstream in the next couple of versions?

I hope so [1]. IIRC Arnd mentioned doing some minor updates, so I'm waiting on that.

Quoted 2 lines

What about my patches that I rebased on Arnd's branch will they be considered?

Definitely. From my point of view they're just lined up behind Arnd's patches.

Sebastian Ott

author

2018-12-05 13:45 UTC #8 lore raw

Hello Bjorn,

Quoted 31 lines · Bjorn Helgaas

Quoted 18 lines · Sebastian Ott
Quoted 17 lines · Bjorn Helgaas
Quoted 15 lines · Sebastian Ott
On s390 we currently handle SRIOV within firmware. Which means that the PF is under firmware control and not visible to operating systems. SRIOV enablement happens within firmware and VFs are passed through to logical partitions.
I'm working on a new mode were the PF is under operating system control (including SRIOV enablement). However we still need firmware support to access the VFs. The way this is supposed to work is that when firmware traps the SRIOV enablement it will present machine checks to the logical partition that triggered the SRIOV enablement and provide the VFs via hotplug events.
The problem I'm faced with is that the VF detection code in sriov_enable leads to unusable functions in s390.
Quoted 8 lines
Quoted 5 lines
We're moving away from the weak function implementation style. Can you take a look at Arnd's work here, which uses pci_host_bridge callbacks instead?
https://lkml.kernel.org/r/[email protected]
What's the status of Arnd's patches - will they go upstream in the next couple of versions?
I hope so [1]. IIRC Arnd mentioned doing some minor updates, so I'm waiting on that.

Quoted 7 lines

Quoted 2 lines
What about my patches that I rebased on Arnd's branch will they be considered?
Definitely. From my point of view they're just lined up behind Arnd's patches.
[1] https://lore.kernel.org/linux-pci/[email protected]

It appears like these patches are not in-line for the next merge window. Would it be possible to go with my original patches (using __weak functions)? (This would also make life easier with regards to backports) I can post patches to convert this to use function pointers once Arnd's patches make it to the kernel.

Regards,
Sebastian

Bjorn Helgaas

2018-12-12 21:54 UTC #9 lore raw

Quoted 40 lines · Sebastian Ott

Hello Bjorn,
Quoted 37 lines · Bjorn Helgaas
Quoted 18 lines · Sebastian Ott
Quoted 17 lines · Bjorn Helgaas
Quoted 15 lines · Sebastian Ott
On s390 we currently handle SRIOV within firmware. Which means that the PF is under firmware control and not visible to operating systems. SRIOV enablement happens within firmware and VFs are passed through to logical partitions.
I'm working on a new mode were the PF is under operating system control (including SRIOV enablement). However we still need firmware support to access the VFs. The way this is supposed to work is that when firmware traps the SRIOV enablement it will present machine checks to the logical partition that triggered the SRIOV enablement and provide the VFs via hotplug events.
The problem I'm faced with is that the VF detection code in sriov_enable leads to unusable functions in s390.
Quoted 8 lines
Quoted 5 lines
We're moving away from the weak function implementation style. Can you take a look at Arnd's work here, which uses pci_host_bridge callbacks instead?
https://lkml.kernel.org/r/[email protected]
What's the status of Arnd's patches - will they go upstream in the next couple of versions?
I hope so [1]. IIRC Arnd mentioned doing some minor updates, so I'm waiting on that.
Quoted 2 lines
What about my patches that I rebased on Arnd's branch will they be considered?
Definitely. From my point of view they're just lined up behind Arnd's patches.

Quoted 7 lines

Quoted 1 line
[1] https://lore.kernel.org/linux-pci/[email protected]
It appears like these patches are not in-line for the next merge window. Would it be possible to go with my original patches (using __weak functions)? (This would also make life easier with regards to backports) I can post patches to convert this to use function pointers once Arnd's patches make it to the kernel.

Yeah, sorry, I think we should just go with your original approach.

Can you repost those patches with minor changelog updates so "git log --online" on the files looks consistent. Also, capitalize "PCI", "VF", etc, consistently when used in English text.

Bjorn

Sebastian Ott

author

2018-12-13 17:54 UTC #10 lore raw

PATCH 1/2 PCI: provide pcibios_sriov_add_vfs

Move VF detection and device creation code to weak functions such that architectures can provide a different implementation.

---
 drivers/pci/iov.c   | 43 +++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  2 ++
 2 files changed, 33 insertions(+), 12 deletions(-)

drivers/pci/iov.c +31 ~~-12~~

index 9616eca3182f..1bfdb4deafd7 100644
@@ -252,6 +252,33 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
 	return 0;
 }
 
+int __weak pcibios_sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
+{
+	unsigned int i;
+	int rc;
+
+	for (i = 0; i < num_vfs; i++) {
+		rc = pci_iov_add_virtfn(dev, i);
+		if (rc)
+			goto failed;
+	}
+	return 0;
+failed:
+	while (i--)
+		pci_iov_remove_virtfn(dev, i);
+
+	return rc;
+}
+
+void __weak pcibios_sriov_del_vfs(struct pci_dev *dev)
+{
+	struct pci_sriov *iov = dev->sriov;
+	int i;
+
+	for (i = 0; i < iov->num_VFs; i++)
+		pci_iov_remove_virtfn(dev, i);
+}
+
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 {
 	int rc;

@@ -337,21 +364,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	msleep(100);
 	pci_cfg_access_unlock(dev);
 
-	for (i = 0; i < initial; i++) {
-		rc = pci_iov_add_virtfn(dev, i);
-		if (rc)
-			goto failed;
-	}
+	rc = pcibios_sriov_add_vfs(dev, initial);
+	if (rc)
+		goto err_pcibios;
 
 	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
 	iov->num_VFs = nr_virtfn;
 
 	return 0;
 
-failed:
-	while (i--)
-		pci_iov_remove_virtfn(dev, i);
-
 err_pcibios:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

@@ -370,14 +391,12 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 
 static void sriov_disable(struct pci_dev *dev)
 {
-	int i;
 	struct pci_sriov *iov = dev->sriov;
 
 	if (!iov->num_VFs)
 		return;
 
-	for (i = 0; i < iov->num_VFs; i++)
-		pci_iov_remove_virtfn(dev, i);
+	pcibios_sriov_del_vfs(dev);
 
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

include/linux/pci.h +2 -0

index 11c71c4ecf75..84ca3bcdac76 100644
@@ -2001,6 +2001,8 @@ void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
 /* Arch may override these (weak) */
 int pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs);
 int pcibios_sriov_disable(struct pci_dev *pdev);
+int pcibios_sriov_add_vfs(struct pci_dev *dev, u16 num_vfs);
+void pcibios_sriov_del_vfs(struct pci_dev *dev);
 resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
 #else
 static inline int pci_iov_virtfn_bus(struct pci_dev *dev, int id)

Signature · 1 line

2.16.4

Christoph Hellwig

2018-12-14 13:12 UTC #11 lore raw

Re: [PATCH 1/2] PCI: provide pcibios_sriov_add_vfs

Quoted 2 lines · Sebastian Ott

Move VF detection and device creation code to weak functions such that architectures can provide a different implementation.

No magic weak functions please. If you really have a reason to override the implementation add a proper Kconfig symbol and an explicit override.

Sebastian Ott

author

2018-12-13 17:54 UTC #12 lore raw

PATCH 2/2 s390/pci: handle function enumeration after sriov enablement

Implement pcibios_sriov_{add|del}_vfs as empty functions. VF creation will be triggered by the hotplug code.

---
 arch/s390/pci/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

arch/s390/pci/pci.c +11 -0

index 9f6f392a4461..b5f8db652bf5 100644
@@ -731,6 +731,17 @@ struct dev_pm_ops pcibios_pm_ops = {
 };
 #endif /* CONFIG_HIBERNATE_CALLBACKS */
 
+#ifdef CONFIG_PCI_IOV
+int pcibios_sriov_add_vfs(struct pci_dev *pdev, u16 num_vfs)
+{
+	return 0;
+}
+
+void pcibios_sriov_del_vfs(struct pci_dev *pdev)
+{
+}
+#endif
+
 static int zpci_alloc_domain(struct zpci_dev *zdev)
 {
 	if (zpci_unique_uid) {

Signature · 1 line

2.16.4

Christoph Hellwig

2018-12-14 13:12 UTC #13 lore raw

Re: [PATCH 2/2] s390/pci: handle function enumeration after sriov enablement

Quoted 2 lines · Sebastian Ott

Implement pcibios_sriov_{add|del}_vfs as empty functions. VF creation will be triggered by the hotplug code.

And instead of having the arch suplply a no-op arch override I think it would be better to have the config option just stub it out in common code.

Christoph Hellwig

2018-12-14 13:18 UTC #14 lore raw

Re: [PATCH 2/2] s390/pci: handle function enumeration after sriov enablement

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Sebastian Ott
Implement pcibios_sriov_{add|del}_vfs as empty functions. VF creation will be triggered by the hotplug code.
And instead of having the arch suplply a no-op arch override I think it would be better to have the config option just stub it out in common code.

Or in fact maybe even a runtime flag in struct pci_dev. Who knows if all future s390 PCIe busses will have exactly the same behavior or if we eventually get the standards compliant behvior back?

Sebastian Ott

author

2018-12-17 17:30 UTC #15 lore raw

Re: [PATCH 2/2] s390/pci: handle function enumeration after sriov enablement

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Christoph Hellwig
Quoted 2 lines · Sebastian Ott
Implement pcibios_sriov_{add|del}_vfs as empty functions. VF creation will be triggered by the hotplug code.

Quoted 7 lines

Quoted 3 lines
And instead of having the arch suplply a no-op arch override I think it would be better to have the config option just stub it out in common code.
Or in fact maybe even a runtime flag in struct pci_dev. Who knows if all future s390 PCIe busses will have exactly the same behavior or if we eventually get the standards compliant behvior back?

Something like this:
https://lore.kernel.org/linux-pci/[email protected]/T/#m649d86ea3c65f669c74d048f89afbaf473876ac3

Not a runtime flag, but a function pointer in struct pci_host_bridge. This would provide the requested flexibility. The problem with this approach is that it requires other patches that are not yet upstream
(https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=pci-probe-rework).

Since this discussion is going on since a few months and I want to have this code upstream and in distributions for HW enablement I've asked Bjorn to go with the initial approach (weak functions) and promised to move that to struct pci_host_bridge once Arnd's patches are upstream. Would that be OK for you too?

Regards,
Sebastian

Christoph Hellwig

2018-12-17 17:35 UTC #16 lore raw

Re: [PATCH 2/2] s390/pci: handle function enumeration after sriov enablement

Quoted 2 lines · Sebastian Ott

Something like this:
https://lore.kernel.org/linux-pci/[email protected]/T/#m649d86ea3c65f669c74d048f89afbaf473876ac3

No, I literally meant a flag to skip the work. Think about it: there is a standard way to probe VFs, which comes from what is defined in the PCIe spec itself. It just turns out s390 for some weird reason decides to already let the VFs show up basically like PFs. There really should be no reason to branch out into per-arch code here as there really isn't much to do on a per-arch level. More just a quirk for the firmware is buggy and already reports the VFs to us, so skip the probing.

Sebastian Ott

author

2018-12-18 10:17 UTC #17 lore raw

PATCH 1/2 PCI/IOV: provide flag to skip VF scanning

Provide a flag to skip scanning for new VFs after SRIOV enablement. This can be set by implementations for which the VFs are already reported by other means.

---
 drivers/pci/iov.c   | 48 ++++++++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  1 +
 2 files changed, 37 insertions(+), 12 deletions(-)

drivers/pci/iov.c +36 ~~-12~~

index 9616eca3182f..3aa115ed3a65 100644
@@ -252,6 +252,27 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
 	return 0;
 }
 
+static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
+{
+	unsigned int i;
+	int rc;
+
+	if (dev->no_vf_scan)
+		return 0;
+
+	for (i = 0; i < num_vfs; i++) {
+		rc = pci_iov_add_virtfn(dev, i);
+		if (rc)
+			goto failed;
+	}
+	return 0;
+failed:
+	while (i--)
+		pci_iov_remove_virtfn(dev, i);
+
+	return rc;
+}
+
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 {
 	int rc;

@@ -337,21 +358,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	msleep(100);
 	pci_cfg_access_unlock(dev);
 
-	for (i = 0; i < initial; i++) {
-		rc = pci_iov_add_virtfn(dev, i);
-		if (rc)
-			goto failed;
-	}
+	rc = sriov_add_vfs(dev, initial);
+	if (rc)
+		goto err_pcibios;
 
 	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
 	iov->num_VFs = nr_virtfn;
 
 	return 0;
 
-failed:
-	while (i--)
-		pci_iov_remove_virtfn(dev, i);
-
 err_pcibios:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

@@ -368,17 +383,26 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	return rc;
 }
 
-static void sriov_disable(struct pci_dev *dev)
+static void sriov_del_vfs(struct pci_dev *dev)
 {
-	int i;
 	struct pci_sriov *iov = dev->sriov;
+	int i;
 
-	if (!iov->num_VFs)
+	if (dev->no_vf_scan)
 		return;
 
 	for (i = 0; i < iov->num_VFs; i++)
 		pci_iov_remove_virtfn(dev, i);
+}
+
+static void sriov_disable(struct pci_dev *dev)
+{
+	struct pci_sriov *iov = dev->sriov;
+
+	if (!iov->num_VFs)
+		return;
 
+	sriov_del_vfs(dev);
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

include/linux/pci.h +1 -0

index 11c71c4ecf75..f70b9ccd3e86 100644
@@ -405,6 +405,7 @@ struct pci_dev {
 	unsigned int	non_compliant_bars:1;	/* Broken BARs; ignore them */
 	unsigned int	is_probed:1;		/* Device probing in progress */
 	unsigned int	link_active_reporting:1;/* Device capable of reporting link active */
+	unsigned int	no_vf_scan:1;		/* Don't scan for VF's after VF enablement */
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */

Signature · 1 line

2.13.4

Christoph Hellwig

Reviewed-by on v1 1/2

2018-12-19 07:52 UTC #18 lore raw

Re: [PATCH 1/2] PCI/IOV: provide flag to skip VF scanning

Quoted 5 lines · Sebastian Ott

Provide a flag to skip scanning for new VFs after SRIOV enablement. This can be set by implementations for which the VFs are already reported by other means.
Signed-off-by Sebastian Ott <[email protected]>

Looks good,

Bjorn Helgaas

2018-12-20 20:07 UTC #19 lore raw

Re: [PATCH 1/2] PCI/IOV: provide flag to skip VF scanning

Hi Sebastian,

Quoted 28 lines · Sebastian Ott

Provide a flag to skip scanning for new VFs after SRIOV enablement. This can be set by implementations for which the VFs are already reported by other means.

---
 drivers/pci/iov.c   | 48 ++++++++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  1 +
 2 files changed, 37 insertions(+), 12 deletions(-)

drivers/pci/iov.c +10 -0

index 9616eca3182f..3aa115ed3a65 100644
@@ -252,6 +252,27 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
 	return 0;
 }
 
+static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
+{
+	unsigned int i;
+	int rc;
+
+	if (dev->no_vf_scan)
+		return 0;
+
+	for (i = 0; i < num_vfs; i++) {
+		rc = pci_iov_add_virtfn(dev, i);

drivers/pci/iov.c int __weak pcibios_sriov_disable(struct pci_dev *pdev) · line 274

Quoted 10 lines

drivers/pci/iov.c +10 -0

+		if (rc)
+			goto failed;
+	}
+	return 0;
+failed:
+	while (i--)
+		pci_iov_remove_virtfn(dev, i);
+
+	return rc;
+}

I think the strategy is fine, but can you restructure the patches like this:

1) Factor out sriov_add_vfs() and sriov_dev_vfs(). This makes no functional change at all.

2) Add dev->no_vf_scan, set it in the s390 pcibios_add_device(), and test it in sriov_add_vfs(), and sriov_del_vfs().

I think both pieces will be easier to review that way.

Quoted 73 lines

drivers/pci/iov.c +15 ~~-12~~

 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 {
 	int rc;

@@ -337,21 +358,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	msleep(100);
 	pci_cfg_access_unlock(dev);
 
-	for (i = 0; i < initial; i++) {
-		rc = pci_iov_add_virtfn(dev, i);
-		if (rc)
-			goto failed;
-	}
+	rc = sriov_add_vfs(dev, initial);
+	if (rc)
+		goto err_pcibios;
 
 	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
 	iov->num_VFs = nr_virtfn;
 
 	return 0;
 
-failed:
-	while (i--)
-		pci_iov_remove_virtfn(dev, i);
-
 err_pcibios:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

@@ -368,17 +383,26 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	return rc;
 }
 
-static void sriov_disable(struct pci_dev *dev)
+static void sriov_del_vfs(struct pci_dev *dev)
 {
-	int i;
 	struct pci_sriov *iov = dev->sriov;
+	int i;
 
-	if (!iov->num_VFs)
+	if (dev->no_vf_scan)
 		return;
 
 	for (i = 0; i < iov->num_VFs; i++)
 		pci_iov_remove_virtfn(dev, i);
+}
+
+static void sriov_disable(struct pci_dev *dev)
+{
+	struct pci_sriov *iov = dev->sriov;
+
+	if (!iov->num_VFs)
+		return;
 
+	sriov_del_vfs(dev);
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

include/linux/pci.h +1 -0

index 11c71c4ecf75..f70b9ccd3e86 100644
@@ -405,6 +405,7 @@ struct pci_dev {
 	unsigned int	non_compliant_bars:1;	/* Broken BARs; ignore them */
 	unsigned int	is_probed:1;		/* Device probing in progress */
 	unsigned int	link_active_reporting:1;/* Device capable of reporting link active */
+	unsigned int	no_vf_scan:1;		/* Don't scan for VF's after VF enablement */
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-12-21 14:14 UTC #20 lore raw

PATCH 1/3 PCI/IOV: factor out sriov_add_vfs

Provide sriov_add_vfs as a wrapper to scan for VFs that cleans up after itself. This is just a code simplification. No functional change.

---
 drivers/pci/iov.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

drivers/pci/iov.c +31 ~~-13~~

index 9616eca3182f..408db232a328 100644
@@ -252,6 +252,24 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
 	return 0;
 }
 
+static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
+{
+	unsigned int i;
+	int rc;
+
+	for (i = 0; i < num_vfs; i++) {
+		rc = pci_iov_add_virtfn(dev, i);
+		if (rc)
+			goto failed;
+	}
+	return 0;
+failed:
+	while (i--)
+		pci_iov_remove_virtfn(dev, i);
+
+	return rc;
+}
+
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 {
 	int rc;

@@ -337,21 +355,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	msleep(100);
 	pci_cfg_access_unlock(dev);
 
-	for (i = 0; i < initial; i++) {
-		rc = pci_iov_add_virtfn(dev, i);
-		if (rc)
-			goto failed;
-	}
+	rc = sriov_add_vfs(dev, initial);
+	if (rc)
+		goto err_pcibios;
 
 	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
 	iov->num_VFs = nr_virtfn;
 
 	return 0;
 
-failed:
-	while (i--)
-		pci_iov_remove_virtfn(dev, i);
-
 err_pcibios:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

@@ -368,17 +380,23 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	return rc;
 }
 
-static void sriov_disable(struct pci_dev *dev)
+static void sriov_del_vfs(struct pci_dev *dev)
 {
+	struct pci_sriov *iov = dev->sriov;
 	int i;
+
+	for (i = 0; i < iov->num_VFs; i++)
+		pci_iov_remove_virtfn(dev, i);
+}
+
+static void sriov_disable(struct pci_dev *dev)
+{
 	struct pci_sriov *iov = dev->sriov;
 
 	if (!iov->num_VFs)
 		return;
 
-	for (i = 0; i < iov->num_VFs; i++)
-		pci_iov_remove_virtfn(dev, i);
-
+	sriov_del_vfs(dev);
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-12-21 14:14 UTC #21 lore raw

PATCH 2/3 PCI/IOV: provide flag to skip VF scanning

Provide a flag to skip scanning for new VFs after SRIOV enablement. This can be set by implementations for which the VFs are already reported by other means.

---
 drivers/pci/iov.c   | 6 ++++++
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+)

drivers/pci/iov.c +6 -0

index 408db232a328..3aa115ed3a65 100644
@@ -257,6 +257,9 @@ static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
 	unsigned int i;
 	int rc;
 
+	if (dev->no_vf_scan)
+		return 0;
+
 	for (i = 0; i < num_vfs; i++) {
 		rc = pci_iov_add_virtfn(dev, i);
 		if (rc)

@@ -385,6 +388,9 @@ static void sriov_del_vfs(struct pci_dev *dev)
 	struct pci_sriov *iov = dev->sriov;
 	int i;
 
+	if (dev->no_vf_scan)
+		return;
+
 	for (i = 0; i < iov->num_VFs; i++)
 		pci_iov_remove_virtfn(dev, i);
 }

include/linux/pci.h +1 -0

index 11c71c4ecf75..f9bc7651c406 100644
@@ -405,6 +405,7 @@ struct pci_dev {
 	unsigned int	non_compliant_bars:1;	/* Broken BARs; ignore them */
 	unsigned int	is_probed:1;		/* Device probing in progress */
 	unsigned int	link_active_reporting:1;/* Device capable of reporting link active */
+	unsigned int	no_vf_scan:1;		/* Don't scan for VFs after IOV enablement */
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-12-21 14:14 UTC #22 lore raw

PATCH 3/3 s390/pci: skip VF scanning

Set the flag to skip scanning for VFs after SRIOV enablement. VF creation will be triggered by the hotplug code.

---
 arch/s390/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

arch/s390/pci/pci.c +3 -0

index 9f6f392a4461..4266a4de3160 100644
@@ -651,6 +651,9 @@ int pcibios_add_device(struct pci_dev *pdev)
 	struct resource *res;
 	int i;
 
+	if (pdev->is_physfn)
+		pdev->no_vf_scan = 1;
+
 	pdev->dev.groups = zpci_attr_groups;
 	pdev->dev.dma_ops = &s390_pci_dma_ops;
 	zpci_map_resources(pdev);

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-12-21 14:20 UTC #23 lore raw

Re: [PATCH 1/2] PCI/IOV: provide flag to skip VF scanning

In reply to Christoph Hellwig

Hello Bjorn,

Quoted 10 lines · Bjorn Helgaas

I think the strategy is fine, but can you restructure the patches like this:
1) Factor out sriov_add_vfs() and sriov_dev_vfs(). This makes no functional change at all.
2) Add dev->no_vf_scan, set it in the s390 pcibios_add_device(), and test it in sriov_add_vfs(), and sriov_del_vfs().
I think both pieces will be easier to review that way.

Done. I took the liberty to add Christoph's R-b to the first two patches since it's just a split of the patch he gave the R-b to.

Thanks!
Sebastian

Bjorn Helgaas

2019-01-02 01:15 UTC #24 lore raw

Re: [PATCH 1/2] PCI/IOV: provide flag to skip VF scanning

Quoted 9 lines · Sebastian Ott

Hello Bjorn,
Quoted 6 lines · Bjorn Helgaas
I think the strategy is fine, but can you restructure the patches like this:
1) Factor out sriov_add_vfs() and sriov_dev_vfs(). This makes no functional change at all.

Quoted 7 lines

Quoted 4 lines
2) Add dev->no_vf_scan, set it in the s390 pcibios_add_device(), and test it in sriov_add_vfs(), and sriov_del_vfs().
I think both pieces will be easier to review that way.
Done. I took the liberty to add Christoph's R-b to the first two patches since it's just a split of the patch he gave the R-b to.

Thanks.

It's really way too late to do this, but they're pretty trivial, and I've been out longer than expected for vacation and illness, so I applied these to pci/virtualization for v4.21.

Bjorn

Sebastian Ott

author

2018-12-18 10:17 UTC #25 lore raw

PATCH 2/2 s390/pci: skip VF scanning

Set the flag to skip scanning for VFs after SRIOV enablement. VF creation will be triggered by the hotplug code.

---
 arch/s390/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

arch/s390/pci/pci.c +3 -0

index 9f6f392a4461..4266a4de3160 100644
@@ -651,6 +651,9 @@ int pcibios_add_device(struct pci_dev *pdev)
 	struct resource *res;
 	int i;
 
+	if (pdev->is_physfn)
+		pdev->no_vf_scan = 1;
+
 	pdev->dev.groups = zpci_attr_groups;
 	pdev->dev.dma_ops = &s390_pci_dma_ops;
 	zpci_map_resources(pdev);

Signature · 1 line

2.13.4

Christoph Hellwig

Reviewed-by on v1 2/2

2018-12-19 07:52 UTC #26 lore raw

Re: [PATCH 2/2] s390/pci: skip VF scanning

Quoted 4 lines · Sebastian Ott

Set the flag to skip scanning for VFs after SRIOV enablement. VF creation will be triggered by the hotplug code.
Signed-off-by Sebastian Ott <[email protected]>

Looks good,

Sebastian Ott

author

2018-09-12 17:38 UTC #27 lore raw

PATCH 1/2 pci: provide pcibios_sriov_add_vfs

Move vf detection and device creation code to weak functions such that architectures can provide a different implementation.

---
 drivers/pci/iov.c   | 43 +++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  2 ++
 2 files changed, 33 insertions(+), 12 deletions(-)

drivers/pci/iov.c +31 ~~-12~~

index c5f3cd4ed766..6452cb8f397f 100644
@@ -251,6 +251,33 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
 	return 0;
 }
 
+int __weak pcibios_sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
+{
+	unsigned int i;
+	int rc;
+
+	for (i = 0; i < num_vfs; i++) {
+		rc = pci_iov_add_virtfn(dev, i);
+		if (rc)
+			goto failed;
+	}
+	return 0;
+failed:
+	while (i--)
+		pci_iov_remove_virtfn(dev, i);
+
+	return rc;
+}
+
+void __weak pcibios_sriov_del_vfs(struct pci_dev *dev)
+{
+	struct pci_sriov *iov = dev->sriov;
+	int i;
+
+	for (i = 0; i < iov->num_VFs; i++)
+		pci_iov_remove_virtfn(dev, i);
+}
+
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 {
 	int rc;

@@ -336,21 +363,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	msleep(100);
 	pci_cfg_access_unlock(dev);
 
-	for (i = 0; i < initial; i++) {
-		rc = pci_iov_add_virtfn(dev, i);
-		if (rc)
-			goto failed;
-	}
+	rc = pcibios_sriov_add_vfs(dev, initial);
+	if (rc)
+		goto err_pcibios;
 
 	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
 	iov->num_VFs = nr_virtfn;
 
 	return 0;
 
-failed:
-	while (i--)
-		pci_iov_remove_virtfn(dev, i);
-
 err_pcibios:
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

@@ -369,14 +390,12 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 
 static void sriov_disable(struct pci_dev *dev)
 {
-	int i;
 	struct pci_sriov *iov = dev->sriov;
 
 	if (!iov->num_VFs)
 		return;
 
-	for (i = 0; i < iov->num_VFs; i++)
-		pci_iov_remove_virtfn(dev, i);
+	pcibios_sriov_del_vfs(dev);
 
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
 	pci_cfg_access_lock(dev);

include/linux/pci.h +2 -0

index e72ca8dd6241..76ff5b70f9a7 100644
@@ -1989,6 +1989,8 @@ void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
 /* Arch may override these (weak) */
 int pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs);
 int pcibios_sriov_disable(struct pci_dev *pdev);
+int pcibios_sriov_add_vfs(struct pci_dev *dev, u16 num_vfs);
+void pcibios_sriov_del_vfs(struct pci_dev *dev);
 resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
 #else
 static inline int pci_iov_virtfn_bus(struct pci_dev *dev, int id)

Signature · 1 line

2.13.4

Sebastian Ott

author

2018-09-12 17:38 UTC #28 lore raw

PATCH 2/2 s390/pci: handle function enumeration after sriov enablement

Implement pcibios_sriov_{add|del}_vfs as empty functions. VF creation will be triggered by the hotplug code.