| Message ID | 20120404033100.GA2754@morn.localdomain |
|---|---|
| State | New |
| Headers | show |
On 04/04/12 15:31, Kevin O'Connor wrote: > Agreed - the only thing it does is force a minimum size for memory bars as you pointed out in a previous email. As above, I did play with > this a little more on Sunday. I also added in two patches from Gerd's series and made alignment handling more explicit. I'm including the > series here if you're interested. Again, I think this should wait until after the 1.7.0 release. -Kevin Hi Kevin, Sorry for late response, was quite busy. I've reviewed and tried your patches. [Patches 1-4] are almost the same as I proposed in this series. The noticeable differences are: 1) instead of sorting entries at mapping stage your patches sort entries at allocation stage. (no difference in behavior or readability so any approach is fine for me) 2) instead of storing pointer to bus resource which the bridge device represents (this_bus), it is obtained from pci structure and array of "busses". - entry->this_bus->r[entry->type].base = entry->base; + struct pci_bus *child_bus = &busses[entry->dev->secondary_bus]; + child_bus->r[entry->type].base = addr; Since "entry->this_bus" member is removed the information about whether the resource is bridge region or PCI BAR is encoded inside "entry->bar". Value "-1" - means this is a bridge region, the positive values mean it is a BAR. IMHO 2) makes code less readable, at least a comment explaining the meaning of negative value of PCI BAR is required. I've found just cosmetic difference to my patches so please don't forget to add my sign-off-by to them. [Patch 5] Track region alignment explicitly. Looks very good and safe. Should reduce address range wastes. [Patch 6] Small patch to account resources of PCI bridges. Looks fine. May be instead of + num_regions = 2; I would add #define PCI_BRIDGE_NUM_REGIONS 2 ..... + num_regions = PCI_BRIDGE_NUM_REGIONS; [Patch 7] 64bit aware code. Patch is incomplete. It is not working. After 1.7.0 is fine.
On Thu, Apr 12, 2012 at 02:44:54PM +1200, Alexey Korolev wrote: > On 04/04/12 15:31, Kevin O'Connor wrote: > > Agreed - the only thing it does is force a minimum size for memory bars as you pointed out in a previous email. As above, I did play with > > this a little more on Sunday. I also added in two patches from Gerd's series and made alignment handling more explicit. I'm including the > > series here if you're interested. Again, I think this should wait until after the 1.7.0 release. -Kevin > Hi Kevin, > > Sorry for late response, was quite busy. > I've reviewed and tried your patches. > > [Patches 1-4] are almost the same as I proposed in this series. Yes. It's your patches tweaked slightly. > The noticeable differences are: > 1) instead of sorting entries at mapping stage your patches sort entries at allocation stage. (no difference in behavior or readability so > any approach is fine for me) > 2) instead of storing pointer to bus resource which the bridge device represents (this_bus), it is obtained from pci structure and array of > "busses". > - entry->this_bus->r[entry->type].base = entry->base; > + struct pci_bus *child_bus = &busses[entry->dev->secondary_bus]; > + child_bus->r[entry->type].base = addr; > Since "entry->this_bus" member is removed the information about whether the resource is bridge region or PCI BAR is encoded inside > "entry->bar". > Value "-1" - means this is a bridge region, the positive values mean it is a BAR. > IMHO 2) makes code less readable, at least a comment explaining the meaning of negative value of PCI BAR is required. > I've found just cosmetic difference to my patches so please don't forget to add my sign-off-by to them. Okay - will do. > [Patch 5] Track region alignment explicitly. Looks very good and safe. Should reduce address range wastes. > > [Patch 6] Small patch to account resources of PCI bridges. > Looks fine. > May be instead of > + num_regions = 2; > I would add > #define PCI_BRIDGE_NUM_REGIONS 2 > ..... > + num_regions = PCI_BRIDGE_NUM_REGIONS; This was me playing with some of Gerd's patches. I think it would require additional changes before committing. In particular, the patch misses the ROM bar on bridges - I think maybe it should be changed to keep the same loop constraints but add a "if (bar > PCI_BRIDGE_NUM_REGIONS && bar < PCI_ROM_SLOT) continue;". > > [Patch 7] 64bit aware code. Patch is incomplete. It is not working. This was also me playing with one of Gerd's patches. It just makes the bar read/write code 64bit aware. It doesn't actually program them. The logic to do real 64bit allocations would require list merging. Is this something you have looked at? -Kevin
On 12/04/12 15:15, Kevin O'Connor wrote: > > This was also me playing with one of Gerd's patches. It just makes > the bar read/write code 64bit aware. It doesn't actually program > them. The logic to do real 64bit allocations would require list > merging. Is this something you have looked at? Right. I see what you mean here. Shall I play around with 64bit support on top of these patches?
On Thu, Apr 12, 2012 at 03:28:02PM +1200, Alexey Korolev wrote: > On 12/04/12 15:15, Kevin O'Connor wrote: > > > > This was also me playing with one of Gerd's patches. It just makes > > the bar read/write code 64bit aware. It doesn't actually program > > them. The logic to do real 64bit allocations would require list > > merging. Is this something you have looked at? > > Right. I see what you mean here. Shall I play around with 64bit > support on top of these patches? If that makes sense, then sure. -Kevin
diff --git a/src/pci.h b/src/pci.h index a2a5a4c..5598100 100644 --- a/src/pci.h +++ b/src/pci.h @@ -51,11 +51,6 @@ struct pci_device { u8 prog_if, revision; u8 header_type; u8 secondary_bus; - struct { - u32 addr; - u32 size; - int is64; - } bars[PCI_NUM_REGIONS]; // Local information on device. int have_driver; diff --git a/src/pciinit.c b/src/pciinit.c index 9f3fdd4..4af4105 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -31,6 +31,15 @@ static const char *region_type_name[] = { [ PCI_REGION_TYPE_PREFMEM ] = "prefmem", }; +struct pci_region_entry { + struct pci_device *dev; + int bar; + u32 size; + int is64; + enum pci_region_type type; + struct pci_region_entry *next; +}; + struct pci_bus { struct { /* pci region stats */ @@ -41,6 +50,7 @@ struct pci_bus { /* pci region assignments */ u32 bases[32 - PCI_MEM_INDEX_SHIFT]; u32 base; + struct pci_region_entry *list; } r[PCI_REGION_TYPE_COUNT]; struct pci_device *bus_dev; }; @@ -352,19 +362,41 @@ pci_bios_get_bar(struct pci_device *pci, int bar, u32 *val, u32 *size) *size = (~(*val & mask)) + 1; } -static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size) +static struct pci_region_entry * +pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev, + int bar, u32 size, int type, int is64) { - u32 index; + struct pci_region_entry *entry = malloc_tmp(sizeof(*entry)); + if (!entry) { + warn_noalloc(); + return NULL; + } + memset(entry, 0, sizeof(*entry)); + entry->dev = dev; + entry->bar = bar; + entry->size = size; + entry->is64 = is64; + entry->type = type; + // Insert into list in sorted order. + struct pci_region_entry **pprev; + for (pprev = &bus->r[type].list; *pprev; pprev = &(*pprev)->next) { + struct pci_region_entry *pos = *pprev; + if (pos->size < size) + break; + } + entry->next = *pprev; + *pprev = entry; - index = pci_size_to_index(size, type); + int index = pci_size_to_index(size, type); size = pci_index_to_size(index, type); bus->r[type].count[index]++; bus->r[type].sum += size; if (bus->r[type].max < size) bus->r[type].max = size; + return entry; } -static void pci_bios_check_devices(struct pci_bus *busses) +static int pci_bios_check_devices(struct pci_bus *busses) { dprintf(1, "PCI: check devices\n"); @@ -383,14 +415,15 @@ static void pci_bios_check_devices(struct pci_bus *busses) 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); + int is64 = (!(val & PCI_BASE_ADDRESS_SPACE_IO) && + (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) + == PCI_BASE_ADDRESS_MEM_TYPE_64); + struct pci_region_entry *entry = pci_region_create_entry( + bus, pci, i, size, pci_addr_to_type(val), is64); + if (!entry) + return -1; - if (pci->bars[i].is64) + if (is64) i++; } } @@ -410,7 +443,10 @@ static void pci_bios_check_devices(struct pci_bus *busses) 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(parent, type, s->r[type].size); + struct pci_region_entry *entry = pci_region_create_entry( + parent, s->bus_dev, -1, s->r[type].size, type, 0); + if (!entry) + return -1; } dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n", secondary_bus, @@ -418,6 +454,7 @@ static void pci_bios_check_devices(struct pci_bus *busses) s->r[PCI_REGION_TYPE_MEM].size, s->r[PCI_REGION_TYPE_PREFMEM].size); } + return 0; } #define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1) @@ -483,6 +520,24 @@ 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_region_map_one_entry(struct pci_bus *busses, struct pci_region_entry *entry) +{ + u16 bdf = entry->dev->bdf; + struct pci_bus *bus = &busses[pci_bdf_to_bus(bdf)]; + if (entry->bar >= 0) { + u32 addr = pci_bios_bus_get_addr(bus, entry->type, entry->size); + dprintf(1, "PCI: map device bdf=%02x:%02x.%x" + " bar %d, addr %08x, size %08x [%s]\n", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf), + entry->bar, addr, entry->size, region_type_name[entry->type]); + + pci_set_io_region_addr(entry->dev, entry->bar, addr); + if (entry->is64) + pci_set_io_region_addr(entry->dev, entry->bar + 1, 0); + } +} + static void pci_bios_map_devices(struct pci_bus *busses) { // Setup bases for root bus. @@ -526,28 +581,16 @@ static void pci_bios_map_devices(struct pci_bus *busses) } // Map regions on each device. - struct pci_device *pci; - foreachpci(pci) { - 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)); - 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++; - pci_set_io_region_addr(pci, i, 0); + int bus; + for (bus = 0; bus<=MaxPCIBus; bus++) { + int type; + for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { + struct pci_region_entry *entry = busses[bus].r[type].list; + while (entry) { + pci_region_map_one_entry(busses, entry); + struct pci_region_entry *next = entry->next; + free(entry); + entry = next; } } } @@ -588,7 +631,9 @@ pci_setup(void) return; } memset(busses, 0, sizeof(*busses) * (MaxPCIBus + 1)); - pci_bios_check_devices(busses); + if (pci_bios_check_devices(busses)) + return; + if (pci_bios_init_root_regions(&busses[0], start, end) != 0) { panic("PCI: out of address space\n"); }