Hello Julius Werner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/43579
to review the following change.
Change subject: libpayload: Cache physical location of cb_table entries ......................................................................
libpayload: Cache physical location of cb_table entries
In the presence of self-relocating payloads, it's safer to keep physical addresses in `libsysinfo`. This updates all the references to coreboot-table entries that are not consumed inside libpayload code.
Change-Id: I95cb0af151e0707a1656deacddb8a5253ea38fc3 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 2 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/43579/1
diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index b592413..12223d0 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -89,8 +89,8 @@
unsigned long *mbtable; /** Pointer to the multiboot table */
- struct cb_header *header; - struct cb_mainboard *mainboard; + uintptr_t cb_header; + uintptr_t cb_mainboard;
void *vboot_workbuf;
diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index 43d9df4..be2eebb 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -287,7 +287,7 @@ header->table_bytes) != header->table_checksum) return -1;
- info->header = header; + info->cb_header = virt_to_phys(header);
/* Initialize IDs as undefined in case they don't show up in table. */ info->board_id = UNDEFINED_STRAPPING_ID; @@ -358,7 +358,7 @@ break; #endif case CB_TAG_MAINBOARD: - info->mainboard = (struct cb_mainboard *)ptr; + info->cb_mainboard = virt_to_phys(ptr); break; case CB_TAG_GPIO: cb_parse_gpios(ptr, info);
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43579 )
Change subject: libpayload: Cache physical location of cb_table entries ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43579/1/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/43579/1/payloads/libpayload/include... PS1, Line 93: uintptr_t cb_mainboard; +Patrick FYI that depthcharge references to this need to be updated when downstreaming this patch. (Would probably be a good opportunity to just change cb_mb_part_string() and friends to not require passing in the sysinfo reference. There's only one global sysinfo anyway, they could just fetch it themselves.)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43579 )
Change subject: libpayload: Cache physical location of cb_table entries ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43579 )
Change subject: libpayload: Cache physical location of cb_table entries ......................................................................
libpayload: Cache physical location of cb_table entries
In the presence of self-relocating payloads, it's safer to keep physical addresses in `libsysinfo`. This updates all the references to coreboot-table entries that are not consumed inside libpayload code.
Change-Id: I95cb0af151e0707a1656deacddb8a5253ea38fc3 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/43579 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 2 files changed, 4 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index 303e08e..33610c3 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -91,8 +91,8 @@
unsigned long *mbtable; /** Pointer to the multiboot table */
- struct cb_header *header; - struct cb_mainboard *mainboard; + uintptr_t cb_header; + uintptr_t cb_mainboard;
void *vboot_workbuf;
diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index 43d9df4..be2eebb 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -287,7 +287,7 @@ header->table_bytes) != header->table_checksum) return -1;
- info->header = header; + info->cb_header = virt_to_phys(header);
/* Initialize IDs as undefined in case they don't show up in table. */ info->board_id = UNDEFINED_STRAPPING_ID; @@ -358,7 +358,7 @@ break; #endif case CB_TAG_MAINBOARD: - info->mainboard = (struct cb_mainboard *)ptr; + info->cb_mainboard = virt_to_phys(ptr); break; case CB_TAG_GPIO: cb_parse_gpios(ptr, info);
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)