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:
quoted
On Tue, Jul 17, 2012 at 01:05:47PM +0800, Ram Pai wrote:
quoted
On Tue, Jul 17, 2012 at 10:23:17AM +0800, Gavin Shan wrote:
quoted
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 <redacted>
'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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-07-17 09:17:12
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.
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:
quoted
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-07-17 10:38:29
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.
On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
[off-list ref] wrote:
On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
quoted
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)
On Tue, Jul 17, 2012 at 11:14:51AM -0600, Bjorn Helgaas wrote:
On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
[off-list ref] wrote:
quoted
On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
quoted
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
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:
quoted
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
On Tue, Jul 17, 2012 at 10:25 PM, Ram Pai [off-list ref] wrote:
On Tue, Jul 17, 2012 at 11:14:51AM -0600, Bjorn Helgaas wrote:
quoted
On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
[off-list ref] wrote:
quoted
On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
quoted
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
On Wed, Jul 18, 2012 at 10:59:52AM -0600, Bjorn Helgaas wrote:
On Tue, Jul 17, 2012 at 10:25 PM, Ram Pai [off-list ref] wrote:
quoted
On Tue, Jul 17, 2012 at 11:14:51AM -0600, Bjorn Helgaas wrote:
quoted
On Tue, Jul 17, 2012 at 4:38 AM, Benjamin Herrenschmidt
[off-list ref] wrote:
quoted
On Tue, 2012-07-17 at 18:03 +0800, Ram Pai wrote:
quoted
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