Peter Stuge wrote:
On Tue, Jun 10, 2008 at 12:47:38PM -0400, Warren Togami wrote:
The attached patch by Peter Jones pjones@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@redhat.com