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

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