SR-IOV: correct broken resource alignment calculations

Commit Message

Chris Wright Aug. 28, 2009, 7:17 p.m. UTC

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 <[e-mail]>
Cc: Jesse Barnes <[e-mail]>
Cc: Ivan Kokshaysky <[e-mail]>
Cc: Linus Torvalds <[e-mail]>
Cc: Matthew Wilcox <[e-mail]>
Cc: Yu Zhao <[e-mail]>
Cc: [e-mail]
---
 drivers/pci/iov.c      |   12 +++++++++++-
 include/linux/ioport.h |    6 ++++++
 kernel/resource.c      |    7 ++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Greg KH Aug. 28, 2009, 7:43 p.m. UTC | #1

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 <[e-mail]>
> Cc: Jesse Barnes <[e-mail]>
> Cc: Ivan Kokshaysky <[e-mail]>
> Cc: Linus Torvalds <[e-mail]>
> Cc: Matthew Wilcox <[e-mail]>
> Cc: Yu Zhao <[e-mail]>
> Cc: [e-mail]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthew Wilcox Aug. 28, 2009, 7:48 p.m. UTC | #2

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.

Chris Wright Aug. 28, 2009, 7:58 p.m. UTC | #3

* Greg KH ([e-mail]) 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 <[e-mail]>
> > Cc: Jesse Barnes <[e-mail]>
> > Cc: Ivan Kokshaysky <[e-mail]>
> > Cc: Linus Torvalds <[e-mail]>
> > Cc: Matthew Wilcox <[e-mail]>
> > Cc: Yu Zhao <[e-mail]>
> > Cc: [e-mail]
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chris Wright Aug. 28, 2009, 7:59 p.m. UTC | #4

* Matthew Wilcox ([e-mail]) 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
 	}