This main patch in this series is patch 2 - replacement of the recursive scan with a linear scan. The other patches are just cosmetic changes.
Gerd - can you take a look at patch 2. Also, what's your test case for confirming multiple bus initialization? The standard qemu/kvm only has one host bus.
-Kevin
Kevin O'Connor (5): Replace pciinit busses_count with MaxPCIBus. Replace recursive pci init system with linear passes. Locally allocate pciinit busses[] variable. Move pciinit device init code together. Simplify pci_bios_init_root_regions().
src/pciinit.c | 245 +++++++++++++++++++++++++-------------------------------- 1 files changed, 107 insertions(+), 138 deletions(-)
Use the existing bus count instead of calculating a new one. Also, the MaxPCIBus is guaranteed to encompass all pci->secondary_bus references, so no need to check for overruns.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 20 ++++++-------------- 1 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index 3c12602..51880c0 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -43,7 +43,6 @@ static struct pci_bus { 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) { @@ -314,7 +313,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; }
@@ -368,12 +366,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) { 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; - } struct pci_bus *s = busses + dev->secondary_bus; pci_bios_check_device_in_bus(dev->secondary_bus); int type; @@ -507,9 +499,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) { 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;
@@ -607,8 +596,12 @@ 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); + busses = malloc_tmp(sizeof(*busses) * (MaxPCIBus + 1)); + if (!busses) { + warn_noalloc(); + return; + } + memset(busses, 0, sizeof(*busses) * (MaxPCIBus + 1)); pci_bios_check_device_in_bus(0 /* host bus */); if (pci_bios_init_root_regions(start, end) != 0) { panic("PCI: out of address space\n"); @@ -627,5 +620,4 @@ pci_setup(void) }
free(busses); - busses_count = 0; }
The existing PCI sizing and mapping uses a recursive algorithm to visit every bus and its devices in order. Replace that with an algorithm that visits every device and then every bus.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 156 +++++++++++++++++++++++++++------------------------------ 1 files changed, 74 insertions(+), 82 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index 51880c0..baafa5b 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -42,6 +42,7 @@ static struct pci_bus { u32 bases[32 - PCI_MEM_INDEX_SHIFT]; u32 base; } r[PCI_REGION_TYPE_COUNT]; + struct pci_device *bus_dev; } *busses;
static int pci_size_to_index(u32 size, enum pci_region_type type) @@ -361,13 +362,44 @@ 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_devices(void) { - if (dev->class == PCI_CLASS_BRIDGE_PCI) { - struct pci_bus *s = busses + dev->secondary_bus; - pci_bios_check_device_in_bus(dev->secondary_bus); + dprintf(1, "PCI: check devices\n"); + + // Calculate resources needed for regular (non-bus) devices. + struct pci_device *pci; + foreachpci(pci) { + if (pci->class == PCI_CLASS_BRIDGE_PCI) { + busses[pci->secondary_bus].bus_dev = pci; + continue; + } + struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)]; + int i; + for (i = 0; i < PCI_NUM_REGIONS; i++) { + u32 val, size; + pci_bios_get_bar(pci, i, &val, &size); + if (val == 0) + continue; + + pci_bios_bus_reserve(bus, pci_addr_to_type(val), size); + pci->bars[i].addr = val; + pci->bars[i].size = size; + pci->bars[i].is64 = (!(val & PCI_BASE_ADDRESS_SPACE_IO) && + (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) + == PCI_BASE_ADDRESS_MEM_TYPE_64); + + if (pci->bars[i].is64) + i++; + } + } + + // Propagate required bus resources to parent busses. + int secondary_bus; + for (secondary_bus=MaxPCIBus; secondary_bus>0; secondary_bus--) { + struct pci_bus *s = &busses[secondary_bus]; + if (!s->bus_dev) + continue; + struct pci_bus *parent = &busses[pci_bdf_to_bus(s->bus_dev->bdf)]; int type; for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { u32 limit = (type == PCI_REGION_TYPE_IO) ? @@ -376,44 +408,13 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev) if (s->r[type].size < limit) s->r[type].size = limit; s->r[type].size = pci_size_roundup(s->r[type].size); - pci_bios_bus_reserve(bus, type, s->r[type].size); + pci_bios_bus_reserve(parent, type, s->r[type].size); } dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n", - dev->secondary_bus, + secondary_bus, s->r[PCI_REGION_TYPE_IO].size, s->r[PCI_REGION_TYPE_MEM].size, s->r[PCI_REGION_TYPE_PREFMEM].size); - return; - } - - int i; - for (i = 0; i < PCI_NUM_REGIONS; i++) { - u32 val, size; - pci_bios_get_bar(dev, i, &val, &size); - if (val == 0) { - continue; - } - pci_bios_bus_reserve(bus, pci_addr_to_type(val), size); - dev->bars[i].addr = val; - dev->bars[i].size = size; - dev->bars[i].is64 = (!(val & PCI_BASE_ADDRESS_SPACE_IO) && - (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64); - - if (dev->bars[i].is64) { - i++; - } - } -} - -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); } }
@@ -494,24 +495,26 @@ 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_devices(void) { - if (dev->class == PCI_CLASS_BRIDGE_PCI) { - struct pci_bus *s = busses + dev->secondary_bus; - u32 base, limit; - + // Map regions on each secondary bus. + int secondary_bus; + for (secondary_bus=1; secondary_bus<=MaxPCIBus; secondary_bus++) { + struct pci_bus *s = &busses[secondary_bus]; + if (!s->bus_dev) + continue; + u16 bdf = s->bus_dev->bdf; + struct pci_bus *parent = &busses[pci_bdf_to_bus(bdf)]; 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); + s->r[type].base = pci_bios_bus_get_addr( + parent, type, s->r[type].size); } - dprintf(1, "PCI: init bases bus %d (secondary)\n", dev->secondary_bus); + dprintf(1, "PCI: init bases bus %d (secondary)\n", 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); @@ -528,42 +531,31 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) pci_config_writew(bdf, PCI_PREF_MEMORY_LIMIT, limit >> PCI_PREF_MEMORY_SHIFT); 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); - return; - } - - int i; - for (i = 0; i < PCI_NUM_REGIONS; i++) { - u32 addr; - if (dev->bars[i].addr == 0) { - continue; - } - - addr = pci_bios_bus_get_addr(bus, pci_addr_to_type(dev->bars[i].addr), - dev->bars[i].size); - dprintf(1, " bar %d, addr %x, size %x [%s]\n", - i, addr, dev->bars[i].size, - region_type_name[pci_addr_to_type(dev->bars[i].addr)]); - pci_set_io_region_addr(dev, i, addr); - - if (dev->bars[i].is64) { - i++; - } } -}
-static void pci_bios_map_device_in_bus(int bus) -{ + // Map regions on each device. struct pci_device *pci; - foreachpci(pci) { - u16 bdf = pci->bdf; - if (pci_bdf_to_bus(bdf) != bus) + if (pci->class == PCI_CLASS_BRIDGE_PCI) continue; + u16 bdf = 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); + struct pci_bus *bus = &busses[pci_bdf_to_bus(bdf)]; + int i; + for (i = 0; i < PCI_NUM_REGIONS; i++) { + if (pci->bars[i].addr == 0) + continue; + + int type = pci_addr_to_type(pci->bars[i].addr); + u32 addr = pci_bios_bus_get_addr(bus, type, pci->bars[i].size); + dprintf(1, " bar %d, addr %x, size %x [%s]\n", + i, addr, pci->bars[i].size, region_type_name[type]); + pci_set_io_region_addr(pci, i, addr); + + if (pci->bars[i].is64) + i++; + } } }
@@ -602,7 +594,7 @@ pci_setup(void) return; } memset(busses, 0, sizeof(*busses) * (MaxPCIBus + 1)); - pci_bios_check_device_in_bus(0 /* host bus */); + pci_bios_check_devices(); if (pci_bios_init_root_regions(start, end) != 0) { panic("PCI: out of address space\n"); } @@ -610,7 +602,7 @@ 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_map_devices();
pci_bios_init_device_in_bus(0 /* host bus */);
No need for a global variable - only a few functions use the busses array.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index baafa5b..3398387 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -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]; @@ -43,7 +43,7 @@ static struct pci_bus { u32 base; } r[PCI_REGION_TYPE_COUNT]; struct pci_device *bus_dev; -} *busses; +};
static int pci_size_to_index(u32 size, enum pci_region_type type) { @@ -362,7 +362,7 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size) bus->r[type].max = size; }
-static void pci_bios_check_devices(void) +static void pci_bios_check_devices(struct pci_bus *busses) { dprintf(1, "PCI: check devices\n");
@@ -420,10 +420,8 @@ static void pci_bios_check_devices(void)
#define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)
-static int pci_bios_init_root_regions(u32 start, u32 end) +static int pci_bios_init_root_regions(struct pci_bus *bus, 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) { @@ -495,8 +493,12 @@ 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_devices(void) +static void pci_bios_map_devices(struct pci_bus *busses) { + // Setup bases for root bus. + dprintf(1, "PCI: init bases bus 0 (primary)\n"); + pci_bios_init_bus_bases(&busses[0]); + // Map regions on each secondary bus. int secondary_bus; for (secondary_bus=1; secondary_bus<=MaxPCIBus; secondary_bus++) { @@ -588,21 +590,19 @@ pci_setup(void) pci_probe_devices();
dprintf(1, "=== PCI new allocation pass #1 ===\n"); - busses = malloc_tmp(sizeof(*busses) * (MaxPCIBus + 1)); + struct pci_bus *busses = malloc_tmp(sizeof(*busses) * (MaxPCIBus + 1)); if (!busses) { warn_noalloc(); return; } memset(busses, 0, sizeof(*busses) * (MaxPCIBus + 1)); - pci_bios_check_devices(); - if (pci_bios_init_root_regions(start, end) != 0) { + pci_bios_check_devices(busses); + if (pci_bios_init_root_regions(&busses[0], start, end) != 0) { panic("PCI: out of address space\n"); }
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_devices(); + pci_bios_map_devices(busses);
pci_bios_init_device_in_bus(0 /* host bus */);
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 19 ++++++++----------- 1 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index 3398387..d927ef2 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -230,17 +230,19 @@ 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) + if (pci_bdf_to_bus(pci->bdf) != 0) + // Only init devices on host bus. break; pci_bios_init_device(pci); } + + foreachpci(pci) { + pci_init_device(pci_isa_bridge_tbl, pci, NULL); + } }
@@ -604,12 +606,7 @@ pci_setup(void) dprintf(1, "=== PCI new allocation pass #2 ===\n"); pci_bios_map_devices(busses);
- pci_bios_init_device_in_bus(0 /* host bus */); - - struct pci_device *pci; - foreachpci(pci) { - pci_init_device(pci_isa_bridge_tbl, pci, NULL); - } + pci_bios_init_devices();
free(busses); }
Add some comments and refactor out some duplicated code.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 38 +++++++++++++------------------------- 1 files changed, 13 insertions(+), 25 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index d927ef2..7d83368 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -422,36 +422,24 @@ static void pci_bios_check_devices(struct pci_bus *busses)
#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(struct pci_bus *bus, u32 start, u32 end) { 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; }
On 10/15/11 18:25, Kevin O'Connor wrote:
This main patch in this series is patch 2 - replacement of the recursive scan with a linear scan. The other patches are just cosmetic changes.
Looks sane at a quick glance.
Gerd - can you take a look at patch 2. Also, what's your test case for confirming multiple bus initialization? The standard qemu/kvm only has one host bus.
That is a bit tricky. Early revisions of the code I've tested with Isaku's q35 patches which have quite a bunch of PCI bridges (pci expresss upstream + down stream ports, standard dec bridge, ...) and even bridges behind bridges.
Isaku, what is the state of the q35 work btw? I havn't seen any pci-e & q35 patches on qemu-devel for quite a while. It also would be nice to have a recent version of the q35 patches to test this patch set.
I've also used with mst's patch which adds a standard pci bridge to qemu (patch attached) to run tests.
cheers, Gerd
On 10/17/11 11:57, Gerd Hoffmann wrote:
On 10/15/11 18:25, Kevin O'Connor wrote:
This main patch in this series is patch 2 - replacement of the recursive scan with a linear scan. The other patches are just cosmetic changes.
Looks sane at a quick glance.
Do you have a git tree somewhere to pull from for testing?
cheers, Gerd
On Mon, Oct 17, 2011 at 03:20:05PM +0200, Gerd Hoffmann wrote:
On 10/17/11 11:57, Gerd Hoffmann wrote:
On 10/15/11 18:25, Kevin O'Connor wrote:
This main patch in this series is patch 2 - replacement of the recursive scan with a linear scan. The other patches are just cosmetic changes.
Looks sane at a quick glance.
Do you have a git tree somewhere to pull from for testing?
I don't have a site for my local tree - it's just the last patch series on top of the current tree, though.
-Kevin
On Mon, Oct 17, 2011 at 11:57:09AM +0200, Gerd Hoffmann wrote:
On 10/15/11 18:25, Kevin O'Connor wrote:
This main patch in this series is patch 2 - replacement of the recursive scan with a linear scan. The other patches are just cosmetic changes.
Looks sane at a quick glance.
Gerd - can you take a look at patch 2. Also, what's your test case for confirming multiple bus initialization? The standard qemu/kvm only has one host bus.
That is a bit tricky. Early revisions of the code I've tested with Isaku's q35 patches which have quite a bunch of PCI bridges (pci expresss upstream + down stream ports, standard dec bridge, ...) and even bridges behind bridges.
Isaku, what is the state of the q35 work btw? I havn't seen any pci-e & q35 patches on qemu-devel for quite a while. It also would be nice to have a recent version of the q35 patches to test this patch set.
I've also used with mst's patch which adds a standard pci bridge to qemu (patch attached) to run tests.
At first, I'd like to make it sure that it's not dead and I'm still working on it. I've been waiting for stabilizing of other related thing like memory API(mostly done?), irq routing cleaning up, QOM etc... I don't want to catch them up so frequently as I don't see any time soon merge.
I'll push the latest my patches to my git repo, but I can't promise when. Sorry for inconvenience.
thanks,
On Sat, Oct 15, 2011 at 12:25:56PM -0400, Kevin O'Connor wrote:
This main patch in this series is patch 2 - replacement of the recursive scan with a linear scan. The other patches are just cosmetic changes.
Gerd - can you take a look at patch 2. Also, what's your test case for confirming multiple bus initialization? The standard qemu/kvm only has one host bus.
FYI - I applied this series.
-Kevin
-
Gerd Hoffmann -
Isaku Yamahata -
Kevin O'Connor