[PATCH V8 0/8] Minimal alignment for p2p bars

[PATCH V8 0/8] 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.

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().

v5 -> v6:
        * Refactor pcibios_window_alignment() so the platform-specific
          implementation needn't return the default alignment according
          to Bjorn's suggestion.
        * Simplify pbus_size_mem() according to Bjorn's suggestion: Just
          check the platform required alignment at very end and adjust
          the "min_align" if necessary.

v6 -> v7:
        * Change "type" to "b_res->flags & mask" while retrieving the
          minimal alignment for memory window according to Ram's suggestion.
        * Refactor pbus_size_mem() according to Ram's suggestion.
        * ppc_md.pcibios_window_alignment returns 1 for those PCI bridges
          behind PCI bridges so that PCI core will use default alignment
          values.

v7 -> v8:
	* Rebase to 3.6.RC2, which starts to use "struct resource" to represent
	  the range of PCI bus numbers that specific p2p bridge covers.
	* Define macros for the default alignment of P2P bars according to
	  Richard's comments.

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(5)
  pci: weak function returns alignment
  pci: resource assignment based on p2p alignment
  pci: refactor function pbus_size_mem
  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 |   38 +++++++++++++++
drivers/pci/host-bridge.c                 |   35 +++++++++-----
drivers/pci/setup-bus.c                   |   74 ++++++++++++++++++++++------
include/linux/pci.h                       |    7 ++-
6 files changed, 148 insertions(+), 29 deletions(-)

Thanks,
Gavin

[PATCH 1/8] 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 files 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.5.4

[PATCH 2/8] 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 files 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.5.4

[PATCH 3/8] 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 fc16357..abcf053 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 5faa831..a47a4d3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -692,9 +692,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.5.4

[PATCH 4/8] pci: weak function returns alignment

[Gavin Shan]

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/setup-bus.c |   24 ++++++++++++++++++++++++
 include/linux/pci.h     |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index fb50613..826a4f3 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -697,6 +697,30 @@ static resource_size_t calculate_memsize(resource_size_t size,
 	return size;
 }
 
+resource_size_t __weak pcibios_window_alignment(struct pci_bus *bus,
+						unsigned long type)
+{
+	return 1;
+}
+
+#define PCI_P2P_DEFAULT_MEM_ALIGN	0x100000	/* 1MiB */
+#define PCI_P2P_DEFAULT_IO_ALIGN	0x1000		/* 4KiB */
+
+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 = PCI_P2P_DEFAULT_MEM_ALIGN;
+	else if (type & IORESOURCE_IO)
+		align = PCI_P2P_DEFAULT_IO_ALIGN;
+
+	arch_align = pcibios_window_alignment(bus, type);
+
+	return max(align, arch_align);
+}
+
 /**
  * pbus_size_io() - size the io window of a given bus
  *
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a47a4d3..739be31 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1034,6 +1034,8 @@ int pci_cfg_space_size_ext(struct pci_dev *dev);
 int pci_cfg_space_size(struct pci_dev *dev);
 unsigned char pci_bus_max_busnr(struct pci_bus *bus);
 void pci_setup_bridge(struct pci_bus *bus);
+resource_size_t pcibios_window_alignment(struct pci_bus *bus,
+					 unsigned long type);
 
 #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
 #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
-- 
1.7.5.4

[PATCH 5/8] 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 for the p2p bridge, which was
retrieved by function window_alignment().

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 826a4f3..bb2eade 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -741,7 +741,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 min_align = 4096, align;
+	resource_size_t io_align, min_align, align;
 
 	if (!b_res)
  		return;
@@ -750,8 +750,14 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	 * Per spec, I/O windows are 4K-aligned, but some bridges have an
 	 * extension to support 1K alignment.
 	 */
-	if (bus->self->io_window_1k)
+	if (bus->self->io_window_1k) {
+		io_align = 4096;
 		min_align = 1024;
+	} else {
+		io_align = window_alignment(bus, IORESOURCE_IO);
+		min_align = io_align;
+	}
+
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
 
@@ -778,8 +784,8 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		}
 	}
 
-	if (min_align > 4096)
-		min_align = 4096;
+	if (min_align > io_align)
+		min_align = io_align;
 
 	size0 = calculate_iosize(size, min_size, size1,
 			resource_size(b_res), min_align);
@@ -901,6 +907,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			min_align = align1 >> 1;
 		align += aligns[order];
 	}
+
+	min_align = max(min_align, window_alignment(bus, b_res->flags & mask));
 	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
 	if (children_add_size > add_size)
 		add_size = children_add_size;
-- 
1.7.5.4

[PATCH 6/8] pci: refactor function pbus_size_mem

[Gavin Shan]

The original idea comes from Ram Pai. The patch puts the chunk of
code for calculating the minimal alignment of memory window into
separate inline function.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index bb2eade..7eef40d 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -815,6 +815,28 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	}
 }
 
+static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
+						  int max_order)
+{
+	resource_size_t align = 0;
+	resource_size_t min_align = 0;
+	int order;
+
+	for (order = 0; order <= max_order; order++) {
+		resource_size_t align1 = 1;
+
+		align1 <<= (order + 20);
+
+		if (!align)
+			min_align = align1;
+		else if (ALIGN(align + min_align, min_align) < align1)
+			min_align = align1 >> 1;
+		align += aligns[order];
+	}
+
+	return min_align;
+}
+
 /**
  * pbus_size_mem() - size the memory window of a given bus
  *
@@ -894,20 +916,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 				children_add_size += get_res_add_size(realloc_head, r);
 		}
 	}
-	align = 0;
-	min_align = 0;
-	for (order = 0; order <= max_order; order++) {
-		resource_size_t align1 = 1;
-
-		align1 <<= (order + 20);
-
-		if (!align)
-			min_align = align1;
-		else if (ALIGN(align + min_align, min_align) < align1)
-			min_align = align1 >> 1;
-		align += aligns[order];
-	}
 
+	min_align = calculate_mem_align(aligns, max_order);
 	min_align = max(min_align, window_alignment(bus, b_res->flags & mask));
 	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
 	if (children_add_size > add_size)
-- 
1.7.5.4

[PATCH 7/8] ppc/pci: override pcibios_window_alignment

[Gavin Shan]

Function pcibios_window_alignment() has been implemented as "weak"
in PCI core to return the default alignment of I/O and memory windows
for the associated p2p bridge. The patch adds same function to
override the weak one so that the default alignment could be changed
for those platforms (e.g. powernv).

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/machdep.h |    3 +++
 arch/powerpc/kernel/pci-common.c   |   20 ++++++++++++++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 42ce570..f7706d7 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -214,6 +214,9 @@ struct machdep_calls {
 	/* Called after scan and before resource survey */
 	void (*pcibios_fixup_phb)(struct pci_controller *hose);
 
+	/* Called during PCI resource reassignment */
+	resource_size_t (*pcibios_window_alignment)(struct pci_bus *, unsigned long type);
+
 	/* Called to shutdown machine specific hardware not already controlled
 	 * by other drivers.
 	 */
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 2aa04f2..43fea54 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -99,6 +99,26 @@ void pcibios_free_controller(struct pci_controller *phb)
 		kfree(phb);
 }
 
+/*
+ * The function is used to return the minimal alignment
+ * for memory or I/O windows of the associated P2P bridge.
+ * By default, 4KiB alignment for I/O windows and 1MiB for
+ * memory windows.
+ */
+resource_size_t pcibios_window_alignment(struct pci_bus *bus,
+					 unsigned long type)
+{
+	if (ppc_md.pcibios_window_alignment)
+		return ppc_md.pcibios_window_alignment(bus, type);
+
+	/*
+	 * PCI core will figure out the default
+	 * alignment: 4KiB for I/O and 1MiB for
+	 * memory window.
+	 */
+	return 1;
+}
+
 static resource_size_t pcibios_io_size(const struct pci_controller *hose)
 {
 #ifdef CONFIG_PPC64
-- 
1.7.5.4

[PATCH 8/8] ppc/pnv: I/O and memory alignment for p2p bridges

[Gavin Shan]

The patch implements ppc_md.pcibios_window_alignment for powernv
platform so that the resource reassignment in PCI core will be
done according to the I/O and memory alignment returned from
powernv platform. The alignments returned from powernv platform
is closely depending on the scheme for PE segmenting. Besides,
the patch isn't useful for now, but the subsequent patches will
be working based on it.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/pci-ioda.c |   38 +++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 9cda6a1..af6b7fb 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1139,6 +1139,43 @@ static void __devinit pnv_pci_ioda_fixup_phb(struct pci_controller *hose)
 	}
 }
 
+/*
+ * Returns the alignment for I/O or memory windows for p2p
+ * bridges. That actually depends on how PEs are segmented.
+ * For now, we return I/O or M32 segment size for PE sensitive
+ * p2p bridges. Otherwise, the default values (4KiB for I/O,
+ * 1MiB for memory) will be returned.
+ */
+static resource_size_t pnv_pci_window_alignment(struct pci_bus *bus,
+						unsigned long type)
+{
+	struct pci_dev *dev = bus->self;
+	struct pci_controller *hose = pci_bus_to_host(bus);
+	struct pnv_phb *phb = hose->private_data;
+
+	/* PCI root bus will be assigned with segment size */
+	if (!dev)
+		goto out;
+
+	/*
+	 * If the parent bridge is PCI bridge, we will return
+	 * the default values
+	 */
+	dev = dev->bus->self;
+	if (!dev)
+		goto out;
+	if (dev->pcie_type == PCI_EXP_TYPE_PCI_BRIDGE) {
+		return 1;
+	}
+
+out:
+	/* We need support prefetchable memory window later */
+	if (type & IORESOURCE_MEM)
+		return phb->ioda.m32_segsize;
+
+	return phb->ioda.io_segsize;
+}
+
 /* Prevent enabling devices for which we couldn't properly
  * assign a PE
  */
@@ -1306,6 +1343,7 @@ void __init pnv_pci_init_ioda1_phb(struct device_node *np)
 	 */
 	ppc_md.pcibios_fixup_phb = pnv_pci_ioda_fixup_phb;
 	ppc_md.pcibios_enable_device_hook = pnv_pci_enable_device_hook;
+	ppc_md.pcibios_window_alignment = pnv_pci_window_alignment;
 	pci_add_flags(PCI_PROBE_ONLY | PCI_REASSIGN_ALL_RSRC);
 
 	/* Reset IODA tables to a clean state */
-- 
1.7.5.4

Re: [PATCH 5/8] pci: resource assignment based on p2p alignment

[Bjorn Helgaas]

On Tue, Aug 21, 2012 at 12:36 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
>
> Last night, I talked to Bjorn through IRC. It seems that Bjorn still have
> concerns on what we're doing in function pcibios_window_alignment() and
> pcibios_align_resource(). Another question that Bjorn has is that's possible
> to put the PPC alignment (alignment on p2p memory windows) to pcibios_align_resource().
> Actually, I've replied to that before and it seems the reply was lost again.

It's not that I missed the previous discussion, it's just that I
didn't think we had come up with a clear explanation for why we needed
two kinds of alignment hooks and a way to decide when to use one vs.
the other.  It might be clear in *your* head, but it's not clear to me
yet :)

> Anyway, here's the full message about the discussion.
>
> https://patchwork.kernel.org/patch/1203171/
>
>>> Hmm..'struct resource *window' may not yet know which aperature it is
>>> allocated from. Will it? We are still in the sizing process, the allocation will
>>> be done much later.
>>
>> Of course, you're absolutely right; I had this backwards.  In
>> pbus_size_io/mem(), we do "b_res = find_free_bus_resource()", so b_res
>> is a bus resource that matches the desired type (IO/MEM).  This
>> resource represents an aperture of the upstream bridge leading to the
>> bus.  I was thinking that b_res->start would contain address
>> information that the arch could use to decide alignment.
>>
>> But at this point, in pbus_size_io/mem(), we set "b_res->start =
>> min_align", so obviously b_res contains no information about the
>> window base that will eventually be assigned.  I think b_res is
>> basically the *container* into which we'll eventually put the P2P
>> aperture start/end, but here, we're using that container to hold the
>> information about the size and alignment we need for that aperture.
>>
>> The fact that we deal with alignment in pbus_size_mem() and again in
>> __pci_assign_resource() (via pcibios_align_resource) is confusing to
>> me -- I don't have a clear idea of what sorts of alignment are done in
>> each place.  Could this powerpc alignment be done in
>> pcibios_align_resource()?  We do have the actual proposed address
>> there, as well as the pci_dev.
>
> Currently, we have 2 phases for resourece reallocation. During the first
> phase that is being covered by function pbus_size_io() and pbus_size_mem(),
> the PCI hierarchy tree is scanned from bottom to top so that we can reserve
> enough resources in p2p I/O or memory windows for its child devices (including
> child p2p bridge possiblly). During the time, pcibios_window_alignment() will
> be called to determine the minimal alignment of the p2p windows (I/O or memory)
> so that the p2p windows could be aligned to what the specific platform requires.
>
> In the 2nd phase, the resources of p2p windows as well as PCI devices will
> be allocated. That will be done according to the PCI hierarchy tree from top
> to bottom. During the period, function pcibios_align_resource() will be called
> to do _little_ adjustment on those resources that have special requirments. If
> I understood correctly, it's unsafe to change the resource size (or start address)
> greatly.
>
> Lets have one example here for more explanation on why it's unsafe to change
> the resource size (or start address) greatly by pcibios_align_resource(). As
> the following figure shows, we have 2 p2p bridges (A, B) and another one PCI
> device (C). The memory resources they're owning are shown below them after the
> phase one. Now, we're running in phase 2 (resource allocation) for (B) when the
> allocation on resources of (A) should have done because phase 2 is expected to
> be done from top to bottom of the PCI tree. It is impossible to extend the resource
> of (B) for another one 1MB to [0x100000, 0x2fffff] through pcibios_align_resource()
> since (C) needs some resources as well. The cause is that we never did enough resource
> reservation for (A) during phase 1. So pcibios_align_resource() isn't safe enough to
> cover the PowerPC alignment we requires, but pcibios_window_alignment() can accomodate
> that very well :-)
>
>                 [ p2p bridge A ]
>                 [0x100000, 0x2fffff]
>                       |
>         ---------------------------------
>         |                               |
>       [ p2p bridge B ]              [ PCI dev C]
>       [0x100000, 0x1fffff]
>
> In summary, pcibios_window_alignment() takes affect in phase 1 (resource reservation),
> but pcibios_align_resource() will function in phase 2 (resource allocation). They're
> different.

This is a good start.  You're saying that "large" alignments must be
done in phase 1 with pcibios_window_alignment() and
pcibios_align_resource() can do "small" adjustments in phase 2.  How
do we quantify that?  *Can* it be quantified?

I think pcibios_align_resource() is primarily used to deal with
devices that only do partial decoding, e.g., ISA 10-bit I/O port
resources.  By the time we call it, I think the upstream aperture has
already been determined, so the allocation may fail completely.

It seems like we can't accurately do even the resource reservation
part without knowing what pcibios_align_resource() is going to do.

Bjorn

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

[Bjorn Helgaas]

On Mon, Aug 20, 2012 at 7:46 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> 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>

I don't understand the point of patches 1-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

As far as I can tell, they don't have anything to do with patches 4-8.

I don't mind considering them later if they make things easier to read
or if they enable some other rework, but right now I want to make
progress on the P2P window alignment stuff, and patches 1-3 seem to be
distractions from that.

>  drivers/pci/host-bridge.c |    4 ++--
>  1 files 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.5.4
>

Re: [PATCH 5/8] pci: resource assignment based on p2p alignment

[Bjorn Helgaas]

On Mon, Aug 20, 2012 at 7:46 AM, Gavin Shan <shangw@linux.vnet.ibm.com> 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 window_alignment().
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/setup-bus.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 826a4f3..bb2eade 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -741,7 +741,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 min_align = 4096, align;
> +       resource_size_t io_align, min_align, align;
>
>         if (!b_res)
>                 return;
> @@ -750,8 +750,14 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>          * Per spec, I/O windows are 4K-aligned, but some bridges have an
>          * extension to support 1K alignment.
>          */
> -       if (bus->self->io_window_1k)
> +       if (bus->self->io_window_1k) {
> +               io_align = 4096;

Why is io_align 4K instead of 1K here?

>                 min_align = 1024;
> +       } else {
> +               io_align = window_alignment(bus, IORESOURCE_IO);

This doesn't seem right.  Only the non-1k path uses
window_alignment(), so there's no way for the platform to influence
the alignment of bridges that support 1k windows.  I think the
io_window_1k checking needs to be moved from pbus_size_io() to
window_alignment().

Maybe you can then make the min_align handling in pbus_size_io() more
similar to that in pbus_size_mem(), e.g.,

    min_align = 0;
    ...
    min_align = max(min_align, window_alignment(bus, b_res->flags & mask));

> +               min_align = io_align;
> +       }
> +
>         list_for_each_entry(dev, &bus->devices, bus_list) {
>                 int i;
>
> @@ -778,8 +784,8 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>                 }
>         }
>
> -       if (min_align > 4096)
> -               min_align = 4096;
> +       if (min_align > io_align)
> +               min_align = io_align;
>
>         size0 = calculate_iosize(size, min_size, size1,
>                         resource_size(b_res), min_align);
> @@ -901,6 +907,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>                         min_align = align1 >> 1;
>                 align += aligns[order];
>         }
> +
> +       min_align = max(min_align, window_alignment(bus, b_res->flags & mask));
>         size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
>         if (children_add_size > add_size)
>                 add_size = children_add_size;
> --
> 1.7.5.4
>
> --
> 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

[PATCH 2/8] 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 files 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.5.4