[flashrom] [PATCH] cbtable mapping fix

[Carl-Daniel Hailfinger]

The cbtable code in flashrom is ancient, and although it grew new
features over time, it is not exactly one of the most readable pieces of
code in flashrom. This is in part due to the complicated integration of
the cbtable code in flashrom, and in part due to the way mapping the
cbtable has to deal with OS constraints.
The goal for cbtable and DMI code is to be well-encapsulated with equal
rights for both.

I believe the following patch is a bugfix, but I'd love to get a very
thorough review for this.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Index: flashrom-cbtable_mapping_fixup/cbtable.c
===================================================================
--- flashrom-cbtable_mapping_fixup/cbtable.c	(Revision 1199)
+++ flashrom-cbtable_mapping_fixup/cbtable.c	(Arbeitskopie)
@@ -203,8 +203,11 @@
 	struct lb_record *rec, *last;
 
 #ifdef __DARWIN__
-	/* This is a hack. DirectHW fails to map physical address 0x00000000.
-	 * Why?
+	/* This is a hack. DirectHW apparently fails to map physical address
+	 * 0x00000000.
+	 * FIXME: Modify DirectHW so that it returns (void *)-1 as error
+	 * instead of NULL. Maybe the mapping works and earlier failure reports
+	 * were just side effects of a direct mapping at 0x00000000.
 	 */
 	start = 0x400;
 #else
@@ -216,16 +219,25 @@
 		return -1;
 	}
 
-	lb_table = find_lb_table(table_area, 0x00000, 0x1000);
+	/* (start - start) is used to make it explicit that the search begins
+	 * at start which is mapped at offset (start - start) == 0.
+	 */
+	lb_table = find_lb_table(table_area, start - start, 0x1000 - start);
 	if (!lb_table)
 		lb_table = find_lb_table(table_area, 0xf0000 - start, BYTES_TO_MAP - start);
 	if (lb_table) {
 		struct lb_forward *forward = (struct lb_forward *)
 			(((char *)lb_table) + lb_table->header_bytes);
 		if (forward->tag == LB_TAG_FORWARD) {
-			start = forward->forward;
-			start &= ~(getpagesize() - 1);
-			physunmap(table_area, BYTES_TO_MAP);
+			unsigned long newstart;
+			newstart = forward->forward;
+			newstart &= ~(getpagesize() - 1);
+			/* We know the location of the new table, unmap the
+			 * previous table.
+			 */
+			physunmap(table_area, BYTES_TO_MAP - start);
+			start = newstart;
+			/* FIXME: Do we really want to map 1 MB? */
 			table_area = physmap_try_ro("high tables", start, BYTES_TO_MAP);
 			if (ERROR_PTR == table_area) {
 				msg_perr("Failed getting access to coreboot "
@@ -252,5 +264,7 @@
 	     lb_table->table_entries);
 	search_lb_records(rec, last, addr + lb_table->header_bytes);
 
+	/* FIXME: unmap the table area here or will any code access it later? */
+
 	return 0;
 }


-- 
http://www.hailfinger.org/