Patchwork Fix hopefully last stopper bug for CBFS. Plan to get rid of non-CBFS

login
register
about
Submitter Patrick Georgi
Date 2009-09-30 16:16:58
Message ID <1254327418.9254.10.camel@tetris>
Download mbox | patch
Permalink /patch/311/
State Accepted
Headers show

Comments

Patrick Georgi - 2009-09-30 16:16:58
Hi,

attached patch reworks the payload handling with CBFS (ie. selfboot).
We use a bounce buffer (both in CBFS and older) to keep coreboot running
while allowing the payload to use the same memory region, and this is a
tricky part of our code.

Assumptions in the code are:
The bounce buffer is at least 2x coreboot_ram size (lb_end - lb_start),
with the first half being the shadow region that the payload's data
lives in (exactly the bytes that cover the area of coreboot_ram), while
the second half exists to host a copy of coreboot while copying.

The change leads to the following behaviour:
At least 2x coreboot_ram size is looked for.

The area before the bounce buffer is assumed to be reasonably free (but
this isn't validated)

Payload segments that conflict with coreboot_ram are loaded to the
bounce buffer, probably using the memory before it, and filling up more
than coreboot_ram bytes there

Right after copying, the "out of area" bytes are copied to their final
location (around coreboot_ram).

I want to revisit the prefix/suffix calculation, as I'm not 100% sure
that it's doing the right thing (those copy operations should copy
_exactly_ the necessary set of bytes, nothing more), review is welcome
(look for "prefix" and "suffix" in selfboot.c)

The code is runtime tested on qemu-x86 (after cranking up the STACK_SIZE
to >16kb, so lzma could run), with the following scenarios:

RAMBASE = 0x4000, payload starting at 0x00100000
 (no collision between coreboot_ram and payload)

RAMBASE = 0x00100000, payload starting at 0x00100000
 (RAMBASE below or at payload starting address, this worked before)

RAMBASE = 0x00101000, payload starting at 0x00100000
 (RAMBASE above payload starting address, this fails without this patch.
  AMD boards were notorious with that one)

Pending the review of the prefix/suffix calculations (by me and
hopefully others), this is

Signed-off-by: Patrick Georgi <patrick.georgi@coresystems.de>


Once this is in, there are no CBFS issues that I know of, and hence I
propose to eliminate non-CBFS support in the tree soonish after that
patch is in.
I have some older patch for this, but it will likely not apply anymore,
but I can update it.

Opinions?


Regards,
Patrick
ron minnich - 2009-09-30 17:21:08
Acked-by: Ronald G. Minnich <rminnich@gmail.com>

Patch

Index: src/boot/selfboot.c
===================================================================
--- src/boot/selfboot.c	(Revision 4691)
+++ src/boot/selfboot.c	(Arbeitskopie)
@@ -423,14 +423,13 @@ 
 {
 	struct segment *ptr;
 	
-	unsigned long required_bounce_size = lb_end - lb_start;
+	unsigned long bounce_high = lb_end;
 	for(ptr = head->next; ptr != head; ptr = ptr->next) {
 		if (!overlaps_coreboot(ptr)) continue;
-		unsigned long bounce = ptr->s_dstaddr + ptr->s_memsz - lb_start;
-		if (bounce > required_bounce_size)
-			required_bounce_size = bounce;
+		if (ptr->s_dstaddr + ptr->s_memsz > bounce_high)
+			bounce_high = ptr->s_dstaddr + ptr->s_memsz;
 	}
-	get_bounce_buffer(mem, required_bounce_size);
+	get_bounce_buffer(mem, bounce_high - lb_start);
 	if (!bounce_buffer) {
 		printk_err("Could not find a bounce buffer...\n");
 		return 0;
@@ -502,6 +501,24 @@ 
 				/* Zero the extra bytes */
 				memset(middle, 0, end - middle);
 			}
+			/* Copy the data that's outside the area that shadows coreboot_ram */
+			printk_debug("dest %lx, end %lx, bouncebuffer %lx\n", dest, end, bounce_buffer);
+			if ((unsigned long)end > bounce_buffer) {
+				if ((unsigned long)dest < bounce_buffer) {
+					unsigned long from = dest;
+					unsigned long to = lb_start-(bounce_buffer-(unsigned long)dest);
+					unsigned long amount = bounce_buffer-(unsigned long)dest;
+					printk_debug("move prefix around: from %lx, to %lx, amount: %lx\n", from, to, amount);
+					memcpy(to, from, amount);
+				}
+				if ((unsigned long)end > bounce_buffer + (lb_end - lb_start)) {
+					unsigned long from = bounce_buffer + bounce_size;
+					unsigned long to = lb_end;
+					unsigned long amount = end - (bounce_buffer + bounce_size);
+					printk_debug("move suffix around: from %lx, to %lx, amount: %lx\n", from, to, amount);
+					memcpy(to, from, amount);
+				}
+			}
 		}
 	}
 	return 1;