Re: [PATCH 0/2] sriov enablement on s390

[+cc Arnd, powerpc folks]

On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:

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.

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/20180817102645.3839621-1-arnd@arndb.de

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.

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

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

-- 
2.13.4

On Wed, 2018-09-12 at 08:02 -0500, Bjorn Helgaas wrote:

[+cc Arnd, powerpc folks]

[+Oliver]

On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:

quoted

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.

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/20180817102645.3839621-1-arnd@arndb.de

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.

quoted

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

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

-- 
2.13.4

On Wed, 12 Sep 2018, Bjorn Helgaas wrote:

[+cc Arnd, powerpc folks]

On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:

quoted

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.

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/20180817102645.3839621-1-arnd@arndb.de

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

-- 
2.13.4

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

Signed-off-by: Sebastian Ott <redacted>
---
 drivers/pci/iov.c   | 51 +++++++++++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  2 ++
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 0f04ae648cf1..b2ddfe30c5d8 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c

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

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 680b6bcd0b97..bf99ae98ecb5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -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 */

-- 
2.13.4

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

Signed-off-by: Sebastian Ott <redacted>
---
 arch/s390/pci/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 9381d5d98156..2ba2cbfaa091 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c

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

-- 
2.13.4

Hello Bjorn,

On Wed, 12 Sep 2018, Bjorn Helgaas wrote:

On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:

quoted

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.

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/20180817102645.3839621-1-arnd@arndb.de

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

On Wed, Oct 10, 2018 at 02:55:07PM +0200, Sebastian Ott wrote:

Hello Bjorn,

On Wed, 12 Sep 2018, Bjorn Helgaas wrote:

quoted

On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:

quoted

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.

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/20180817102645.3839621-1-arnd@arndb.de

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.

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/20181002205903.GD120535@bhelgaas-glaptop.roam.corp.google.com

Hello Bjorn,

On Wed, 10 Oct 2018, Bjorn Helgaas wrote:

On Wed, Oct 10, 2018 at 02:55:07PM +0200, Sebastian Ott wrote:

quoted

On Wed, 12 Sep 2018, Bjorn Helgaas wrote:

quoted

On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:

quoted

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.

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/20180817102645.3839621-1-arnd@arndb.de

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

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/20181002205903.GD120535@bhelgaas-glaptop.roam.corp.google.com

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

On Wed, Dec 05, 2018 at 02:45:14PM +0100, Sebastian Ott wrote:

Hello Bjorn,

On Wed, 10 Oct 2018, Bjorn Helgaas wrote:

quoted

On Wed, Oct 10, 2018 at 02:55:07PM +0200, Sebastian Ott wrote:

quoted

On Wed, 12 Sep 2018, Bjorn Helgaas wrote:

quoted

On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:

quoted

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.

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/20180817102645.3839621-1-arnd@arndb.de

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

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/20181002205903.GD120535@bhelgaas-glaptop.roam.corp.google.com

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

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

Signed-off-by: Sebastian Ott <redacted>
---
 drivers/pci/iov.c   | 43 +++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  2 ++
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9616eca3182f..1bfdb4deafd7 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c

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

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 11c71c4ecf75..84ca3bcdac76 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

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

-- 
2.16.4

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

Signed-off-by: Sebastian Ott <redacted>
---
 arch/s390/pci/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 9f6f392a4461..b5f8db652bf5 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c

@@ -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) {

-- 
2.16.4

On Thu, Dec 13, 2018 at 06:54:27PM +0100, Sebastian Ott wrote:

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.

On Thu, Dec 13, 2018 at 06:54:28PM +0100, Sebastian Ott wrote:

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.

On Fri, Dec 14, 2018 at 05:12:45AM -0800, Christoph Hellwig wrote:

On Thu, Dec 13, 2018 at 06:54:28PM +0100, Sebastian Ott wrote:

quoted

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?

On Fri, 14 Dec 2018, Christoph Hellwig wrote:

On Fri, Dec 14, 2018 at 05:12:45AM -0800, Christoph Hellwig wrote:

quoted

On Thu, Dec 13, 2018 at 06:54:28PM +0100, Sebastian Ott wrote:

quoted

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?

Something like this:
https://lore.kernel.org/linux-pci/20181212215453.GJ99796@google.com/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

On Mon, Dec 17, 2018 at 06:30:18PM +0100, Sebastian Ott wrote:

Something like this:
https://lore.kernel.org/linux-pci/20181212215453.GJ99796@google.com/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.

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 <redacted>
---
 drivers/pci/iov.c   | 48 ++++++++++++++++++++++++++++++++++++------------
 include/linux/pci.h |  1 +
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9616eca3182f..3aa115ed3a65 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c

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

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 11c71c4ecf75..f70b9ccd3e86 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -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 */
 

-- 
2.13.4

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 <redacted>
---
 arch/s390/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 9f6f392a4461..4266a4de3160 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c

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

-- 
2.13.4

On Tue, Dec 18, 2018 at 11:16:49AM +0100, Sebastian Ott wrote:

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

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

On Tue, Dec 18, 2018 at 11:16:50AM +0100, Sebastian Ott wrote:

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

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Hi Sebastian,

On Tue, Dec 18, 2018 at 11:16:49AM +0100, Sebastian Ott wrote:

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.

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.

Signed-off-by: Sebastian Ott <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/iov.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9616eca3182f..408db232a328 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c

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

-- 
2.13.4

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 <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/iov.c   | 6 ++++++
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 408db232a328..3aa115ed3a65 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c

@@ -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);
 }

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 11c71c4ecf75..f9bc7651c406 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -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 */
 

-- 
2.13.4

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 <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 arch/s390/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 9f6f392a4461..4266a4de3160 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c

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

-- 
2.13.4

Hello Bjorn,

On Thu, 20 Dec 2018, Bjorn Helgaas wrote:

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

On Fri, Dec 21, 2018 at 03:19:49PM +0100, Sebastian Ott wrote:

Hello Bjorn,

On Thu, 20 Dec 2018, Bjorn Helgaas wrote:

quoted

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.

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