* [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs
@ 2016-11-29 4:13 Bjorn Helgaas
2016-11-29 4:14 ` [PATCH v4 1/7] PCI: Do any VF BAR updates before enabling the BARs Bjorn Helgaas
` (7 more replies)
0 siblings, 8 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:13 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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(-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 1/7] PCI: Do any VF BAR updates before enabling the BARs
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
@ 2016-11-29 4:14 ` Bjorn Helgaas
2016-11-29 4:14 ` [PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions Bjorn Helgaas
` (6 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:14 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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)
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
2016-11-29 4:14 ` [PATCH v4 1/7] PCI: Do any VF BAR updates before enabling the BARs Bjorn Helgaas
@ 2016-11-29 4:14 ` Bjorn Helgaas
2016-11-29 4:45 ` Gavin Shan
2016-11-29 4:15 ` [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates Bjorn Helgaas
` (5 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:14 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
2016-11-29 4:14 ` [PATCH v4 1/7] PCI: Do any VF BAR updates before enabling the BARs Bjorn Helgaas
2016-11-29 4:14 ` [PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions Bjorn Helgaas
@ 2016-11-29 4:15 ` Bjorn Helgaas
2016-11-29 4:55 ` Gavin Shan
2016-11-29 4:15 ` [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled Bjorn Helgaas
` (4 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:15 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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, ®ion, 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];
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
` (2 preceding siblings ...)
2016-11-29 4:15 ` [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates Bjorn Helgaas
@ 2016-11-29 4:15 ` Bjorn Helgaas
2016-11-29 4:57 ` Gavin Shan
2016-11-30 17:56 ` David Laight
2016-11-29 4:15 ` [PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar() Bjorn Helgaas
` (3 subsequent siblings)
7 siblings, 2 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:15 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar()
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
` (3 preceding siblings ...)
2016-11-29 4:15 ` [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled Bjorn Helgaas
@ 2016-11-29 4:15 ` Bjorn Helgaas
2016-11-29 5:02 ` Gavin Shan
2016-11-29 4:16 ` [PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE Bjorn Helgaas
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:15 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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,
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
` (4 preceding siblings ...)
2016-11-29 4:15 ` [PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar() Bjorn Helgaas
@ 2016-11-29 4:16 ` Bjorn Helgaas
2016-11-29 5:03 ` Gavin Shan
2016-11-29 4:16 ` [PATCH v4 7/7] PCI: Add comments about ROM BAR updating Bjorn Helgaas
2016-12-01 22:21 ` [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
7 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:16 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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;
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 7/7] PCI: Add comments about ROM BAR updating
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
` (5 preceding siblings ...)
2016-11-29 4:16 ` [PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE Bjorn Helgaas
@ 2016-11-29 4:16 ` Bjorn Helgaas
2016-11-29 5:05 ` Gavin Shan
2016-12-01 22:21 ` [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
7 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 4:16 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, benh, linux-pci, linuxppc-dev, mpe
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, ®ion, 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;
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions
2016-11-29 4:14 ` [PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions Bjorn Helgaas
@ 2016-11-29 4:45 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2016-11-29 4:45 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, benh, linux-pci, linuxppc-dev, mpe
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-29 4:15 ` [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates Bjorn Helgaas
@ 2016-11-29 4:55 ` Gavin Shan
2016-11-29 14:48 ` Bjorn Helgaas
0 siblings, 1 reply; 23+ messages in thread
From: Gavin Shan @ 2016-11-29 4:55 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, benh, linux-pci, linuxppc-dev, mpe
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, ®ion, 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled
2016-11-29 4:15 ` [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled Bjorn Helgaas
@ 2016-11-29 4:57 ` Gavin Shan
2016-11-30 17:56 ` David Laight
1 sibling, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2016-11-29 4:57 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, benh, linux-pci, linuxppc-dev, mpe
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar()
2016-11-29 4:15 ` [PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar() Bjorn Helgaas
@ 2016-11-29 5:02 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2016-11-29 5:02 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, benh, linux-pci, linuxppc-dev, mpe
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,
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE
2016-11-29 4:16 ` [PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE Bjorn Helgaas
@ 2016-11-29 5:03 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2016-11-29 5:03 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, benh, linux-pci, linuxppc-dev, mpe
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 7/7] PCI: Add comments about ROM BAR updating
2016-11-29 4:16 ` [PATCH v4 7/7] PCI: Add comments about ROM BAR updating Bjorn Helgaas
@ 2016-11-29 5:05 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2016-11-29 5:05 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, benh, linux-pci, linuxppc-dev, mpe
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, ®ion, 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-29 4:55 ` Gavin Shan
@ 2016-11-29 14:48 ` Bjorn Helgaas
2016-11-29 23:20 ` Gavin Shan
0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 14:48 UTC (permalink / raw)
To: Gavin Shan; +Cc: Bjorn Helgaas, clsoto, benh, linux-pci, linuxppc-dev, mpe
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, ®ion, 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, ®ion, 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, ®ion, 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);
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-29 14:48 ` Bjorn Helgaas
@ 2016-11-29 23:20 ` Gavin Shan
2016-11-30 0:06 ` Bjorn Helgaas
0 siblings, 1 reply; 23+ messages in thread
From: Gavin Shan @ 2016-11-29 23:20 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Gavin Shan, Bjorn Helgaas, clsoto, benh, linux-pci, linuxppc-dev, mpe
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, ®ion, 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, ®ion, 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, ®ion, 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-29 23:20 ` Gavin Shan
@ 2016-11-30 0:06 ` Bjorn Helgaas
2016-11-30 23:02 ` Gavin Shan
0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-30 0:06 UTC (permalink / raw)
To: Gavin Shan; +Cc: Bjorn Helgaas, clsoto, benh, linux-pci, linuxppc-dev, mpe
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, ®ion, 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled
2016-11-29 4:15 ` [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled Bjorn Helgaas
2016-11-29 4:57 ` Gavin Shan
@ 2016-11-30 17:56 ` David Laight
2016-11-30 18:52 ` Bjorn Helgaas
1 sibling, 1 reply; 23+ messages in thread
From: David Laight @ 2016-11-30 17:56 UTC (permalink / raw)
To: 'Bjorn Helgaas', Gavin Shan; +Cc: clsoto, linuxppc-dev, linux-pci
RnJvbTogQmpvcm4gSGVsZ2Fhcw0KPiBTZW50OiAyOSBOb3ZlbWJlciAyMDE2IDA0OjE1DQo+IElm
IHdlIHVwZGF0ZSBhIFZGIEJBUiB3aGlsZSBpdCdzIGVuYWJsZWQsIHRoZXJlIGFyZSB0d28gcG90
ZW50aWFsIHByb2JsZW1zOg0KPiANCj4gICAxKSBBbnkgZHJpdmVyIHRoYXQncyB1c2luZyB0aGUg
VkYgaGFzIGEgY2FjaGVkIEJBUiB2YWx1ZSB0aGF0IGlzIHN0YWxlDQo+ICAgICAgYWZ0ZXIgdGhl
IHVwZGF0ZSwgYW5kDQo+IA0KPiAgIDIpIFdlIGNhbid0IHVwZGF0ZSA2NC1iaXQgQkFScyBhdG9t
aWNhbGx5LCBzbyB0aGUgaW50ZXJtZWRpYXRlIHN0YXRlDQo+ICAgICAgKG5ldyBsb3dlciBkd29y
ZCB3aXRoIG9sZCB1cHBlciBkd29yZCkgbWF5IGNvbmZsaWN0IHdpdGggYW5vdGhlcg0KPiAgICAg
IGRldmljZSwgYW5kIGFuIGFjY2VzcyBieSBhIGRyaXZlciB1bnJlbGF0ZWQgdG8gdGhlIFZGIG1h
eSBjYXVzZSBhIGJ1cw0KPiAgICAgIGVycm9yLg0KDQpDYW4gdGhlIGhpZ2ggd29yZCBiZSBmaXJz
dCBzZXQgdG8gYSB2YWx1ZSB0aGF0IGlzIGludmFsaWQgKGllIGEgNEcgYmxvY2sNCnRoYXQgaGFz
IG5vIHZhbGlkIFBDSWUgc2xhdmVzKSBiZWZvcmUgdXBkYXRpbmcgdGhlIGxvdyB3b3JkIGFuZCBm
aW5hbGx5DQp0aGUgY29ycmVjdCBoaWdoIHdvcmQuDQoNCk5vdGUgdGhhdCB0aGUgYWRkcmVzcyBv
bmx5IGhhcyB0byBiZSBvdXRzaWRlIHRoZSByYW5nZSB0aGF0IHRoZSBicmlkZ2UNCmZvcndhcmRz
IG9udG8gdGhhdCBzcGVjaWZpYyBidXMuDQoNCglEYXZpZA0KDQo=
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled
2016-11-30 17:56 ` David Laight
@ 2016-11-30 18:52 ` Bjorn Helgaas
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-30 18:52 UTC (permalink / raw)
To: David Laight; +Cc: Gavin Shan, clsoto, linuxppc-dev, linux-pci
[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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-30 0:06 ` Bjorn Helgaas
@ 2016-11-30 23:02 ` Gavin Shan
2016-11-30 23:45 ` Bjorn Helgaas
0 siblings, 1 reply; 23+ messages in thread
From: Gavin Shan @ 2016-11-30 23:02 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, linux-pci, Bjorn Helgaas, linuxppc-dev
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, ®ion, 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-30 23:02 ` Gavin Shan
@ 2016-11-30 23:45 ` Bjorn Helgaas
2016-12-01 0:00 ` Gavin Shan
0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2016-11-30 23:45 UTC (permalink / raw)
To: Gavin Shan; +Cc: clsoto, linux-pci, Bjorn Helgaas, linuxppc-dev
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, ®ion, 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!
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates
2016-11-30 23:45 ` Bjorn Helgaas
@ 2016-12-01 0:00 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2016-12-01 0:00 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, linux-pci, Bjorn Helgaas, linuxppc-dev
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, ®ion, 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 :-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
` (6 preceding siblings ...)
2016-11-29 4:16 ` [PATCH v4 7/7] PCI: Add comments about ROM BAR updating Bjorn Helgaas
@ 2016-12-01 22:21 ` Bjorn Helgaas
7 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2016-12-01 22:21 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Gavin Shan, clsoto, benh, linux-pci, linuxppc-dev, mpe
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-12-01 22:21 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-29 4:13 [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
2016-11-29 4:14 ` [PATCH v4 1/7] PCI: Do any VF BAR updates before enabling the BARs Bjorn Helgaas
2016-11-29 4:14 ` [PATCH v4 2/7] PCI: Ignore BAR updates on virtual functions Bjorn Helgaas
2016-11-29 4:45 ` Gavin Shan
2016-11-29 4:15 ` [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates Bjorn Helgaas
2016-11-29 4:55 ` Gavin Shan
2016-11-29 14:48 ` Bjorn Helgaas
2016-11-29 23:20 ` Gavin Shan
2016-11-30 0:06 ` Bjorn Helgaas
2016-11-30 23:02 ` Gavin Shan
2016-11-30 23:45 ` Bjorn Helgaas
2016-12-01 0:00 ` Gavin Shan
2016-11-29 4:15 ` [PATCH v4 4/7] PCI: Don't update VF BARs while VF memory space is enabled Bjorn Helgaas
2016-11-29 4:57 ` Gavin Shan
2016-11-30 17:56 ` David Laight
2016-11-30 18:52 ` Bjorn Helgaas
2016-11-29 4:15 ` [PATCH v4 5/7] PCI: Remove pci_resource_bar() and pci_iov_resource_bar() Bjorn Helgaas
2016-11-29 5:02 ` Gavin Shan
2016-11-29 4:16 ` [PATCH v4 6/7] PCI: Decouple IORESOURCE_ROM_ENABLE and PCI_ROM_ADDRESS_ENABLE Bjorn Helgaas
2016-11-29 5:03 ` Gavin Shan
2016-11-29 4:16 ` [PATCH v4 7/7] PCI: Add comments about ROM BAR updating Bjorn Helgaas
2016-11-29 5:05 ` Gavin Shan
2016-12-01 22:21 ` [PATCH v4 0/7] Disable VF's memory space on updating IOV BARs Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.