[PATCH 0/6] PCI init code refactoring

[Kevin O'Connor]

Some cleanups to the pciinit.c code.

These are some things I noticed while tracking down the recent bug
reports caused by calling ALIGN_DOWN with a zero alignment.  The fix
to that bug was small enough to commit immediately.  However, I think
it would be worthwhile to also commit the cleanups I found.

Given this is just a cleanup, I don't intend to commit until after
v1.6.3 is tagged.

-Kevin

Kevin O'Connor (6):
  Use standard formatting for PCI info during PCI init pass.
  Separate pciinit.c into clearly delineated sections.
  Simplify pci_bios_init_root_regions().
  Use pci->header_type in pci_bar() to avoid unnecessary
    pci_config_readb.
  Introduce PCI child device iterators and use in pciinit.c.
  Simplify pci_slot_get_irq().

src/pci.c     |    2 +
 src/pci.h     |   12 ++
 src/pciinit.c |  337 +++++++++++++++++++++++++++------------------------------
 3 files changed, 173 insertions(+), 178 deletions(-)

[PATCH 1/6] Use standard formatting for PCI info during PCI init pass.

[Kevin O'Connor]

Format BDF (bus, device, fn) and vendor:device debug output in a more
user-readable format.

Signed-off-by: Kevin O'Connor 
---
 src/pciinit.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index 0d8758e..ebd8152 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -235,8 +235,8 @@ static void pci_bios_init_device(struct pci_device *pci)
     u16 bdf = pci->bdf;
     int pin, pic_irq;

-    dprintf(1, "PCI: bus=%d devfn=0x%02x: vendor_id=0x%04x device_id=0x%04x\n"
-            , pci_bdf_to_bus(bdf), pci_bdf_to_devfn(bdf)
+    dprintf(1, "PCI: init bdf=%02x:%02x.%x id=%04x:%04x\n"
+            , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf)
             , pci->vendor, pci->device);
     pci_init_device(pci_class_tbl, pci, NULL);

@@ -507,9 +507,11 @@ static void pci_bios_map_device_in_bus(int bus)
     struct pci_device *pci;

foreachpci(pci) {
-        if (pci_bdf_to_bus(pci->bdf) != bus)
+        u16 bdf = pci->bdf;
+        if (pci_bdf_to_bus(bdf) != bus)
             continue;
-        dprintf(1, "PCI: map device bus %d, bfd 0x%x\n", bus, pci->bdf);
+        dprintf(1, "PCI: map device bdf=%02x:%02x.%x\n"
+                , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
         pci_bios_map_device(&busses[bus], pci);
     }
 }

[PATCH 2/6] Separate pciinit.c into clearly delineated sections.

[Kevin O'Connor]

There are four separate phases of the current PCI initialization code:
bus initialization, bus sizing, bar allocation, and misc device init.
Move the code exclusively called in each phase next to each other, and
clearly mark each section.

Signed-off-by: Kevin O'Connor 
---
 src/pciinit.c |  221 +++++++++++++++++++++++++++++++--------------------------
 1 files changed, 121 insertions(+), 100 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index ebd8152..62583a3 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -45,11 +45,6 @@ static struct pci_bus {
 } *busses;
 static int busses_count;

-static void pci_bios_init_device_in_bus(int bus);
-static void pci_bios_check_device_in_bus(int bus);
-static void pci_bios_init_bus_bases(struct pci_bus *bus);
-static void pci_bios_map_device_in_bus(int bus);
-
 static int pci_size_to_index(u32 size, enum pci_region_type type)
 {
     int index = __fls(size);
@@ -79,17 +74,6 @@ static enum pci_region_type pci_addr_to_type(u32 addr)
     return PCI_REGION_TYPE_MEM;
 }

-static u32 pci_size_roundup(u32 size)
-{
-    int index = __fls(size-1)+1;
-    return 0x1 << index;
-}
-
-/* host irqs corresponding to PCI irqs A-D */
-const u8 pci_irqs[4] = {
-    10, 10, 11, 11
-};
-
 static u32 pci_bar(u16 bdf, int region_num)
 {
     if (region_num != PCI_ROM_SLOT) {
@@ -111,6 +95,16 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
     pci_config_writel(bdf, ofs, addr);
 }

+
+/****************************************************************
+ * Misc. device init
+ ****************************************************************/
+
+/* host irqs corresponding to PCI irqs A-D */
+const u8 pci_irqs[4] = {
+    10, 10, 11, 11
+};
+
 /* return the global irq number corresponding to a given device irq
    pin. We could also use the bus number to have a more precise
    mapping. */
@@ -150,13 +144,6 @@ static const struct pci_device_id pci_isa_bridge_tbl[] = {
     PCI_DEVICE_END
 };

-#define PCI_IO_ALIGN            4096
-#define PCI_IO_SHIFT            8
-#define PCI_MEMORY_ALIGN        (1UL << 20)
-#define PCI_MEMORY_SHIFT        16
-#define PCI_PREF_MEMORY_ALIGN   (1UL << 20)
-#define PCI_PREF_MEMORY_SHIFT   16
-
 static void storage_ide_init(struct pci_device *pci, void *arg)
 {
     u16 bdf = pci->bdf;
@@ -267,6 +254,11 @@ static void pci_bios_init_device_in_bus(int bus)
     }
 }

+
+/****************************************************************
+ * Bus initialization
+ ****************************************************************/
+
 static void
 pci_bios_init_bus_rec(int bus, u8 *pci_bus)
 {
@@ -336,6 +328,17 @@ pci_bios_init_bus(void)
     busses_count = pci_bus + 1;
 }

+
+/****************************************************************
+ * Bus sizing
+ ****************************************************************/
+
+static u32 pci_size_roundup(u32 size)
+{
+    int index = __fls(size-1)+1;
+    return 0x1 << index;
+}
+
 static void pci_bios_bus_get_bar(struct pci_bus *bus, int bdf, int bar,
                                  u32 *val, u32 *size)
 {
@@ -370,15 +373,7 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
         bus->r[type].max = size;
 }

-static u32 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size)
-{
-    u32 index, addr;
-
-    index = pci_size_to_index(size, type);
-    addr = bus->r[type].bases[index];
-    bus->r[type].bases[index] += pci_index_to_size(index, type);
-    return addr;
-}
+static void pci_bios_check_device_in_bus(int bus);

static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
 {
@@ -430,6 +425,97 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
     }
 }

+static void pci_bios_check_device_in_bus(int bus)
+{
+    struct pci_device *pci;
+
+    dprintf(1, "PCI: check devices bus %d\n", bus);
+    foreachpci(pci) {
+        if (pci_bdf_to_bus(pci->bdf) != bus)
+            continue;
+        pci_bios_check_device(&busses[bus], pci);
+    }
+}
+
+#define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)
+
+static int pci_bios_init_root_regions(u32 start, u32 end)
+{
+    struct pci_bus *bus = &busses[0];
+
+    bus->r[PCI_REGION_TYPE_IO].base = 0xc000;
+
+    if (bus->r[PCI_REGION_TYPE_MEM].sum < bus->r[PCI_REGION_TYPE_PREFMEM].sum) {
+        bus->r[PCI_REGION_TYPE_MEM].base =
+            ROOT_BASE(end,
+                      bus->r[PCI_REGION_TYPE_MEM].sum,
+                      bus->r[PCI_REGION_TYPE_MEM].max);
+        bus->r[PCI_REGION_TYPE_PREFMEM].base =
+            ROOT_BASE(bus->r[PCI_REGION_TYPE_MEM].base,
+                      bus->r[PCI_REGION_TYPE_PREFMEM].sum,
+                      bus->r[PCI_REGION_TYPE_PREFMEM].max);
+        if (bus->r[PCI_REGION_TYPE_PREFMEM].base >= start) {
+            return 0;
+        }
+    } else {
+        bus->r[PCI_REGION_TYPE_PREFMEM].base =
+            ROOT_BASE(end,
+                      bus->r[PCI_REGION_TYPE_PREFMEM].sum,
+                      bus->r[PCI_REGION_TYPE_PREFMEM].max);
+        bus->r[PCI_REGION_TYPE_MEM].base =
+            ROOT_BASE(bus->r[PCI_REGION_TYPE_PREFMEM].base,
+                      bus->r[PCI_REGION_TYPE_MEM].sum,
+                      bus->r[PCI_REGION_TYPE_MEM].max);
+        if (bus->r[PCI_REGION_TYPE_MEM].base >= start) {
+            return 0;
+        }
+    }
+    return -1;
+}
+
+
+/****************************************************************
+ * BAR assignment
+ ****************************************************************/
+
+static void pci_bios_init_bus_bases(struct pci_bus *bus)
+{
+    u32 base, newbase, size;
+    int type, i;
+
+    for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
+        dprintf(1, "  type %s max %x sum %x base %x\n", region_type_name[type],
+                bus->r[type].max, bus->r[type].sum, bus->r[type].base);
+        base = bus->r[type].base;
+        for (i = ARRAY_SIZE(bus->r[type].count)-1; i >= 0; i--) {
+            size = pci_index_to_size(i, type);
+            if (!bus->r[type].count[i])
+                continue;
+            newbase = base + size * bus->r[type].count[i];
+            dprintf(1, "    size %8x: %d bar(s), %8x -> %8x\n",
+                    size, bus->r[type].count[i], base, newbase - 1);
+            bus->r[type].bases[i] = base;
+            base = newbase;
+        }
+    }
+}
+
+static u32 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size)
+{
+    u32 index, addr;
+
+    index = pci_size_to_index(size, type);
+    addr = bus->r[type].bases[index];
+    bus->r[type].bases[index] += pci_index_to_size(index, type);
+    return addr;
+}
+
+#define PCI_IO_SHIFT            8
+#define PCI_MEMORY_SHIFT        16
+#define PCI_PREF_MEMORY_SHIFT   16
+
+static void pci_bios_map_device_in_bus(int bus);
+
 static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
 {
     u16 bdf = dev->bdf;
@@ -490,18 +576,6 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
     }
 }

-static void pci_bios_check_device_in_bus(int bus)
-{
-    struct pci_device *pci;
-
-    dprintf(1, "PCI: check devices bus %d\n", bus);
-    foreachpci(pci) {
-        if (pci_bdf_to_bus(pci->bdf) != bus)
-            continue;
-        pci_bios_check_device(&busses[bus], pci);
-    }
-}
-
 static void pci_bios_map_device_in_bus(int bus)
 {
     struct pci_device *pci;
@@ -516,63 +590,10 @@ static void pci_bios_map_device_in_bus(int bus)
     }
 }

-static void pci_bios_init_bus_bases(struct pci_bus *bus)
-{
-    u32 base, newbase, size;
-    int type, i;
-
-    for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
-        dprintf(1, "  type %s max %x sum %x base %x\n", region_type_name[type],
-                bus->r[type].max, bus->r[type].sum, bus->r[type].base);
-        base = bus->r[type].base;
-        for (i = ARRAY_SIZE(bus->r[type].count)-1; i >= 0; i--) {
-            size = pci_index_to_size(i, type);
-            if (!bus->r[type].count[i])
-                continue;
-            newbase = base + size * bus->r[type].count[i];
-            dprintf(1, "    size %8x: %d bar(s), %8x -> %8x\n",
-                    size, bus->r[type].count[i], base, newbase - 1);
-            bus->r[type].bases[i] = base;
-            base = newbase;
-        }
-    }
-}
-
-#define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)
-
-static int pci_bios_init_root_regions(u32 start, u32 end)
-{
-    struct pci_bus *bus = &busses[0];
-
-    bus->r[PCI_REGION_TYPE_IO].base = 0xc000;

-    if (bus->r[PCI_REGION_TYPE_MEM].sum < bus->r[PCI_REGION_TYPE_PREFMEM].sum) {
-        bus->r[PCI_REGION_TYPE_MEM].base =
-            ROOT_BASE(end,
-                      bus->r[PCI_REGION_TYPE_MEM].sum,
-                      bus->r[PCI_REGION_TYPE_MEM].max);
-        bus->r[PCI_REGION_TYPE_PREFMEM].base =
-            ROOT_BASE(bus->r[PCI_REGION_TYPE_MEM].base,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].sum,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].max);
-        if (bus->r[PCI_REGION_TYPE_PREFMEM].base >= start) {
-            return 0;
-        }
-    } else {
-        bus->r[PCI_REGION_TYPE_PREFMEM].base =
-            ROOT_BASE(end,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].sum,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].max);
-        bus->r[PCI_REGION_TYPE_MEM].base =
-            ROOT_BASE(bus->r[PCI_REGION_TYPE_PREFMEM].base,
-                      bus->r[PCI_REGION_TYPE_MEM].sum,
-                      bus->r[PCI_REGION_TYPE_MEM].max);
-        if (bus->r[PCI_REGION_TYPE_MEM].base >= start) {
-            return 0;
-        }
-    }
-    return -1;
-}
+/****************************************************************
+ * Main setup code
+ ****************************************************************/

void
 pci_setup(void)

[PATCH 3/6] Simplify pci_bios_init_root_regions().

[Kevin O'Connor]

Add some comments and refactor out some duplicated code.

Signed-off-by: Kevin O'Connor 
---
 src/pciinit.c |   38 +++++++++++++-------------------------
 1 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index 62583a3..ccf71d1 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -439,38 +439,26 @@ static void pci_bios_check_device_in_bus(int bus)

#define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)

+// Setup region bases (given the regions' size and alignment)
 static int pci_bios_init_root_regions(u32 start, u32 end)
 {
     struct pci_bus *bus = &busses[0];

bus->r[PCI_REGION_TYPE_IO].base = 0xc000;

-    if (bus->r[PCI_REGION_TYPE_MEM].sum < bus->r[PCI_REGION_TYPE_PREFMEM].sum) {
-        bus->r[PCI_REGION_TYPE_MEM].base =
-            ROOT_BASE(end,
-                      bus->r[PCI_REGION_TYPE_MEM].sum,
-                      bus->r[PCI_REGION_TYPE_MEM].max);
-        bus->r[PCI_REGION_TYPE_PREFMEM].base =
-            ROOT_BASE(bus->r[PCI_REGION_TYPE_MEM].base,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].sum,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].max);
-        if (bus->r[PCI_REGION_TYPE_PREFMEM].base >= start) {
-            return 0;
-        }
-    } else {
-        bus->r[PCI_REGION_TYPE_PREFMEM].base =
-            ROOT_BASE(end,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].sum,
-                      bus->r[PCI_REGION_TYPE_PREFMEM].max);
-        bus->r[PCI_REGION_TYPE_MEM].base =
-            ROOT_BASE(bus->r[PCI_REGION_TYPE_PREFMEM].base,
-                      bus->r[PCI_REGION_TYPE_MEM].sum,
-                      bus->r[PCI_REGION_TYPE_MEM].max);
-        if (bus->r[PCI_REGION_TYPE_MEM].base >= start) {
-            return 0;
-        }
+    int reg1 = PCI_REGION_TYPE_PREFMEM, reg2 = PCI_REGION_TYPE_MEM;
+    if (bus->r[reg1].sum < bus->r[reg2].sum) {
+        // Swap regions so larger area is more likely to align well.
+        reg1 = PCI_REGION_TYPE_MEM;
+        reg2 = PCI_REGION_TYPE_PREFMEM;
     }
-    return -1;
+    bus->r[reg2].base = ROOT_BASE(end, bus->r[reg2].sum, bus->r[reg2].max);
+    bus->r[reg1].base = ROOT_BASE(bus->r[reg2].base, bus->r[reg1].sum
+                                  , bus->r[reg1].max);
+    if (bus->r[reg1].base < start)
+        // Memory range requested is larger than available.
+        return -1;
+    return 0;
 }

[PATCH 4/6] Use pci->header_type in pci_bar() to avoid unnecessary pci_config_readb.

[Kevin O'Connor]

Pass a 'struct pci_device' into pci_bar and update all callers.

Signed-off-by: Kevin O'Connor 
---
 src/pciinit.c |   52 +++++++++++++++++++++++-----------------------------
 1 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index ccf71d1..e384a68 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -74,25 +74,21 @@ static enum pci_region_type pci_addr_to_type(u32 addr)
     return PCI_REGION_TYPE_MEM;
 }

-static u32 pci_bar(u16 bdf, int region_num)
+static u32 pci_bar(struct pci_device *pci, int region_num)
 {
     if (region_num != PCI_ROM_SLOT) {
         return PCI_BASE_ADDRESS_0 + region_num * 4;
     }

#define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
-    u8 type = pci_config_readb(bdf, PCI_HEADER_TYPE);
-    type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+    u8 type = pci->header_type & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
     return type == PCI_HEADER_TYPE_BRIDGE ? PCI_ROM_ADDRESS1 : PCI_ROM_ADDRESS;
 }

-static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
+static void
+pci_set_io_region_addr(struct pci_device *pci, int region_num, u32 addr)
 {
-    u32 ofs;
-
-    ofs = pci_bar(bdf, region_num);
-
-    pci_config_writel(bdf, ofs, addr);
+    pci_config_writel(pci->bdf, pci_bar(pci, region_num), addr);
 }

@@ -146,12 +142,11 @@ static const struct pci_device_id pci_isa_bridge_tbl[] = {

static void storage_ide_init(struct pci_device *pci, void *arg)
 {
-    u16 bdf = pci->bdf;
     /* IDE: we map it as in ISA mode */
-    pci_set_io_region_addr(bdf, 0, PORT_ATA1_CMD_BASE);
-    pci_set_io_region_addr(bdf, 1, PORT_ATA1_CTRL_BASE);
-    pci_set_io_region_addr(bdf, 2, PORT_ATA2_CMD_BASE);
-    pci_set_io_region_addr(bdf, 3, PORT_ATA2_CTRL_BASE);
+    pci_set_io_region_addr(pci, 0, PORT_ATA1_CMD_BASE);
+    pci_set_io_region_addr(pci, 1, PORT_ATA1_CTRL_BASE);
+    pci_set_io_region_addr(pci, 2, PORT_ATA2_CMD_BASE);
+    pci_set_io_region_addr(pci, 3, PORT_ATA2_CTRL_BASE);
 }

/* PIIX3/PIIX4 IDE */
@@ -165,13 +160,13 @@ static void piix_ide_init(struct pci_device *pci, void *arg)
 static void pic_ibm_init(struct pci_device *pci, void *arg)
 {
     /* PIC, IBM, MPIC & MPIC2 */
-    pci_set_io_region_addr(pci->bdf, 0, 0x80800000 + 0x00040000);
+    pci_set_io_region_addr(pci, 0, 0x80800000 + 0x00040000);
 }

static void apple_macio_init(struct pci_device *pci, void *arg)
 {
     /* macio bridge */
-    pci_set_io_region_addr(pci->bdf, 0, 0x80800000);
+    pci_set_io_region_addr(pci, 0, 0x80800000);
 }

static const struct pci_device_id pci_class_tbl[] = {
@@ -339,10 +334,11 @@ static u32 pci_size_roundup(u32 size)
     return 0x1 << index;
 }

-static void pci_bios_bus_get_bar(struct pci_bus *bus, int bdf, int bar,
-                                 u32 *val, u32 *size)
+static void
+pci_bios_get_bar(struct pci_device *pci, int bar, u32 *val, u32 *size)
 {
-    u32 ofs = pci_bar(bdf, bar);
+    u32 ofs = pci_bar(pci, bar);
+    u16 bdf = pci->bdf;
     u32 old = pci_config_readl(bdf, ofs);
     u32 mask;

@@ -377,10 +373,6 @@ static void pci_bios_check_device_in_bus(int bus);

static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
 {
-    u16 bdf = dev->bdf;
-    u32 limit;
-    int i,type;
-
     if (dev->class == PCI_CLASS_BRIDGE_PCI) {
         if (dev->secondary_bus >= busses_count) {
             /* should never trigger */
@@ -390,8 +382,9 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
         }
         struct pci_bus *s = busses + dev->secondary_bus;
         pci_bios_check_device_in_bus(dev->secondary_bus);
+        int type;
         for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
-            limit = (type == PCI_REGION_TYPE_IO) ?
+            u32 limit = (type == PCI_REGION_TYPE_IO) ?
                 PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
             s->r[type].size = s->r[type].sum;
             if (s->r[type].size < limit)
@@ -407,9 +400,10 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
         return;
     }

+    int i;
     for (i = 0; i < PCI_NUM_REGIONS; i++) {
         u32 val, size;
-        pci_bios_bus_get_bar(bus, bdf, i, &val, &size);
+        pci_bios_get_bar(dev, i, &val, &size);
         if (val == 0) {
             continue;
         }
@@ -506,9 +500,6 @@ static void pci_bios_map_device_in_bus(int bus);

static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
 {
-    u16 bdf = dev->bdf;
-    int type, i;
-
     if (dev->class == PCI_CLASS_BRIDGE_PCI) {
         if (dev->secondary_bus >= busses_count) {
             return;
@@ -516,6 +507,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
         struct pci_bus *s = busses + dev->secondary_bus;
         u32 base, limit;

+        int type;
         for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
             s->r[type].base = pci_bios_bus_get_addr(bus, type, s->r[type].size);
         }
@@ -524,6 +516,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)

base = s->r[PCI_REGION_TYPE_IO].base;
         limit = base + s->r[PCI_REGION_TYPE_IO].size - 1;
+        u16 bdf = dev->bdf;
         pci_config_writeb(bdf, PCI_IO_BASE, base >> PCI_IO_SHIFT);
         pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
         pci_config_writeb(bdf, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT);
@@ -545,6 +538,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
         return;
     }

+    int i;
     for (i = 0; i < PCI_NUM_REGIONS; i++) {
         u32 addr;
         if (dev->bars[i].addr == 0) {
@@ -556,7 +550,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
         dprintf(1, "  bar %d, addr %x, size %x [%s]\n",
                 i, addr, dev->bars[i].size,
                 dev->bars[i].addr & PCI_BASE_ADDRESS_SPACE_IO ? "io" : "mem");
-        pci_set_io_region_addr(bdf, i, addr);
+        pci_set_io_region_addr(dev, i, addr);

if (dev->bars[i].is64) {
             i++;

[PATCH 5/6] Introduce PCI child device iterators and use in pciinit.c.

[Kevin O'Connor]

Replace O(n^2) bus iterators with new a host iterator (for bus 0) and
a new pci bridge child bus iterator.

Signed-off-by: Kevin O'Connor 
---
 src/pci.c     |    2 +
 src/pci.h     |   12 +++++++
 src/pciinit.c |   94 +++++++++++++++++++++++----------------------------------
 3 files changed, 52 insertions(+), 56 deletions(-)

diff --git a/src/pci.c b/src/pci.c
index 6031c9f..7697583 100644
--- a/src/pci.c
+++ b/src/pci.c
@@ -137,6 +137,8 @@ pci_probe_devices(void)
                     MaxPCIBus = bus;
             } else {
                 rootbus = parent->rootbus;
+                if (!parent->firstchild)
+                    parent->firstchild = dev;
             }

// Populate pci_device info.
diff --git a/src/pci.h b/src/pci.h
index a2a5a4c..b19b2e9 100644
--- a/src/pci.h
+++ b/src/pci.h
@@ -44,6 +44,7 @@ struct pci_device {
     u8 rootbus;
     struct pci_device *next;
     struct pci_device *parent;
+    struct pci_device *firstchild;

// Configuration space device information
     u16 vendor, device;
@@ -68,9 +69,20 @@ static inline u32 pci_classprog(struct pci_device *pci) {
     return (pci->class << 8) | pci->prog_if;
 }

+// Iterator for every PCI device in the system
 #define foreachpci(PCI)                         \
     for (PCI=PCIDevices; PCI; PCI=PCI->next)

+// Iterator for every PCI device on the main host bus (bus 0)
+#define foreachhostpci(PCI)                                             \
+    for (PCI=PCIDevices; PCI && pci_bdf_to_bus(PCI->bdf) == 0; PCI=PCI->next)
+
+// Iterator for every PCI device hanging off a PCI bridge
+#define foreachchildpci(PCI, PARENT)                                    \
+    for (PCI=PARENT->firstchild                                         \
+             ; PCI && pci_bdf_to_bus(PCI->bdf) == pci_bdf_to_bus(PARENT->bdf) \
+             ; PCI=PCI->next)
+
 int pci_next(int bdf, int bus);
 #define foreachbdf(BDF, BUS)                                    \
     for (BDF=pci_next(pci_bus_devfn_to_bdf((BUS), 0)-1, (BUS))  \
diff --git a/src/pciinit.c b/src/pciinit.c
index e384a68..b87825d 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -236,17 +236,16 @@ static void pci_bios_init_device(struct pci_device *pci)
     pci_init_device(pci_device_tbl, pci, NULL);
 }

-static void pci_bios_init_device_in_bus(int bus)
+static void pci_bios_init_devices(void)
 {
     struct pci_device *pci;
-    foreachpci(pci) {
-        u8 pci_bus = pci_bdf_to_bus(pci->bdf);
-        if (pci_bus < bus)
-            continue;
-        if (pci_bus > bus)
-            break;
+    foreachhostpci(pci) {
         pci_bios_init_device(pci);
     }
+
+    foreachpci(pci) {
+        pci_init_device(pci_isa_bridge_tbl, pci, NULL);
+    }
 }

@@ -369,10 +368,10 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
         bus->r[type].max = size;
 }

-static void pci_bios_check_device_in_bus(int bus);
-
-static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
+static void pci_bios_check_device(struct pci_device *dev)
 {
+    struct pci_bus *bus = &busses[pci_bdf_to_bus(dev->bdf)];
+
     if (dev->class == PCI_CLASS_BRIDGE_PCI) {
         if (dev->secondary_bus >= busses_count) {
             /* should never trigger */
@@ -380,8 +379,13 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
                     busses_count, dev->secondary_bus);
             return;
         }
-        struct pci_bus *s = busses + dev->secondary_bus;
-        pci_bios_check_device_in_bus(dev->secondary_bus);
+        dprintf(1, "PCI: check devices bus %d\n", dev->secondary_bus);
+        struct pci_device *pci;
+        foreachchildpci(pci, dev) {
+            pci_bios_check_device(pci);
+        }
+
+        struct pci_bus *s = &busses[dev->secondary_bus];
         int type;
         for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
             u32 limit = (type == PCI_REGION_TYPE_IO) ?
@@ -419,18 +423,6 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev)
     }
 }

-static void pci_bios_check_device_in_bus(int bus)
-{
-    struct pci_device *pci;
-
-    dprintf(1, "PCI: check devices bus %d\n", bus);
-    foreachpci(pci) {
-        if (pci_bdf_to_bus(pci->bdf) != bus)
-            continue;
-        pci_bios_check_device(&busses[bus], pci);
-    }
-}
-
 #define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)

// Setup region bases (given the regions' size and alignment)
@@ -496,17 +488,19 @@ static u32 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size)
 #define PCI_MEMORY_SHIFT        16
 #define PCI_PREF_MEMORY_SHIFT   16

-static void pci_bios_map_device_in_bus(int bus);
-
-static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
+static void pci_bios_map_device(struct pci_device *dev)
 {
+    struct pci_bus *bus = &busses[pci_bdf_to_bus(dev->bdf)];
+    u16 bdf = dev->bdf;
+
+    dprintf(1, "PCI: map device bdf=%02x:%02x.%x\n"
+            , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
+
     if (dev->class == PCI_CLASS_BRIDGE_PCI) {
         if (dev->secondary_bus >= busses_count) {
             return;
         }
-        struct pci_bus *s = busses + dev->secondary_bus;
-        u32 base, limit;
-
+        struct pci_bus *s = &busses[dev->secondary_bus];
         int type;
         for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
             s->r[type].base = pci_bios_bus_get_addr(bus, type, s->r[type].size);
@@ -514,9 +508,8 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
         dprintf(1, "PCI: init bases bus %d (secondary)\n", dev->secondary_bus);
         pci_bios_init_bus_bases(s);

-        base = s->r[PCI_REGION_TYPE_IO].base;
-        limit = base + s->r[PCI_REGION_TYPE_IO].size - 1;
-        u16 bdf = dev->bdf;
+        u32 base = s->r[PCI_REGION_TYPE_IO].base;
+        u32 limit = base + s->r[PCI_REGION_TYPE_IO].size - 1;
         pci_config_writeb(bdf, PCI_IO_BASE, base >> PCI_IO_SHIFT);
         pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
         pci_config_writeb(bdf, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT);
@@ -534,7 +527,10 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
         pci_config_writel(bdf, PCI_PREF_BASE_UPPER32, 0);
         pci_config_writel(bdf, PCI_PREF_LIMIT_UPPER32, 0);

-        pci_bios_map_device_in_bus(dev->secondary_bus);
+        struct pci_device *pci;
+        foreachchildpci(pci, dev) {
+            pci_bios_map_device(pci);
+        }
         return;
     }

@@ -558,20 +554,6 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
     }
 }

-static void pci_bios_map_device_in_bus(int bus)
-{
-    struct pci_device *pci;
-
-    foreachpci(pci) {
-        u16 bdf = pci->bdf;
-        if (pci_bdf_to_bus(bdf) != bus)
-            continue;
-        dprintf(1, "PCI: map device bdf=%02x:%02x.%x\n"
-                , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
-        pci_bios_map_device(&busses[bus], pci);
-    }
-}
-

/****************************************************************
  * Main setup code
@@ -603,7 +585,10 @@ pci_setup(void)
     dprintf(1, "=== PCI new allocation pass #1 ===\n");
     busses = malloc_tmp(sizeof(*busses) * busses_count);
     memset(busses, 0, sizeof(*busses) * busses_count);
-    pci_bios_check_device_in_bus(0 /* host bus */);
+    struct pci_device *pci;
+    foreachhostpci(pci) {
+        pci_bios_check_device(pci);
+    }
     if (pci_bios_init_root_regions(start, end) != 0) {
         panic("PCI: out of address space\n");
     }
@@ -611,15 +596,12 @@ pci_setup(void)
     dprintf(1, "=== PCI new allocation pass #2 ===\n");
     dprintf(1, "PCI: init bases bus 0 (primary)\n");
     pci_bios_init_bus_bases(&busses[0]);
-    pci_bios_map_device_in_bus(0 /* host bus */);
-
-    pci_bios_init_device_in_bus(0 /* host bus */);
-
-    struct pci_device *pci;
-    foreachpci(pci) {
-        pci_init_device(pci_isa_bridge_tbl, pci, NULL);
+    foreachhostpci(pci) {
+        pci_bios_map_device(pci);
     }

+    pci_bios_init_devices();
+
     free(busses);
     busses_count = 0;
 }

[PATCH 6/6] Simplify pci_slot_get_irq().

[Kevin O'Connor]

Signed-off-by: Kevin O'Connor 
---
 src/pciinit.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index b87825d..e2c5f3c 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -101,13 +101,11 @@ const u8 pci_irqs[4] = {
     10, 10, 11, 11
 };

-/* return the global irq number corresponding to a given device irq
-   pin. We could also use the bus number to have a more precise
-   mapping. */
-static int pci_slot_get_pirq(u16 bdf, int irq_num)
+// Return the global irq number corresponding to a host bus device irq pin.
+static int pci_slot_get_irq(u16 bdf, int pin)
 {
     int slot_addend = pci_bdf_to_dev(bdf) - 1;
-    return (irq_num + slot_addend) & 3;
+    return pci_irqs[(pin - 1 + slot_addend) & 3];
 }

/* PIIX3/PIIX4 PCI to ISA bridge */
@@ -215,23 +213,19 @@ static const struct pci_device_id pci_device_tbl[] = {
 static void pci_bios_init_device(struct pci_device *pci)
 {
     u16 bdf = pci->bdf;
-    int pin, pic_irq;
-
     dprintf(1, "PCI: init bdf=%02x:%02x.%x id=%04x:%04x\n"
             , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf)
             , pci->vendor, pci->device);
+
     pci_init_device(pci_class_tbl, pci, NULL);

/* enable memory mappings */
     pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY);

/* map the interrupt */
-    pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
-    if (pin != 0) {
-        pin = pci_slot_get_pirq(bdf, pin - 1);
-        pic_irq = pci_irqs[pin];
-        pci_config_writeb(bdf, PCI_INTERRUPT_LINE, pic_irq);
-    }
+    int pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
+    if (pin != 0)
+        pci_config_writeb(bdf, PCI_INTERRUPT_LINE, pci_slot_get_irq(bdf, pin));

pci_init_device(pci_device_tbl, pci, NULL);
 }

[PATCH] Locally allocate busses[] and always make it maximum sized.

[Kevin O'Connor]

One more patch.

-Kevin
> From c0068c484162337afda1ede0322a4cfcd37ded23 Mon Sep 17 00:00:00 2001

From: Kevin O'Connor 
Date: Sat, 1 Oct 2011 15:50:55 -0400
Subject: [PATCH] Locally allocate busses[] and always make it maximum sized.
To: [e-mail]

Don't bother tracking busses_count - there is plenty of dynamic ram -
just always allocate the maximum size it can be (256).

Since only a few functions need to access busses[] pass it by
reference instead of using a global.

Signed-off-by: Kevin O'Connor 
---
 src/pciinit.c |   42 +++++++++++++++++-------------------------
 1 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index e2c5f3c..c618a8f 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -1,6 +1,6 @@
 // Initialize PCI devices (on emulators)
 //
-// Copyright (C) 2008  Kevin O'Connor 
+// Copyright (C) 2008-2011  Kevin O'Connor 
 // Copyright (C) 2006 Fabrice Bellard
 //
 // This file may be distributed under the terms of the GNU LGPLv3 license.
@@ -31,7 +31,7 @@ static const char *region_type_name[] = {
     [ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
 };

-static struct pci_bus {
+struct pci_bus {
     struct {
         /* pci region stats */
         u32 count[32 - PCI_MEM_INDEX_SHIFT];
@@ -42,8 +42,7 @@ static struct pci_bus {
         u32 bases[32 - PCI_MEM_INDEX_SHIFT];
         u32 base;
     } r[PCI_REGION_TYPE_COUNT];
-} *busses;
-static int busses_count;
+};

static int pci_size_to_index(u32 size, enum pci_region_type type)
 {
@@ -313,7 +312,6 @@ pci_bios_init_bus(void)
 {
     u8 pci_bus = 0;
     pci_bios_init_bus_rec(0 /* host bus */, &pci_bus);
-    busses_count = pci_bus + 1;
 }

@@ -362,21 +360,15 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
         bus->r[type].max = size;
 }

-static void pci_bios_check_device(struct pci_device *dev)
+static void pci_bios_check_device(struct pci_bus *busses, struct pci_device *dev)
 {
     struct pci_bus *bus = &busses[pci_bdf_to_bus(dev->bdf)];

if (dev->class == PCI_CLASS_BRIDGE_PCI) {
-        if (dev->secondary_bus >= busses_count) {
-            /* should never trigger */
-            dprintf(1, "PCI: bus count too small (%d), skipping bus #%d\n",
-                    busses_count, dev->secondary_bus);
-            return;
-        }
         dprintf(1, "PCI: check devices bus %d\n", dev->secondary_bus);
         struct pci_device *pci;
         foreachchildpci(pci, dev) {
-            pci_bios_check_device(pci);
+            pci_bios_check_device(busses, pci);
         }

struct pci_bus *s = &busses[dev->secondary_bus];
@@ -420,7 +412,7 @@ static void pci_bios_check_device(struct pci_device *dev)
 #define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)

// Setup region bases (given the regions' size and alignment)
-static int pci_bios_init_root_regions(u32 start, u32 end)
+static int pci_bios_init_root_regions(struct pci_bus *busses, u32 start, u32 end)
 {
     struct pci_bus *bus = &busses[0];

@@ -482,7 +474,7 @@ static u32 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size)
 #define PCI_MEMORY_SHIFT        16
 #define PCI_PREF_MEMORY_SHIFT   16

-static void pci_bios_map_device(struct pci_device *dev)
+static void pci_bios_map_device(struct pci_bus *busses, struct pci_device *dev)
 {
     struct pci_bus *bus = &busses[pci_bdf_to_bus(dev->bdf)];
     u16 bdf = dev->bdf;
@@ -491,9 +483,6 @@ static void pci_bios_map_device(struct pci_device *dev)
             , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));

if (dev->class == PCI_CLASS_BRIDGE_PCI) {
-        if (dev->secondary_bus >= busses_count) {
-            return;
-        }
         struct pci_bus *s = &busses[dev->secondary_bus];
         int type;
         for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
@@ -523,7 +512,7 @@ static void pci_bios_map_device(struct pci_device *dev)

struct pci_device *pci;
         foreachchildpci(pci, dev) {
-            pci_bios_map_device(pci);
+            pci_bios_map_device(busses, pci);
         }
         return;
     }
@@ -577,13 +566,17 @@ pci_setup(void)
     pci_probe_devices();

dprintf(1, "=== PCI new allocation pass #1 ===\n");
-    busses = malloc_tmp(sizeof(*busses) * busses_count);
-    memset(busses, 0, sizeof(*busses) * busses_count);
+    struct pci_bus *busses = malloc_tmp(sizeof(*busses) * 256);
+    if (!busses) {
+        warn_noalloc();
+        return;
+    }
+    memset(busses, 0, sizeof(*busses) * 256);
     struct pci_device *pci;
     foreachhostpci(pci) {
-        pci_bios_check_device(pci);
+        pci_bios_check_device(busses, pci);
     }
-    if (pci_bios_init_root_regions(start, end) != 0) {
+    if (pci_bios_init_root_regions(busses, start, end) != 0) {
         panic("PCI: out of address space\n");
     }

@@ -591,11 +584,10 @@ pci_setup(void)
     dprintf(1, "PCI: init bases bus 0 (primary)\n");
     pci_bios_init_bus_bases(&busses[0]);
     foreachhostpci(pci) {
-        pci_bios_map_device(pci);
+        pci_bios_map_device(busses, pci);
     }

pci_bios_init_devices();

free(busses);
-    busses_count = 0;
 }