[PATCH v4 0/7] Disable VF's memory space on updating IOV BARs

[PATCH v4 0/7] Disable VF's memory space on updating IOV BARs

[Bjorn Helgaas]

This is a v4 of Gavin's series for handling VF BAR updates.  The
important piece is the first patch ("PCI: Do any VF BAR updates before
enabling the BARs").  That makes sure that if we update VF BARs, we do
it before enabling the VFs, and that is unchanged from v3.

The second patch in Gavin's series ("PCI: Disable VF's memory space on
updating IOV BAR in pci_update_resource()") temporarily disabled VF
memory space during an update.  Since the first patch does the update
before enabling VFs, this case shouldn't happen in practice.

But even if we want to update a VF BAR while VF memory space is
enabled, I think temporarily disabling it is wrong.  So I replaced
the second patch with a few patches that make that such an update
fail.

Please comment.  These are on my pci/virtualization branch.

Changelog
=========
v4:
  * Don't disable VF's memory space when IOV BARs are updated; fail the
    update instead. 
  * Split IOV BAR updates from standard BAR updates so IOV updates can go
    in pci/iov.c.
  * Remove pci_resource_bar() and pci_iov_resource_bar() (the relevant
    code is simpler when inlined into the callers).
  * Cleanup IORESOURCE_ROM_ENABLE usage.
  * Add comments about why ROMs are updated differently.
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.

---

Bjorn Helgaas (6):
      PCI: Ignore BAR updates on virtual functions
      PCI: Separate VF BAR updates from standard BAR updates
      PCI: Don't update VF BARs while VF memory space is enabled
      PCI: Remove pci_resource_bar() and pci_iov_resource_bar()
      PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE
      PCI: Add comments about ROM BAR updating

Gavin Shan (1):
      PCI: Do any VF BAR updates before enabling the BARs


 drivers/pci/iov.c       |   69 +++++++++++++++++++++++++++++++++++++----------
 drivers/pci/pci.c       |   34 -----------------------
 drivers/pci/pci.h       |    7 +----
 drivers/pci/probe.c     |    3 +-
 drivers/pci/rom.c       |    5 +++
 drivers/pci/setup-res.c |   37 ++++++++++++++++++-------
 6 files changed, 88 insertions(+), 67 deletions(-)

[PATCH v4 1/7] PCI: Do any VF BAR updates before enabling the BARs

[Bjorn Helgaas]

From: Gavin Shan <gwshan@linux.vnet.ibm.com>

Previously we enabled VFs and enable their memory space before calling
pcibios_sriov_enable().  But pcibios_sriov_enable() may update the VF BARs:
for example, on PPC PowerNV we may change them to manage the association of
VFs to PEs.

Because 64-bit BARs cannot be updated atomically, it's unsafe to update
them while they're enabled.  The half-updated state may conflict with other
devices in the system.

Call pcibios_sriov_enable() before enabling the VFs so any BAR updates
happen while the VF BARs are disabled.

[bhelgaas: changelog]
Tested-by: Carol Soto <clsoto@us.ibm.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.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)

[PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions

[Bjorn Helgaas]

VF BARs are read-only zero, so updating VF BARs will not have any effect.
See the SR-IOV spec r1.1, sec 3.4.1.11.

We already ignore these updates because of 70675e0b6a1a ("PCI: Don't try to
restore VF BARs"); this merely restructures it slightly to make it easier
to split updates for standard and SR-IOV BARs.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Wei Yang <richard.weiyang@gmail.com>
---
 drivers/pci/pci.c       |    4 ----
 drivers/pci/setup-res.c |    5 ++---
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ba34907..631eac2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -564,10 +564,6 @@ static void pci_restore_bars(struct pci_dev *dev)
 {
 	int i;
 
-	/* Per SR-IOV spec 3.4.1.11, VF BARs are RO zero */
-	if (dev->is_virtfn)
-		return;
-
 	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
 		pci_update_resource(dev, i);
 }
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 66c4d8f..d2a32d8 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -36,10 +36,9 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 	enum pci_bar_type type;
 	struct resource *res = dev->resource + resno;
 
-	if (dev->is_virtfn) {
-		dev_warn(&dev->dev, "can't update VF BAR%d\n", resno);
+	/* Per SR-IOV spec 3.4.1.11, VF BARs are RO zero */
+	if (dev->is_virtfn)
 		return;
-	}
 
 	/*
 	 * Ignore resources for unimplemented BARs and unused resource slots

Re: [PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions

[Gavin Shan]

On Mon, Nov 28, 2016 at 10:14:29PM -0600, Bjorn Helgaas wrote:
>VF BARs are read-only zero, so updating VF BARs will not have any effect.
>See the SR-IOV spec r1.1, sec 3.4.1.11.
>
>We already ignore these updates because of 70675e0b6a1a ("PCI: Don't try to
>restore VF BARs"); this merely restructures it slightly to make it easier
>to split updates for standard and SR-IOV BARs.
>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>CC: Wei Yang <richard.weiyang@gmail.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/pci.c       |    4 ----
> drivers/pci/setup-res.c |    5 ++---
> 2 files changed, 2 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index ba34907..631eac2 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -564,10 +564,6 @@ static void pci_restore_bars(struct pci_dev *dev)
> {
> 	int i;
>
>-	/* Per SR-IOV spec 3.4.1.11, VF BARs are RO zero */
>-	if (dev->is_virtfn)
>-		return;
>-
> 	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
> 		pci_update_resource(dev, i);
> }
>diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>index 66c4d8f..d2a32d8 100644
>--- a/drivers/pci/setup-res.c
>+++ b/drivers/pci/setup-res.c
>@@ -36,10 +36,9 @@ void pci_update_resource(struct pci_dev *dev, int resno)
> 	enum pci_bar_type type;
> 	struct resource *res = dev->resource + resno;
>
>-	if (dev->is_virtfn) {
>-		dev_warn(&dev->dev, "can't update VF BAR%d\n", resno);
>+	/* Per SR-IOV spec 3.4.1.11, VF BARs are RO zero */
>+	if (dev->is_virtfn)
> 		return;
>-	}
>
> 	/*
> 	 * Ignore resources for unimplemented BARs and unused resource slots
>

[PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Bjorn Helgaas]

Previously pci_update_resource() used the same code path for updating
standard BARs and VF BARs in SR-IOV capabilities.

Split the VF BAR update into a new pci_iov_update_resource() internal
interface, which makes it simpler to compute the BAR address (we can get
rid of pci_resource_bar() and pci_iov_resource_bar()).

This patch:

  - Renames pci_update_resource() to pci_std_update_resource(),
  - Adds pci_iov_update_resource(),
  - Makes pci_update_resource() a wrapper that calls the appropriate one,

No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c       |   49 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h       |    1 +
 drivers/pci/setup-res.c |   13 +++++++++++-
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index d41ec29..d00ed5c 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -571,6 +571,55 @@ int pci_iov_resource_bar(struct pci_dev *dev, int resno)
 		4 * (resno - PCI_IOV_RESOURCES);
 }
 
+/**
+ * pci_iov_update_resource - update a VF BAR
+ * @dev: the PCI device
+ * @resno: the resource number
+ *
+ * Update a VF BAR in the SR-IOV capability of a PF.
+ */
+void pci_iov_update_resource(struct pci_dev *dev, int resno)
+{
+	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
+	struct resource *res = dev->resource + resno;
+	int vf_bar = resno - PCI_IOV_RESOURCES;
+	struct pci_bus_region region;
+	u32 new;
+	int reg;
+
+	/*
+	 * The generic pci_restore_bars() path calls this for all devices,
+	 * including VFs and non-SR-IOV devices.  If this is not a PF, we
+	 * have nothing to do.
+	 */
+	if (!iov)
+		return;
+
+	/*
+	 * Ignore unimplemented BARs, unused resource slots for 64-bit
+	 * BARs, and non-movable resources, e.g., those described via
+	 * Enhanced Allocation.
+	 */
+	if (!res->flags)
+		return;
+
+	if (res->flags & IORESOURCE_UNSET)
+		return;
+
+	if (res->flags & IORESOURCE_PCI_FIXED)
+		return;
+
+	pcibios_resource_to_bus(dev->bus, &region, res);
+	new = region.start;
+
+	reg = iov->pos + PCI_SRIOV_BAR + 4 * vf_bar;
+	pci_write_config_dword(dev, reg, new);
+	if (res->flags & IORESOURCE_MEM_64) {
+		new = region.start >> 16 >> 16;
+		pci_write_config_dword(dev, reg + 4, new);
+	}
+}
+
 resource_size_t __weak pcibios_iov_resource_alignment(struct pci_dev *dev,
 						      int resno)
 {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4518562..5bfcb92 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -290,6 +290,7 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
 int pci_iov_init(struct pci_dev *dev);
 void pci_iov_release(struct pci_dev *dev);
 int pci_iov_resource_bar(struct pci_dev *dev, int resno);
+void pci_iov_update_resource(struct pci_dev *dev, int resno);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index d2a32d8..ee0be34 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -25,8 +25,7 @@
 #include <linux/slab.h>
 #include "pci.h"
 
-
-void pci_update_resource(struct pci_dev *dev, int resno)
+static void pci_std_update_resource(struct pci_dev *dev, int resno)
 {
 	struct pci_bus_region region;
 	bool disable;
@@ -109,6 +108,16 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 		pci_write_config_word(dev, PCI_COMMAND, cmd);
 }
 
+void pci_update_resource(struct pci_dev *dev, int resno)
+{
+	if (resno <= PCI_ROM_RESOURCE)
+		pci_std_update_resource(dev, resno);
+#ifdef CONFIG_PCI_IOV
+	else if (resno >= PCI_IOV_RESOURCES && resno < PCI_IOV_RESOURCE_END)
+		pci_iov_update_resource(dev, resno);
+#endif
+}
+
 int pci_claim_resource(struct pci_dev *dev, int resource)
 {
 	struct resource *res = &dev->resource[resource];

Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Gavin Shan]

On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
>Previously pci_update_resource() used the same code path for updating
>standard BARs and VF BARs in SR-IOV capabilities.
>
>Split the VF BAR update into a new pci_iov_update_resource() internal
>interface, which makes it simpler to compute the BAR address (we can get
>rid of pci_resource_bar() and pci_iov_resource_bar()).
>
>This patch:
>
>  - Renames pci_update_resource() to pci_std_update_resource(),
>  - Adds pci_iov_update_resource(),
>  - Makes pci_update_resource() a wrapper that calls the appropriate one,
>
>No functional change intended.
>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

With below minor comments fixed:

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c       |   49 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.h       |    1 +
> drivers/pci/setup-res.c |   13 +++++++++++-
> 3 files changed, 61 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index d41ec29..d00ed5c 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -571,6 +571,55 @@ int pci_iov_resource_bar(struct pci_dev *dev, int resno)
> 		4 * (resno - PCI_IOV_RESOURCES);
> }
>
>+/**
>+ * pci_iov_update_resource - update a VF BAR
>+ * @dev: the PCI device
>+ * @resno: the resource number
>+ *
>+ * Update a VF BAR in the SR-IOV capability of a PF.
>+ */
>+void pci_iov_update_resource(struct pci_dev *dev, int resno)
>+{
>+	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
>+	struct resource *res = dev->resource + resno;
>+	int vf_bar = resno - PCI_IOV_RESOURCES;
>+	struct pci_bus_region region;
>+	u32 new;
>+	int reg;
>+
>+	/*
>+	 * The generic pci_restore_bars() path calls this for all devices,
>+	 * including VFs and non-SR-IOV devices.  If this is not a PF, we
>+	 * have nothing to do.
>+	 */
>+	if (!iov)
>+		return;
>+
>+	/*
>+	 * Ignore unimplemented BARs, unused resource slots for 64-bit
>+	 * BARs, and non-movable resources, e.g., those described via
>+	 * Enhanced Allocation.
>+	 */
>+	if (!res->flags)
>+		return;
>+
>+	if (res->flags & IORESOURCE_UNSET)
>+		return;
>+
>+	if (res->flags & IORESOURCE_PCI_FIXED)
>+		return;
>+
>+	pcibios_resource_to_bus(dev->bus, &region, res);
>+	new = region.start;
>+

The bits indicating the BAR's property (e.g. memory, IO etc) are missed in @new.

>+	reg = iov->pos + PCI_SRIOV_BAR + 4 * vf_bar;
>+	pci_write_config_dword(dev, reg, new);
>+	if (res->flags & IORESOURCE_MEM_64) {
>+		new = region.start >> 16 >> 16;

I think it was copied from pci_update_resource(). Why we can't just have "new = region.start >> 32"? 

>+		pci_write_config_dword(dev, reg + 4, new);
>+	}
>+}
>+
> resource_size_t __weak pcibios_iov_resource_alignment(struct pci_dev *dev,
> 						      int resno)
> {
>diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>index 4518562..5bfcb92 100644
>--- a/drivers/pci/pci.h
>+++ b/drivers/pci/pci.h
>@@ -290,6 +290,7 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
> int pci_iov_init(struct pci_dev *dev);
> void pci_iov_release(struct pci_dev *dev);
> int pci_iov_resource_bar(struct pci_dev *dev, int resno);
>+void pci_iov_update_resource(struct pci_dev *dev, int resno);
> resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
> void pci_restore_iov_state(struct pci_dev *dev);
> int pci_iov_bus_range(struct pci_bus *bus);
>diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>index d2a32d8..ee0be34 100644
>--- a/drivers/pci/setup-res.c
>+++ b/drivers/pci/setup-res.c
>@@ -25,8 +25,7 @@
> #include <linux/slab.h>
> #include "pci.h"
>
>-
>-void pci_update_resource(struct pci_dev *dev, int resno)
>+static void pci_std_update_resource(struct pci_dev *dev, int resno)
> {
> 	struct pci_bus_region region;
> 	bool disable;
>@@ -109,6 +108,16 @@ void pci_update_resource(struct pci_dev *dev, int resno)
> 		pci_write_config_word(dev, PCI_COMMAND, cmd);
> }
>
>+void pci_update_resource(struct pci_dev *dev, int resno)
>+{
>+	if (resno <= PCI_ROM_RESOURCE)
>+		pci_std_update_resource(dev, resno);
>+#ifdef CONFIG_PCI_IOV
>+	else if (resno >= PCI_IOV_RESOURCES && resno < PCI_IOV_RESOURCE_END)

The last BAR is missed:

	else if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)

>+		pci_iov_update_resource(dev, resno);
>+#endif
>+}
>+
> int pci_claim_resource(struct pci_dev *dev, int resource)
> {
> 	struct resource *res = &dev->resource[resource];
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Bjorn Helgaas]

On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:
> On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
> >Previously pci_update_resource() used the same code path for updating
> >standard BARs and VF BARs in SR-IOV capabilities.
> >
> >Split the VF BAR update into a new pci_iov_update_resource() internal
> >interface, which makes it simpler to compute the BAR address (we can get
> >rid of pci_resource_bar() and pci_iov_resource_bar()).
> >
> >This patch:
> >
> >  - Renames pci_update_resource() to pci_std_update_resource(),
> >  - Adds pci_iov_update_resource(),
> >  - Makes pci_update_resource() a wrapper that calls the appropriate one,
> >
> >No functional change intended.
> >
> >Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> With below minor comments fixed:
> 
> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> 
> >---
> > drivers/pci/iov.c       |   49 +++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/pci/pci.h       |    1 +
> > drivers/pci/setup-res.c |   13 +++++++++++-
> > 3 files changed, 61 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >index d41ec29..d00ed5c 100644
> >--- a/drivers/pci/iov.c
> >+++ b/drivers/pci/iov.c
> >@@ -571,6 +571,55 @@ int pci_iov_resource_bar(struct pci_dev *dev, int resno)
> > 		4 * (resno - PCI_IOV_RESOURCES);
> > }
> >
> >+/**
> >+ * pci_iov_update_resource - update a VF BAR
> >+ * @dev: the PCI device
> >+ * @resno: the resource number
> >+ *
> >+ * Update a VF BAR in the SR-IOV capability of a PF.
> >+ */
> >+void pci_iov_update_resource(struct pci_dev *dev, int resno)
> >+{
> >+	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
> >+	struct resource *res = dev->resource + resno;
> >+	int vf_bar = resno - PCI_IOV_RESOURCES;
> >+	struct pci_bus_region region;
> >+	u32 new;
> >+	int reg;
> >+
> >+	/*
> >+	 * The generic pci_restore_bars() path calls this for all devices,
> >+	 * including VFs and non-SR-IOV devices.  If this is not a PF, we
> >+	 * have nothing to do.
> >+	 */
> >+	if (!iov)
> >+		return;
> >+
> >+	/*
> >+	 * Ignore unimplemented BARs, unused resource slots for 64-bit
> >+	 * BARs, and non-movable resources, e.g., those described via
> >+	 * Enhanced Allocation.
> >+	 */
> >+	if (!res->flags)
> >+		return;
> >+
> >+	if (res->flags & IORESOURCE_UNSET)
> >+		return;
> >+
> >+	if (res->flags & IORESOURCE_PCI_FIXED)
> >+		return;
> >+
> >+	pcibios_resource_to_bus(dev->bus, &region, res);
> >+	new = region.start;
> >+
> 
> The bits indicating the BAR's property (e.g. memory, IO etc) are missed in @new.

Hmm, yes.  I omitted those because those bits are supposed to be
read-only, per spec (PCI r3.0, sec 6.2.5.1).  But I guess it would be
more conservative to keep them, and this shouldn't be needlessly
different from pci_std_update_resource().

However, I don't think this code in pci_update_resource() is obviously
correct:

  new = region.start | (res->flags & PCI_REGION_FLAG_MASK);

PCI_REGION_FLAG_MASK is 0xf.  For memory BARs, bits 0-3 are read-only
property bits.  For I/O BARs, bits 0-1 are read-only and bits 2-3 are
part of the address, so on the face of it, the above could corrupt two
bits of an I/O address.

It's true that decode_bar() initializes flags correctly, using
PCI_BASE_ADDRESS_IO_MASK for I/O BARs and PCI_BASE_ADDRESS_MEM_MASK
for memory BARs, but it would take a little more digging to be sure
that we never set bits 2-3 of flags for an I/O resource elsewhere.

How about this in pci_std_update_resource():

        pcibios_resource_to_bus(dev->bus, &region, res);
        new = region.start;

        if (res->flags & IORESOURCE_IO) {
                mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
                new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
        } else {
                mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
                new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
        }

and this in pci_iov_update_resource():

        pcibios_resource_to_bus(dev->bus, &region, res);
        new = region.start;
        new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;

It shouldn't fix anything, but I think it is more obvious that we
can't corrupt bits 2-3 of an I/O BAR.

> >+	reg = iov->pos + PCI_SRIOV_BAR + 4 * vf_bar;
> >+	pci_write_config_dword(dev, reg, new);
> >+	if (res->flags & IORESOURCE_MEM_64) {
> >+		new = region.start >> 16 >> 16;
> 
> I think it was copied from pci_update_resource(). Why we can't just have "new = region.start >> 32"? 

Right; I did copy this from pci_update_resource().  The changelog from
cf7bee5a0bf2 ("[PATCH] Fix restore of 64-bit PCI BAR's") says "Also
make sure to write high bits - use "x >> 16 >> 16" (rather than the
simpler ">> 32") to avoid warnings on 32-bit architectures where we're
not going to have any high bits."

I didn't take the time to revalidate whether that's still applicable.

> >+void pci_update_resource(struct pci_dev *dev, int resno)
> >+{
> >+	if (resno <= PCI_ROM_RESOURCE)
> >+		pci_std_update_resource(dev, resno);
> >+#ifdef CONFIG_PCI_IOV
> >+	else if (resno >= PCI_IOV_RESOURCES && resno < PCI_IOV_RESOURCE_END)
> 
> The last BAR is missed:
> 
> 	else if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)

Ah, right, thanks!

> >+		pci_iov_update_resource(dev, resno);

Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Gavin Shan]

On Tue, Nov 29, 2016 at 08:48:26AM -0600, Bjorn Helgaas wrote:
>On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:
>> On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
>> >Previously pci_update_resource() used the same code path for updating
>> >standard BARs and VF BARs in SR-IOV capabilities.
>> >
>> >Split the VF BAR update into a new pci_iov_update_resource() internal
>> >interface, which makes it simpler to compute the BAR address (we can get
>> >rid of pci_resource_bar() and pci_iov_resource_bar()).
>> >
>> >This patch:
>> >
>> >  - Renames pci_update_resource() to pci_std_update_resource(),
>> >  - Adds pci_iov_update_resource(),
>> >  - Makes pci_update_resource() a wrapper that calls the appropriate one,
>> >
>> >No functional change intended.
>> >
>> >Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> 
>> With below minor comments fixed:
>> 
>> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> 
>> >---
>> > drivers/pci/iov.c       |   49 +++++++++++++++++++++++++++++++++++++++++++++++
>> > drivers/pci/pci.h       |    1 +
>> > drivers/pci/setup-res.c |   13 +++++++++++-
>> > 3 files changed, 61 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> >index d41ec29..d00ed5c 100644
>> >--- a/drivers/pci/iov.c
>> >+++ b/drivers/pci/iov.c
>> >@@ -571,6 +571,55 @@ int pci_iov_resource_bar(struct pci_dev *dev, int resno)
>> > 		4 * (resno - PCI_IOV_RESOURCES);
>> > }
>> >
>> >+/**
>> >+ * pci_iov_update_resource - update a VF BAR
>> >+ * @dev: the PCI device
>> >+ * @resno: the resource number
>> >+ *
>> >+ * Update a VF BAR in the SR-IOV capability of a PF.
>> >+ */
>> >+void pci_iov_update_resource(struct pci_dev *dev, int resno)
>> >+{
>> >+	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
>> >+	struct resource *res = dev->resource + resno;
>> >+	int vf_bar = resno - PCI_IOV_RESOURCES;
>> >+	struct pci_bus_region region;
>> >+	u32 new;
>> >+	int reg;
>> >+
>> >+	/*
>> >+	 * The generic pci_restore_bars() path calls this for all devices,
>> >+	 * including VFs and non-SR-IOV devices.  If this is not a PF, we
>> >+	 * have nothing to do.
>> >+	 */
>> >+	if (!iov)
>> >+		return;
>> >+
>> >+	/*
>> >+	 * Ignore unimplemented BARs, unused resource slots for 64-bit
>> >+	 * BARs, and non-movable resources, e.g., those described via
>> >+	 * Enhanced Allocation.
>> >+	 */
>> >+	if (!res->flags)
>> >+		return;
>> >+
>> >+	if (res->flags & IORESOURCE_UNSET)
>> >+		return;
>> >+
>> >+	if (res->flags & IORESOURCE_PCI_FIXED)
>> >+		return;
>> >+
>> >+	pcibios_resource_to_bus(dev->bus, &region, res);
>> >+	new = region.start;
>> >+
>> 
>> The bits indicating the BAR's property (e.g. memory, IO etc) are missed in @new.
>
>Hmm, yes.  I omitted those because those bits are supposed to be
>read-only, per spec (PCI r3.0, sec 6.2.5.1).  But I guess it would be
>more conservative to keep them, and this shouldn't be needlessly
>different from pci_std_update_resource().
>

Yeah, Agree.

>However, I don't think this code in pci_update_resource() is obviously
>correct:
>
>  new = region.start | (res->flags & PCI_REGION_FLAG_MASK);
>
>PCI_REGION_FLAG_MASK is 0xf.  For memory BARs, bits 0-3 are read-only
>property bits.  For I/O BARs, bits 0-1 are read-only and bits 2-3 are
>part of the address, so on the face of it, the above could corrupt two
>bits of an I/O address.
>
>It's true that decode_bar() initializes flags correctly, using
>PCI_BASE_ADDRESS_IO_MASK for I/O BARs and PCI_BASE_ADDRESS_MEM_MASK
>for memory BARs, but it would take a little more digging to be sure
>that we never set bits 2-3 of flags for an I/O resource elsewhere.
>

The BAR's property bits are probed from device-tree, not hardware
on some platforms (e.g. pSeries). Also, there is only one (property)
bit if it's a ROM BAR. So more check as below might be needed because
the code (without the enhancement) should also work fine.

>How about this in pci_std_update_resource():
>
>        pcibios_resource_to_bus(dev->bus, &region, res);
>        new = region.start;
>
>        if (res->flags & IORESOURCE_IO) {
>                mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
>                new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
>        } else {
>                mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
>                new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
>        }
>

	if (res->flags & IORESOURCE_IO) {
		mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
		new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
	} else if (resno < PCI_ROM_RESOURCE) {
		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
		new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
	} else if (resno == PCI_ROM_RESOURCE) {
		mask = ~((u32)IORESOURCE_ROM_ENABLE);
		new |= res->flags & IORESOURCE_ROM_ENABLE);
	} else {
		dev_warn(&dev->dev, "BAR#%d out of range\n", resno);
		return;
	}


>and this in pci_iov_update_resource():
>
>        pcibios_resource_to_bus(dev->bus, &region, res);
>        new = region.start;
>        new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
>
>It shouldn't fix anything, but I think it is more obvious that we
>can't corrupt bits 2-3 of an I/O BAR.
>

Agree and the this part of changes look good to me.

>> >+	reg = iov->pos + PCI_SRIOV_BAR + 4 * vf_bar;
>> >+	pci_write_config_dword(dev, reg, new);
>> >+	if (res->flags & IORESOURCE_MEM_64) {
>> >+		new = region.start >> 16 >> 16;
>> 
>> I think it was copied from pci_update_resource(). Why we can't just have "new = region.start >> 32"? 
>
>Right; I did copy this from pci_update_resource().  The changelog from
>cf7bee5a0bf2 ("[PATCH] Fix restore of 64-bit PCI BAR's") says "Also
>make sure to write high bits - use "x >> 16 >> 16" (rather than the
>simpler ">> 32") to avoid warnings on 32-bit architectures where we're
>not going to have any high bits."
>
>I didn't take the time to revalidate whether that's still applicable.
>

Ah, I see. I think we still need this on 32-bits systems.

>> >+void pci_update_resource(struct pci_dev *dev, int resno)
>> >+{
>> >+	if (resno <= PCI_ROM_RESOURCE)
>> >+		pci_std_update_resource(dev, resno);
>> >+#ifdef CONFIG_PCI_IOV
>> >+	else if (resno >= PCI_IOV_RESOURCES && resno < PCI_IOV_RESOURCE_END)
>> 
>> The last BAR is missed:
>> 
>> 	else if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
>
>Ah, right, thanks!
>
>> >+		pci_iov_update_resource(dev, resno);
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Bjorn Helgaas]

On Wed, Nov 30, 2016 at 10:20:28AM +1100, Gavin Shan wrote:
> On Tue, Nov 29, 2016 at 08:48:26AM -0600, Bjorn Helgaas wrote:
> >On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:
> >> On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
> >> >Previously pci_update_resource() used the same code path for updating
> >> >standard BARs and VF BARs in SR-IOV capabilities.
> >> >
> >> >Split the VF BAR update into a new pci_iov_update_resource() internal
> >> >interface, which makes it simpler to compute the BAR address (we can get
> >> >rid of pci_resource_bar() and pci_iov_resource_bar()).
> >> >
> >> >This patch:
> >> >
> >> >  - Renames pci_update_resource() to pci_std_update_resource(),
> >> >  - Adds pci_iov_update_resource(),
> >> >  - Makes pci_update_resource() a wrapper that calls the appropriate one,
> >> >
> >> >No functional change intended.

> >However, I don't think this code in pci_update_resource() is obviously
> >correct:
> >
> >  new = region.start | (res->flags & PCI_REGION_FLAG_MASK);
> >
> >PCI_REGION_FLAG_MASK is 0xf.  For memory BARs, bits 0-3 are read-only
> >property bits.  For I/O BARs, bits 0-1 are read-only and bits 2-3 are
> >part of the address, so on the face of it, the above could corrupt two
> >bits of an I/O address.
> >
> >It's true that decode_bar() initializes flags correctly, using
> >PCI_BASE_ADDRESS_IO_MASK for I/O BARs and PCI_BASE_ADDRESS_MEM_MASK
> >for memory BARs, but it would take a little more digging to be sure
> >that we never set bits 2-3 of flags for an I/O resource elsewhere.
> >
> 
> The BAR's property bits are probed from device-tree, not hardware
> on some platforms (e.g. pSeries). Also, there is only one (property)
> bit if it's a ROM BAR. So more check as below might be needed because
> the code (without the enhancement) should also work fine.

Ah, right, I forgot about that.  I didn't do enough digging :)

> >How about this in pci_std_update_resource():
> >
> >        pcibios_resource_to_bus(dev->bus, &region, res);
> >        new = region.start;
> >
> >        if (res->flags & IORESOURCE_IO) {
> >                mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
> >                new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
> >        } else {
> >                mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
> >                new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
> >        }
> >
> 
> 	if (res->flags & IORESOURCE_IO) {
> 		mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
> 		new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
> 	} else if (resno < PCI_ROM_RESOURCE) {
> 		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
> 		new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
> 	} else if (resno == PCI_ROM_RESOURCE) {
> 		mask = ~((u32)IORESOURCE_ROM_ENABLE);
> 		new |= res->flags & IORESOURCE_ROM_ENABLE);
> 	} else {
> 		dev_warn(&dev->dev, "BAR#%d out of range\n", resno);
> 		return;
> 	}

After this patch, the only thing we OR into a ROM BAR value is
PCI_ROM_ADDRESS_ENABLE, and that's done below, only if the ROM is
already enabled.

I did update the ROM mask (to PCI_ROM_ADDRESS_MASK).  I'm not 100%
sure about doing that -- it follows the spec, but it is a change from
what we've been doing before.  I guess it should be safe because it
means we're checking fewer bits than before (only the top 21 bits for
ROMs, where we used check the top 28), so the only possible difference
is that we might not warn about "error updating" in some case where we
used to.

I'm not really sure about the value of the "error updating" checks to
begin with, though I guess it does help us find broken devices that
put non-BARs where BARs are supposed to be.

Bjorn

Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Gavin Shan]

On Tue, Nov 29, 2016 at 06:06:05PM -0600, Bjorn Helgaas wrote:
>On Wed, Nov 30, 2016 at 10:20:28AM +1100, Gavin Shan wrote:
>> On Tue, Nov 29, 2016 at 08:48:26AM -0600, Bjorn Helgaas wrote:
>> >On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:
>> >> On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
>> >> >Previously pci_update_resource() used the same code path for updating
>> >> >standard BARs and VF BARs in SR-IOV capabilities.
>> >> >
>> >> >Split the VF BAR update into a new pci_iov_update_resource() internal
>> >> >interface, which makes it simpler to compute the BAR address (we can get
>> >> >rid of pci_resource_bar() and pci_iov_resource_bar()).
>> >> >
>> >> >This patch:
>> >> >
>> >> >  - Renames pci_update_resource() to pci_std_update_resource(),
>> >> >  - Adds pci_iov_update_resource(),
>> >> >  - Makes pci_update_resource() a wrapper that calls the appropriate one,
>> >> >
>> >> >No functional change intended.
>
>> >However, I don't think this code in pci_update_resource() is obviously
>> >correct:
>> >
>> >  new = region.start | (res->flags & PCI_REGION_FLAG_MASK);
>> >
>> >PCI_REGION_FLAG_MASK is 0xf.  For memory BARs, bits 0-3 are read-only
>> >property bits.  For I/O BARs, bits 0-1 are read-only and bits 2-3 are
>> >part of the address, so on the face of it, the above could corrupt two
>> >bits of an I/O address.
>> >
>> >It's true that decode_bar() initializes flags correctly, using
>> >PCI_BASE_ADDRESS_IO_MASK for I/O BARs and PCI_BASE_ADDRESS_MEM_MASK
>> >for memory BARs, but it would take a little more digging to be sure
>> >that we never set bits 2-3 of flags for an I/O resource elsewhere.
>> >
>> 
>> The BAR's property bits are probed from device-tree, not hardware
>> on some platforms (e.g. pSeries). Also, there is only one (property)
>> bit if it's a ROM BAR. So more check as below might be needed because
>> the code (without the enhancement) should also work fine.
>
>Ah, right, I forgot about that.  I didn't do enough digging :)
>
>> >How about this in pci_std_update_resource():
>> >
>> >        pcibios_resource_to_bus(dev->bus, &region, res);
>> >        new = region.start;
>> >
>> >        if (res->flags & IORESOURCE_IO) {
>> >                mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
>> >                new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
>> >        } else {
>> >                mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
>> >                new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
>> >        }
>> >
>> 
>> 	if (res->flags & IORESOURCE_IO) {
>> 		mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
>> 		new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
>> 	} else if (resno < PCI_ROM_RESOURCE) {
>> 		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
>> 		new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
>> 	} else if (resno == PCI_ROM_RESOURCE) {
>> 		mask = ~((u32)IORESOURCE_ROM_ENABLE);
>> 		new |= res->flags & IORESOURCE_ROM_ENABLE);
>> 	} else {
>> 		dev_warn(&dev->dev, "BAR#%d out of range\n", resno);
>> 		return;
>> 	}
>
>After this patch, the only thing we OR into a ROM BAR value is
>PCI_ROM_ADDRESS_ENABLE, and that's done below, only if the ROM is
>already enabled.
>
>I did update the ROM mask (to PCI_ROM_ADDRESS_MASK).  I'm not 100%
>sure about doing that -- it follows the spec, but it is a change from
>what we've been doing before.  I guess it should be safe because it
>means we're checking fewer bits than before (only the top 21 bits for
>ROMs, where we used check the top 28), so the only possible difference
>is that we might not warn about "error updating" in some case where we
>used to.
>
>I'm not really sure about the value of the "error updating" checks to
>begin with, though I guess it does help us find broken devices that
>put non-BARs where BARs are supposed to be.
>

Yeah, agree. Bjorn, I don't have more comments. please take your time
to respin the series and maybe applied it. I really want to see the
fixes can be in 4.10 if possible :-)

Thanks,
Gavin

Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 10:02:54AM +1100, Gavin Shan wrote:
> On Tue, Nov 29, 2016 at 06:06:05PM -0600, Bjorn Helgaas wrote:
> >On Wed, Nov 30, 2016 at 10:20:28AM +1100, Gavin Shan wrote:
> >> On Tue, Nov 29, 2016 at 08:48:26AM -0600, Bjorn Helgaas wrote:
> >> >On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:
> >> >> On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
> >> >> >Previously pci_update_resource() used the same code path for updating
> >> >> >standard BARs and VF BARs in SR-IOV capabilities.
> >> >> >
> >> >> >Split the VF BAR update into a new pci_iov_update_resource() internal
> >> >> >interface, which makes it simpler to compute the BAR address (we can get
> >> >> >rid of pci_resource_bar() and pci_iov_resource_bar()).
> >> >> >
> >> >> >This patch:
> >> >> >
> >> >> >  - Renames pci_update_resource() to pci_std_update_resource(),
> >> >> >  - Adds pci_iov_update_resource(),
> >> >> >  - Makes pci_update_resource() a wrapper that calls the appropriate one,
> >> >> >
> >> >> >No functional change intended.
> >
> >> >However, I don't think this code in pci_update_resource() is obviously
> >> >correct:
> >> >
> >> >  new = region.start | (res->flags & PCI_REGION_FLAG_MASK);
> >> >
> >> >PCI_REGION_FLAG_MASK is 0xf.  For memory BARs, bits 0-3 are read-only
> >> >property bits.  For I/O BARs, bits 0-1 are read-only and bits 2-3 are
> >> >part of the address, so on the face of it, the above could corrupt two
> >> >bits of an I/O address.
> >> >
> >> >It's true that decode_bar() initializes flags correctly, using
> >> >PCI_BASE_ADDRESS_IO_MASK for I/O BARs and PCI_BASE_ADDRESS_MEM_MASK
> >> >for memory BARs, but it would take a little more digging to be sure
> >> >that we never set bits 2-3 of flags for an I/O resource elsewhere.
> >> >
> >> 
> >> The BAR's property bits are probed from device-tree, not hardware
> >> on some platforms (e.g. pSeries). Also, there is only one (property)
> >> bit if it's a ROM BAR. So more check as below might be needed because
> >> the code (without the enhancement) should also work fine.
> >
> >Ah, right, I forgot about that.  I didn't do enough digging :)
> >
> >> >How about this in pci_std_update_resource():
> >> >
> >> >        pcibios_resource_to_bus(dev->bus, &region, res);
> >> >        new = region.start;
> >> >
> >> >        if (res->flags & IORESOURCE_IO) {
> >> >                mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
> >> >                new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
> >> >        } else {
> >> >                mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
> >> >                new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
> >> >        }
> >> >
> >> 
> >> 	if (res->flags & IORESOURCE_IO) {
> >> 		mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
> >> 		new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
> >> 	} else if (resno < PCI_ROM_RESOURCE) {
> >> 		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
> >> 		new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
> >> 	} else if (resno == PCI_ROM_RESOURCE) {
> >> 		mask = ~((u32)IORESOURCE_ROM_ENABLE);
> >> 		new |= res->flags & IORESOURCE_ROM_ENABLE);
> >> 	} else {
> >> 		dev_warn(&dev->dev, "BAR#%d out of range\n", resno);
> >> 		return;
> >> 	}
> >
> >After this patch, the only thing we OR into a ROM BAR value is
> >PCI_ROM_ADDRESS_ENABLE, and that's done below, only if the ROM is
> >already enabled.
> >
> >I did update the ROM mask (to PCI_ROM_ADDRESS_MASK).  I'm not 100%
> >sure about doing that -- it follows the spec, but it is a change from
> >what we've been doing before.  I guess it should be safe because it
> >means we're checking fewer bits than before (only the top 21 bits for
> >ROMs, where we used check the top 28), so the only possible difference
> >is that we might not warn about "error updating" in some case where we
> >used to.
> >
> >I'm not really sure about the value of the "error updating" checks to
> >begin with, though I guess it does help us find broken devices that
> >put non-BARs where BARs are supposed to be.
> >
> 
> Yeah, agree. Bjorn, I don't have more comments. please take your time
> to respin the series and maybe applied it. I really want to see the
> fixes can be in 4.10 if possible :-)

These will definitely be in v4.10.  Thanks for all your help!

Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

[Gavin Shan]

On Wed, Nov 30, 2016 at 05:45:18PM -0600, Bjorn Helgaas wrote:
>On Thu, Dec 01, 2016 at 10:02:54AM +1100, Gavin Shan wrote:
>> On Tue, Nov 29, 2016 at 06:06:05PM -0600, Bjorn Helgaas wrote:
>> >On Wed, Nov 30, 2016 at 10:20:28AM +1100, Gavin Shan wrote:
>> >> On Tue, Nov 29, 2016 at 08:48:26AM -0600, Bjorn Helgaas wrote:
>> >> >On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:
>> >> >> On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
>> >> >> >Previously pci_update_resource() used the same code path for updating
>> >> >> >standard BARs and VF BARs in SR-IOV capabilities.
>> >> >> >
>> >> >> >Split the VF BAR update into a new pci_iov_update_resource() internal
>> >> >> >interface, which makes it simpler to compute the BAR address (we can get
>> >> >> >rid of pci_resource_bar() and pci_iov_resource_bar()).
>> >> >> >
>> >> >> >This patch:
>> >> >> >
>> >> >> >  - Renames pci_update_resource() to pci_std_update_resource(),
>> >> >> >  - Adds pci_iov_update_resource(),
>> >> >> >  - Makes pci_update_resource() a wrapper that calls the appropriate one,
>> >> >> >
>> >> >> >No functional change intended.
>> >
>> >> >However, I don't think this code in pci_update_resource() is obviously
>> >> >correct:
>> >> >
>> >> >  new = region.start | (res->flags & PCI_REGION_FLAG_MASK);
>> >> >
>> >> >PCI_REGION_FLAG_MASK is 0xf.  For memory BARs, bits 0-3 are read-only
>> >> >property bits.  For I/O BARs, bits 0-1 are read-only and bits 2-3 are
>> >> >part of the address, so on the face of it, the above could corrupt two
>> >> >bits of an I/O address.
>> >> >
>> >> >It's true that decode_bar() initializes flags correctly, using
>> >> >PCI_BASE_ADDRESS_IO_MASK for I/O BARs and PCI_BASE_ADDRESS_MEM_MASK
>> >> >for memory BARs, but it would take a little more digging to be sure
>> >> >that we never set bits 2-3 of flags for an I/O resource elsewhere.
>> >> >
>> >> 
>> >> The BAR's property bits are probed from device-tree, not hardware
>> >> on some platforms (e.g. pSeries). Also, there is only one (property)
>> >> bit if it's a ROM BAR. So more check as below might be needed because
>> >> the code (without the enhancement) should also work fine.
>> >
>> >Ah, right, I forgot about that.  I didn't do enough digging :)
>> >
>> >> >How about this in pci_std_update_resource():
>> >> >
>> >> >        pcibios_resource_to_bus(dev->bus, &region, res);
>> >> >        new = region.start;
>> >> >
>> >> >        if (res->flags & IORESOURCE_IO) {
>> >> >                mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
>> >> >                new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
>> >> >        } else {
>> >> >                mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
>> >> >                new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
>> >> >        }
>> >> >
>> >> 
>> >> 	if (res->flags & IORESOURCE_IO) {
>> >> 		mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
>> >> 		new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
>> >> 	} else if (resno < PCI_ROM_RESOURCE) {
>> >> 		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
>> >> 		new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
>> >> 	} else if (resno == PCI_ROM_RESOURCE) {
>> >> 		mask = ~((u32)IORESOURCE_ROM_ENABLE);
>> >> 		new |= res->flags & IORESOURCE_ROM_ENABLE);
>> >> 	} else {
>> >> 		dev_warn(&dev->dev, "BAR#%d out of range\n", resno);
>> >> 		return;
>> >> 	}
>> >
>> >After this patch, the only thing we OR into a ROM BAR value is
>> >PCI_ROM_ADDRESS_ENABLE, and that's done below, only if the ROM is
>> >already enabled.
>> >
>> >I did update the ROM mask (to PCI_ROM_ADDRESS_MASK).  I'm not 100%
>> >sure about doing that -- it follows the spec, but it is a change from
>> >what we've been doing before.  I guess it should be safe because it
>> >means we're checking fewer bits than before (only the top 21 bits for
>> >ROMs, where we used check the top 28), so the only possible difference
>> >is that we might not warn about "error updating" in some case where we
>> >used to.
>> >
>> >I'm not really sure about the value of the "error updating" checks to
>> >begin with, though I guess it does help us find broken devices that
>> >put non-BARs where BARs are supposed to be.
>> >
>> 
>> Yeah, agree. Bjorn, I don't have more comments. please take your time
>> to respin the series and maybe applied it. I really want to see the
>> fixes can be in 4.10 if possible :-)
>
>These will definitely be in v4.10.  Thanks for all your help!
>

Thanks for your helps actually :-)

[PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled

[Bjorn Helgaas]

If we update a VF BAR while it's enabled, there are two potential problems:

  1) Any driver that's using the VF has a cached BAR value that is stale
     after the update, and

  2) We can't update 64-bit BARs atomically, so the intermediate state
     (new lower dword with old upper dword) may conflict with another
     device, and an access by a driver unrelated to the VF may cause a bus
     error.

Warn about attempts to update VF BARs while they are enabled.  This is a
programming error, so use dev_WARN() to get a backtrace.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index d00ed5c..a800ba2 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -584,6 +584,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
 	struct resource *res = dev->resource + resno;
 	int vf_bar = resno - PCI_IOV_RESOURCES;
 	struct pci_bus_region region;
+	u16 cmd;
 	u32 new;
 	int reg;
 
@@ -595,6 +596,13 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
 	if (!iov)
 		return;
 
+	pci_read_config_word(dev, iov->pos + PCI_SRIOV_CTRL, &cmd);
+	if ((cmd & PCI_SRIOV_CTRL_VFE) && (cmd & PCI_SRIOV_CTRL_MSE)) {
+		dev_WARN(&dev->dev, "can't update enabled VF BAR%d %pR\n",
+			 vf_bar, res);
+		return;
+	}
+
 	/*
 	 * Ignore unimplemented BARs, unused resource slots for 64-bit
 	 * BARs, and non-movable resources, e.g., those described via

Re: [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled

[Gavin Shan]

On Mon, Nov 28, 2016 at 10:15:21PM -0600, Bjorn Helgaas wrote:
>If we update a VF BAR while it's enabled, there are two potential problems:
>
>  1) Any driver that's using the VF has a cached BAR value that is stale
>     after the update, and
>
>  2) We can't update 64-bit BARs atomically, so the intermediate state
>     (new lower dword with old upper dword) may conflict with another
>     device, and an access by a driver unrelated to the VF may cause a bus
>     error.
>
>Warn about attempts to update VF BARs while they are enabled.  This is a
>programming error, so use dev_WARN() to get a backtrace.
>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c |    8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index d00ed5c..a800ba2 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -584,6 +584,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
> 	struct resource *res = dev->resource + resno;
> 	int vf_bar = resno - PCI_IOV_RESOURCES;
> 	struct pci_bus_region region;
>+	u16 cmd;
> 	u32 new;
> 	int reg;
>
>@@ -595,6 +596,13 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
> 	if (!iov)
> 		return;
>
>+	pci_read_config_word(dev, iov->pos + PCI_SRIOV_CTRL, &cmd);
>+	if ((cmd & PCI_SRIOV_CTRL_VFE) && (cmd & PCI_SRIOV_CTRL_MSE)) {
>+		dev_WARN(&dev->dev, "can't update enabled VF BAR%d %pR\n",
>+			 vf_bar, res);
>+		return;
>+	}
>+
> 	/*
> 	 * Ignore unimplemented BARs, unused resource slots for 64-bit
> 	 * BARs, and non-movable resources, e.g., those described via
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled

[David Laight]

RnJvbTogQmpvcm4gSGVsZ2Fhcw0KPiBTZW50OiAyOSBOb3ZlbWJlciAyMDE2IDA0OjE1DQo+IElm
IHdlIHVwZGF0ZSBhIFZGIEJBUiB3aGlsZSBpdCdzIGVuYWJsZWQsIHRoZXJlIGFyZSB0d28gcG90
ZW50aWFsIHByb2JsZW1zOg0KPiANCj4gICAxKSBBbnkgZHJpdmVyIHRoYXQncyB1c2luZyB0aGUg
VkYgaGFzIGEgY2FjaGVkIEJBUiB2YWx1ZSB0aGF0IGlzIHN0YWxlDQo+ICAgICAgYWZ0ZXIgdGhl
IHVwZGF0ZSwgYW5kDQo+IA0KPiAgIDIpIFdlIGNhbid0IHVwZGF0ZSA2NC1iaXQgQkFScyBhdG9t
aWNhbGx5LCBzbyB0aGUgaW50ZXJtZWRpYXRlIHN0YXRlDQo+ICAgICAgKG5ldyBsb3dlciBkd29y
ZCB3aXRoIG9sZCB1cHBlciBkd29yZCkgbWF5IGNvbmZsaWN0IHdpdGggYW5vdGhlcg0KPiAgICAg
IGRldmljZSwgYW5kIGFuIGFjY2VzcyBieSBhIGRyaXZlciB1bnJlbGF0ZWQgdG8gdGhlIFZGIG1h
eSBjYXVzZSBhIGJ1cw0KPiAgICAgIGVycm9yLg0KDQpDYW4gdGhlIGhpZ2ggd29yZCBiZSBmaXJz
dCBzZXQgdG8gYSB2YWx1ZSB0aGF0IGlzIGludmFsaWQgKGllIGEgNEcgYmxvY2sNCnRoYXQgaGFz
IG5vIHZhbGlkIFBDSWUgc2xhdmVzKSBiZWZvcmUgdXBkYXRpbmcgdGhlIGxvdyB3b3JkIGFuZCBm
aW5hbGx5DQp0aGUgY29ycmVjdCBoaWdoIHdvcmQuDQoNCk5vdGUgdGhhdCB0aGUgYWRkcmVzcyBv
bmx5IGhhcyB0byBiZSBvdXRzaWRlIHRoZSByYW5nZSB0aGF0IHRoZSBicmlkZ2UNCmZvcndhcmRz
IG9udG8gdGhhdCBzcGVjaWZpYyBidXMuDQoNCglEYXZpZA0KDQo=

Re: [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled

[Bjorn Helgaas]

[Your response didn't make it to the list, I think because it's some
non-plaintext encoding that is rejected by the list (see
http://vger.kernel.org/majordomo-info.html)]

On Wed, Nov 30, 2016 at 11:56 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Bjorn Helgaas
>> Sent: 29 November 2016 04:15
>> If we update a VF BAR while it's enabled, there are two potential problems:
>>
>>   1) Any driver that's using the VF has a cached BAR value that is stale
>>      after the update, and
>>
>>   2) We can't update 64-bit BARs atomically, so the intermediate state
>>      (new lower dword with old upper dword) may conflict with another
>>      device, and an access by a driver unrelated to the VF may cause a bus
>>      error.
>
> Can the high word be first set to a value that is invalid (ie a 4G block
> that has no valid PCIe slaves) before updating the low word and finally
> the correct high word.
>
> Note that the address only has to be outside the range that the bridge
> forwards onto that specific bus.

Maybe, but I think that's getting way too complicated.  I think we
should just think of it as a higher level bug if we're trying to
update an enabled BAR.  We might have to live with that scenario for
standard BARs because firmware might have enabled it for us, and
things might break if we disable it, but I don't think we should try
to make it work for VF BARs.

Bjorn

[PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar()

[Bjorn Helgaas]

pci_std_update_resource() only deals with standard BARs, so we don't have
to worry about the complications of VF BARs in an SR-IOV capability.

Compute the BAR address inline and remove pci_resource_bar().  That makes
pci_iov_resource_bar() unused, so remove that as well.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c       |   18 ------------------
 drivers/pci/pci.c       |   30 ------------------------------
 drivers/pci/pci.h       |    6 ------
 drivers/pci/setup-res.c |   13 +++++++------
 4 files changed, 7 insertions(+), 60 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index a800ba2..c9c84ba 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -554,24 +554,6 @@ void pci_iov_release(struct pci_dev *dev)
 }
 
 /**
- * pci_iov_resource_bar - get position of the SR-IOV BAR
- * @dev: the PCI device
- * @resno: the resource number
- *
- * Returns position of the BAR encapsulated in the SR-IOV capability.
- */
-int pci_iov_resource_bar(struct pci_dev *dev, int resno)
-{
-	if (resno < PCI_IOV_RESOURCES || resno > PCI_IOV_RESOURCE_END)
-		return 0;
-
-	BUG_ON(!dev->is_physfn);
-
-	return dev->sriov->pos + PCI_SRIOV_BAR +
-		4 * (resno - PCI_IOV_RESOURCES);
-}
-
-/**
  * pci_iov_update_resource - update a VF BAR
  * @dev: the PCI device
  * @resno: the resource number
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 631eac2..ec3f16d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4827,36 +4827,6 @@ int pci_select_bars(struct pci_dev *dev, unsigned long flags)
 }
 EXPORT_SYMBOL(pci_select_bars);
 
-/**
- * pci_resource_bar - get position of the BAR associated with a resource
- * @dev: the PCI device
- * @resno: the resource number
- * @type: the BAR type to be filled in
- *
- * Returns BAR position in config space, or 0 if the BAR is invalid.
- */
-int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type)
-{
-	int reg;
-
-	if (resno < PCI_ROM_RESOURCE) {
-		*type = pci_bar_unknown;
-		return PCI_BASE_ADDRESS_0 + 4 * resno;
-	} else if (resno == PCI_ROM_RESOURCE) {
-		*type = pci_bar_mem32;
-		return dev->rom_base_reg;
-	} else if (resno < PCI_BRIDGE_RESOURCES) {
-		/* device specific resource */
-		*type = pci_bar_unknown;
-		reg = pci_iov_resource_bar(dev, resno);
-		if (reg)
-			return reg;
-	}
-
-	dev_err(&dev->dev, "BAR %d: invalid resource\n", resno);
-	return 0;
-}
-
 /* Some architectures require additional programming to enable VGA */
 static arch_set_vga_state_t arch_set_vga_state;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5bfcb92..a5d37f6 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -245,7 +245,6 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 int pci_setup_device(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
-int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);
 void pci_configure_ari(struct pci_dev *dev);
 void __pci_bus_size_bridges(struct pci_bus *bus,
 			struct list_head *realloc_head);
@@ -289,7 +288,6 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
 #ifdef CONFIG_PCI_IOV
 int pci_iov_init(struct pci_dev *dev);
 void pci_iov_release(struct pci_dev *dev);
-int pci_iov_resource_bar(struct pci_dev *dev, int resno);
 void pci_iov_update_resource(struct pci_dev *dev, int resno);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
@@ -304,10 +302,6 @@ static inline void pci_iov_release(struct pci_dev *dev)
 
 {
 }
-static inline int pci_iov_resource_bar(struct pci_dev *dev, int resno)
-{
-	return 0;
-}
 static inline void pci_restore_iov_state(struct pci_dev *dev)
 {
 }
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index ee0be34..09bdff7 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -32,7 +32,6 @@ static void pci_std_update_resource(struct pci_dev *dev, int resno)
 	u16 cmd;
 	u32 new, check, mask;
 	int reg;
-	enum pci_bar_type type;
 	struct resource *res = dev->resource + resno;
 
 	/* Per SR-IOV spec 3.4.1.11, VF BARs are RO zero */
@@ -65,14 +64,16 @@ static void pci_std_update_resource(struct pci_dev *dev, int resno)
 	else
 		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
 
-	reg = pci_resource_bar(dev, resno, &type);
-	if (!reg)
-		return;
-	if (type != pci_bar_unknown) {
+	if (resno < PCI_ROM_RESOURCE) {
+		reg = PCI_BASE_ADDRESS_0 + 4 * resno;
+	} else if (resno == PCI_ROM_RESOURCE) {
 		if (!(res->flags & IORESOURCE_ROM_ENABLE))
 			return;
+
+		reg = dev->rom_base_reg;
 		new |= PCI_ROM_ADDRESS_ENABLE;
-	}
+	} else
+		return;
 
 	/*
 	 * We can't update a 64-bit BAR atomically, so when possible,

Re: [PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar()

[Gavin Shan]

On Mon, Nov 28, 2016 at 10:15:42PM -0600, Bjorn Helgaas wrote:
>pci_std_update_resource() only deals with standard BARs, so we don't have
>to worry about the complications of VF BARs in an SR-IOV capability.
>
>Compute the BAR address inline and remove pci_resource_bar().  That makes
>pci_iov_resource_bar() unused, so remove that as well.
>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c       |   18 ------------------
> drivers/pci/pci.c       |   30 ------------------------------
> drivers/pci/pci.h       |    6 ------
> drivers/pci/setup-res.c |   13 +++++++------
> 4 files changed, 7 insertions(+), 60 deletions(-)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index a800ba2..c9c84ba 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -554,24 +554,6 @@ void pci_iov_release(struct pci_dev *dev)
> }
>
> /**
>- * pci_iov_resource_bar - get position of the SR-IOV BAR
>- * @dev: the PCI device
>- * @resno: the resource number
>- *
>- * Returns position of the BAR encapsulated in the SR-IOV capability.
>- */
>-int pci_iov_resource_bar(struct pci_dev *dev, int resno)
>-{
>-	if (resno < PCI_IOV_RESOURCES || resno > PCI_IOV_RESOURCE_END)
>-		return 0;
>-
>-	BUG_ON(!dev->is_physfn);
>-
>-	return dev->sriov->pos + PCI_SRIOV_BAR +
>-		4 * (resno - PCI_IOV_RESOURCES);
>-}
>-
>-/**
>  * pci_iov_update_resource - update a VF BAR
>  * @dev: the PCI device
>  * @resno: the resource number
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index 631eac2..ec3f16d 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -4827,36 +4827,6 @@ int pci_select_bars(struct pci_dev *dev, unsigned long flags)
> }
> EXPORT_SYMBOL(pci_select_bars);
>
>-/**
>- * pci_resource_bar - get position of the BAR associated with a resource
>- * @dev: the PCI device
>- * @resno: the resource number
>- * @type: the BAR type to be filled in
>- *
>- * Returns BAR position in config space, or 0 if the BAR is invalid.
>- */
>-int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type)
>-{
>-	int reg;
>-
>-	if (resno < PCI_ROM_RESOURCE) {
>-		*type = pci_bar_unknown;
>-		return PCI_BASE_ADDRESS_0 + 4 * resno;
>-	} else if (resno == PCI_ROM_RESOURCE) {
>-		*type = pci_bar_mem32;
>-		return dev->rom_base_reg;
>-	} else if (resno < PCI_BRIDGE_RESOURCES) {
>-		/* device specific resource */
>-		*type = pci_bar_unknown;
>-		reg = pci_iov_resource_bar(dev, resno);
>-		if (reg)
>-			return reg;
>-	}
>-
>-	dev_err(&dev->dev, "BAR %d: invalid resource\n", resno);
>-	return 0;
>-}
>-
> /* Some architectures require additional programming to enable VGA */
> static arch_set_vga_state_t arch_set_vga_state;
>
>diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>index 5bfcb92..a5d37f6 100644
>--- a/drivers/pci/pci.h
>+++ b/drivers/pci/pci.h
>@@ -245,7 +245,6 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
> int pci_setup_device(struct pci_dev *dev);
> int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> 		    struct resource *res, unsigned int reg);
>-int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);
> void pci_configure_ari(struct pci_dev *dev);
> void __pci_bus_size_bridges(struct pci_bus *bus,
> 			struct list_head *realloc_head);
>@@ -289,7 +288,6 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
> #ifdef CONFIG_PCI_IOV
> int pci_iov_init(struct pci_dev *dev);
> void pci_iov_release(struct pci_dev *dev);
>-int pci_iov_resource_bar(struct pci_dev *dev, int resno);
> void pci_iov_update_resource(struct pci_dev *dev, int resno);
> resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
> void pci_restore_iov_state(struct pci_dev *dev);
>@@ -304,10 +302,6 @@ static inline void pci_iov_release(struct pci_dev *dev)
>
> {
> }
>-static inline int pci_iov_resource_bar(struct pci_dev *dev, int resno)
>-{
>-	return 0;
>-}
> static inline void pci_restore_iov_state(struct pci_dev *dev)
> {
> }
>diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>index ee0be34..09bdff7 100644
>--- a/drivers/pci/setup-res.c
>+++ b/drivers/pci/setup-res.c
>@@ -32,7 +32,6 @@ static void pci_std_update_resource(struct pci_dev *dev, int resno)
> 	u16 cmd;
> 	u32 new, check, mask;
> 	int reg;
>-	enum pci_bar_type type;
> 	struct resource *res = dev->resource + resno;
>
> 	/* Per SR-IOV spec 3.4.1.11, VF BARs are RO zero */
>@@ -65,14 +64,16 @@ static void pci_std_update_resource(struct pci_dev *dev, int resno)
> 	else
> 		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
>
>-	reg = pci_resource_bar(dev, resno, &type);
>-	if (!reg)
>-		return;
>-	if (type != pci_bar_unknown) {
>+	if (resno < PCI_ROM_RESOURCE) {
>+		reg = PCI_BASE_ADDRESS_0 + 4 * resno;
>+	} else if (resno == PCI_ROM_RESOURCE) {
> 		if (!(res->flags & IORESOURCE_ROM_ENABLE))
> 			return;
>+
>+		reg = dev->rom_base_reg;
> 		new |= PCI_ROM_ADDRESS_ENABLE;
>-	}
>+	} else
>+		return;
>
> 	/*
> 	 * We can't update a 64-bit BAR atomically, so when possible,
>

[PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE

[Bjorn Helgaas]

Remove the assumption that IORESOURCE_ROM_ENABLE == PCI_ROM_ADDRESS_ENABLE.
PCI_ROM_ADDRESS_ENABLE is the ROM enable bit defined by the PCI spec, so if
we're reading or writing a BAR register value, that's what we should use.
IORESOURCE_ROM_ENABLE is a corresponding bit in struct resource flags.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ab00267..cf7670e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -227,7 +227,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;
 		}
 	} else {
-		res->flags |= (l & IORESOURCE_ROM_ENABLE);
+		if (l & PCI_ROM_ADDRESS_ENABLE)
+			res->flags |= IORESOURCE_ROM_ENABLE;
 		l64 = l & PCI_ROM_ADDRESS_MASK;
 		sz64 = sz & PCI_ROM_ADDRESS_MASK;
 		mask64 = (u32)PCI_ROM_ADDRESS_MASK;

Re: [PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE

[Gavin Shan]

On Mon, Nov 28, 2016 at 10:16:07PM -0600, Bjorn Helgaas wrote:
>Remove the assumption that IORESOURCE_ROM_ENABLE == PCI_ROM_ADDRESS_ENABLE.
>PCI_ROM_ADDRESS_ENABLE is the ROM enable bit defined by the PCI spec, so if
>we're reading or writing a BAR register value, that's what we should use.
>IORESOURCE_ROM_ENABLE is a corresponding bit in struct resource flags.
>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/probe.c |    3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>index ab00267..cf7670e 100644
>--- a/drivers/pci/probe.c
>+++ b/drivers/pci/probe.c
>@@ -227,7 +227,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> 			mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;
> 		}
> 	} else {
>-		res->flags |= (l & IORESOURCE_ROM_ENABLE);
>+		if (l & PCI_ROM_ADDRESS_ENABLE)
>+			res->flags |= IORESOURCE_ROM_ENABLE;
> 		l64 = l & PCI_ROM_ADDRESS_MASK;
> 		sz64 = sz & PCI_ROM_ADDRESS_MASK;
> 		mask64 = (u32)PCI_ROM_ADDRESS_MASK;
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH v4 7/7] PCI: Add comments about ROM BAR updating

[Bjorn Helgaas]

pci_update_resource() updates a hardware BAR so its address matches the
kernel's struct resource UNLESS it's a disabled ROM BAR.  We only update
those when we enable the ROM.

It's not obvious from the code why ROM BARs should be handled specially.
Apparently there are Matrox devices with defective ROM BARs that read as
zero when disabled.  That means that if pci_enable_rom() reads the disabled
BAR, sets PCI_ROM_ADDRESS_ENABLE (without re-inserting the address), and
writes it back, it would enable the ROM at address zero.

Add comments and references to explain why we can't make the code look more
rational.

The code changes are from 755528c860b0 ("Ignore disabled ROM resources at
setup") and 8085ce084c0f ("[PATCH] Fix PCI ROM mapping").

Link: https://lkml.org/lkml/2005/8/30/138
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/rom.c       |    5 +++++
 drivers/pci/setup-res.c |    6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 06663d3..b6edb18 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -35,6 +35,11 @@ int pci_enable_rom(struct pci_dev *pdev)
 	if (res->flags & IORESOURCE_ROM_SHADOW)
 		return 0;
 
+	/*
+	 * Ideally pci_update_resource() would update the ROM BAR address,
+	 * and we would only set the enable bit here.  But apparently some
+	 * devices have buggy ROM BARs that read as zero when disabled.
+	 */
 	pcibios_resource_to_bus(pdev->bus, &region, res);
 	pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr);
 	rom_addr &= ~PCI_ROM_ADDRESS_MASK;
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 09bdff7..57d7041 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -67,6 +67,12 @@ static void pci_std_update_resource(struct pci_dev *dev, int resno)
 	if (resno < PCI_ROM_RESOURCE) {
 		reg = PCI_BASE_ADDRESS_0 + 4 * resno;
 	} else if (resno == PCI_ROM_RESOURCE) {
+
+		/*
+		 * Apparently some Matrox devices have ROM BARs that read
+		 * as zero when disabled, so don't update ROM BARs unless
+		 * they're enabled.  See https://lkml.org/lkml/2005/8/30/138.
+		 */
 		if (!(res->flags & IORESOURCE_ROM_ENABLE))
 			return;
 

Re: [PATCH v4 7/7] PCI: Add comments about ROM BAR updating

[Gavin Shan]

On Mon, Nov 28, 2016 at 10:16:48PM -0600, Bjorn Helgaas wrote:
>pci_update_resource() updates a hardware BAR so its address matches the
>kernel's struct resource UNLESS it's a disabled ROM BAR.  We only update
>those when we enable the ROM.
>
>It's not obvious from the code why ROM BARs should be handled specially.
>Apparently there are Matrox devices with defective ROM BARs that read as
>zero when disabled.  That means that if pci_enable_rom() reads the disabled
>BAR, sets PCI_ROM_ADDRESS_ENABLE (without re-inserting the address), and
>writes it back, it would enable the ROM at address zero.
>
>Add comments and references to explain why we can't make the code look more
>rational.
>
>The code changes are from 755528c860b0 ("Ignore disabled ROM resources at
>setup") and 8085ce084c0f ("[PATCH] Fix PCI ROM mapping").
>
>Link: https://lkml.org/lkml/2005/8/30/138
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/rom.c       |    5 +++++
> drivers/pci/setup-res.c |    6 ++++++
> 2 files changed, 11 insertions(+)
>
>diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
>index 06663d3..b6edb18 100644
>--- a/drivers/pci/rom.c
>+++ b/drivers/pci/rom.c
>@@ -35,6 +35,11 @@ int pci_enable_rom(struct pci_dev *pdev)
> 	if (res->flags & IORESOURCE_ROM_SHADOW)
> 		return 0;
>
>+	/*
>+	 * Ideally pci_update_resource() would update the ROM BAR address,
>+	 * and we would only set the enable bit here.  But apparently some
>+	 * devices have buggy ROM BARs that read as zero when disabled.
>+	 */
> 	pcibios_resource_to_bus(pdev->bus, &region, res);
> 	pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr);
> 	rom_addr &= ~PCI_ROM_ADDRESS_MASK;
>diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>index 09bdff7..57d7041 100644
>--- a/drivers/pci/setup-res.c
>+++ b/drivers/pci/setup-res.c
>@@ -67,6 +67,12 @@ static void pci_std_update_resource(struct pci_dev *dev, int resno)
> 	if (resno < PCI_ROM_RESOURCE) {
> 		reg = PCI_BASE_ADDRESS_0 + 4 * resno;
> 	} else if (resno == PCI_ROM_RESOURCE) {
>+
>+		/*
>+		 * Apparently some Matrox devices have ROM BARs that read
>+		 * as zero when disabled, so don't update ROM BARs unless
>+		 * they're enabled.  See https://lkml.org/lkml/2005/8/30/138.
>+		 */
> 		if (!(res->flags & IORESOURCE_ROM_ENABLE))
> 			return;
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs

[Bjorn Helgaas]

On Mon, Nov 28, 2016 at 10:13:41PM -0600, Bjorn Helgaas wrote:
> This is a v4 of Gavin's series for handling VF BAR updates.  The
> important piece is the first patch ("PCI: Do any VF BAR updates before
> enabling the BARs").  That makes sure that if we update VF BARs, we do
> it before enabling the VFs, and that is unchanged from v3.
> 
> The second patch in Gavin's series ("PCI: Disable VF's memory space on
> updating IOV BAR in pci_update_resource()") temporarily disabled VF
> memory space during an update.  Since the first patch does the update
> before enabling VFs, this case shouldn't happen in practice.
> 
> But even if we want to update a VF BAR while VF memory space is
> enabled, I think temporarily disabling it is wrong.  So I replaced
> the second patch with a few patches that make that such an update
> fail.
> 
> Please comment.  These are on my pci/virtualization branch.
> 
> Changelog
> =========
> v4:
>   * Don't disable VF's memory space when IOV BARs are updated; fail the
>     update instead. 
>   * Split IOV BAR updates from standard BAR updates so IOV updates can go
>     in pci/iov.c.
>   * Remove pci_resource_bar() and pci_iov_resource_bar() (the relevant
>     code is simpler when inlined into the callers).
>   * Cleanup IORESOURCE_ROM_ENABLE usage.
>   * Add comments about why ROMs are updated differently.
> 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.
> 
> ---
> 
> Bjorn Helgaas (6):
>       PCI: Ignore BAR updates on virtual functions
>       PCI: Separate VF BAR updates from standard BAR updates
>       PCI: Don't update VF BARs while VF memory space is enabled
>       PCI: Remove pci_resource_bar() and pci_iov_resource_bar()
>       PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE
>       PCI: Add comments about ROM BAR updating
> 
> Gavin Shan (1):
>       PCI: Do any VF BAR updates before enabling the BARs
> 
> 
>  drivers/pci/iov.c       |   69 +++++++++++++++++++++++++++++++++++++----------
>  drivers/pci/pci.c       |   34 -----------------------
>  drivers/pci/pci.h       |    7 +----
>  drivers/pci/probe.c     |    3 +-
>  drivers/pci/rom.c       |    5 +++
>  drivers/pci/setup-res.c |   37 ++++++++++++++++++-------
>  6 files changed, 88 insertions(+), 67 deletions(-)

I applied this on pci/virtualization for v4.10.