[PATCH V5 0/7] minimal alignment for p2p bars

[PATCH V5 0/7] minimal alignment for p2p bars

[Gavin Shan]

v1 -> v2:
	* Shorten the varaible names so that they looks more short.
	* Changelog adjustment so that they looks more meaningful.

v2 -> v3:
	* Rebase to 3.5.RC4

v3 -> v4:
	* Merge Yinghai's patches.

v3 -> v4:
	* Split patch for easy review.
	* Add function to retrieve the minimal alignment of p2p bridge. 

Lu Yinghai(3):
  pci: change variable name for find_pci_host_bridge
  pci: argument pci_bus for find_pci_host_bridge
  pci: fiddle with conversion of pci and CPU address

Gavin Shan(4)
  pci: make find_pci_host_bridge global
  pci: minimal alignment for bars of P2P bridges
  pci: function to retrieve alignment of p2p bars
  pci: resource assignment based on p2p alignment

 drivers/pci/host-bridge.c |   59 ++++++++++++++++++++++++++++++++++++---------
 drivers/pci/probe.c       |    5 ++++
 drivers/pci/setup-bus.c   |   22 +++++++++++------
 include/linux/pci.h       |   15 +++++++++++-
 4 files changed, 81 insertions(+), 20 deletions(-)

Thanks,
Gavin

[PATCH 1/7] pci: change variable name for find_pci_host_bridge

[Gavin Shan]

The patch changes the variable name for function find_pci_host_bridge()
so that it looks more meaningful. More specificly, the "bus" has been
replaced with "root_bus".

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/host-bridge.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index a68dc61..c19776a 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -22,9 +22,9 @@ static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
 
 static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
 {
-	struct pci_bus *bus = find_pci_root_bus(dev);
+	struct pci_bus *root_bus = find_pci_root_bus(dev);
 
-	return to_pci_host_bridge(bus->bridge);
+	return to_pci_host_bridge(root_bus->bridge);
 }
 
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
-- 
1.7.9.5

[PATCH 2/7] pci: argument pci_bus for find_pci_host_bridge

[Gavin Shan]

The patch changes the argument of find_pci_host_bridge() to pci_bus.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/host-bridge.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index c19776a..fc16357 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -9,20 +9,17 @@
 
 #include "pci.h"
 
-static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
+static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 {
-	struct pci_bus *bus;
-
-	bus = dev->bus;
 	while (bus->parent)
 		bus = bus->parent;
 
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
+static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
 {
-	struct pci_bus *root_bus = find_pci_root_bus(dev);
+	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
 	return to_pci_host_bridge(root_bus->bridge);
 }
@@ -43,7 +40,7 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
 void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
 			     struct resource *res)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(dev->bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
@@ -71,7 +68,7 @@ static bool region_contains(struct pci_bus_region *region1,
 void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 			     struct pci_bus_region *region)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(dev->bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
-- 
1.7.9.5

[PATCH 3/7] pci: make find_pci_host_bridge global

[Gavin Shan]

The patch makes function find_pci_host_bridge() global so that the
platforms (e.g. PPC) can access pci_host_bridge.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/host-bridge.c |    2 +-
 include/linux/pci.h       |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index fc16357..a43d393 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -17,7 +17,7 @@ static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
 {
 	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fefb4e1..615ac90 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -385,6 +385,7 @@ struct pci_host_bridge {
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
 		     void (*release_fn)(struct pci_host_bridge *),
 		     void *release_data);
-- 
1.7.9.5

[PATCH 4/7] pci: fiddle with conversion of pci and CPU address

[Gavin Shan]

The patch fiddles with the those functions used to do conversion
between PCI and CPU address. More specificly, 2 functions are
involved: pcibios_bus_to_resource() and pcibios_resource_to_bus()

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/host-bridge.c |   24 +++++++++++++++++++-----
 include/linux/pci.h       |    5 ++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index a43d393..1854a2d 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -37,10 +37,10 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
 	return res1->start <= res2->start && res1->end >= res2->end;
 }
 
-void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
-			     struct resource *res)
+void __pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region,
+			       struct resource *res)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev->bus);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
@@ -57,6 +57,13 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
 	region->start = res->start - offset;
 	region->end = res->end - offset;
 }
+
+void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
+			     struct resource *res)
+{
+	__pcibios_resource_to_bus(dev->bus, region, res);
+}
+
 EXPORT_SYMBOL(pcibios_resource_to_bus);
 
 static bool region_contains(struct pci_bus_region *region1,
@@ -65,10 +72,10 @@ static bool region_contains(struct pci_bus_region *region1,
 	return region1->start <= region2->start && region1->end >= region2->end;
 }
 
-void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
+void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
 			     struct pci_bus_region *region)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev->bus);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
@@ -90,4 +97,11 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 	res->start = region->start + offset;
 	res->end = region->end + offset;
 }
+
+void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
+			     struct pci_bus_region *region)
+{
+	__pcibios_bus_to_resource(dev->bus, res, region);
+}
+
 EXPORT_SYMBOL(pcibios_bus_to_resource);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 615ac90..e66f4b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -657,9 +657,12 @@ void pcibios_update_irq(struct pci_dev *, int irq);
 void pci_fixup_cardbus(struct pci_bus *);
 
 /* Generic PCI functions used internally */
-
+void __pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region,
+				struct resource *res);
 void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
 			     struct resource *res);
+void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+			       struct pci_bus_region *region);
 void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 			     struct pci_bus_region *region);
 void pcibios_scan_specific_bus(int busn);
-- 
1.7.9.5

[PATCH 5/7] pci: minimal alignment for bars of P2P bridges

[Gavin Shan]

On some powerpc platforms, device BARs need to be assigned to separate
"segments" of the address space in order for the error isolation and HW
virtualization mechanisms (EEH) to work properly. Those "segments" have
a minimum size that can be fairly large (16M). In order to be able to
use the generic resource assignment code rather than re-inventing our
own, we chose to group devices by bus. That way, a simple change of the
minimum alignment requirements of resources assigned to PCI to PCI (P2P)
bridges is enough to ensure that all BARs for devices below those bridges
will fit into contiguous sets of segments and there will be no overlap.

This patch provides a way for the host bridge to override the default
alignment values used by the resource allocation code for that purpose.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/probe.c |    5 +++++
 include/linux/pci.h |    8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 658ac97..a196529 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
 	if (bridge) {
 		INIT_LIST_HEAD(&bridge->windows);
 		bridge->bus = b;
+
+		/* Set minimal alignment shift of P2P bridges */
+		bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
+		bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
+		bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
 	}
 
 	return bridge;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e66f4b2..2b2b38d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,9 +376,17 @@ struct pci_host_bridge_window {
 	resource_size_t offset;		/* bus address + offset = CPU address */
 };
 
+/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
+#define PCI_DEFAULT_IO_ALIGN_SHIFT	12	/* 4KB  */
+#define PCI_DEFAULT_MEM_ALIGN_SHIFT	20	/* 1MB  */
+#define PCI_DEFAULT_PMEM_ALIGN_SHIFT	20	/* 1MB */
+
 struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
+	int io_align_shift;		/* P2P I/O bar minimal alignment shift  */
+	int mem_align_shift;		/* P2P MMIO bar minimal alignment shift */
+	int pmem_align_shift;		/* P2P prefetchable MMIO bar minimal alignment shift */
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
-- 
1.7.9.5

[PATCH 6/7] pci: function to retrieve alignment of p2p bars

[Gavin Shan]

The patch introduces function pci_align_boundary() to retrieve the
minimal alignment of p2p bars according to the argument.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/host-bridge.c |   24 ++++++++++++++++++++++++
 include/linux/pci.h       |    1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 1854a2d..dc9a95e 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -105,3 +105,27 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 }
 
 EXPORT_SYMBOL(pcibios_bus_to_resource);
+
+resource_size_t pci_align_boundary(struct pci_bus *bus, unsigned long flags)
+{
+	resource_size_t align = 0;
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
+
+	flags &= (IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH);
+	switch (flags) {
+	case IORESOURCE_IO:
+		align = (1 << bridge->io_align_shift);
+		break;
+	case IORESOURCE_MEM:
+		align = (1 << bridge->mem_align_shift);
+		break;
+	case (IORESOURCE_MEM | IORESOURCE_PREFETCH):
+		align = (1 << bridge->pmem_align_shift);
+		break;
+	default:
+		printk(KERN_WARNING "%s: invalid flags 0x%lx\n",
+			__func__, flags);
+	}
+
+	return align;
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2b2b38d..64523ef 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -673,6 +673,7 @@ void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
 			       struct pci_bus_region *region);
 void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 			     struct pci_bus_region *region);
+resource_size_t pci_align_boundary(struct pci_bus *bus, unsigned long flags);
 void pcibios_scan_specific_bus(int busn);
 extern struct pci_bus *pci_find_bus(int domain, int busnr);
 void pci_bus_add_devices(const struct pci_bus *bus);
-- 
1.7.9.5

[PATCH 7/7] pci: resource assignment based on p2p alignment

[Gavin Shan]

The patch changes function pbus_size_io() and pbus_size_mem() to
do resource (I/O, memory and prefetchable memory) reassignment
based on the minimal alignments from the p2p bridge.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8fa2d4b..52b60af 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -710,6 +710,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
 	unsigned long size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
+	resource_size_t io_align;
 
 	if (!b_res)
  		return;
@@ -735,13 +736,15 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 				children_add_size += get_res_add_size(realloc_head, r);
 		}
 	}
+
+	io_align = pci_align_boundary(bus, IORESOURCE_IO);
 	size0 = calculate_iosize(size, min_size, size1,
-			resource_size(b_res), 4096);
+			resource_size(b_res), io_align);
 	if (children_add_size > add_size)
 		add_size = children_add_size;
 	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
 		calculate_iosize(size, min_size, add_size + size1,
-			resource_size(b_res), 4096);
+			resource_size(b_res), io_align);
 	if (!size0 && !size1) {
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window "
@@ -751,11 +754,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		return;
 	}
 	/* Alignment of the IO window is always 4K */
-	b_res->start = 4096;
+	b_res->start = io_align;
 	b_res->end = b_res->start + size0 - 1;
 	b_res->flags |= IORESOURCE_STARTALIGN;
 	if (size1 > size0 && realloc_head) {
-		add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
+		add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
 		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
 				 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
 				 bus->secondary, bus->subordinate, size1-size0);
@@ -785,10 +788,15 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	struct resource *b_res = find_free_bus_resource(bus, type);
 	unsigned int mem64_mask = 0;
 	resource_size_t children_add_size = 0;
+	resource_size_t mem_align;
+	int mem_align_shift;
 
 	if (!b_res)
 		return 0;
 
+	mem_align = pci_align_boundary(bus, type);
+	mem_align_shift = __ffs(mem_align);
+
 	memset(aligns, 0, sizeof(aligns));
 	max_order = 0;
 	size = 0;
@@ -818,8 +826,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 #endif
 			/* For bridges size != alignment */
 			align = pci_resource_alignment(dev, r);
-			order = __ffs(align) - 20;
-			if (order > 11) {
+			order = __ffs(align) - mem_align_shift;
+			if (order > (11 - (mem_align_shift - 20))) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR "
 					 "(bad alignment %#llx)\n", i, r,
 					 (unsigned long long) align);
@@ -846,7 +854,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	for (order = 0; order <= max_order; order++) {
 		resource_size_t align1 = 1;
 
-		align1 <<= (order + 20);
+		align1 <<= (order + mem_align_shift);
 
 		if (!align)
 			min_align = align1;
-- 
1.7.9.5

Re: [PATCH V5 0/7] minimal alignment for p2p bars

[Gavin Shan]

Hi Bjorn,

Could you please review it when you have time?

Thanks,
Gavin

>v1 -> v2:
>	* Shorten the varaible names so that they looks more short.
>	* Changelog adjustment so that they looks more meaningful.
>
>v2 -> v3:
>	* Rebase to 3.5.RC4
>
>v3 -> v4:
>	* Merge Yinghai's patches.
>
>v3 -> v4:
>	* Split patch for easy review.
>	* Add function to retrieve the minimal alignment of p2p bridge. 
>
>Lu Yinghai(3):
>  pci: change variable name for find_pci_host_bridge
>  pci: argument pci_bus for find_pci_host_bridge
>  pci: fiddle with conversion of pci and CPU address
>
>Gavin Shan(4)
>  pci: make find_pci_host_bridge global
>  pci: minimal alignment for bars of P2P bridges
>  pci: function to retrieve alignment of p2p bars
>  pci: resource assignment based on p2p alignment
>
> drivers/pci/host-bridge.c |   59 ++++++++++++++++++++++++++++++++++++---------
> drivers/pci/probe.c       |    5 ++++
> drivers/pci/setup-bus.c   |   22 +++++++++++------
> include/linux/pci.h       |   15 +++++++++++-
> 4 files changed, 81 insertions(+), 20 deletions(-)
>
>Thanks,
>Gavin
>

Re: [PATCH 5/7] pci: minimal alignment for bars of P2P bridges

[Bjorn Helgaas]

On Fri, Jun 29, 2012 at 02:47:48PM +0800, Gavin Shan wrote:
> On some powerpc platforms, device BARs need to be assigned to separate
> "segments" of the address space in order for the error isolation and HW
> virtualization mechanisms (EEH) to work properly. Those "segments" have
> a minimum size that can be fairly large (16M). In order to be able to
> use the generic resource assignment code rather than re-inventing our
> own, we chose to group devices by bus. That way, a simple change of the
> minimum alignment requirements of resources assigned to PCI to PCI (P2P)
> bridges is enough to ensure that all BARs for devices below those bridges
> will fit into contiguous sets of segments and there will be no overlap.

If I understand correctly, you might have something like this:

  PCI host bridge to bus 0000:00
  pci_bus 0000:00: root bus resource [mem 0xc0000000-0xcfffffff]
  0000:00:01.0: PCI bridge to [bus 10-1f]
  0000:00:01.0:   bridge window [mem 0xc1000000-0xc1ffffff]
  0000:00:02.0: PCI bridge to [bus 20-2f]
  0000:00:02.0:   bridge window [mem 0xc2000000-0xc2ffffff]

where everything under bridge 00:01.0 is in one EEH segment, and
everything under 00:02.0 is in another.  In this case, each EEH
segment is 16MB.

I think your proposal is basically that when we add up resources required
below the P2P bridges, we round up to the default 1MB (the minimum P2P
bridge memory aperture size per spec) *or* to a larger value, e.g., 16MB,
if the architecture requires it.

That makes sense to me, but I have some implementation questions.

Your patches make the required alignment a property of the host bridge.
But don't you want to do this rounding up only at certain levels of the
hierarchy?  For example, what if you had another P2P bridge:

  0000:10:01.0: PCI bridge to [bus 18-1f]

I assume the devices on bus 0000:18 would still be in the first EEH
segment, and you wouldn't necessarily want to round up the 10:01.0
apertures to 16MB.

Maybe there should be an interface like this:

  resource_size_t __weak pcibios_window_alignment(struct pci_bus *bus,
						  unsigned long type)
  {
    if (type & IORESOURCE_MEM)
      return 1024*1024;		/* mem windows must be 1MB aligned */
    if (bus->self->io_window_1k)
      return 1024;
    return 4*1024;		/* I/O windows default to 4K alignment */
  }

that the arch could override?  Then you could return the 16MB alignment
for the top-level P2P bridge leading to an EEH segment, and use the
default alignment for P2P bridges *inside* the segment.

> This patch provides a way for the host bridge to override the default
> alignment values used by the resource allocation code for that purpose.
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/probe.c |    5 +++++
>  include/linux/pci.h |    8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 658ac97..a196529 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
>  	if (bridge) {
>  		INIT_LIST_HEAD(&bridge->windows);
>  		bridge->bus = b;
> +
> +		/* Set minimal alignment shift of P2P bridges */
> +		bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
> +		bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
> +		bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
>  	}
>  
>  	return bridge;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e66f4b2..2b2b38d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -376,9 +376,17 @@ struct pci_host_bridge_window {
>  	resource_size_t offset;		/* bus address + offset = CPU address */
>  };
>  
> +/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
> +#define PCI_DEFAULT_IO_ALIGN_SHIFT	12	/* 4KB  */
> +#define PCI_DEFAULT_MEM_ALIGN_SHIFT	20	/* 1MB  */
> +#define PCI_DEFAULT_PMEM_ALIGN_SHIFT	20	/* 1MB */
> +
>  struct pci_host_bridge {
>  	struct device dev;
>  	struct pci_bus *bus;		/* root bus */
> +	int io_align_shift;		/* P2P I/O bar minimal alignment shift  */
> +	int mem_align_shift;		/* P2P MMIO bar minimal alignment shift */
> +	int pmem_align_shift;		/* P2P prefetchable MMIO bar minimal alignment shift */
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> -- 
> 1.7.9.5
> 

Re: [PATCH 5/7] pci: minimal alignment for bars of P2P bridges

[Bjorn Helgaas]

On Sun, Jul 15, 2012 at 9:50 PM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> On Fri, Jul 13, 2012 at 02:12:50PM -0600, Bjorn Helgaas wrote:
>>On Fri, Jun 29, 2012 at 02:47:48PM +0800, Gavin Shan wrote:
>>> On some powerpc platforms, device BARs need to be assigned to separate
>>> "segments" of the address space in order for the error isolation and HW
>>> virtualization mechanisms (EEH) to work properly. Those "segments" have
>>> a minimum size that can be fairly large (16M). In order to be able to
>>> use the generic resource assignment code rather than re-inventing our
>>> own, we chose to group devices by bus. That way, a simple change of the
>>> minimum alignment requirements of resources assigned to PCI to PCI (P2P)
>>> bridges is enough to ensure that all BARs for devices below those bridges
>>> will fit into contiguous sets of segments and there will be no overlap.
>
> I send the previous reply in a rush and that missed some
> necessary information. So I resend it with some makeup.
>
>>If I understand correctly, you might have something like this:
>>
>>  PCI host bridge to bus 0000:00
>>  pci_bus 0000:00: root bus resource [mem 0xc0000000-0xcfffffff]
>>  0000:00:01.0: PCI bridge to [bus 10-1f]
>>  0000:00:01.0:   bridge window [mem 0xc1000000-0xc1ffffff]
>>  0000:00:02.0: PCI bridge to [bus 20-2f]
>>  0000:00:02.0:   bridge window [mem 0xc2000000-0xc2ffffff]
>>
>>where everything under bridge 00:01.0 is in one EEH segment, and
>>everything under 00:02.0 is in another.  In this case, each EEH
>>segment is 16MB.
>>
>>I think your proposal is basically that when we add up resources required
>>below the P2P bridges, we round up to the default 1MB (the minimum P2P
>>bridge memory aperture size per spec) *or* to a larger value, e.g., 16MB,
>>if the architecture requires it.
>
> Yes, you're correct.
>
>>That makes sense to me, but I have some implementation questions.
>>
>>Your patches make the required alignment a property of the host bridge.
>>But don't you want to do this rounding up only at certain levels of the
>>hierarchy?  For example, what if you had another P2P bridge:
>>
>>  0000:10:01.0: PCI bridge to [bus 18-1f]
>>
>>I assume the devices on bus 0000:18 would still be in the first EEH
>>segment, and you wouldn't necessarily want to round up the 10:01.0
>>apertures to 16MB.
>>
>>Maybe there should be an interface like this:
>>
>>  resource_size_t __weak pcibios_window_alignment(struct pci_bus *bus,
>>                                                 unsigned long type)
>>  {
>>    if (type & IORESOURCE_MEM)
>>      return 1024*1024;                /* mem windows must be 1MB aligned */
>>    if (bus->self->io_window_1k)
>>      return 1024;
>>    return 4*1024;             /* I/O windows default to 4K alignment */
>>  }
>>
>>that the arch could override?  Then you could return the 16MB alignment
>>for the top-level P2P bridge leading to an EEH segment, and use the
>>default alignment for P2P bridges *inside* the segment.
>>
>
> Yes. I think it's good mechanism to apply the minimal resource alignment
> for P2P bridges. Also, it wouldn't waste more resources if the specific
> PCI bridge (not PCIe bridge) needn't form separate EEH segment. However,
> I have some implementation questions for this.
>
> 1. I just checked out source code from following link, but it seems that
>    pci_dev doesn't have field called "io_window_1k", so I'm not sure I
>    should add that by myself?
>
>         git clone git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git linux-pci-next

No, don't add io_window_1k in your patch.  That was added in a recent
patch and should already be in my "next" branch:

http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=commitdiff;h=2b28ae1912e5ce5bb0527e352ae6ff04e76183d1

If there are merge conflicts with your patch, I can resolve them.

> 2. With the mechanism (__weak function) you suggested, the alignment of
>    the specific P2P bridge should be figured out by PowerPC platform. That's
>    to say, the PPC platform has to introduce function pcibios_window_alignment()
>    and return the appropriate alignment.

Yes.

> 3. In order to return appropriate alignment from PPC platform, We need
>    to introduce same function for PPC platform. Also, we probably need
>    introduce function "ppc_md.pcibios_window_alignment" so that those
>    specific platforms (e.g. powernv) could override that.

Yes.

Re: [PATCH v5 0/7] minimal alignment for p2p bars

[Bjorn Helgaas]

On Mon, Jul 16, 2012 at 11:30:23PM +0800, Gavin Shan wrote:
> v1 -> v2:
> 	* Shorten the varaible names so that they looks more short.
> 	* Changelog adjustment so that they looks more meaningful.
> 
> v2 -> v3:
> 	* Rebase to 3.5.RC4
> 
> v3 -> v4:
> 	* Merge Yinghai's patches.
> 
> v3 -> v4:
> 	* Split patch for easy review.
> 	* Add function to retrieve the minimal alignment of p2p bridge. 
> 
> v4 -> v5:
> 	* Rebase to 3.5.RC7
> 	* Introduce weak function pcibios_window_alignment() to retrieve
> 	  I/O and memory alignment for P2P bridges.
> 	* Introduce pcibios_window_alignment() for ppc to override the
> 	  PCI function.
> 	* Add ppc_md.pcibios_window_alignment() for specific platform like
> 	  powernv can override ppc's pcibios_window_alignment().

You don't have to rebase your patches -- I can take care of merging
them with previous patches I've applied -- but if you *do* rebase,
the most convenient target would be my "next" branch, since it
already contains the things I would have to merge with.

> Lu Yinghai(3):
>   pci: change variable name for find_pci_host_bridge
>   pci: argument pci_bus for find_pci_host_bridge
>   pci: fiddle with conversion of pci and CPU address

I think the above patches are not needed for the ones below, are they?

> Gavin Shan(4)
>   pci: weak function returns alignment
>   pci: resource assignment based on p2p alignment
>   ppc/pci: override pcibios_window_alignment
>   ppc/pnv: I/O and memory alignment for p2p bridges
> 
> 
> arch/powerpc/include/asm/machdep.h        |    3 ++
> arch/powerpc/kernel/pci-common.c          |   20 +++++++++++
> arch/powerpc/platforms/powernv/pci-ioda.c |   41 ++++++++++++++++++++++
> drivers/pci/host-bridge.c                 |   52 ++++++++++++++++++++++-------
> drivers/pci/setup-bus.c                   |   22 ++++++++----
> include/linux/pci.h                       |    7 +++-
> 6 files changed, 125 insertions(+), 20 deletions(-)
> 
> Thanks,
> Gavin
> 

Re: [PATCH 04/15] pci: weak function returns alignment

[Bjorn Helgaas]

On Mon, Jul 16, 2012 at 11:30:27PM +0800, Gavin Shan wrote:
> The patch implements the weak function to return the default I/O
> or memory alignment for P2P bridge. Currently, I/O window has 4KiB
> alignment and memory window is 4MiB aligned by default. On the other
> hand, those platforms (e.g. powernv) that have special requirements
> on the alignment could override the function by themselves.
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/host-bridge.c |   17 +++++++++++++++++
>  include/linux/pci.h       |    2 ++
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index abcf053..dcbc47d 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -105,3 +105,20 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>  }
>  
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> +
> +/*
> + * Retrieve the default memory or I/O alignment for the
> + * specific P2P bridge. The function has been implemented
> + * as weak so that it can be overrided by platform that
> + * has special requirments for memory and I/O alignment.
> + */
> +resource_size_t __weak pcibios_window_alignment(struct pci_bus *bus,
> +						unsigned long type)

This no longer has anything to do with the host bridge, so I think it
could be moved to setup-bus.c and made static.  We shouldn't have to
repeat the default 1M and 4K code in the arch versions, so maybe we
could do something like this:

    resource_size_t __weak pcibios_window_alignment(struct pci_bus *bus, unsigned long type)
    {
	return 1;
    }

    static resource_size_t window_alignment(struct pci_bus *bus, unsigned long type)
    {
	resource_size_t align = 1, arch_align;

	if (type & IORESOURCE_MEM)
	    align = 1024*1024;
	else if (type & IORESOURCE_IO)
	    align = 4*1024;
    
	arch_align = pcibios_window_alignment(bus, type);
	return max(align, arch_align);
    }

I made the default 1, thinking of bus number apertures (though we don't use this
path for bus numbers today).

> +{
> +	/* Memory windows must be 1MiB aligned */
> +	if (type & IORESOURCE_MEM)
> +		return 1024*1024;
> +
> +	/* I/O windows have default alignment of 4KiB */
> +	return 4*1024;
> +}
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 9acea4b..283da11 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -662,6 +662,8 @@ void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>  			       struct pci_bus_region *region);
>  void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>  			     struct pci_bus_region *region);
> +resource_size_t pcibios_window_alignment(struct pci_bus *bus,
> +					 unsigned long type);
>  void pcibios_scan_specific_bus(int busn);
>  extern struct pci_bus *pci_find_bus(int domain, int busnr);
>  void pci_bus_add_devices(const struct pci_bus *bus);
> -- 
> 1.7.5.4
> 

Re: [PATCH 05/15] pci: resource assignment based on p2p alignment

[Bjorn Helgaas]

On Mon, Jul 16, 2012 at 11:30:28PM +0800, Gavin Shan wrote:
> The patch changes function pbus_size_io() and pbus_size_mem() to
> do resource (I/O, memory and prefetchable memory) reassignment
> based on the minimal alignments for the p2p bridge, which was
> retrieved by function pcibios_window_alignment().
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/setup-bus.c |   22 +++++++++++++++-------
>  1 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8fa2d4b..6ccf31a 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -710,6 +710,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
>  	unsigned long size = 0, size0 = 0, size1 = 0;
>  	resource_size_t children_add_size = 0;
> +	resource_size_t io_align;
>  
>  	if (!b_res)
>   		return;
> @@ -735,13 +736,15 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  				children_add_size += get_res_add_size(realloc_head, r);
>  		}
>  	}
> +
> +	io_align = pcibios_window_alignment(bus, IORESOURCE_IO);
>  	size0 = calculate_iosize(size, min_size, size1,
> -			resource_size(b_res), 4096);
> +			resource_size(b_res), io_align);
>  	if (children_add_size > add_size)
>  		add_size = children_add_size;
>  	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
>  		calculate_iosize(size, min_size, add_size + size1,
> -			resource_size(b_res), 4096);
> +			resource_size(b_res), io_align);
>  	if (!size0 && !size1) {
>  		if (b_res->start || b_res->end)
>  			dev_info(&bus->self->dev, "disabling bridge window "
> @@ -751,11 +754,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  		return;
>  	}
>  	/* Alignment of the IO window is always 4K */
> -	b_res->start = 4096;
> +	b_res->start = io_align;
>  	b_res->end = b_res->start + size0 - 1;
>  	b_res->flags |= IORESOURCE_STARTALIGN;
>  	if (size1 > size0 && realloc_head) {
> -		add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
> +		add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
>  		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
>  				 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
>  				 bus->secondary, bus->subordinate, size1-size0);
> @@ -785,10 +788,15 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  	struct resource *b_res = find_free_bus_resource(bus, type);
>  	unsigned int mem64_mask = 0;
>  	resource_size_t children_add_size = 0;
> +	resource_size_t mem_align;
> +	int mem_align_shift;
>  
>  	if (!b_res)
>  		return 0;
>  
> +	mem_align = pcibios_window_alignment(bus, type);
> +	mem_align_shift = __ffs(mem_align);
> +
>  	memset(aligns, 0, sizeof(aligns));
>  	max_order = 0;
>  	size = 0;
> @@ -818,8 +826,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  #endif
>  			/* For bridges size != alignment */
>  			align = pci_resource_alignment(dev, r);
> -			order = __ffs(align) - 20;
> -			if (order > 11) {
> +			order = __ffs(align) - mem_align_shift;
> +			if (order > (11 - (mem_align_shift - 20))) {

This doesn't seem right to me.  Aren't we accumulating the amount of
space required at each alignment in aligns[]?  That should be independent
of whatever arch-specific minimum alignment we have.

Does it have to be more complicated than something like this at the
very end?

    min_align = max(min_align, pci_window_alignment(bus, IORESOURCE_MEM));

>  				dev_warn(&dev->dev, "disabling BAR %d: %pR "
>  					 "(bad alignment %#llx)\n", i, r,
>  					 (unsigned long long) align);
> @@ -846,7 +854,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  	for (order = 0; order <= max_order; order++) {
>  		resource_size_t align1 = 1;
>  
> -		align1 <<= (order + 20);
> +		align1 <<= (order + mem_align_shift);
>  
>  		if (!align)
>  			min_align = align1;
> -- 
> 1.7.5.4
>