[PATCH] SR-IOV: correct broken resource alignment calculations

[PATCH] SR-IOV: correct broken resource alignment calculations

[Chris Wright]

An SR-IOV capable device includes an SR-IOV PCIe capability which
describes the Virtual Function (VF) BAR requirements.  A typical SR-IOV
device can support multiple VFs whose BARs must be in a contiguous region,
effectively an array of VF BARs.  The BAR reports the size requirement
for a single VF.  We calculate the full range needed by simply multiplying
the VF BAR size with the number of possible VFs and create a resource
spanning the full range.

This all seems sane enough except it artificially inflates the alignment
requirement for the VF BAR.  The VF BAR need only be aligned to the size
of a single BAR not the contiguous range of VF BARs.  This can cause us
to fail to allocate resources for the BAR despite the fact that we
actually have enough space.

This patch adds a support for a new resource alignment type,
IORESOURCE_VSIZEALIGN, and allows struct resource to keep track of the
size requirements of a VF BAR which are smaller than the full resource
size.  This could also be done all within the PCI layer w/out bloating
struct resource or using the last available bit for alignment types.

Comments?

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Yu Zhao <yu.zhao@intel.com>
Cc: stable@kernel.org
---
 drivers/pci/iov.c      |   12 +++++++++++-
 include/linux/ioport.h |    6 ++++++
 kernel/resource.c      |    7 ++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index e3a8721..c860dc6 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -101,7 +101,9 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
 		if (!res->parent)
 			continue;
 		virtfn->resource[i].name = pci_name(virtfn);
-		virtfn->resource[i].flags = res->flags;
+		/* single VF BAR with normal size/alignment */
+		virtfn->resource[i].flags = res->flags & ~IORESOURCE_VSIZEALIGN;
+		virtfn->resource[i].flags |= IORESOURCE_SIZEALIGN;
 		size = resource_size(res);
 		do_div(size, iov->total);
 		virtfn->resource[i].start = res->start + size * id;
@@ -468,6 +470,14 @@ found:
 			rc = -EIO;
 			goto failed;
 		}
+		/*
+		 * VF BAR need only be aligned to size of single BAR, not
+		 * whole contiguous range.  So keep vsize available for
+		 * alignment queries.
+		 */
+		res->flags &= ~IORESOURCE_SIZEALIGN;
+		res->flags |= IORESOURCE_VSIZEALIGN;
+		res->vend = res->end;
 		res->end = res->start + resource_size(res) * total - 1;
 		nres++;
 	}
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 786e7b8..cd30c66 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -18,6 +18,7 @@
 struct resource {
 	resource_size_t start;
 	resource_size_t end;
+	resource_size_t vend;
 	const char *name;
 	unsigned long flags;
 	struct resource *parent, *sibling, *child;
@@ -48,6 +49,7 @@ struct resource_list {
 
 #define IORESOURCE_SIZEALIGN	0x00020000	/* size indicates alignment */
 #define IORESOURCE_STARTALIGN	0x00040000	/* start field is alignment */
+#define IORESOURCE_VSIZEALIGN	0x00080000	/* vsize indicates alignment */
 
 #define IORESOURCE_MEM_64	0x00100000
 
@@ -130,6 +132,10 @@ static inline resource_size_t resource_size(struct resource *res)
 {
 	return res->end - res->start + 1;
 }
+static inline resource_size_t resource_vsize(struct resource *res)
+{
+	return res->vend - res->start + 1;
+}
 static inline unsigned long resource_type(struct resource *res)
 {
 	return res->flags & IORESOURCE_TYPE_BITS;
diff --git a/kernel/resource.c b/kernel/resource.c
index 78b0872..6e24295 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -569,11 +569,16 @@ EXPORT_SYMBOL(adjust_resource);
  */
 resource_size_t resource_alignment(struct resource *res)
 {
-	switch (res->flags & (IORESOURCE_SIZEALIGN | IORESOURCE_STARTALIGN)) {
+	unsigned long mask = (IORESOURCE_SIZEALIGN |
+			      IORESOURCE_STARTALIGN |
+			      IORESOURCE_VSIZEALIGN);
+	switch (res->flags & mask) {
 	case IORESOURCE_SIZEALIGN:
 		return resource_size(res);
 	case IORESOURCE_STARTALIGN:
 		return res->start;
+	case IORESOURCE_VSIZEALIGN:
+		return resource_vsize(res);
 	default:
 		return 0;
 	}

Re: [PATCH] SR-IOV: correct broken resource alignment calculations

[Greg KH]

On Fri, Aug 28, 2009 at 12:17:14PM -0700, Chris Wright wrote:
> An SR-IOV capable device includes an SR-IOV PCIe capability which
> describes the Virtual Function (VF) BAR requirements.  A typical SR-IOV
> device can support multiple VFs whose BARs must be in a contiguous region,
> effectively an array of VF BARs.  The BAR reports the size requirement
> for a single VF.  We calculate the full range needed by simply multiplying
> the VF BAR size with the number of possible VFs and create a resource
> spanning the full range.
> 
> This all seems sane enough except it artificially inflates the alignment
> requirement for the VF BAR.  The VF BAR need only be aligned to the size
> of a single BAR not the contiguous range of VF BARs.  This can cause us
> to fail to allocate resources for the BAR despite the fact that we
> actually have enough space.
> 
> This patch adds a support for a new resource alignment type,
> IORESOURCE_VSIZEALIGN, and allows struct resource to keep track of the
> size requirements of a VF BAR which are smaller than the full resource
> size.  This could also be done all within the PCI layer w/out bloating
> struct resource or using the last available bit for alignment types.
> 
> Comments?
> 
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> Cc: Yu Zhao <yu.zhao@intel.com>
> Cc: stable@kernel.org

This is a new feature, which seems to be odd to have it sent to stable
:)

Does this fix reported problems in the "wild"?

Other than that minor procedural thing, the patch looks good to me.

thanks,

greg k-h

Re: [PATCH] SR-IOV: correct broken resource alignment calculations

[Matthew Wilcox]

On Fri, Aug 28, 2009 at 12:17:14PM -0700, Chris Wright wrote:
> An SR-IOV capable device includes an SR-IOV PCIe capability which
> describes the Virtual Function (VF) BAR requirements.  A typical SR-IOV
> device can support multiple VFs whose BARs must be in a contiguous region,
> effectively an array of VF BARs.  The BAR reports the size requirement
> for a single VF.  We calculate the full range needed by simply multiplying
> the VF BAR size with the number of possible VFs and create a resource
> spanning the full range.
> 
> This all seems sane enough except it artificially inflates the alignment
> requirement for the VF BAR.  The VF BAR need only be aligned to the size
> of a single BAR not the contiguous range of VF BARs.  This can cause us
> to fail to allocate resources for the BAR despite the fact that we
> actually have enough space.
> 
> This patch adds a support for a new resource alignment type,
> IORESOURCE_VSIZEALIGN, and allows struct resource to keep track of the
> size requirements of a VF BAR which are smaller than the full resource
> size.  This could also be done all within the PCI layer w/out bloating
> struct resource or using the last available bit for alignment types.

Yes, I think that would be preferable.  We have a *LOT* of resources in
the kernel, and the embedded folks would not find it funny if they all
grew in size suddenly.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] SR-IOV: correct broken resource alignment calculations

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> On Fri, Aug 28, 2009 at 12:17:14PM -0700, Chris Wright wrote:
> > An SR-IOV capable device includes an SR-IOV PCIe capability which
> > describes the Virtual Function (VF) BAR requirements.  A typical SR-IOV
> > device can support multiple VFs whose BARs must be in a contiguous region,
> > effectively an array of VF BARs.  The BAR reports the size requirement
> > for a single VF.  We calculate the full range needed by simply multiplying
> > the VF BAR size with the number of possible VFs and create a resource
> > spanning the full range.
> > 
> > This all seems sane enough except it artificially inflates the alignment
> > requirement for the VF BAR.  The VF BAR need only be aligned to the size
> > of a single BAR not the contiguous range of VF BARs.  This can cause us
> > to fail to allocate resources for the BAR despite the fact that we
> > actually have enough space.
> > 
> > This patch adds a support for a new resource alignment type,
> > IORESOURCE_VSIZEALIGN, and allows struct resource to keep track of the
> > size requirements of a VF BAR which are smaller than the full resource
> > size.  This could also be done all within the PCI layer w/out bloating
> > struct resource or using the last available bit for alignment types.
> > 
> > Comments?
> > 
> > Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Matthew Wilcox <matthew@wil.cx>
> > Cc: Yu Zhao <yu.zhao@intel.com>
> > Cc: stable@kernel.org
> 
> This is a new feature, which seems to be odd to have it sent to stable
> :)

Yeah, it's broken now (and shipped in 2.6.30).

> Does this fix reported problems in the "wild"?


Depending on card firwmware, yeah, I've hit this problem.


thanks,
-chris

Re: [PATCH] SR-IOV: correct broken resource alignment calculations

[Chris Wright]

* Matthew Wilcox (matthew@wil.cx) wrote:
> On Fri, Aug 28, 2009 at 12:17:14PM -0700, Chris Wright wrote:
> > An SR-IOV capable device includes an SR-IOV PCIe capability which
> > describes the Virtual Function (VF) BAR requirements.  A typical SR-IOV
> > device can support multiple VFs whose BARs must be in a contiguous region,
> > effectively an array of VF BARs.  The BAR reports the size requirement
> > for a single VF.  We calculate the full range needed by simply multiplying
> > the VF BAR size with the number of possible VFs and create a resource
> > spanning the full range.
> > 
> > This all seems sane enough except it artificially inflates the alignment
> > requirement for the VF BAR.  The VF BAR need only be aligned to the size
> > of a single BAR not the contiguous range of VF BARs.  This can cause us
> > to fail to allocate resources for the BAR despite the fact that we
> > actually have enough space.
> > 
> > This patch adds a support for a new resource alignment type,
> > IORESOURCE_VSIZEALIGN, and allows struct resource to keep track of the
> > size requirements of a VF BAR which are smaller than the full resource
> > size.  This could also be done all within the PCI layer w/out bloating
> > struct resource or using the last available bit for alignment types.
> 
> Yes, I think that would be preferable.  We have a *LOT* of resources in
> the kernel, and the embedded folks would not find it funny if they all
> grew in size suddenly.

OK, I'll send that momentarily.  It has one downside which is we re-read
the BAR size multiple times.

[PATCH v2] SR-IOV: correct broken resource alignment calculations

[Chris Wright]

* Matthew Wilcox (matthew@wil.cx) wrote:
> On Fri, Aug 28, 2009 at 12:17:14PM -0700, Chris Wright wrote:
> > This patch adds a support for a new resource alignment type,
> > IORESOURCE_VSIZEALIGN, and allows struct resource to keep track of the
> > size requirements of a VF BAR which are smaller than the full resource
> > size.  This could also be done all within the PCI layer w/out bloating
> > struct resource or using the last available bit for alignment types.
> 
> Yes, I think that would be preferable.  We have a *LOT* of resources in
> the kernel, and the embedded folks would not find it funny if they all
> grew in size suddenly.

An SR-IOV capable device includes an SR-IOV PCIe capability which
describes the Virtual Function (VF) BAR requirements.  A typical SR-IOV
device can support multiple VFs whose BARs must be in a contiguous region,
effectively an array of VF BARs.  The BAR reports the size requirement
for a single VF.  We calculate the full range needed by simply multiplying
the VF BAR size with the number of possible VFs and create a resource
spanning the full range.

This all seems sane enough except it artificially inflates the alignment
requirement for the VF BAR.  The VF BAR need only be aligned to the size
of a single BAR not the contiguous range of VF BARs.  This can cause us
to fail to allocate resources for the BAR despite the fact that we
actually have enough space.

This patch adds a thin PCI specific layer over the generic
resource_alignment() function which is aware of the special nature of
VF BARs and does sorting and allocation based on the smaller alignment
requirement.

I recognize that while resource_alignment is generic, it's basically a
PCI helper.  An alternative to this patch is to add PCI VF BAR specific
information to struct resource.  I opted for the extra layer rather than
adding such PCI specific information to struct resource.  This does
have the slight downside that we don't cache the BAR size and re-read
for each alignment query (happens a small handful of times during boot
for each VF BAR).

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Yu Zhao <yu.zhao@intel.com>
Cc: stable@kernel.org
---
 drivers/pci/iov.c       |   23 +++++++++++++++++++++++
 drivers/pci/pci.h       |   13 +++++++++++++
 drivers/pci/setup-bus.c |    4 ++--
 drivers/pci/setup-res.c |    8 ++++----
 4 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index e3a8721..e03fe98 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -598,6 +598,29 @@ int pci_iov_resource_bar(struct pci_dev *dev, int resno,
 }
 
 /**
+ * pci_sriov_resource_alignment - get resource alignment for VF BAR
+ * @dev: the PCI device
+ * @resno: the resource number
+ *
+ * Returns the alignment of the VF BAR found in the SR-IOV capability.
+ * This is not the same as the resource size which is defined as
+ * the VF BAR size multiplied by the number of VFs.  The alignment
+ * is just the VF BAR size.
+ */
+int pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
+{
+	struct resource tmp;
+	enum pci_bar_type type;
+	int reg = pci_iov_resource_bar(dev, resno, &type);
+	
+	if (!reg)
+		return 0;
+
+	 __pci_read_base(dev, type, &tmp, reg);
+	return resource_alignment(&tmp);
+}
+
+/**
  * pci_restore_iov_state - restore the state of the IOV capability
  * @dev: the PCI device
  */
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f73bcbe..5ff4d25 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -243,6 +243,7 @@ extern int pci_iov_init(struct pci_dev *dev);
 extern void pci_iov_release(struct pci_dev *dev);
 extern int pci_iov_resource_bar(struct pci_dev *dev, int resno,
 				enum pci_bar_type *type);
+extern int pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 extern void pci_restore_iov_state(struct pci_dev *dev);
 extern int pci_iov_bus_range(struct pci_bus *bus);
 
@@ -298,4 +299,16 @@ static inline int pci_ats_enabled(struct pci_dev *dev)
 }
 #endif /* CONFIG_PCI_IOV */
 
+static inline int pci_resource_alignment(struct pci_dev *dev,
+					 struct resource *res)
+{
+#ifdef CONFIG_PCI_IOV
+	int resno = res - dev->resource;
+
+	if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
+		return pci_sriov_resource_alignment(dev, resno);
+#endif
+	return resource_alignment(res);
+}
+
 #endif /* DRIVERS_PCI_H */
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index b636e24..7c443b4 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -25,7 +25,7 @@
 #include <linux/ioport.h>
 #include <linux/cache.h>
 #include <linux/slab.h>
-
+#include "pci.h"
 
 static void pbus_assign_resources_sorted(const struct pci_bus *bus)
 {
@@ -384,7 +384,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, unsigned long
 				continue;
 			r_size = resource_size(r);
 			/* For bridges size != alignment */
-			align = resource_alignment(r);
+			align = pci_resource_alignment(dev, r);
 			order = __ffs(align) - 20;
 			if (order > 11) {
 				dev_warn(&dev->dev, "BAR %d bad alignment %llx: "
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 1898c7b..88cdd1a 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -144,7 +144,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 
 	size = resource_size(res);
 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
-	align = resource_alignment(res);
+	align = pci_resource_alignment(dev, res);
 
 	/* First, try exact prefetching match.. */
 	ret = pci_bus_alloc_resource(bus, res, size, align, min,
@@ -178,7 +178,7 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	struct pci_bus *bus;
 	int ret;
 
-	align = resource_alignment(res);
+	align = pci_resource_alignment(dev, res);
 	if (!align) {
 		dev_info(&dev->dev, "BAR %d: can't allocate resource (bogus "
 			"alignment) %pR flags %#lx\n",
@@ -259,7 +259,7 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 		if (!(r->flags) || r->parent)
 			continue;
 
-		r_align = resource_alignment(r);
+		r_align = pci_resource_alignment(dev, r);
 		if (!r_align) {
 			dev_warn(&dev->dev, "BAR %d: bogus alignment "
 				"%pR flags %#lx\n",
@@ -271,7 +271,7 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 			struct resource_list *ln = list->next;
 
 			if (ln)
-				align = resource_alignment(ln->res);
+				align = pci_resource_alignment(ln->dev, ln->res);
 
 			if (r_align > align) {
 				tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);

Re: [PATCH v2] SR-IOV: correct broken resource alignment calculations

[Jesse Barnes]

On Fri, 28 Aug 2009 13:00:06 -0700
Chris Wright <chrisw@sous-sol.org> wrote:

> * Matthew Wilcox (matthew@wil.cx) wrote:
> > On Fri, Aug 28, 2009 at 12:17:14PM -0700, Chris Wright wrote:
> > > This patch adds a support for a new resource alignment type,
> > > IORESOURCE_VSIZEALIGN, and allows struct resource to keep track
> > > of the size requirements of a VF BAR which are smaller than the
> > > full resource size.  This could also be done all within the PCI
> > > layer w/out bloating struct resource or using the last available
> > > bit for alignment types.
> > 
> > Yes, I think that would be preferable.  We have a *LOT* of
> > resources in the kernel, and the embedded folks would not find it
> > funny if they all grew in size suddenly.
> 
> An SR-IOV capable device includes an SR-IOV PCIe capability which
> describes the Virtual Function (VF) BAR requirements.  A typical
> SR-IOV device can support multiple VFs whose BARs must be in a
> contiguous region, effectively an array of VF BARs.  The BAR reports
> the size requirement for a single VF.  We calculate the full range
> needed by simply multiplying the VF BAR size with the number of
> possible VFs and create a resource spanning the full range.
> 
> This all seems sane enough except it artificially inflates the
> alignment requirement for the VF BAR.  The VF BAR need only be
> aligned to the size of a single BAR not the contiguous range of VF
> BARs.  This can cause us to fail to allocate resources for the BAR
> despite the fact that we actually have enough space.
> 
> This patch adds a thin PCI specific layer over the generic
> resource_alignment() function which is aware of the special nature of
> VF BARs and does sorting and allocation based on the smaller alignment
> requirement.
> 
> I recognize that while resource_alignment is generic, it's basically a
> PCI helper.  An alternative to this patch is to add PCI VF BAR
> specific information to struct resource.  I opted for the extra layer
> rather than adding such PCI specific information to struct resource.
> This does have the slight downside that we don't cache the BAR size
> and re-read for each alignment query (happens a small handful of
> times during boot for each VF BAR).
> 
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> Cc: Yu Zhao <yu.zhao@intel.com>
> Cc: stable@kernel.org

Yeah, I like this one better.  I've applied it to my for-linus branch;
would be nice to have a Tested-by for it before I send it to Linus...

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center