[SeaBIOS] [PATCH 3/4] Switch from array based resource allocation to list
Kevin O'Connor
kevin at koconnor.net
Thu Apr 12 05:15:51 CEST 2012
On Thu, Apr 12, 2012 at 02:44:54PM +1200, Alexey Korolev wrote:
> On 04/04/12 15:31, Kevin O'Connor wrote:
> > Agreed - the only thing it does is force a minimum size for memory bars as you pointed out in a previous email. As above, I did play with
> > this a little more on Sunday. I also added in two patches from Gerd's series and made alignment handling more explicit. I'm including the
> > series here if you're interested. Again, I think this should wait until after the 1.7.0 release. -Kevin
> Hi Kevin,
>
> Sorry for late response, was quite busy.
> I've reviewed and tried your patches.
>
> [Patches 1-4] are almost the same as I proposed in this series.
Yes. It's your patches tweaked slightly.
> The noticeable differences are:
> 1) instead of sorting entries at mapping stage your patches sort entries at allocation stage. (no difference in behavior or readability so
> any approach is fine for me)
> 2) instead of storing pointer to bus resource which the bridge device represents (this_bus), it is obtained from pci structure and array of
> "busses".
> - entry->this_bus->r[entry->type].base = entry->base;
> + struct pci_bus *child_bus = &busses[entry->dev->secondary_bus];
> + child_bus->r[entry->type].base = addr;
> Since "entry->this_bus" member is removed the information about whether the resource is bridge region or PCI BAR is encoded inside
> "entry->bar".
> Value "-1" - means this is a bridge region, the positive values mean it is a BAR.
> IMHO 2) makes code less readable, at least a comment explaining the meaning of negative value of PCI BAR is required.
> I've found just cosmetic difference to my patches so please don't forget to add my sign-off-by to them.
Okay - will do.
> [Patch 5] Track region alignment explicitly. Looks very good and safe. Should reduce address range wastes.
>
> [Patch 6] Small patch to account resources of PCI bridges.
> Looks fine.
> May be instead of
> + num_regions = 2;
> I would add
> #define PCI_BRIDGE_NUM_REGIONS 2
> .....
> + num_regions = PCI_BRIDGE_NUM_REGIONS;
This was me playing with some of Gerd's patches. I think it would
require additional changes before committing. In particular, the
patch misses the ROM bar on bridges - I think maybe it should be
changed to keep the same loop constraints but add a "if (bar >
PCI_BRIDGE_NUM_REGIONS && bar < PCI_ROM_SLOT) continue;".
>
> [Patch 7] 64bit aware code. Patch is incomplete. It is not working.
This was also me playing with one of Gerd's patches. It just makes
the bar read/write code 64bit aware. It doesn't actually program
them. The logic to do real 64bit allocations would require list
merging. Is this something you have looked at?
-Kevin
More information about the SeaBIOS
mailing list