| Message ID | 20161129041521.21453.33146.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 > -- 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
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. David
[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 -- 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
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
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(+) -- 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