Re: [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list

[Laszlo Ersek <lersek@redhat.com>]

On 05/02/18 20:07, Roman Bacik wrote:
> Some processors require resource list sorted in ascending order.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf       |  1 +
>  .../Bus/Pci/PciBusDxe/PciResourceSupport.c         | 43 ++++++++++++++++++----
>  MdeModulePkg/MdeModulePkg.dec                      |  3 ++
>  MdeModulePkg/MdeModulePkg.dsc                      |  1 +
>  4 files changed, 41 insertions(+), 7 deletions(-)

I don't understand the goal of this patch.

(Please don't take my comments as "review" or arguments against this
patch, I'd just like to understand the issue.)

To my understanding, InsertResourceNode() keeps PCI resources sorted in
descending *alignment* order -- because a larger power-of-two alignment
is also suitable for a smaller power-of-two alignment.

Say, we have a bridge with two devices behind it. The first device has
one MMIO BAR of size 32MB (requiring the same 32MB natural alignment),
while the other device has one MMIO BAR, of size 4MB (requiring the same
4MB natural alignment).

Ordering the resources in descending *alignment* order means that we'll
1st allocate the 32MB BAR, at a suitably aligned address. The important
thing is that the end of that allocation will *also* be aligned at 32MB,
hence we can allocate, as 2nd step, the 4MB BAR right there. No gaps
needed.

If the 4MB BAR was allocated 1st (naturally aligned), then its end
address might not be naturally aligned for becoming the base address of
the remaining 32MB BAR. Thus we might have to waste some MMIO aperture
to align the large BAR correctly.

Here's an example output from PciBusDxe running as part of OVMF:

> PciBus: Resource Map for Root Bridge PciRoot(0x0)
> [...]
> Type =  Mem32; Base = 0x80000000;       Length = 0x8100000;     Alignment = 0x3FFFFFF
>    Base = 0x80000000;   Length = 0x4000000;     Alignment = 0x3FFFFFF;  Owner = PCI [00|02|00:14]
>    Base = 0x84000000;   Length = 0x4000000;     Alignment = 0x3FFFFFF;  Owner = PCI [00|02|00:10]
>    Base = 0x88000000;   Length = 0x2000;        Alignment = 0x1FFF;     Owner = PCI [00|02|00:18]
>    Base = 0x88002000;   Length = 0x1000;        Alignment = 0xFFF;      Owner = PCI [00|07|00:14]
>    Base = 0x88003000;   Length = 0x1000;        Alignment = 0xFFF;      Owner = PCI [00|06|07:10]
>    Base = 0x88004000;   Length = 0x1000;        Alignment = 0xFFF;      Owner = PCI [00|05|00:14]
>    Base = 0x88005000;   Length = 0x1000;        Alignment = 0xFFF;      Owner = PCI [00|03|00:14]
> [...]

The base addresses are already sorted in ascending order; what's
descending is the alignment -- and that seems correct, because it
conserves MMIO aperture (no gaps needed).

Can you please explain what's wrong with this approach for your
platform, and what the desired behavior is?

Can you post a log snippet that shows a placement that's right for your
platform?

Thanks,
Laszlo