[coreboot] [patch] mkelfimage intelligent placement of ramdisk

[Warren Togami]

Peter Stuge wrote:
> On Tue, Jun 10, 2008 at 12:47:38PM -0400, Warren Togami wrote:
>> The attached patch by Peter Jones <pjones at redhat.com> implements 
>> intelligent placement of the ramdisk in memory during boot of an
>> ELF image created by mkelfimage.
> 
> I appreciate this patch!
> 
> But can you (or Peter) please describe the algorithm in english?
> 
> I don't like a commit message that claims code to be intelligent.. :p

I personally don't understand the algorithm beyond your interpretation.

Peter explained only a few bits below.  Lots of this algorithm is simply 
how other bootloaders have worked for years.  This was just a simple way 
to implement something known to work.

In any case it is certainly better than the previous hard coded value.

I hope others test this patch so we can be certain it doesn't break 
anyone where it previously worked.  It shouldn't.

> 
>> diff -up mkelfImage-2.7/linux-i386/convert_params.c.ramdisk_base mkelfImage-2.7/linux-i386/convert_params.c
>> --- mkelfImage-2.7/linux-i386/convert_params.c.ramdisk_base	2006-03-27 18:44:59.000000000 -0500
>> +++ mkelfImage-2.7/linux-i386/convert_params.c	2008-05-21 12:55:44.000000000 -0400
>> @@ -1301,6 +1301,44 @@ static void query_firmware_values(struct
>>  	
>>  }
>>  
>> +static void relocate_ramdisk(struct param_info *info)
>> +{
>> +	struct e820entry *e820_map;
>> +	struct e820entry *highest = 0;
>> +	unsigned long load_addr;
>> +	int i;
>> +
>> +	e820_map = info->real_mode->e820_map;
>> +#if 0
>> +	printf("initrd_start = 0x%lx\n", info->real_mode->initrd_start);
>> +	printf("real_mode->e820_map_nr: %d\n", info->real_mode->e820_map_nr);
>> +#endif
> 
> I think these debugging messages should either be hooked to some
> verbose option or simply removed.

For merging with upstream I would just remove these blocks unless you 
already have an established way of building with runtime debug messages. 
  Just go ahead and change it as you see fit for merging.

>> +		if (e820_map[i].addr + info->real_mode->initrd_size >= 0x38000000)
>> +			continue;
> 
> ..what is this 3.5MB limit about? Also might this condition be
> sensitive to an overflow error?
> 

Peter isn't sure why 3.5MB specifically.  He said it is known to work 
for many years in other boot loaders.  There may have been a historic 
reason for this particular value but he doesn't know.

>> diff -up mkelfImage-2.7/linux-i386/mkelf-linux-i386.c.ramdisk_base mkelfImage-2.7/linux-i386/mkelf-linux-i386.c
>> --- mkelfImage-2.7/linux-i386/mkelf-linux-i386.c.ramdisk_base	2006-03-17 09:08:22.000000000 -0500
>> +++ mkelfImage-2.7/linux-i386/mkelf-linux-i386.c	2008-05-21 10:47:42.000000000 -0400
>> @@ -352,6 +352,9 @@ int linux_i386_mkelf(int argc, char **ar
>>  	 */
>>  	params->initrd_start = params->initrd_size = 0;
>>  	if (ramdisk_size) {
>> +		while (ramdisk_base <= kernel_size)
>> +			ramdisk_base <<= 1;
>> +
> 
> Does this start with 1MB, then try 2MB, 4MB and so on?

According to Peter there this is only a simple way that avoids alignment 
earlier.  While not expressly necessary (you could add), this requires 
less code, and more importantly, it is known to work.

> 
> 
>>  		phdr[index].p_paddr  = ramdisk_base;
>>  		phdr[index].p_vaddr  = ramdisk_base;
>>  		phdr[index].p_filesz = ramdisk_size;
> 
> 
> Please keep in mind to also include Signed-off-by: lines from all who
> have touched the patch. Please see
> http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure
> 

OK, he agreed.  Attached is the same patch with his signing off (but not 
me since I didn't change it.)

Warren Togami
wtogami at redhat.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mkelfImage-2.7-ramdisk_base.patch
Type: text/x-patch
Size: 2646 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20080610/8be6ac4e/attachment.patch>