On Tue, Mar 10, 2015 at 09:36:58PM -0500, Bjorn Helgaas wrote:

On Mon, Mar 02, 2015 at 03:32:47PM +0800, Wei Yang wrote:

quoted

On Tue, Feb 24, 2015 at 02:41:52AM -0600, Bjorn Helgaas wrote:

quoted

On Tue, Feb 24, 2015 at 02:34:06AM -0600, Bjorn Helgaas wrote:

quoted

From: Wei Yang <redacted>

When sizing and assigning resources, we divide the resources into two
lists: the requested list and the additional list.  We don't consider the
alignment of additional VF(n) BAR space.

This is reasonable because the alignment required for the VF(n) BAR space
is the size of an individual VF BAR, not the size of the space for *all*
VFs.  But some platforms, e.g., PowerNV, require additional alignment.

Consider the additional IOV BAR alignment when sizing and assigning
resources.  When there is not enough system MMIO space, the PF's IOV BAR
alignment will not contribute to the bridge.  When there is enough system
MMIO space, the additional alignment will contribute to the bridge.

I don't understand the ""when there is not enough system MMIO space" part.
How do we tell if there's enough MMIO space?

In __assign_resources_sorted(), it has two resources list, one for requested
(head) and one for additional (realloc_head). This function will first try to
combine them and assign. If failed, this means we don't have enough MMIO
space.

How about this text:

 This is because the alignment required for the VF(n) BAR space is the size
 of an individual VF BAR, not the size of the space for *all* VFs.  But we
 want additional alignment to support partitioning on PowerNV.

 Consider the additional IOV BAR alignment when sizing and assigning
 resources.  When there is not enough system MMIO space to accomodate both
 the requested list and the additional list, the PF's IOV BAR alignment will
 not contribute to the bridge.  When there is enough system MMIO space for
 both lists, the additional alignment will contribute to the bridge.

We're doing something specifically for PowerNV.  I would really like to be
able to read this patch and say "Oh, here's the hook where we get the
PowerNV behavior, and it's obvious that other platforms are unaffected."
But I don't see a pcibios or similar hook, so I don't know where that
PowerNV behavior is.

Is it something to do with get_res_add_align()?  That uses min_align, but I
don't know how that's connected ...  ah, I see, "add_align" is computed
from pci_resource_alignment(), which has this path:

 pci_resource_alignment
   pci_sriov_resource_alignment
     pcibios_iov_resource_alignment

and powerpc has a special pcibios_iov_resource_alignment() for PowerNV.

Thanks for the text. I have added these in the change log and some description
about how it give arch a chance to be involved.

quoted

quoted

quoted

Also, take advantage of pci_dev_resource::min_align to store this
additional alignment.

This comment doesn't seem to make sense; this patch doesn't save anything
in min_align.

At the end of this patch:

   add_to_list(realloc_head, bus->self, b_res, size1-size0, add_align);

The add_align is stored in pci_dev_resource::min_align in add_to_list(). And
retrieved by get_res_add_align() in below code. This field is not used
previously, so I took advantage of this field to store the alignment of the
additional resources.

Hmm.  pci_dev_resource::min_align *is* already used in
reassign_resources_sorted().  Maybe there's no overlap; I gave up the
analysis before I could convince myself.

Bjorn,

I know you may have some concern on this, let me try to explain how I
understand the code. If my understanding is not correct, please let me know.

In __assign_resources_sorted(), we pass two resources list, one is required
and the other is the additional. First, we try our best to assigned both of
them by merge them together. If this fails, we will assign the required list
first and then take care of the additional list.

There is one interesting thing in the first step. We merge these two list to
the required list and in this patch I fix the alignment in required list.
(Which is the "head" list in code.) And before doing so, we save the original
information in "save_head". When we fail to assign the merged list, we will
restore the required list, this mean we clean the alignment done in this patch
and make sure we assign the required resource just with basic alignment.

The usage of the min_align in reassign_resources_sorted() happens in the
second part to assign the additional list individually. In the realloc_head
list, those resources still have the add_align which is calculated in
pbus_size_mem(). And we try to allocate it with this alignment, which is
exactly what we want.

BTW, by reading the code again, it looks I missed to change one place in
reassign_resources_sorted(). In condition when (!resource_size(res)) is true,
we rely on the res->start to be the alignment. Since the alignment is no
longer the start address, we need to fix this part too.

I may miss some background of the code, if my understanding is not correct,
glad to hear from you.

The changelog needs to mention the add_to_list() connection.

Added in the change log.

quoted

quoted

quoted

+		/*
+		 * There are two kinds of additional resources in the list:
+		 * 1. bridge resource  -- IORESOURCE_STARTALIGN
+		 * 2. SR-IOV resource   -- IORESOURCE_SIZEALIGN
+		 * Here just fix the additional alignment for bridge
+		 */
+		if (!(dev_res->res->flags & IORESOURCE_STARTALIGN))
+			continue;
+
+		add_align = get_res_add_align(realloc_head, dev_res->res);
+
+		/* Reorder the list by their alignment */

Why do we need to reorder the list by alignment?

Resource list "head" is sorted by the alignment, while the alignment would be
changed after we considering the additional resource.

Take powernv platform as an example. The IOV BAR is expanded and need to be
aligned with its total size instead of the individual VF BAR size. If we don't
reorder it, the IOV BAR would be assigned after some other resources, which
may cause the real assignment fail even the total size is enough.

This is worthy of a comment in the code.

Bjorn
--
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

-- 
Richard Yang
Help you, Help me