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

[Kevin O'Connor]

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