| Message ID | 20161129041506.21453.18478.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 >
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);
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 >
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
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
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!
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 :-)
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];
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(-)