linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs
@ 2016-10-26  1:15 Gavin Shan
  2016-10-26  1:15 ` [PATCH v3 1/2] PCI: Call pcibios_sriov_enable() before IOV BARs are enabled Gavin Shan
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Gavin Shan @ 2016-10-26  1:15 UTC (permalink / raw)
  To: linux-pci; +Cc: linuxppc-dev, bhelgaas, benh, mpe, clsoto, Gavin Shan

This moves pcibios_sriov_enable() to the point before VF and VF BARs
are enabled on PowerNV platform. Also, pci_update_resource() is used
to update IOV BARs on PowerNV platform, the PF might have been functional
when the function is called. We shouldn't disable PF's memory decoding
at that point. Instead, the VF's memory space should be disabled.

Changelog
=========
v3:
  * Disable VF's memory space when IOV BARs are updated in
    pcibios_sriov_enable().
v2:
  * Added one patch calling pcibios_sriov_enable() before the VF
    and VF BARs are enabled.

Gavin Shan (2):
  PCI: Call pcibios_sriov_enable() before IOV BARs are enabled
  PCI: Disable VF's memory space on updating IOV BAR in
    pci_update_resource()

 drivers/pci/iov.c       | 14 +++++++-------
 drivers/pci/setup-res.c | 28 ++++++++++++++++++++--------
 2 files changed, 27 insertions(+), 15 deletions(-)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 1/2] PCI: Call pcibios_sriov_enable() before IOV BARs are enabled
  2016-10-26  1:15 [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Gavin Shan
@ 2016-10-26  1:15 ` Gavin Shan
  2016-10-26  1:15 ` [PATCH v3 2/2] PCI: Disable VF's memory space on updating IOV BAR in pci_update_resource() Gavin Shan
  2016-11-23 23:49 ` [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
  2 siblings, 0 replies; 5+ messages in thread
From: Gavin Shan @ 2016-10-26  1:15 UTC (permalink / raw)
  To: linux-pci; +Cc: linuxppc-dev, bhelgaas, benh, mpe, clsoto, Gavin Shan

In current implementation, pcibios_sriov_enable() is used by PPC
PowerNV platform only. In PowerNV specific pcibios_sriov_enable(),
PF's IOV BARs might be updated (shifted) by pci_update_resource().
It means the IOV BARs aren't ready for decoding incoming memory
address until pcibios_sriov_enable() returns.

This calls pcibios_sriov_enable() earlier before the IOV BARs are
enabled. As the result, the IOV BARs have been configured correctly
when they are enabled.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Tested-by: Carol Soto <clsoto@us.ibm.com>
---
 drivers/pci/iov.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index e30f05c..d41ec29 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -306,13 +306,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 			return rc;
 	}
 
-	pci_iov_set_numvfs(dev, nr_virtfn);
-	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);
-	msleep(100);
-	pci_cfg_access_unlock(dev);
-
 	iov->initial_VFs = initial;
 	if (nr_virtfn < initial)
 		initial = nr_virtfn;
@@ -323,6 +316,13 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 		goto err_pcibios;
 	}
 
+	pci_iov_set_numvfs(dev, nr_virtfn);
+	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);
+	msleep(100);
+	pci_cfg_access_unlock(dev);
+
 	for (i = 0; i < initial; i++) {
 		rc = pci_iov_add_virtfn(dev, i, 0);
 		if (rc)
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 2/2] PCI: Disable VF's memory space on updating IOV BAR in pci_update_resource()
  2016-10-26  1:15 [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Gavin Shan
  2016-10-26  1:15 ` [PATCH v3 1/2] PCI: Call pcibios_sriov_enable() before IOV BARs are enabled Gavin Shan
@ 2016-10-26  1:15 ` Gavin Shan
  2016-11-23 23:49 ` [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
  2 siblings, 0 replies; 5+ messages in thread
From: Gavin Shan @ 2016-10-26  1:15 UTC (permalink / raw)
  To: linux-pci; +Cc: linuxppc-dev, bhelgaas, benh, mpe, clsoto, Gavin Shan

pci_update_resource() might be called to update (shift) IOV BARs
in PPC PowerNV specific pcibios_sriov_enable() when enabling PF's
SRIOV capability. At that point, the PF have been functional if
the SRIOV is enabled through sysfs entry "sriov_numvfs". The PF's
memory decoding (0x2 in PCI_COMMAND) shouldn't be disabled when
updating its IOV BARs with pci_update_resource(). Otherwise, we
receives EEH error caused by MMIO access to PF's memory BARs during
the window when PF's memory decoding is disabled.

   sriov_numvfs_store
   pdev->driver->sriov_configure
   mlx5_core_sriov_configure
   pci_enable_sriov
   sriov_enable
   pcibios_sriov_enable
   pnv_pci_sriov_enable
   pnv_pci_vf_resource_shift
   pci_update_resource

This disables VF's memory space instead of PF's memory decoding
when 64-bits IOV BARs are updated in pci_update_resource().

Reported-by: Carol Soto <clsoto@us.ibm.com>
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Tested-by: Carol Soto <clsoto@us.ibm.com>
---
 drivers/pci/setup-res.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 66c4d8f..1456896 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -29,10 +29,10 @@
 void pci_update_resource(struct pci_dev *dev, int resno)
 {
 	struct pci_bus_region region;
-	bool disable;
-	u16 cmd;
+	bool disable = false;
+	u16 cmd, bit;
 	u32 new, check, mask;
-	int reg;
+	int reg, cmd_reg;
 	enum pci_bar_type type;
 	struct resource *res = dev->resource + resno;
 
@@ -81,11 +81,23 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 	 * disable decoding so that a half-updated BAR won't conflict
 	 * with another device.
 	 */
-	disable = (res->flags & IORESOURCE_MEM_64) && !dev->mmio_always_on;
+	if (res->flags & IORESOURCE_MEM_64) {
+		if (resno <= PCI_ROM_RESOURCE) {
+			disable = !dev->mmio_always_on;
+			cmd_reg = PCI_COMMAND;
+			bit = PCI_COMMAND_MEMORY;
+		} else {
+#ifdef CONFIG_PCI_IOV
+			disable = true;
+			cmd_reg = dev->sriov->pos + PCI_SRIOV_CTRL;
+			bit = PCI_SRIOV_CTRL_MSE;
+#endif
+		}
+	}
+
 	if (disable) {
-		pci_read_config_word(dev, PCI_COMMAND, &cmd);
-		pci_write_config_word(dev, PCI_COMMAND,
-				      cmd & ~PCI_COMMAND_MEMORY);
+		pci_read_config_word(dev, cmd_reg, &cmd);
+		pci_write_config_word(dev, cmd_reg, cmd & ~bit);
 	}
 
 	pci_write_config_dword(dev, reg, new);
@@ -107,7 +119,7 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 	}
 
 	if (disable)
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
+		pci_write_config_word(dev, cmd_reg, cmd);
 }
 
 int pci_claim_resource(struct pci_dev *dev, int resource)
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs
  2016-10-26  1:15 [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Gavin Shan
  2016-10-26  1:15 ` [PATCH v3 1/2] PCI: Call pcibios_sriov_enable() before IOV BARs are enabled Gavin Shan
  2016-10-26  1:15 ` [PATCH v3 2/2] PCI: Disable VF's memory space on updating IOV BAR in pci_update_resource() Gavin Shan
@ 2016-11-23 23:49 ` Bjorn Helgaas
  2016-11-29  3:48   ` Bjorn Helgaas
  2 siblings, 1 reply; 5+ messages in thread
From: Bjorn Helgaas @ 2016-11-23 23:49 UTC (permalink / raw)
  To: Gavin Shan; +Cc: linux-pci, linuxppc-dev, bhelgaas, benh, mpe, clsoto

On Wed, Oct 26, 2016 at 12:15:34PM +1100, Gavin Shan wrote:
> This moves pcibios_sriov_enable() to the point before VF and VF BARs
> are enabled on PowerNV platform. Also, pci_update_resource() is used
> to update IOV BARs on PowerNV platform, the PF might have been functional
> when the function is called. We shouldn't disable PF's memory decoding
> at that point. Instead, the VF's memory space should be disabled.
> 
> Changelog
> =========
> v3:
>   * Disable VF's memory space when IOV BARs are updated in
>     pcibios_sriov_enable().
> v2:
>   * Added one patch calling pcibios_sriov_enable() before the VF
>     and VF BARs are enabled.
> 
> Gavin Shan (2):
>   PCI: Call pcibios_sriov_enable() before IOV BARs are enabled
>   PCI: Disable VF's memory space on updating IOV BAR in
>     pci_update_resource()
> 
>  drivers/pci/iov.c       | 14 +++++++-------
>  drivers/pci/setup-res.c | 28 ++++++++++++++++++++--------
>  2 files changed, 27 insertions(+), 15 deletions(-)

I applied these to pci/virtualization for v4.10.  Thanks for your
patience, Gavin.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs
  2016-11-23 23:49 ` [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
@ 2016-11-29  3:48   ` Bjorn Helgaas
  0 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2016-11-29  3:48 UTC (permalink / raw)
  To: Gavin Shan; +Cc: linux-pci, linuxppc-dev, bhelgaas, benh, mpe, clsoto

On Wed, Nov 23, 2016 at 05:49:10PM -0600, Bjorn Helgaas wrote:
> On Wed, Oct 26, 2016 at 12:15:34PM +1100, Gavin Shan wrote:
> > This moves pcibios_sriov_enable() to the point before VF and VF BARs
> > are enabled on PowerNV platform. Also, pci_update_resource() is used
> > to update IOV BARs on PowerNV platform, the PF might have been functional
> > when the function is called. We shouldn't disable PF's memory decoding
> > at that point. Instead, the VF's memory space should be disabled.
> > 
> > Changelog
> > =========
> > v3:
> >   * Disable VF's memory space when IOV BARs are updated in
> >     pcibios_sriov_enable().
> > v2:
> >   * Added one patch calling pcibios_sriov_enable() before the VF
> >     and VF BARs are enabled.
> > 
> > Gavin Shan (2):
> >   PCI: Call pcibios_sriov_enable() before IOV BARs are enabled
> >   PCI: Disable VF's memory space on updating IOV BAR in
> >     pci_update_resource()
> > 
> >  drivers/pci/iov.c       | 14 +++++++-------
> >  drivers/pci/setup-res.c | 28 ++++++++++++++++++++--------
> >  2 files changed, 27 insertions(+), 15 deletions(-)
> 
> I applied these to pci/virtualization for v4.10.  Thanks for your
> patience, Gavin.

This nagged at me over the holidays, and I think that if the VF has
memory decoding enabled, it's wrong to allow a VF BAR to be updated,
even if we temporarily disable memory decoding.

If the VF is enabled, a driver may have claimed it, and that means the
driver probably has ioremapped the BAR, and if we update the BAR, that
ioremap is now stale.  The driver has no idea that anything changed.

Of course, we may know that pci_update_resource() is never called
after a driver has claimed the VF, but I think it's much better to
restructure the callers so the VF BAR updates happen *before* enabling
the VFs, as you did in patch 1.

Once we have that, there's no point in temporarily disabling memory
decoding, so I worked up some patches to make the update fail.  I'll
post those in a minute and you can see what you think.

Bjorn

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-11-29  3:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-26  1:15 [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Gavin Shan
2016-10-26  1:15 ` [PATCH v3 1/2] PCI: Call pcibios_sriov_enable() before IOV BARs are enabled Gavin Shan
2016-10-26  1:15 ` [PATCH v3 2/2] PCI: Disable VF's memory space on updating IOV BAR in pci_update_resource() Gavin Shan
2016-11-23 23:49 ` [PATCH v3 0/2] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
2016-11-29  3:48   ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).