Re: [SeaBIOS] [PATCH 11/12] Migrate 64bit entries to 64bit pci regions

[Alexey Korolev]

On 25/04/12 13:48, Kevin O'Connor wrote:
> On Tue, Apr 24, 2012 at 06:25:39PM +1200, Alexey Korolev wrote:
> > Migrate 64bit entries to 64bit pci regions if they do
> > not fit in 32bit range.
>
> [...]
> > +static void pci_region_migrate_64bit_entries(struct pci_region *from,
> >                                         struct pci_region *to)
> >
> > +{
> > struct pci_region_entry **pprev = &from->list;
> > struct pci_region_entry **last = &to->list;
> > while(*pprev) {
> >    if ((*pprev)->is64) {
> >        struct pci_region_entry *entry;
> >        entry = *pprev;
> >        /* Delete the entry and move next */
> >        *pprev = (*pprev)->next;
> >        /* Add entry at tail to keep a sorted order */
> >        entry->next = NULL;
> >        if (*last) {
> >           (*last)->next = entry;
> >            last  = &(*last)->next;
> >        }
> >        else
> >           (*last) = entry;
> >    }
> >    else
> >        pprev = &(*pprev)->next;
> > }
> >
> > +}
>
> It should be possible to simplify this - something like (untested):
>
> static void pci_region_migrate_64bit_entries(struct pci_region *from,
>                                              struct pci_region *to)
> {
>     struct pci_region_entry **pprev = &from->list, **last = &to->list;
>     for (; *pprev; pprev = &(*pprev)->next) {
>         struct pci_region_entry *entry = *pprev;
>         if (!entry->is64)
>             continue;
>         // Move from source list to dest list.
>         *pprev = entry->next;
>         entry->next = NULL;
>         *last = entry;
>         last = &entry->next;
>     }
> }

Sorry it's not working.
I agree it's possible to simplify code a bit.

static void pci_region_migrate_64bit_entries(struct pci_region *from,
                                             struct pci_region *to)
{
    struct pci_region_entry **pprev = &from->list;
    struct pci_region_entry **last = &to->list;
    while(*pprev) {
        if ((*pprev)->is64) {
            struct pci_region_entry *entry;
            entry = *pprev;
            /* Delete the entry and move next */
            *pprev = (*pprev)->next;
            /* Add entry at tail to keep the order */
            entry->next = NULL;
            *last = entry;
            last = &entry->next;
        }
        else
            pprev = &(*pprev)->next;
    }
}

That should work.
> [...]
> > static void pci_bios_map_devices(struct pci_bus *busses)
> >  {
> > if (pci_bios_init_root_regions(busses)) {
> >    struct pci_region r64_mem, r64_pref;
> >    r64_mem.list = NULL;
> >    r64_pref.list = NULL;
> >    pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_MEM],
> >                                     &r64_mem);
> >    pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_PREFMEM],
> >                                     &r64_pref);
> >    if (pci_bios_init_root_regions(busses))
> >        panic("PCI: out of address space\n");
> >    r64_mem.base = BUILD_PCIMEM64_START;
> >    r64_pref.base = ALIGN(r64_mem.base + pci_region_sum(&r64_mem),
> >                          pci_region_align(&r64_pref));
>
> There should be a check to see if the regions fit.  Maybe pass
> start/end into pci_bios_init_root_regions() and call it again for the
> > 4g region?

Agree, I just ignored the check as 64bit range size is 2^39.
I will think how to make it better.
> >    pci_region_map_entries(busses, &r64_mem);
> >    pci_region_map_entries(busses, &r64_pref);
> > }
> > // Map regions on each device.
>
> This doesn't look right to me.  This will map the devices on bus 0 to
> the proper >4g address, but devices on any subsequent bus will use
> busses[0].r[].base which will be reset to the <4gig address.  Perhaps
> pull base out of pci_region and make pci_region_map_entries()
> recursive?

No recursion is need here!
We map all entries which are 64bit on root bus.
If entry is a bridge region - a corresponding bus address will be updated.
Region won't be reseted to <4gig address as address is derived from parent region only.

Thanks,
Alexey