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

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

[Ram Pai]

On Tue, Jul 17, 2012 at 10:23:12AM +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().
> 
> 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.
> 
> 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,
	Do you need Yinghai's first 3 patches to implement your
	functionality?

	I dont see those new functions being called from anywhere
	in your patches.

RP

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

[Ram Pai]

On Tue, Jul 17, 2012 at 10:23:17AM +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 window_alignment().
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/setup-bus.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index c0fb9da..a29483a 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -731,6 +731,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;
> @@ -756,13 +757,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 = window_alignment(bus, IORESOURCE_IO);

this should also be
	io_align = max(4096, window_alignment(bus, IORESOURCE_IO));

right?


>  	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 "
> @@ -772,11 +775,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);
> @@ -875,6 +878,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, type));

  'type' can sometimes be (IORESOURCE_MEM | IORESOURCE_PREFETCH), which
  can lead to unpredictable results depending on how window_alignment()
  is implemented... Hence to be on the safer side I suggest

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

  

RP

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

[Ram Pai]

On Tue, Jul 17, 2012 at 01:05:47PM +0800, Ram Pai wrote:
> On Tue, Jul 17, 2012 at 10:23:17AM +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 window_alignment().
> > 
> > Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> > ---
> >  drivers/pci/setup-bus.c |   13 +++++++++----
> >  1 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index c0fb9da..a29483a 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -731,6 +731,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;
> > @@ -756,13 +757,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 = window_alignment(bus, IORESOURCE_IO);
> 
> this should also be
> 	io_align = max(4096, window_alignment(bus, IORESOURCE_IO));
> 
> right?
> 
> 
> >  	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 "
> > @@ -772,11 +775,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);
> > @@ -875,6 +878,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, type));
> 
>   'type' can sometimes be (IORESOURCE_MEM | IORESOURCE_PREFETCH), which
>   can lead to unpredictable results depending on how window_alignment()
>   is implemented... Hence to be on the safer side I suggest
> 
> 	min_align = max(min_align, window_alignment(bus, b_res->flags & mask));

While you are at it, can we move the min_align calculation into
a separate function?

Somewhat around the following patch


diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8fa2d4b..426c8ad6 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -762,6 +762,25 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	}
 }
 
+static inline calculate_min_align(resource_size_t aligns *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
  *
@@ -841,19 +860,9 @@ 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);
+	min_align = calculate_min_align(aligns, max_order);
 
-		if (!align)
-			min_align = align1;
-		else if (ALIGN(align + min_align, min_align) < align1)
-			min_align = align1 >> 1;
-		align += aligns[order];
-	}
 	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
 	if (children_add_size > add_size)
 		add_size = children_add_size;
@@ -880,6 +889,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	return 1;
 }

 RP

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

[Ram Pai]

On Tue, Jul 17, 2012 at 01:36:49PM +0800, Gavin Shan wrote:
> On Tue, Jul 17, 2012 at 01:23:33PM +0800, Ram Pai wrote:
> >On Tue, Jul 17, 2012 at 01:05:47PM +0800, Ram Pai wrote:
> >> On Tue, Jul 17, 2012 at 10:23:17AM +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 window_alignment().
> >> > 
> >> > Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> 
> [snip]
> 
> >> > @@ -772,11 +775,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);
> >> > @@ -875,6 +878,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, type));
> >> 
> >>   'type' can sometimes be (IORESOURCE_MEM | IORESOURCE_PREFETCH), which
> >>   can lead to unpredictable results depending on how window_alignment()
> >>   is implemented... Hence to be on the safer side I suggest
> >> 
> >> 	min_align = max(min_align, window_alignment(bus, b_res->flags & mask));
> 
> Sorry, Ram. I didn't see your concern in last reply. So I have to
> cover your conver in this reply.
> 
> I think it'd better to pass "type" directly because platform (e.g. powernv)
> expects both IORESOURCE_MEM as well as IORESOURCE_PREFETCH. 
> In future, powernv platform will return M32 segment size for IORESOURCE_MEM, but
> might return M64 segment size for (IORESOURCE_MEM | IORESOURCE_PREFETCH).

Hmm.. this code is not about determining what kind of segment the
platform is returning. This code is about using the right alignment
constraints for the type of segment from which resource will be
allocated. right?

b_res is the resource that is being sized. b_res already knows
what kind of resource it is, i.e IORESOURCE_MEM or IORESOURCE_PREFETCH.
Hence we should be exactly using the same alignment constraints as
that dictated by the type of b_res. no?

RP

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

[Benjamin Herrenschmidt]

On Tue, 2012-07-17 at 13:57 +0800, Ram Pai wrote:
> Hmm.. this code is not about determining what kind of segment the
> platform is returning. This code is about using the right alignment
> constraints for the type of segment from which resource will be
> allocated. right?
> 
> b_res is the resource that is being sized. b_res already knows
> what kind of resource it is, i.e IORESOURCE_MEM or
> IORESOURCE_PREFETCH.
> Hence we should be exactly using the same alignment constraints as
> that dictated by the type of b_res. no? 

This is unclear.... ideally we want to know which of the host bridge
"apertures" is about to be used...

IE. A prefetchable resource can very well be allocated to a
non-prefetchable window, though the other way isn't supposed to happen.

Additionally, our PHB doesn't actually differenciate prefetchable and
non-prefetchable windows (whether you can prefetch or not is an
attribute of the CPU mapping, but basically non-cachable mappings are
never prefetchable for us).

So we can be lax in how we assign things between our single 32-bit
window divided in 128 segments and our 16x64-bit windows divided in 8
segments (and future HW will do thins differently even).

For example we would like in some cases to use M64's (64-bit windows) to
map SR-IOV BARs regardless of the "prefetchability" though that can only
work if we are not behind a PCIe switch, as those are technically
allowed to prefetch :-)

Worst is that the alignment constraint is based on the segment size, and
while we more/less fix the size of the 32-bit window, we plan to
dynamically allocate/resize the 64-bit ones which will mean variable
segment sizes as well.

So the more information you can get at that point, the better. The type
is useful because it allows us to know if you are trying to put a
prefetchable memory BAR inside a non-prefetchable region, in which case
we know it has to be in M32.

Cheers,
Ben.

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

[Ram Pai]

On Tue, Jul 17, 2012 at 07:16:59PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2012-07-17 at 13:57 +0800, Ram Pai wrote:
> > Hmm.. this code is not about determining what kind of segment the
> > platform is returning. This code is about using the right alignment
> > constraints for the type of segment from which resource will be
> > allocated. right?
> > 
> > b_res is the resource that is being sized. b_res already knows
> > what kind of resource it is, i.e IORESOURCE_MEM or
> > IORESOURCE_PREFETCH.
> > Hence we should be exactly using the same alignment constraints as
> > that dictated by the type of b_res. no? 
> 
> This is unclear.... ideally we want to know which of the host bridge
> "apertures" is about to be used...
> 
> IE. A prefetchable resource can very well be allocated to a
> non-prefetchable window, though the other way isn't supposed to happen.
> 
> Additionally, our PHB doesn't actually differenciate prefetchable and
> non-prefetchable windows (whether you can prefetch or not is an
> attribute of the CPU mapping, but basically non-cachable mappings are
> never prefetchable for us).
> 
> So we can be lax in how we assign things between our single 32-bit
> window divided in 128 segments and our 16x64-bit windows divided in 8
> segments (and future HW will do thins differently even).
> 
> For example we would like in some cases to use M64's (64-bit windows) to
> map SR-IOV BARs regardless of the "prefetchability" though that can only
> work if we are not behind a PCIe switch, as those are technically
> allowed to prefetch :-)
> 
> Worst is that the alignment constraint is based on the segment size, and
> while we more/less fix the size of the 32-bit window, we plan to
> dynamically allocate/resize the 64-bit ones which will mean variable
> segment sizes as well.
> 
> So the more information you can get at that point, the better. The type
> is useful because it allows us to know if you are trying to put a
> prefetchable memory BAR inside a non-prefetchable region, in which case
> we know it has to be in M32.


Ben,
	Lets say we passed that 'type' flag to size the minimum
	alignment constraints for that b_res.  And window_alignment(bus,
	type) of your platform  used that 'type' information to
	determine whether to use the alignment constraints of 32-bit
	window or 64-bit window.

	However, later when the b_res is actually allocated a resource,
	the pci_assign_resource() has no idea whether to allocate 32-bit
	window resource or 64-bit window resource, because the 'type'
	information is not captured anywhere in b_res.

	You would basically have a disconnect between what is sized and 
	what is allocated. Unless offcourse you pass that 'type' to
	the b_res->flags, which is currently not the case.

RP

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

[Benjamin Herrenschmidt]

On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
>         Lets say we passed that 'type' flag to size the minimum
>         alignment constraints for that b_res.  And window_alignment(bus,
>         type) of your platform  used that 'type' information to
>         determine whether to use the alignment constraints of 32-bit
>         window or 64-bit window.
> 
>         However, later when the b_res is actually allocated a resource,
>         the pci_assign_resource() has no idea whether to allocate 32-bit
>         window resource or 64-bit window resource, because the 'type'
>         information is not captured anywhere in b_res.
> 
>         You would basically have a disconnect between what is sized and 
>         what is allocated. Unless offcourse you pass that 'type' to
>         the b_res->flags, which is currently not the case. 

Right, we ideally would need the core to query the alignment once per
"apertures" it tries as a candidate to allocate a given resource... but
that's for later.

For now we can probably live with giving out the max of the minimum
alignment we support for M64 and our M32 segment size.

Cheers,
Ben.

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

[Bjorn Helgaas]

On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
>>         Lets say we passed that 'type' flag to size the minimum
>>         alignment constraints for that b_res.  And window_alignment(bus,
>>         type) of your platform  used that 'type' information to
>>         determine whether to use the alignment constraints of 32-bit
>>         window or 64-bit window.
>>
>>         However, later when the b_res is actually allocated a resource,
>>         the pci_assign_resource() has no idea whether to allocate 32-bit
>>         window resource or 64-bit window resource, because the 'type'
>>         information is not captured anywhere in b_res.
>>
>>         You would basically have a disconnect between what is sized and
>>         what is allocated. Unless offcourse you pass that 'type' to
>>         the b_res->flags, which is currently not the case.
>
> Right, we ideally would need the core to query the alignment once per
> "apertures" it tries as a candidate to allocate a given resource... but
> that's for later.
>
> For now we can probably live with giving out the max of the minimum
> alignment we support for M64 and our M32 segment size.

We already know the aperture we're proposing to allocate from (the
result of find_free_bus_resource()), don't we?  What if we passed it
to pcibios_window_alignment() along with the struct pci_bus *, e.g.:

  resource_size_t pcibios_window_alignment(struct pci_bus *bus, struct
resource *window)

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

[Ram Pai]

On Tue, Jul 17, 2012 at 11:14:51AM -0600, Bjorn Helgaas wrote:
> On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
> >>         Lets say we passed that 'type' flag to size the minimum
> >>         alignment constraints for that b_res.  And window_alignment(bus,
> >>         type) of your platform  used that 'type' information to
> >>         determine whether to use the alignment constraints of 32-bit
> >>         window or 64-bit window.
> >>
> >>         However, later when the b_res is actually allocated a resource,
> >>         the pci_assign_resource() has no idea whether to allocate 32-bit
> >>         window resource or 64-bit window resource, because the 'type'
> >>         information is not captured anywhere in b_res.
> >>
> >>         You would basically have a disconnect between what is sized and
> >>         what is allocated. Unless offcourse you pass that 'type' to
> >>         the b_res->flags, which is currently not the case.
> >
> > Right, we ideally would need the core to query the alignment once per
> > "apertures" it tries as a candidate to allocate a given resource... but
> > that's for later.
> >
> > For now we can probably live with giving out the max of the minimum
> > alignment we support for M64 and our M32 segment size.
> 
> We already know the aperture we're proposing to allocate from (the
> result of find_free_bus_resource()), don't we?  What if we passed it
> to pcibios_window_alignment() along with the struct pci_bus *, e.g.:
> 
>   resource_size_t pcibios_window_alignment(struct pci_bus *bus, struct
> resource *window)

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.

RP

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

[Bjorn Helgaas]

On Tue, Jul 17, 2012 at 10:25 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Tue, Jul 17, 2012 at 11:14:51AM -0600, Bjorn Helgaas wrote:
>> On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>> > On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
>> >>         Lets say we passed that 'type' flag to size the minimum
>> >>         alignment constraints for that b_res.  And window_alignment(bus,
>> >>         type) of your platform  used that 'type' information to
>> >>         determine whether to use the alignment constraints of 32-bit
>> >>         window or 64-bit window.
>> >>
>> >>         However, later when the b_res is actually allocated a resource,
>> >>         the pci_assign_resource() has no idea whether to allocate 32-bit
>> >>         window resource or 64-bit window resource, because the 'type'
>> >>         information is not captured anywhere in b_res.
>> >>
>> >>         You would basically have a disconnect between what is sized and
>> >>         what is allocated. Unless offcourse you pass that 'type' to
>> >>         the b_res->flags, which is currently not the case.
>> >
>> > Right, we ideally would need the core to query the alignment once per
>> > "apertures" it tries as a candidate to allocate a given resource... but
>> > that's for later.
>> >
>> > For now we can probably live with giving out the max of the minimum
>> > alignment we support for M64 and our M32 segment size.
>>
>> We already know the aperture we're proposing to allocate from (the
>> result of find_free_bus_resource()), don't we?  What if we passed it
>> to pcibios_window_alignment() along with the struct pci_bus *, e.g.:
>>
>>   resource_size_t pcibios_window_alignment(struct pci_bus *bus, struct
>> resource *window)
>
> 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.

Bjorn

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

[Gavin Shan]

On Wed, Jul 18, 2012 at 10:59:52AM -0600, Bjorn Helgaas wrote:
>On Tue, Jul 17, 2012 at 10:25 PM, Ram Pai <linuxram@us.ibm.com> wrote:
>> On Tue, Jul 17, 2012 at 11:14:51AM -0600, Bjorn Helgaas wrote:
>>> On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
>>> <benh@kernel.crashing.org> wrote:
>>> > On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
>>> >>         Lets say we passed that 'type' flag to size the minimum
>>> >>         alignment constraints for that b_res.  And window_alignment(bus,
>>> >>         type) of your platform  used that 'type' information to
>>> >>         determine whether to use the alignment constraints of 32-bit
>>> >>         window or 64-bit window.
>>> >>
>>> >>         However, later when the b_res is actually allocated a resource,
>>> >>         the pci_assign_resource() has no idea whether to allocate 32-bit
>>> >>         window resource or 64-bit window resource, because the 'type'
>>> >>         information is not captured anywhere in b_res.
>>> >>
>>> >>         You would basically have a disconnect between what is sized and
>>> >>         what is allocated. Unless offcourse you pass that 'type' to
>>> >>         the b_res->flags, which is currently not the case.
>>> >
>>> > Right, we ideally would need the core to query the alignment once per
>>> > "apertures" it tries as a candidate to allocate a given resource... but
>>> > that's for later.
>>> >
>>> > For now we can probably live with giving out the max of the minimum
>>> > alignment we support for M64 and our M32 segment size.
>>>
>>> We already know the aperture we're proposing to allocate from (the
>>> result of find_free_bus_resource()), don't we?  What if we passed it
>>> to pcibios_window_alignment() along with the struct pci_bus *, e.g.:
>>>
>>>   resource_size_t pcibios_window_alignment(struct pci_bus *bus, struct
>>> resource *window)
>>
>> 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.
>

If I understood correctly, it's a bit hard to put PowerPC alignment in
the function pcibios_align_resource(). The target of those patches is
to make those I/O and memory windows of p2p bridges aligned based on
the special requirement from specific platform, so that we can put
the corresponding PCI bus directed from the p2p bridge into separate
EEH segment. Unforunately, pcibios_align_resource() was implemented
based on individual bars (resources) and individual bars doesn't
have the alignment requirement under current situation :-)

Thanks,
Gavin

>Bjorn
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev
>

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

[Ram Pai]

On Wed, Jul 18, 2012 at 09:07:46AM +0800, Gavin Shan wrote:
> 4. Either "b_res->flags & mask" or "type" to be used for window_alignment(),
>    I don't think it's big deal because "b_res->flags & mask == type" for
>    current implementation. However, I'm not sure I still need change it
>    to "type"?
> 
>    min_align = max(min_align, window_alignment(bus, b_res->flags & mask));

Ah. you are right.

(b_res->flags & mask) or type, either one is ok. I had a wrong
assumption about b_res->flags. I thought it has either IORESOURCE_MEM
set or IORESOURCE_PREFETCH set, but not both.

Whatever concern I had, i dont have it any more.
RP

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

[Ram Pai]

On Tue, Jul 17, 2012 at 08:38:18PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
> >         Lets say we passed that 'type' flag to size the minimum
> >         alignment constraints for that b_res.  And window_alignment(bus,
> >         type) of your platform  used that 'type' information to
> >         determine whether to use the alignment constraints of 32-bit
> >         window or 64-bit window.
> > 
> >         However, later when the b_res is actually allocated a resource,
> >         the pci_assign_resource() has no idea whether to allocate 32-bit
> >         window resource or 64-bit window resource, because the 'type'
> >         information is not captured anywhere in b_res.
> > 
> >         You would basically have a disconnect between what is sized and 
> >         what is allocated. Unless offcourse you pass that 'type' to
> >         the b_res->flags, which is currently not the case. 
> 
> Right, we ideally would need the core to query the alignment once per
> "apertures" it tries as a candidate to allocate a given resource... but
> that's for later.
> 
> For now we can probably live with giving out the max of the minimum
> alignment we support for M64 and our M32 segment size.

Its an approximation, which may not be terribly bad. But not comforting enough.

RP

[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

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
>