Re: [Qemu-devel] [PATCH 2/6] Redesign of pciinit.c (take 2)

[Alexey Korolev <alexey.korolev@endace.com>]

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