Hello Julius Werner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/43576
to review the following change.
Change subject: libpayload: Cache physical CMOS option table location ......................................................................
libpayload: Cache physical CMOS option table location
In the presence of self-relocating payloads, it's safer to keep physical addresses in `libsysinfo`.
Change-Id: I64a37bef263022edb504086c02a3fd22ce068ba4 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/options.c M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 3 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/43576/1
diff --git a/payloads/libpayload/drivers/options.c b/payloads/libpayload/drivers/options.c index 3a14d77..9e437f9 100644 --- a/payloads/libpayload/drivers/options.c +++ b/payloads/libpayload/drivers/options.c @@ -53,7 +53,7 @@
struct cb_cmos_option_table *get_system_option_table(void) { - return lib_sysinfo.option_table; + return phys_to_virt(lib_sysinfo.cmos_option_table); }
int options_checksum_valid(const struct nvram_accessor *nvram) diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index ee081d0..31a3974 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -61,7 +61,7 @@ unsigned int type; } memrange[SYSINFO_MAX_MEM_RANGES];
- struct cb_cmos_option_table *option_table; + uintptr_t cmos_option_table; u32 cmos_range_start; u32 cmos_range_end; u32 cmos_checksum_location; diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index 25812e5..cb47a83 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -170,8 +170,8 @@ #if CONFIG(LP_NVRAM) static void cb_parse_optiontable(void *ptr, struct sysinfo_t *info) { - /* ptr points to a coreboot table entry and is already virtual */ - info->option_table = ptr; + /* ptr is already virtual, but we want to keep physical addresses */ + info->cmos_option_table = virt_to_phys(ptr); }
static void cb_parse_checksum(void *ptr, struct sysinfo_t *info)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43576 )
Change subject: libpayload: Cache physical CMOS option table location ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43576 )
Change subject: libpayload: Cache physical CMOS option table location ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43576 )
Change subject: libpayload: Cache physical CMOS option table location ......................................................................
libpayload: Cache physical CMOS option table location
In the presence of self-relocating payloads, it's safer to keep physical addresses in `libsysinfo`.
Change-Id: I64a37bef263022edb504086c02a3fd22ce068ba4 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/43576 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/drivers/options.c M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 3 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/drivers/options.c b/payloads/libpayload/drivers/options.c index 3a14d77..9e437f9 100644 --- a/payloads/libpayload/drivers/options.c +++ b/payloads/libpayload/drivers/options.c @@ -53,7 +53,7 @@
struct cb_cmos_option_table *get_system_option_table(void) { - return lib_sysinfo.option_table; + return phys_to_virt(lib_sysinfo.cmos_option_table); }
int options_checksum_valid(const struct nvram_accessor *nvram) diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index 3b1b9c9..c3d8c7f 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -63,7 +63,7 @@ unsigned int type; } memrange[SYSINFO_MAX_MEM_RANGES];
- struct cb_cmos_option_table *option_table; + uintptr_t cmos_option_table; u32 cmos_range_start; u32 cmos_range_end; u32 cmos_checksum_location; diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index 25812e5..cb47a83 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -170,8 +170,8 @@ #if CONFIG(LP_NVRAM) static void cb_parse_optiontable(void *ptr, struct sysinfo_t *info) { - /* ptr points to a coreboot table entry and is already virtual */ - info->option_table = ptr; + /* ptr is already virtual, but we want to keep physical addresses */ + info->cmos_option_table = virt_to_phys(ptr); }
static void cb_parse_checksum(void *ptr, struct sysinfo_t *info)
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43576 )
Change subject: libpayload: Cache physical CMOS option table location ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 6/1/7 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/16266 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16265 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/16264 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16263 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/16262 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : FAIL : https://lava.9esec.io/r/16268 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/16267
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)