[U-Boot] [PATCH v2 4/6] x86: Add infrastructure to extract an e820 table from the coreboot tables

[Graeme Russ]

Hi Gabe,

On Tue, Dec 6, 2011 at 9:06 AM, Gabe Black <gabeblack at google.com> wrote:
>
>
> On Sat, Dec 3, 2011 at 4:52 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
>>
>> Hi Gabe,
>>
>> Last nit, I promise, and then I'll apply it all to u-boot-x86/next
>> >
>> > +/* Implementation defined function to install an e820 map. */
>> > +unsigned install_e820_map(unsigned max_entries, struct e820entry *);
>> > +
>>
>> Should be u8 as boot_params.e820_entries is u8 (__u8 really, but we don't
>> use the kernel data types)
>>
>
> I don't think this is a good idea. The value here isn't consumed by the
> boot_params structure, it's taken from the size of the e820 array there and
> consumed by whatever function is written to copy over e820 entries. That
> function doesn't really care about the boot_params structure at all. It
> could be used to fill in entries for, say, some future boot protocol,
> debugging where you want to see all entries beyond the first 256 (a lot, but
> EFI tends to have a lot), or maybe I want to boot some other OS that doesn't
> use a u8 to determine the size of the e820 table. There isn't any danger of
> the size being mismatched because it's called with the size of the array
> being filled, and the boot_params structure is smart enough to use a data
> type that's sized appropriately for the array.

Good point - I'm happy for you to leave as is

I must say, I am somewhat suprised that the memory map would ever need
to be split into so many pieces...

Regards,

Graeme