[Qemu-devel] [PATCH v2 0/2] pciinit: fix overflow when bar allocation

[Qemu-devel] [PATCH v2 0/2] pciinit: fix overflow when bar allocation

[Isaku Yamahata]

Okay, now added comments to members and functions as requested.
No essential code change.

Changes v1 -> v2:
- add comment.

Patch description:
This patch set fixes PCI bar allocation when bar overflow occured.
I checked if pmm_alloc facility can be used, but it doesn't suit for
pci bar allocation. So I resulted in new API, pci_region which
encapsulates region allocation and overflow checks.
The first patch introduces pci_region, and the second patch fixes
the overflow case with pci_region.

Isaku Yamahata (2):
  pci: introduce pci_region to manage pci io/memory/prefmemory regions.
  pciinit: use pci_region functions.

 Makefile         |    3 +-
 src/pci_region.c |   77 ++++++++++++++++++++++++++++++++++
 src/pciinit.c    |  122 ++++++++++++++++++++++++++---------------------------
 src/util.h       |   28 ++++++++++++
 4 files changed, 167 insertions(+), 63 deletions(-)
 create mode 100644 src/pci_region.c

[Qemu-devel] [PATCH v2 1/2] pci: introduce pci_region to manage pci io/memory/prefmemory regions.

[Isaku Yamahata]

This patch adds helper functions to manage pci area.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

---
Changes v1 -> v2
- add comments
---
 Makefile         |    3 +-
 src/pci_region.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/util.h       |   28 +++++++++++++++++++
 3 files changed, 107 insertions(+), 1 deletions(-)
 create mode 100644 src/pci_region.c

diff --git a/Makefile b/Makefile
index 9d412f1..1663a5d 100644
--- a/Makefile
+++ b/Makefile
@@ -19,7 +19,8 @@ SRCBOTH=misc.c pmm.c stacks.c output.c util.c block.c floppy.c ata.c mouse.c \
 SRC16=$(SRCBOTH) system.c disk.c font.c
 SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c coreboot.c boot.c \
       acpi.c smm.c mptable.c smbios.c pciinit.c optionroms.c mtrr.c \
-      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c
+      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c \
+      pci_region.c
 SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
 
 cc-option = $(shell if test -z "`$(1) $(2) -S -o /dev/null -xc \
diff --git a/src/pci_region.c b/src/pci_region.c
new file mode 100644
index 0000000..c8656cb
--- /dev/null
+++ b/src/pci_region.c
@@ -0,0 +1,77 @@
+// helper functions to manage pci io/memory/prefetch memory region
+//
+// Copyright (C) 2009 Isaku Yamahata <yamahata at valinux co jp>
+//
+// This file may be distributed under the terms of the GNU LGPLv3 license.
+//
+//
+
+#include "util.h"
+
+#define PCI_REGION_DISABLED     (-1)
+
+void pci_region_init(struct pci_region *r, u32 start, u32 end)
+{
+    r->start = start;
+    r->end = end;
+
+    r->cur_end = start;
+}
+
+// PCI_REGION_DISABLED represents that the region is in special state.
+// its value is chosen such that cur_end can't be PCI_REGION_DISABLED normally.
+u32 pci_region_disable(struct pci_region *r)
+{
+    return r->cur_end = PCI_REGION_DISABLED;
+}
+
+static int pci_region_disabled(const struct pci_region *r)
+{
+    return r->cur_end == PCI_REGION_DISABLED;
+}
+
+static u32 pci_region_alloc_align(struct pci_region *r, u32 size, u32 align)
+{
+    if (pci_region_disabled(r)) {
+        return 0;
+    }
+
+    u32 s = ALIGN(r->cur_end, align);
+    if (s > r->end || s < r->cur_end) {
+        return 0;
+    }
+    u32 e = s + size;
+    if (e > r->end || e < s) {
+        return 0;
+    }
+    r->cur_end = e;
+    return s;
+}
+
+u32 pci_region_alloc(struct pci_region *r, u32 size)
+{
+    return pci_region_alloc_align(r, size, size);
+}
+
+u32 pci_region_align(struct pci_region *r, u32 align)
+{
+    return pci_region_alloc_align(r, 0, align);
+}
+
+void pci_region_revert(struct pci_region *r, u32 addr)
+{
+    r->cur_end = addr;
+}
+
+u32 pci_region_addr(const struct pci_region *r)
+{
+    if (pci_region_disabled(r)){
+        return r->end;
+    }
+    return r->cur_end;
+}
+
+u32 pci_region_size(const struct pci_region *r)
+{
+    return r->end - r->start;
+}
diff --git a/src/util.h b/src/util.h
index 5cc9f17..17eedd0 100644
--- a/src/util.h
+++ b/src/util.h
@@ -344,6 +344,34 @@ void qemu_prep_reset(void);
 void smm_save_and_copy(void);
 void smm_relocate_and_restore(void);
 
+// pci_region.c
+// region allocator. pci region allocates the requested region
+// sequentially with overflow check.
+struct pci_region {
+    // The region is [start, region).
+    u32 start;
+    u32 end;
+
+    // The next allocation is start from this.
+    // i.e. [start, cur_end) is allocated.
+    // Right after initialization cur_end == start.
+    u32 cur_end;
+};
+// initialize the pci_region of [start, end)
+void pci_region_init(struct pci_region *r, u32 start, u32 end);
+// allocate the region of size
+u32 pci_region_alloc(struct pci_region *r, u32 size);
+// make the next allocation aligned to align
+u32 pci_region_align(struct pci_region *r, u32 align);
+// revert the allocation to addr.
+void pci_region_revert(struct pci_region *r, u32 addr);
+// make the allocation fail.
+u32 pci_region_disable(struct pci_region *r);
+// returns the current allocation point.
+u32 pci_region_addr(const struct pci_region *r);
+// returns the region size.
+u32 pci_region_size(const struct pci_region *r);
+
 // pciinit.c
 extern const u8 pci_irqs[4];
 void pci_bios_allocate_regions(u16 bdf, void *arg);
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH v2 1/2] pci: introduce pci_region to manage pci io/memory/prefmemory regions.

[Michael S. Tsirkin]

On Thu, Oct 28, 2010 at 02:04:05PM +0900, Isaku Yamahata wrote:
> This patch adds helper functions to manage pci area.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> ---
> Changes v1 -> v2
> - add comments
> ---
>  Makefile         |    3 +-
>  src/pci_region.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util.h       |   28 +++++++++++++++++++
>  3 files changed, 107 insertions(+), 1 deletions(-)
>  create mode 100644 src/pci_region.c
> 
> diff --git a/Makefile b/Makefile
> index 9d412f1..1663a5d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -19,7 +19,8 @@ SRCBOTH=misc.c pmm.c stacks.c output.c util.c block.c floppy.c ata.c mouse.c \
>  SRC16=$(SRCBOTH) system.c disk.c font.c
>  SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c coreboot.c boot.c \
>        acpi.c smm.c mptable.c smbios.c pciinit.c optionroms.c mtrr.c \
> -      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c
> +      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c \
> +      pci_region.c
>  SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
>  
>  cc-option = $(shell if test -z "`$(1) $(2) -S -o /dev/null -xc \
> diff --git a/src/pci_region.c b/src/pci_region.c
> new file mode 100644
> index 0000000..c8656cb
> --- /dev/null
> +++ b/src/pci_region.c
> @@ -0,0 +1,77 @@
> +// helper functions to manage pci io/memory/prefetch memory region
> +//
> +// Copyright (C) 2009 Isaku Yamahata <yamahata at valinux co jp>
> +//
> +// This file may be distributed under the terms of the GNU LGPLv3 license.
> +//
> +//
> +
> +#include "util.h"
> +
> +#define PCI_REGION_DISABLED     (-1)
> +
> +void pci_region_init(struct pci_region *r, u32 start, u32 end)
> +{
> +    r->start = start;
> +    r->end = end;
> +
> +    r->cur_end = start;
> +}
> +
> +// PCI_REGION_DISABLED represents that the region is in special state.
> +// its value is chosen such that cur_end can't be PCI_REGION_DISABLED normally.
> +u32 pci_region_disable(struct pci_region *r)
> +{
> +    return r->cur_end = PCI_REGION_DISABLED;
> +}
> +
> +static int pci_region_disabled(const struct pci_region *r)
> +{
> +    return r->cur_end == PCI_REGION_DISABLED;
> +}
> +
> +static u32 pci_region_alloc_align(struct pci_region *r, u32 size, u32 align)
> +{
> +    if (pci_region_disabled(r)) {
> +        return 0;
> +    }
> +
> +    u32 s = ALIGN(r->cur_end, align);
> +    if (s > r->end || s < r->cur_end) {
> +        return 0;
> +    }
> +    u32 e = s + size;
> +    if (e > r->end || e < s) {
> +        return 0;
> +    }
> +    r->cur_end = e;
> +    return s;
> +}
> +
> +u32 pci_region_alloc(struct pci_region *r, u32 size)
> +{
> +    return pci_region_alloc_align(r, size, size);
> +}
> +
> +u32 pci_region_align(struct pci_region *r, u32 align)
> +{
> +    return pci_region_alloc_align(r, 0, align);
> +}
> +
> +void pci_region_revert(struct pci_region *r, u32 addr)
> +{
> +    r->cur_end = addr;
> +}
> +
> +u32 pci_region_addr(const struct pci_region *r)
> +{
> +    if (pci_region_disabled(r)){
> +        return r->end;
> +    }
> +    return r->cur_end;
> +}
> +
> +u32 pci_region_size(const struct pci_region *r)
> +{
> +    return r->end - r->start;
> +}
> diff --git a/src/util.h b/src/util.h
> index 5cc9f17..17eedd0 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -344,6 +344,34 @@ void qemu_prep_reset(void);
>  void smm_save_and_copy(void);
>  void smm_relocate_and_restore(void);
>  
> +// pci_region.c
> +// region allocator. pci region allocates the requested region
> +// sequentially with overflow check.
> +struct pci_region {
> +    // The region is [start, region).

[start end)?

> +    u32 start;
> +    u32 end;
> +
> +    // The next allocation is start from this.
> +    // i.e. [start, cur_end) is allocated.
> +    // Right after initialization cur_end == start.
> +    u32 cur_end;

What is the value for a region at 0xfffff000 and size 0x1000?

Maybe first, last is better to detect this?

> +};
> +// initialize the pci_region of [start, end)
> +void pci_region_init(struct pci_region *r, u32 start, u32 end);
> +// allocate the region of size
> +u32 pci_region_alloc(struct pci_region *r, u32 size);
> +// make the next allocation aligned to align
> +u32 pci_region_align(struct pci_region *r, u32 align);
> +// revert the allocation to addr.
> +void pci_region_revert(struct pci_region *r, u32 addr);
> +// make the allocation fail.
> +u32 pci_region_disable(struct pci_region *r);
> +// returns the current allocation point.
> +u32 pci_region_addr(const struct pci_region *r);
> +// returns the region size.
> +u32 pci_region_size(const struct pci_region *r);
> +
>  // pciinit.c
>  extern const u8 pci_irqs[4];
>  void pci_bios_allocate_regions(u16 bdf, void *arg);
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH v2 1/2] pci: introduce pci_region to manage pci io/memory/prefmemory regions.

[Isaku Yamahata]

On Thu, Oct 28, 2010 at 07:56:41AM +0200, Michael S. Tsirkin wrote:
> > diff --git a/src/util.h b/src/util.h
> > index 5cc9f17..17eedd0 100644
> > --- a/src/util.h
> > +++ b/src/util.h
> > @@ -344,6 +344,34 @@ void qemu_prep_reset(void);
> >  void smm_save_and_copy(void);
> >  void smm_relocate_and_restore(void);
> >  
> > +// pci_region.c
> > +// region allocator. pci region allocates the requested region
> > +// sequentially with overflow check.
> > +struct pci_region {
> > +    // The region is [start, region).
> 
> [start end)?

Yes. Will fix.

> > +    u32 start;
> > +    u32 end;
> > +
> > +    // The next allocation is start from this.
> > +    // i.e. [start, cur_end) is allocated.
> > +    // Right after initialization cur_end == start.
> > +    u32 cur_end;
> 
> What is the value for a region at 0xfffff000 and size 0x1000?
>
> 
> Maybe first, last is better to detect this?

You mean [first, last] instead of [start, end).
Maybe Makes sense.
Anyway the area right below 4G can't be used for PCI
because it's used for LAPIC and so on.


> 
> > +};
> > +// initialize the pci_region of [start, end)
> > +void pci_region_init(struct pci_region *r, u32 start, u32 end);
> > +// allocate the region of size
> > +u32 pci_region_alloc(struct pci_region *r, u32 size);
> > +// make the next allocation aligned to align
> > +u32 pci_region_align(struct pci_region *r, u32 align);
> > +// revert the allocation to addr.
> > +void pci_region_revert(struct pci_region *r, u32 addr);
> > +// make the allocation fail.
> > +u32 pci_region_disable(struct pci_region *r);
> > +// returns the current allocation point.
> > +u32 pci_region_addr(const struct pci_region *r);
> > +// returns the region size.
> > +u32 pci_region_size(const struct pci_region *r);
> > +
> >  // pciinit.c
> >  extern const u8 pci_irqs[4];
> >  void pci_bios_allocate_regions(u16 bdf, void *arg);
> > -- 
> > 1.7.1.1
> 

-- 
yamahata

[Qemu-devel] [PATCH v2 2/2] pciinit: use pci_region functions.

[Isaku Yamahata]

This patch cleans up pci region allocation with pci_region.
Now it is aware of overflow.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 src/pciinit.c |  122 ++++++++++++++++++++++++++++-----------------------------
 1 files changed, 60 insertions(+), 62 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index 0346423..2a01aaa 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -17,9 +17,10 @@
 
 static void pci_bios_init_device_in_bus(int bus);
 
-static u32 pci_bios_io_addr;
-static u32 pci_bios_mem_addr;
-static u32 pci_bios_prefmem_addr;
+static struct pci_region pci_bios_io_region;
+static struct pci_region pci_bios_mem_region;
+static struct pci_region pci_bios_prefmem_region;
+
 /* host irqs corresponding to PCI irqs A-D */
 const u8 pci_irqs[4] = {
     10, 10, 11, 11
@@ -54,7 +55,7 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
  */
 static int pci_bios_allocate_region(u16 bdf, int region_num)
 {
-    u32 *paddr;
+    struct pci_region *r;
     u32 ofs = pci_bar(bdf, region_num);
 
     u32 old = pci_config_readl(bdf, ofs);
@@ -74,41 +75,34 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
 
     u32 size = (~(val & mask)) + 1;
     if (val != 0) {
+        const char *type;
+        const char *msg;
         if (val & PCI_BASE_ADDRESS_SPACE_IO) {
-            paddr = &pci_bios_io_addr;
-            if (ALIGN(*paddr, size) + size >= 64 * 1024) {
-                dprintf(1,
-                        "io region of (bdf 0x%x bar %d) can't be mapped.\n",
-                        bdf, region_num);
-                size = 0;
-            }
+            r = &pci_bios_io_region;
+            type = "io";
+            msg = "";
         } else if ((val & PCI_BASE_ADDRESS_MEM_PREFETCH) &&
-                 /* keep behaviour on bus = 0 */
-                 pci_bdf_to_bus(bdf) != 0 &&
-                 /* If pci_bios_prefmem_addr == 0, keep old behaviour */
-                 pci_bios_prefmem_addr != 0) {
-            paddr = &pci_bios_prefmem_addr;
-            if (ALIGN(*paddr, size) + size >= BUILD_PCIPREFMEM_END) {
-                dprintf(1,
-                        "prefmem region of (bdf 0x%x bar %d) can't be mapped. "
-                        "decrease BUILD_PCIMEM_SIZE and recompile. size %x\n",
-                        bdf, region_num, BUILD_PCIPREFMEM_SIZE);
-                size = 0;
-            }
+                   /* keep behaviour on bus = 0 */
+                   pci_bdf_to_bus(bdf) != 0 &&
+                   /* If pci_bios_prefmem_addr == 0, keep old behaviour */
+                   pci_region_addr(&pci_bios_prefmem_region) != 0) {
+            r = &pci_bios_prefmem_region;
+            type = "prefmem";
+            msg = "decrease BUILD_PCIMEM_SIZE and recompile. size %x";
         } else {
-            paddr = &pci_bios_mem_addr;
-            if (ALIGN(*paddr, size) + size >= BUILD_PCIMEM_END) {
-                dprintf(1,
-                        "mem region of (bdf 0x%x bar %d) can't be mapped. "
-                        "increase BUILD_PCIMEM_SIZE and recompile. size %x\n",
-                        bdf, region_num, BUILD_PCIMEM_SIZE);
-                size = 0;
-            }
+            r = &pci_bios_mem_region;
+            type = "mem";
+            msg = "increase BUILD_PCIMEM_SIZE and recompile.";
         }
-        if (size > 0) {
-            *paddr = ALIGN(*paddr, size);
-            pci_set_io_region_addr(bdf, region_num, *paddr);
-            *paddr += size;
+        u32 addr = pci_region_alloc(r, size);
+        if (addr > 0) {
+            pci_set_io_region_addr(bdf, region_num, addr);
+        } else {
+            size = 0;
+            dprintf(1,
+                    "%s region of (bdf 0x%x bar %d) can't be mapped. "
+                    "%s size %x\n",
+                    type, bdf, region_num, msg, pci_region_size(r));
         }
     }
 
@@ -163,33 +157,34 @@ static void pci_bios_init_device_bridge(u16 bdf, void *arg)
     pci_bios_allocate_region(bdf, 1);
     pci_bios_allocate_region(bdf, PCI_ROM_SLOT);
 
-    u32 io_old = pci_bios_io_addr;
-    u32 mem_old = pci_bios_mem_addr;
-    u32 prefmem_old = pci_bios_prefmem_addr;
+    u32 io_old = pci_region_addr(&pci_bios_io_region);
+    u32 mem_old = pci_region_addr(&pci_bios_mem_region);
+    u32 prefmem_old = pci_region_addr(&pci_bios_prefmem_region);
 
     /* IO BASE is assumed to be 16 bit */
-    pci_bios_io_addr = ALIGN(pci_bios_io_addr, PCI_IO_ALIGN);
-    pci_bios_mem_addr = ALIGN(pci_bios_mem_addr, PCI_MEMORY_ALIGN);
-    pci_bios_prefmem_addr =
-        ALIGN(pci_bios_prefmem_addr, PCI_PREF_MEMORY_ALIGN);
+    if (pci_region_align(&pci_bios_io_region, PCI_IO_ALIGN) == 0) {
+        pci_region_disable(&pci_bios_io_region);
+    }
+    if (pci_region_align(&pci_bios_mem_region, PCI_MEMORY_ALIGN) == 0) {
+        pci_region_disable(&pci_bios_mem_region);
+    }
+    if (pci_region_align(&pci_bios_prefmem_region,
+                         PCI_PREF_MEMORY_ALIGN) == 0) {
+        pci_region_disable(&pci_bios_prefmem_region);
+    }
 
-    u32 io_base = pci_bios_io_addr;
-    u32 mem_base = pci_bios_mem_addr;
-    u32 prefmem_base = pci_bios_prefmem_addr;
+    u32 io_base = pci_region_addr(&pci_bios_io_region);
+    u32 mem_base = pci_region_addr(&pci_bios_mem_region);
+    u32 prefmem_base = pci_region_addr(&pci_bios_prefmem_region);
 
     u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS);
     if (secbus > 0) {
         pci_bios_init_device_in_bus(secbus);
     }
 
-    pci_bios_io_addr = ALIGN(pci_bios_io_addr, PCI_IO_ALIGN);
-    pci_bios_mem_addr = ALIGN(pci_bios_mem_addr, PCI_MEMORY_ALIGN);
-    pci_bios_prefmem_addr =
-        ALIGN(pci_bios_prefmem_addr, PCI_PREF_MEMORY_ALIGN);
-
-    u32 io_end = pci_bios_io_addr;
-    if (io_end == io_base) {
-        pci_bios_io_addr = io_old;
+    u32 io_end = pci_region_align(&pci_bios_io_region, PCI_IO_ALIGN);
+    if (io_end == 0) {
+        pci_region_revert(&pci_bios_io_region, io_old);
         io_base = 0xffff;
         io_end = 1;
     }
@@ -198,18 +193,19 @@ static void pci_bios_init_device_bridge(u16 bdf, void *arg)
     pci_config_writeb(bdf, PCI_IO_LIMIT, (io_end - 1) >> PCI_IO_SHIFT);
     pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, 0);
 
-    u32 mem_end = pci_bios_mem_addr;
-    if (mem_end == mem_base) {
-        pci_bios_mem_addr = mem_old;
+    u32 mem_end = pci_region_align(&pci_bios_mem_region, PCI_MEMORY_ALIGN);
+    if (mem_end == 0) {
+        pci_region_revert(&pci_bios_mem_region, mem_old);
         mem_base = 0xffffffff;
         mem_end = 1;
     }
     pci_config_writew(bdf, PCI_MEMORY_BASE, mem_base >> PCI_MEMORY_SHIFT);
     pci_config_writew(bdf, PCI_MEMORY_LIMIT, (mem_end -1) >> PCI_MEMORY_SHIFT);
 
-    u32 prefmem_end = pci_bios_prefmem_addr;
-    if (prefmem_end == prefmem_base) {
-        pci_bios_prefmem_addr = prefmem_old;
+    u32 prefmem_end = pci_region_align(&pci_bios_prefmem_region,
+                                       PCI_PREF_MEMORY_ALIGN);
+    if (prefmem_end == 0) {
+        pci_region_revert(&pci_bios_prefmem_region, prefmem_old);
         prefmem_base = 0xffffffff;
         prefmem_end = 1;
     }
@@ -406,9 +402,11 @@ pci_setup(void)
 
     dprintf(3, "pci setup\n");
 
-    pci_bios_io_addr = 0xc000;
-    pci_bios_mem_addr = BUILD_PCIMEM_START;
-    pci_bios_prefmem_addr = BUILD_PCIPREFMEM_START;
+    pci_region_init(&pci_bios_io_region, 0xc000, 64 * 1024);
+    pci_region_init(&pci_bios_mem_region,
+                    BUILD_PCIMEM_START, BUILD_PCIMEM_END);
+    pci_region_init(&pci_bios_prefmem_region,
+                    BUILD_PCIPREFMEM_START, BUILD_PCIPREFMEM_END);
 
     pci_bios_init_bus();
 
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH v2 2/2] pciinit: use pci_region functions.

[Michael S. Tsirkin]

As a sepaate note, BIOS currently seems to allocate regions
in-order, correct?

A classical trick is to allocate regions behind each bridge in the
reverse order of their size. This avoids holes due to alignment.

-- 
MST

[Qemu-devel] Re: [PATCH v2 2/2] pciinit: use pci_region functions.

[Isaku Yamahata]

On Thu, Oct 28, 2010 at 08:33:55AM +0200, Michael S. Tsirkin wrote:
> As a sepaate note, BIOS currently seems to allocate regions
> in-order, correct?

Yes.


> A classical trick is to allocate regions behind each bridge in the
> reverse order of their size. This avoids holes due to alignment.

This is another topic.
For that trick, one more (or two?) path to walk pci bus would be necessary.
At the moment the simple one path is used.
-- 
yamahata