Patch set updated for coreboot: ebfafa0 Cleanup needed to make coreboot converge with linux driver
Philip Prindeville (pprindeville@gmail.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/495
-gerrit
commit ebfafa07bc5c92d4c14b890e245e57124d1b7edb Author: Philip Prindeville philipp@redfish-solutions.com Date: Wed Dec 21 13:29:53 2011 -0700
Cleanup needed to make coreboot converge with linux driver
Having submitted changes upstream to add a linux kernel library for Coreboot, I was given a list of changes to make. While all good suggestions, they would have caused the linux version to diverge significantly with the original coreboot code from which it was cloned.
Those changes are:
* more use of inline functions rather than macros (they avoid typecasts); * convenience functions to access the motherboard vendor and part # from the parsed sysinfo. * use of void * for pointers to untyped memory; * eliminate unnecessary casts; * pass end-boundary of E820 memory region when parsing coreboot tables (in case of corruption); * add missing phys_to_virt() when handling FORWARD records; * save mainboard record pointer into sysinfo structure for later retrieval; * pass through return value from get_coreboot_info() in lib_get_sysinfo();
The fifth change merits some more detail: on some of the systems I develop on the CB table gets indirected to by a CB_FORWARD record. When the final table (and not the initial stub containing the CB_FORWARD) is found, it is tucked up in the last bytes of the E820 region. Passing 0x1000 as the length might not always be correct if the table begins at less than 0x1000 bytes from the end of that region. Explicitly passing in the region's end is a more accurate semantic.
Change-Id: Iffe7061fa62fa639e0cb6ccb9125eb3403d06b1a Signed-off-by: Philip Prindeville philipp@redfish-solutions.com --- payloads/coreinfo/coreboot_module.c | 12 ++-- payloads/libpayload/arch/i386/coreboot.c | 66 +++++++++++++++---------- payloads/libpayload/arch/i386/sysinfo.c | 8 ++- payloads/libpayload/include/coreboot_tables.h | 33 ++++++++---- payloads/libpayload/include/libpayload.h | 2 +- payloads/libpayload/include/sysinfo.h | 3 + 6 files changed, 77 insertions(+), 47 deletions(-)
diff --git a/payloads/coreinfo/coreboot_module.c b/payloads/coreinfo/coreboot_module.c index dd589ac..f298595 100644 --- a/payloads/coreinfo/coreboot_module.c +++ b/payloads/coreinfo/coreboot_module.c @@ -17,8 +17,8 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */
-#include <coreboot_tables.h> #include "coreinfo.h" +#include <coreboot_tables.h>
#ifdef CONFIG_MODULE_COREBOOT
@@ -112,9 +112,9 @@ int coreboot_module_redraw(WINDOW *win) }
wprintw(win, "%16.16llx - %16.16llx", - UNPACK_CB64(cb_info.range[i].start), - UNPACK_CB64(cb_info.range[i].start) + - UNPACK_CB64(cb_info.range[i].size) - 1); + cb_unpack64(cb_info.range[i].start), + cb_unpack64(cb_info.range[i].start) + + cb_unpack64(cb_info.range[i].size) - 1); }
return 0; @@ -142,8 +142,8 @@ static void parse_mainboard(unsigned char *ptr) { struct cb_mainboard *mb = (struct cb_mainboard *)ptr;
- strncpy(cb_info.vendor, (const char *)MB_VENDOR_STRING(mb), 31); - strncpy(cb_info.part, (const char *)MB_PART_STRING(mb), 31); + strncpy(cb_info.vendor, (const char *)cb_mb_vendor_string(mb), 31); + strncpy(cb_info.part, (const char *)cb_mb_part_string(mb), 31); }
static void parse_strings(unsigned char *ptr) diff --git a/payloads/libpayload/arch/i386/coreboot.c b/payloads/libpayload/arch/i386/coreboot.c index 365445e..e49cba8 100644 --- a/payloads/libpayload/arch/i386/coreboot.c +++ b/payloads/libpayload/arch/i386/coreboot.c @@ -42,9 +42,9 @@ /* === Parsing code === */ /* This is the generic parsing code. */
-static void cb_parse_memory(unsigned char *ptr, struct sysinfo_t *info) +static void cb_parse_memory(void *ptr, struct sysinfo_t *info) { - struct cb_memory *mem = (struct cb_memory *)ptr; + struct cb_memory *mem = ptr; int count = MEM_RANGE_COUNT(mem); int i;
@@ -54,8 +54,7 @@ static void cb_parse_memory(unsigned char *ptr, struct sysinfo_t *info) info->n_memranges = 0;
for (i = 0; i < count; i++) { - struct cb_memory_range *range = - (struct cb_memory_range *)MEM_RANGE_PTR(mem, i); + struct cb_memory_range *range = MEM_RANGE_PTR(mem, i);
#ifdef CONFIG_MEMMAP_RAM_ONLY if (range->type != CB_MEM_RAM) @@ -63,10 +62,10 @@ static void cb_parse_memory(unsigned char *ptr, struct sysinfo_t *info) #endif
info->memrange[info->n_memranges].base = - UNPACK_CB64(range->start); + cb_unpack64(range->start);
info->memrange[info->n_memranges].size = - UNPACK_CB64(range->size); + cb_unpack64(range->size);
info->memrange[info->n_memranges].type = range->type;
@@ -74,29 +73,29 @@ static void cb_parse_memory(unsigned char *ptr, struct sysinfo_t *info) } }
-static void cb_parse_serial(unsigned char *ptr, struct sysinfo_t *info) +static void cb_parse_serial(void *ptr, struct sysinfo_t *info) { - struct cb_serial *ser = (struct cb_serial *)ptr; + struct cb_serial *ser = ptr; if (ser->type != CB_SERIAL_TYPE_IO_MAPPED) return; info->ser_ioport = ser->baseaddr; }
-static void cb_parse_version(unsigned char *ptr, struct sysinfo_t *info) +static void cb_parse_version(void *ptr, struct sysinfo_t *info) { - struct cb_string *ver = (struct cb_string *)ptr; + struct cb_string *ver = ptr; info->cb_version = (char *)ver->string; }
#ifdef CONFIG_NVRAM -static void cb_parse_optiontable(unsigned char *ptr, struct sysinfo_t *info) +static void cb_parse_optiontable(void *ptr, struct sysinfo_t *info) { - info->option_table = (struct cb_cmos_option_table *)ptr; + info->option_table = ptr; }
-static void cb_parse_checksum(unsigned char *ptr, struct sysinfo_t *info) +static void cb_parse_checksum(void *ptr, struct sysinfo_t *info) { - struct cb_cmos_checksum *cmos_cksum = (struct cb_cmos_checksum *)ptr; + struct cb_cmos_checksum *cmos_cksum = ptr; info->cmos_range_start = cmos_cksum->range_start; info->cmos_range_end = cmos_cksum->range_end; info->cmos_checksum_location = cmos_cksum->location; @@ -104,39 +103,42 @@ static void cb_parse_checksum(unsigned char *ptr, struct sysinfo_t *info) #endif
#ifdef CONFIG_COREBOOT_VIDEO_CONSOLE -static void cb_parse_framebuffer(unsigned char *ptr, struct sysinfo_t *info) +static void cb_parse_framebuffer(void *ptr, struct sysinfo_t *info) { - info->framebuffer = (struct cb_framebuffer *)ptr; + info->framebuffer = ptr; } #endif
-static int cb_parse_header(void *addr, int len, struct sysinfo_t *info) +static int cb_parse_header(void *addr, void *end, struct sysinfo_t *info) { struct cb_header *header; - unsigned char *ptr = (unsigned char *)addr; + unsigned char *ptr = addr; + void *forward; int i;
- for (i = 0; i < len; i += 16, ptr += 16) { + for (i = 0; (void *)ptr < end; i += 16, ptr += 16) { header = (struct cb_header *)ptr; if (!strncmp((const char *)header->signature, "LBIO", 4)) break; }
/* We walked the entire space and didn't find anything. */ - if (i >= len) + if ((void *)ptr >= end) return -1;
if (!header->table_bytes) return 0;
/* Make sure the checksums match. */ - if (ipchksum((u16 *) header, sizeof(*header)) != 0) + if (cb_checksum(header, sizeof(*header)) != 0) return -1;
- if (ipchksum((u16 *) (ptr + sizeof(*header)), + if (cb_checksum((ptr + sizeof(*header)), header->table_bytes) != header->table_checksum) return -1;
+ info->header = header; + /* Now, walk the tables. */ ptr += header->header_bytes;
@@ -146,8 +148,8 @@ static int cb_parse_header(void *addr, int len, struct sysinfo_t *info) /* We only care about a few tags here (maybe more later). */ switch (rec->tag) { case CB_TAG_FORWARD: - return cb_parse_header((void *)(unsigned long)((struct cb_forward *)rec)->forward, len, info); - continue; + forward = phys_to_virt((void *)(unsigned long)((struct cb_forward *)rec)->forward); + return cb_parse_header(forward, forward + 0x1000, info); case CB_TAG_MEMORY: cb_parse_memory(ptr, info); break; @@ -172,9 +174,16 @@ static int cb_parse_header(void *addr, int len, struct sysinfo_t *info) cb_parse_framebuffer(ptr, info); break; #endif + + case CB_TAG_MAINBOARD: + info->mainboard = (struct cb_mainboard *)ptr; + break; }
ptr += rec->size; + + if ((void *)ptr >= end) + return -1; }
return 1; @@ -185,10 +194,13 @@ static int cb_parse_header(void *addr, int len, struct sysinfo_t *info)
int get_coreboot_info(struct sysinfo_t *info) { - int ret = cb_parse_header(phys_to_virt(0x00000000), 0x1000, info); + void *base = phys_to_virt(0x00000000); + int ret = cb_parse_header(base, base + 0x1000, info);
- if (ret != 1) - ret = cb_parse_header(phys_to_virt(0x000f0000), 0x1000, info); + if (ret != 1) { + base = phys_to_virt(0x000f0000); + ret = cb_parse_header(base, base + 0x1000, info); + }
return (ret == 1) ? 0 : -1; } diff --git a/payloads/libpayload/arch/i386/sysinfo.c b/payloads/libpayload/arch/i386/sysinfo.c index 599a811..6c1ef3f 100644 --- a/payloads/libpayload/arch/i386/sysinfo.c +++ b/payloads/libpayload/arch/i386/sysinfo.c @@ -45,8 +45,10 @@ struct sysinfo_t lib_sysinfo = { #endif };
-void lib_get_sysinfo(void) +int lib_get_sysinfo(void) { + int ret; + /* Get the CPU speed (for delays). */ lib_sysinfo.cpu_khz = get_cpu_speed();
@@ -59,7 +61,7 @@ void lib_get_sysinfo(void) /* Get information from the coreboot tables, * if they exist */
- get_coreboot_info(&lib_sysinfo); + ret = get_coreboot_info(&lib_sysinfo);
if (!lib_sysinfo.n_memranges) { /* If we can't get a good memory range, use the default. */ @@ -73,4 +75,6 @@ void lib_get_sysinfo(void) lib_sysinfo.memrange[1].size = 31 * 1024 * 1024; lib_sysinfo.memrange[1].type = CB_MEM_RAM; } + + return ret; } diff --git a/payloads/libpayload/include/coreboot_tables.h b/payloads/libpayload/include/coreboot_tables.h index d342c99..706e714 100644 --- a/payloads/libpayload/include/coreboot_tables.h +++ b/payloads/libpayload/include/coreboot_tables.h @@ -216,22 +216,33 @@ struct cb_cmos_checksum { u32 type; };
+static inline u16 cb_checksum(const void *ptr, unsigned len) +{ + return ipchksum(ptr, len); +} + +static inline const u8 *cb_mb_vendor_string(const struct cb_mainboard *cbm) +{ + return cbm->strings + cbm->vendor_idx; +} + +static inline const u8 *cb_mb_part_string(const struct cb_mainboard *cbm) +{ + return cbm->strings + cbm->part_number_idx; +} + +static inline u64 cb_unpack64(struct cbuint64 val) +{ + return (((u64) val.hi) << 32) | val.lo; +} + /* Helpful macros */
#define MEM_RANGE_COUNT(_rec) \ (((_rec)->size - sizeof(*(_rec))) / sizeof((_rec)->map[0]))
#define MEM_RANGE_PTR(_rec, _idx) \ - (((u8 *) (_rec)) + sizeof(*(_rec)) \ - + (sizeof((_rec)->map[0]) * (_idx))) - -#define MB_VENDOR_STRING(_mb) \ - (((unsigned char *) ((_mb)->strings)) + (_mb)->vendor_idx) - -#define MB_PART_STRING(_mb) \ - (((unsigned char *) ((_mb)->strings)) + (_mb)->part_number_idx) - -#define UNPACK_CB64(_in) \ - ( (((u64) _in.hi) << 32) | _in.lo ) + (void *)(((u8 *) (_rec)) + sizeof(*(_rec)) \ + + (sizeof((_rec)->map[0]) * (_idx)))
#endif diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 8c8abc4..d9f7d21 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -369,7 +369,7 @@ int sysinfo_have_multiboot(unsigned long *addr); int get_coreboot_info(struct sysinfo_t *info); int get_multiboot_info(struct sysinfo_t *info);
-void lib_get_sysinfo(void); +int lib_get_sysinfo(void);
/* Timer functions - defined by each architecture. */ unsigned int get_cpu_speed(void); diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index c1d2002..778dfe9 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -56,6 +56,9 @@ struct sysinfo_t { struct cb_framebuffer *framebuffer;
unsigned long *mbtable; /** Pointer to the multiboot table */ + + struct cb_header *header; + struct cb_mainboard *mainboard; };
extern struct sysinfo_t lib_sysinfo;
-
Philip Prindeville