Re: [Etherboot-developers] mknbi-1.2-7rc2 released

[<ebi...@ln...>]

ke...@us... (Ken Yap) writes:

> >Just doing a code review I can say that there are some serious logic bugs.
> >Usually there are at least 2 ram segments in the returned e820 information
> >which is a little screwy.
> 
> Can you elaborate?

Here is the e820 map from one of my test machines:
BIOS-e820: 0000000000000000 - 000000000009FC00 (ram)
BIOS-e820: 000000000009FC00 - 00000000000A0000 (ram)
BIOS-e820: 00000000000F0000 - 0000000000100000 (reserved)
BIOS-e820: 00000000FEC00000 - 0000000100000000 (reserved)
BIOS-e820: 0000000000100000 - 000000001FFF0000 (ram)
BIOS-e820: 000000001FFF3000 - 0000000020000000 (ACPI data)
BIOS-e820: 000000001FFF0000 - 000000001FFF3000 (ACPI NVS)

Your code if I read it right would decide to use:
BIOS-e820: 000000000009FC00 - 00000000000A0000 (ram)
to compute the top of memory.  

There are no gaurantees that the e820 result will even be sorted.
 
> >But more interesting the following logic in first32.c is just wrong.
> >
> >		top_of_mem = ((setup->su_version >= 0x203) ?
> >			setup->ramdisk_max :
> >			(e->addr + e->size)
> >			) >> 10;
> >
> >setup->ramdisk_max is in bytes not kilobytes.
> >
> >And if you set the top_of_ram to just under 1GB on a 16M machine your
> >ramdisk will fail miserably.  Unless there is some magic that just
> >makes this code work...
> 
> But top_of_mem is measured in kB (I know, it's confusing, maybe I should
> rationalise that). Note the parentheses. ramdisk_max is at the same
> level as e->addr + e->size. If expanded it looks like this:
> 
> 	if (su_version >= 0x203)
> 		temp = ramdisk_max;
> 	else
> 		temp = e->addr + e->size;
> 	top_of_mem = temp >> 10;

O.k.  I missed that the >> applied to both parts of the assignment.

But the bigger issue that ramdisk_max =~ 1GB.  And you set
top_of_mem to that even when you only have 16MB of memory.  And then
you copy your ramdisk to just below top_of_mem is a real issue.

Better logic looks like:
ramdisk_max =  0x37FFFFFF >> 10;
if (su_version >= 0x203) {
        ramdisk_max = setup->ramdisk_max >> 10;
}

if (top_of_mem > ramdisk_max) {
        top_of_mem = ramdisk_max;
}

Eric