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

[Alexey Korolev]

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:
> > Introduce list operations
> > 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