[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

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;
 }

[Sean Nelson]

On 10/6/10 6:09 PM, Carl-Daniel Hailfinger wrote:
> 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

Acked-by: Sean Nelson

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [101007 03:09]:
> 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
> 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.
> */

Unfortunately not. The issues are unrelated. (so the FIXME should be
dropped)
>     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? */

Just put the actual unmap there. The pointer to the mapped area is a
local variable so unless some other code is broken in much more evil
ways, noone will (nor should, that's the point of this code) access the
tables later.

Stefan