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
