Ronald G. Minnich (rminnich@gmail.com) just uploaded a new patch set to gerrit, which you can find at https://review.coreboot.org/18139
-gerrit
commit 359db64884d8ebcb66bc7c3f5c37ae4eb94bf7a4 Author: Ronald G. Minnich rminnich@gmail.com Date: Fri Jan 13 18:27:34 2017 -0800
riscv: fix up issues related to lb_tables
In the lb_tables code, some functions made a reasonable effort to align structs on reasonable boundaries, and others did not. Even the ones that did did not get it quite right, setting alignments to 32 bits, not 64, such that riscv will still take alignment traps in a mode (M-mode) in which such traps are fatal.
Change lb_new_record to take a tag and size parameter. This further removes the need for each function to assign tag and size to the record. And, it lets the allocator (lb_new_record) get some idea of the final size. Most importantly, it lets the allocator ensure that entities are 128-bit rounded, and hence future proof for 128-bit processors.
This gets me back up to a shell prompt on harvey on the lowriscv, where before we would fail with an alignment trap in coreboot.
Change-Id: Id683fe2baa8cbea9abfccab72cf66a44fc04429e Signed-off-by: Ronald G. Minnich rminnich@gmail.com --- src/include/boot/coreboot_tables.h | 4 ++- src/lib/coreboot_table.c | 67 ++++++++++++++++++++------------------ src/lib/imd_cbmem.c | 6 ++-- 3 files changed, 41 insertions(+), 36 deletions(-)
diff --git a/src/include/boot/coreboot_tables.h b/src/include/boot/coreboot_tables.h index 5647629..f7e76dd 100644 --- a/src/include/boot/coreboot_tables.h +++ b/src/include/boot/coreboot_tables.h @@ -36,6 +36,8 @@ void lb_table_add_macs_from_vpd(struct lb_header *header);
void lb_table_add_serialno_from_vpd(struct lb_header *header);
-struct lb_record *lb_new_record(struct lb_header *header); +struct lb_record *lb_new_record(struct lb_header *header, + uint32_t tag, + uint32_t size);
#endif /* COREBOOT_TABLES_H */ diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index f8da658..9f3df30 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -82,7 +82,8 @@ static struct lb_record *lb_last_record(struct lb_header *header) return rec; }
-struct lb_record *lb_new_record(struct lb_header *header) +struct lb_record *lb_new_record(struct lb_header *header, + uint32_t tag, uint32_t size) { struct lb_record *rec; rec = lb_last_record(header); @@ -91,8 +92,15 @@ struct lb_record *lb_new_record(struct lb_header *header) } rec = lb_last_record(header); header->table_entries++; - rec->tag = LB_TAG_UNUSED; - rec->size = sizeof(*rec); + rec->tag = tag; + // If the rec->size is not aligned to something reasonable, + // some architectures can take alignment traps. + // Round up table_bytes to something that is future-proof, + // namely 128 bits (16 bytes). This will ensure that even + // the 128-bit RISCV won't get traps. + // We have roundup macros all over the tree; we need to + // clean that up at some point. + rec->size = (sizeof(*rec) + size + 0xf) & ~0xf; return rec; }
@@ -100,10 +108,8 @@ static struct lb_memory *lb_memory(struct lb_header *header) { struct lb_record *rec; struct lb_memory *mem; - rec = lb_new_record(header); + rec = lb_new_record(header, LB_TAG_MEMORY, sizeof(*mem)); mem = (struct lb_memory *)rec; - mem->tag = LB_TAG_MEMORY; - mem->size = sizeof(*mem); return mem; }
@@ -112,9 +118,9 @@ void lb_add_serial(struct lb_serial *new_serial, void *data) struct lb_header *header = (struct lb_header *)data; struct lb_serial *serial;
- serial = (struct lb_serial *)lb_new_record(header); - serial->tag = LB_TAG_SERIAL; - serial->size = sizeof(*serial); + serial = (struct lb_serial *)lb_new_record(header, + LB_TAG_SERIAL, + sizeof(*serial)); serial->type = new_serial->type; serial->baseaddr = new_serial->baseaddr; serial->baud = new_serial->baud; @@ -128,9 +134,9 @@ void lb_add_console(uint16_t consoletype, void *data) struct lb_header *header = (struct lb_header *)data; struct lb_console *console;
- console = (struct lb_console *)lb_new_record(header); - console->tag = LB_TAG_CONSOLE; - console->size = sizeof(*console); + console = (struct lb_console *)lb_new_record(header, + LB_TAG_CONSOLE, + sizeof(*console)); console->type = consoletype; }
@@ -276,9 +282,8 @@ static void lb_boot_media_params(struct lb_header *header) if (boot_dev == NULL) return;
- bmp = (struct lb_boot_media_params *)lb_new_record(header); - bmp->tag = LB_TAG_BOOT_MEDIA_PARAMS; - bmp->size = sizeof(*bmp); + bmp = (struct lb_boot_media_params *) + lb_new_record(header, LB_TAG_BOOT_MEDIA_PARAMS, sizeof(*bmp));
bmp->cbfs_offset = props.offset; bmp->cbfs_size = props.size; @@ -329,7 +334,9 @@ static void add_cbmem_pointers(struct lb_header *header) if (!cbmem_addr) continue; /* This section is not present */
- cbmem_ref = (struct lb_cbmem_ref *)lb_new_record(header); + cbmem_ref = (struct lb_cbmem_ref *) + lb_new_record(header, sid->table_tag, + sizeof(*cbmem_ref)); if (!cbmem_ref) { printk(BIOS_ERR, "No more room in coreboot table!\n"); break; @@ -344,14 +351,13 @@ static struct lb_mainboard *lb_mainboard(struct lb_header *header) { struct lb_record *rec; struct lb_mainboard *mainboard; - rec = lb_new_record(header); - mainboard = (struct lb_mainboard *)rec; - mainboard->tag = LB_TAG_MAINBOARD; + rec = lb_new_record(header, LB_TAG_MAINBOARD, + sizeof(*mainboard) + + strlen(mainboard_vendor) + 1 + + strlen(mainboard_part_number) + 1);
- mainboard->size = (sizeof(*mainboard) + - strlen(mainboard_vendor) + 1 + - strlen(mainboard_part_number) + 1 + - 3) & ~3; + + mainboard = (struct lb_mainboard *)rec;
mainboard->vendor_idx = 0; mainboard->part_number_idx = strlen(mainboard_vendor) + 1; @@ -399,10 +405,10 @@ static void lb_strings(struct lb_header *header) for(i = 0; i < ARRAY_SIZE(strings); i++) { struct lb_string *rec; size_t len; - rec = (struct lb_string *)lb_new_record(header); len = strlen(strings[i].string); - rec->tag = strings[i].tag; - rec->size = (sizeof(*rec) + len + 1 + 3) & ~3; + rec = (struct lb_string *)lb_new_record(header, + strings[i].tag, + sizeof(*rec) + len + 1); memcpy(rec->string, strings[i].string, len+1); }
@@ -411,9 +417,8 @@ static void lb_strings(struct lb_header *header) static void lb_record_version_timestamp(struct lb_header *header) { struct lb_timestamp *rec; - rec = (struct lb_timestamp *)lb_new_record(header); - rec->tag = LB_TAG_VERSION_TIMESTAMP; - rec->size = sizeof(*rec); + rec = (struct lb_timestamp *) + lb_new_record(header, LB_TAG_VERSION_TIMESTAMP, sizeof(*rec)); rec->timestamp = coreboot_version_timestamp; }
@@ -431,10 +436,8 @@ static struct lb_forward *lb_forward(struct lb_header *header, struct lb_header { struct lb_record *rec; struct lb_forward *forward; - rec = lb_new_record(header); + rec = lb_new_record(header, LB_TAG_FORWARD, sizeof(*forward)); forward = (struct lb_forward *)rec; - forward->tag = LB_TAG_FORWARD; - forward->size = sizeof(*forward); forward->forward = (uint64_t)(unsigned long)next_header; return forward; } diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index b0273f4..8187ffa 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -335,9 +335,9 @@ void cbmem_add_records_to_cbtable(struct lb_header *header) if (id == CBMEM_ID_IMD_ROOT || id == CBMEM_ID_IMD_SMALL) continue;
- lbe = (struct lb_cbmem_entry *)lb_new_record(header); - lbe->tag = LB_TAG_CBMEM_ENTRY; - lbe->size = sizeof(*lbe); + lbe = (struct lb_cbmem_entry *)lb_new_record(header, + LB_TAG_CBMEM_ENTRY, + sizeof(*lbe)); lbe->address = (uintptr_t)imd_entry_at(imd, e); lbe->entry_size = imd_entry_size(imd, e); lbe->id = id;