[PATCH v3 0/2] pciinit: fix overflow when bar allocation

[Isaku Yamahata]

Changes v2 -> v3:
- use [first, last] instead of [start, end)

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       |   29 +++++++++++++
 4 files changed, 168 insertions(+), 63 deletions(-)
 create mode 100644 src/pci_region.c

[PATCH v3 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

---
Changes v2 -> v3
- [first, last] instead of [start, end)

Changes v1 -> v2
- add comments
---
 Makefile         |    3 +-
 src/pci_region.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/util.h       |   29 ++++++++++++++++++++
 3 files changed, 108 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..1d9de71
--- /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 first, u32 last)
+{
+    r->first = first;
+    r->last = last;
+
+    r->cur_first = r->first;
+}
+
+// PCI_REGION_DISABLED represents that the region is in special state.
+// its value is chosen such that cur_first can't be PCI_REGION_DISABLED
+// normally.
+// NOTE: the area right below 4G is used for LAPIC, so such area can't
+//       be used for PCI memory.
+u32 pci_region_disable(struct pci_region *r)
+{
+    return r->cur_first = PCI_REGION_DISABLED;
+}
+
+static int pci_region_disabled(const struct pci_region *r)
+{
+    return r->cur_first == 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_first, align);
+    if (s > r->last || s < r->cur_first) {
+        return 0;
+    }
+    u32 e = s + size;
+    if (e < s || e - 1 > r->last) {
+        return 0;
+    }
+    r->cur_first = 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_first = addr;
+}
+
+u32 pci_region_addr(const struct pci_region *r)
+{
+    return r->cur_first;
+}
+
+u32 pci_region_size(const struct pci_region *r)
+{
+    return r->last - r->first + 1;
+}
diff --git a/src/util.h b/src/util.h
index 5cc9f17..18ab814 100644
--- a/src/util.h
+++ b/src/util.h
@@ -344,6 +344,35 @@ 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 [first, last].
+    u32 first;
+    u32 last;
+
+    // The next allocation starts from here.
+    // i.e. [start, cur_first) is allocated.
+    // Right after initialization cur_first == first.
+    u32 cur_first;
+};
+// initialize the pci_region of [first, last]
+// last must not be 0xffffffff
+void pci_region_init(struct pci_region *r, u32 first, u32 last);
+// 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);

[PATCH v3 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

---
Changes v2 -> v3
- pci_region_init() adjustment.
---
 src/pciinit.c |  122 ++++++++++++++++++++++++++++-----------------------------
 1 files changed, 60 insertions(+), 62 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index 0346423..795672b 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 - 1);
+    pci_region_init(&pci_bios_prefmem_region,
+                    BUILD_PCIPREFMEM_START, BUILD_PCIPREFMEM_END - 1);

pci_bios_init_bus();

[Kevin O'Connor]

On Thu, Oct 28, 2010 at 03:54:34PM +0900, Isaku Yamahata wrote:
> Changes v2 -> v3:
> use [first, last] instead of [start, end)
>
> 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.

Looks okay to me.  If there are no further comments, I'll commit.

BTW, as a minor nit - I'd prefer to put function descriptions in the
.c file next to the code instead of in the .h file - no need to resend
the patch though.

-Kevin

[Kevin O'Connor]

On Thu, Oct 28, 2010 at 03:54:34PM +0900, Isaku Yamahata wrote:
> Changes v2 -> v3:
> use [first, last] instead of [start, end)
>
> 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.

Thanks.  I've committed these.  I put them on the master branch - I
can also place them on the stable branch if they're needed there.

-Kevin