Message ID | 4F61621A.40805@endace.com |
---|---|
State | New |
Headers | show |
On Thu, Mar 15, 2012 at 04:29:30PM +1300, Alexey Korolev wrote: > On 14/03/12 13:48, Kevin O'Connor wrote: > > On Tue, Mar 13, 2012 at 05:45:19PM +1300, Alexey Korolev wrote: > >> Added pci_region_entry structure and list operations to pciinit.c > >> List is filled with entries during pci_check_devices. > >> List is used just for printing space allocation if we were using lists. > >> Next step will resource allocation using mapping functions. [...] > > - struct { > > - u32 addr; > > - u32 size; > > - int is64; > > - } bars[PCI_NUM_REGIONS]; [...] > Yes I see what you mean here. Thanks - I do find this patch much easier to understand. I do have some comments on the patch (see below). > > The code is being changed - > > it's not new code being added and old code being deleted - the patches > > need to reflect that. > Because of structural changes it is not possible to completely avoid > this scenario where new code is added and old deleted. In this > patch series I tried my best to make migration as obvious and safe > as possible. So the existing approach (with your suggestions) for > pciinit.c redesign is this: > 1. Introduce list operations > 2. Introduce pci_region_entry structure and add code which fills this new structure. > We don't modify resource addresses calculations, but we use pci_region_entry data to do resource assignment. > 3. Modify resource addresses calculations to be based on linked lists of region entries. > 4. Remove code which fills the arrays, remove use of arrays for mapping. > (note 3&4 could be combined altogether but it will be harder to read then) On patch 3/4, neither patch should introduce code that isn't actually used nor leave code that isn't used still in. So, for example, if the arrays are still used after patch 3 then it's okay to leave them to patch 4, but if they are no longer used at all they should be removed within patch 3. > Could you please have a look at the other parts in this series and > let me know if you are happy about this approach, so I won't have to > redo patchwork too many times? patch 1/6 - I'd prefer to have a list.h with struct hlist_head/hlist_node and container_of before extending to other parts of seabios. That said, I'm okay with what you have for pciinit - we can introduce list.h afterwards. patch 3/4 - was confusing to me as it added new code in one patch and removed the replaced code in the second patch. patch 5 - looked okay to me. Thanks for looking at this - I know it's time consuming. Given the churn in this area I want to make sure there is a good understanding before any big changes. comments on the patch: [...] > +struct pci_region_entry * > +pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev, > + u32 size, int type, int is64bit) > +{ > + struct pci_region_entry *entry= malloc_tmp(sizeof(*entry)); > + if (!entry) { > + warn_noalloc(); > + return NULL; Minor - whitespace. [...] > +static int pci_bios_check_devices(struct pci_bus *busses) > { > dprintf(1, "PCI: check devices\n"); > + struct pci_region_entry *entry; > > // Calculate resources needed for regular (non-bus) devices. > struct pci_device *pci; > @@ -378,19 +419,27 @@ static void pci_bios_check_devices(struct pci_bus *busses) > struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)]; > int i; > for (i = 0; i < PCI_NUM_REGIONS; i++) { > - u32 val, size; > + u32 val, size, min_size; > + int type, is64bit; Minor - I prefer to use C99 inline variable declarations though it isn't a requirement. > + min_size = (type == PCI_REGION_TYPE_IO) ? (1<<PCI_IO_INDEX_SHIFT): > + (1<<PCI_MEM_INDEX_SHIFT); > + size = (size > min_size) ? size : min_size; My only real comment: Why the min_size change? Is that a fix of some sort or is it related to the move to lists? [...] > > - > /**************************************************************** > * Main setup code Minor - whitespace change. -Kevin
On 16/03/12 13:55, Kevin O'Connor wrote: > On Thu, Mar 15, 2012 at 04:29:30PM +1300, Alexey Korolev wrote: >> On 14/03/12 13:48, Kevin O'Connor wrote: >>> On Tue, Mar 13, 2012 at 05:45:19PM +1300, Alexey Korolev wrote: >>>> Added pci_region_entry structure and list operations to pciinit.c >>>> List is filled with entries during pci_check_devices. >>>> List is used just for printing space allocation if we were using lists. >>>> Next step will resource allocation using mapping functions. > [...] >>> - struct { >>> - u32 addr; >>> - u32 size; >>> - int is64; >>> - } bars[PCI_NUM_REGIONS]; > [...] >> Yes I see what you mean here. > Thanks - I do find this patch much easier to understand. I do have > some comments on the patch (see below). > >>> The code is being changed - >>> it's not new code being added and old code being deleted - the patches >>> need to reflect that. >> Because of structural changes it is not possible to completely avoid >> this scenario where new code is added and old deleted. In this >> patch series I tried my best to make migration as obvious and safe >> as possible. So the existing approach (with your suggestions) for >> pciinit.c redesign is this: >> 1. Introduce list operations >> 2. Introduce pci_region_entry structure and add code which fills this new structure. >> We don't modify resource addresses calculations, but we use pci_region_entry data to do resource assignment. >> 3. Modify resource addresses calculations to be based on linked lists of region entries. >> 4. Remove code which fills the arrays, remove use of arrays for mapping. >> (note 3&4 could be combined altogether but it will be harder to read then) > On patch 3/4, neither patch should introduce code that isn't actually > used nor leave code that isn't used still in. So, for example, if the > arrays are still used after patch 3 then it's okay to leave them to > patch 4, but if they are no longer used at all they should be removed > within patch 3. > >> Could you please have a look at the other parts in this series and >> let me know if you are happy about this approach, so I won't have to >> redo patchwork too many times? > patch 1/6 - I'd prefer to have a list.h with struct > hlist_head/hlist_node and container_of before extending to other > parts of seabios. That said, I'm okay with what you have for > pciinit - we can introduce list.h afterwards. Then, it should be a separate patch. It's is better to do this afterwards. > patch 3/4 - was confusing to me as it added new code in one patch and > removed the replaced code in the second patch. > > patch 5 - looked okay to me. > > Thanks for looking at this - I know it's time consuming. Given the > churn in this area I want to make sure there is a good understanding > before any big changes. > > comments on the patch: > [...] >> +struct pci_region_entry * >> +pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev, >> + u32 size, int type, int is64bit) >> +{ >> + struct pci_region_entry *entry= malloc_tmp(sizeof(*entry)); >> + if (!entry) { >> + warn_noalloc(); >> + return NULL; > Minor - whitespace. > > [...] >> +static int pci_bios_check_devices(struct pci_bus *busses) >> { >> dprintf(1, "PCI: check devices\n"); >> + struct pci_region_entry *entry; >> >> // Calculate resources needed for regular (non-bus) devices. >> struct pci_device *pci; >> @@ -378,19 +419,27 @@ static void pci_bios_check_devices(struct pci_bus *busses) >> struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)]; >> int i; >> for (i = 0; i < PCI_NUM_REGIONS; i++) { >> - u32 val, size; >> + u32 val, size, min_size; >> + int type, is64bit; > Minor - I prefer to use C99 inline variable declarations though it > isn't a requirement. > >> + min_size = (type == PCI_REGION_TYPE_IO) ? (1<<PCI_IO_INDEX_SHIFT): >> + (1<<PCI_MEM_INDEX_SHIFT); >> + size = (size > min_size) ? size : min_size; > My only real comment: Why the min_size change? Is that a fix of some > sort or is it related to the move to lists? This is a good question. The min_size changes are to keep the exactly the same behaviour as the original implementation, when each PCI MEM bar is aligned to 4KB (1<<PCI_MEM_INDEX_SHIFT). The PCI spec. doesn't state the 4KB align requirement. So if this 4KB requirement is not coming from qemu, it makes sense to remove the min_size changes. Probably it will be better to remove this as a separate commit, to simplify bug location in case of problems. > [...] >> >> - >> /**************************************************************** >> * Main setup code > Minor - whitespace change. > > -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..f75f393 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -31,6 +31,20 @@ static const char *region_type_name[] = { [ PCI_REGION_TYPE_PREFMEM ] = "prefmem", }; +struct pci_bus; +struct pci_region_entry { + struct pci_device *dev; + int bar; + u32 base; + u32 size; + int is64bit; + enum pci_region_type type; + struct pci_bus *this_bus; + struct pci_bus *parent_bus; + struct pci_region_entry *next; + struct pci_region_entry **pprev; +}; + struct pci_bus { struct { /* pci region stats */ @@ -41,6 +55,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,6 +367,31 @@ pci_bios_get_bar(struct pci_device *pci, int bar, u32 *val, u32 *size) *size = (~(*val & mask)) + 1; } +/**************************************************************** + * Build topology and calculate size of entries + ****************************************************************/ + +struct pci_region_entry * +pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev, + u32 size, int type, int is64bit) +{ + struct pci_region_entry *entry= malloc_tmp(sizeof(*entry)); + if (!entry) { + warn_noalloc(); + return NULL; + } + memset(entry, 0, sizeof(*entry)); + + entry->dev = dev; + entry->type = type; + entry->is64bit = is64bit; + entry->size = size; + + list_add_head(&bus->r[type].list, entry); + entry->parent_bus = bus; + return entry; +} + static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size) { u32 index; @@ -364,9 +404,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_devices(struct pci_bus *busses) +static int pci_bios_check_devices(struct pci_bus *busses) { dprintf(1, "PCI: check devices\n"); + struct pci_region_entry *entry; // Calculate resources needed for regular (non-bus) devices. struct pci_device *pci; @@ -378,19 +419,27 @@ static void pci_bios_check_devices(struct pci_bus *busses) struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)]; int i; for (i = 0; i < PCI_NUM_REGIONS; i++) { - u32 val, size; + u32 val, size, min_size; + int type, is64bit; 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) && + type = pci_addr_to_type(val); + min_size = (type == PCI_REGION_TYPE_IO) ? (1<<PCI_IO_INDEX_SHIFT): + (1<<PCI_MEM_INDEX_SHIFT); + size = (size > min_size) ? size : min_size; + is64bit = (!(val & PCI_BASE_ADDRESS_SPACE_IO) && (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64); - if (pci->bars[i].is64) + pci_bios_bus_reserve(bus, pci_addr_to_type(val), size); + + entry = pci_region_create_entry(bus, pci, size, type, is64bit); + if (!entry) + return -1; + entry->bar = i; + + if (is64bit) i++; } } @@ -411,6 +460,11 @@ static void pci_bios_check_devices(struct pci_bus *busses) 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); + entry = pci_region_create_entry(parent, s->bus_dev, + s->r[type].size, type, 0); + if (!entry) + return -1; + entry->this_bus = s; } dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n", secondary_bus, @@ -418,6 +472,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) @@ -526,34 +581,35 @@ 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; + struct pci_region_entry *entry, *next; + for (bus = 0; bus<=MaxPCIBus; bus++) { + int type; + for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { + list_foreach_entry_safe(busses[bus].r[type].list, + next, entry) { + if (!entry->this_bus) { + entry->base = pci_bios_bus_get_addr(&busses[bus], + entry->type, entry->size); + pci_set_io_region_addr(entry->dev, entry->bar, entry->base); + if (entry->is64bit) + pci_set_io_region_addr(entry->dev, entry->bar, 0); + + dprintf(1, "PCI: map device bdf=%02x:%02x.%x \tbar %d" + "\tsize\t0x%08x\tbase 0x%x type %s\n", + pci_bdf_to_bus(entry->dev->bdf), + pci_bdf_to_dev(entry->dev->bdf), + pci_bdf_to_fn(entry->dev->bdf), + entry->bar, entry->size, entry->base, + region_type_name[entry->type]); + } + list_del(entry); + free(entry); } } } } - /**************************************************************** * Main setup code ****************************************************************/ @@ -588,7 +644,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"); }