Hello Julius Werner, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38918
to review the following change.
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
vboot/Kconfig: Handle VBNV RTC offset in code
This way, programmers and reviewers don't have to make the calculation over and over in their heads.
Change-Id: I928457febd399526fa77108d12b804ca7e53f80b Signed-off-by: Nico Huber nico.h@gmx.de --- M src/lib/coreboot_table.c M src/mainboard/emulation/qemu-i440fx/Kconfig M src/mainboard/emulation/qemu-q35/Kconfig M src/mainboard/google/kahlee/Kconfig M src/mainboard/hp/z220_sff_workstation/Kconfig M src/mainboard/lenovo/t400/Kconfig M src/mainboard/lenovo/t410/Kconfig M src/mainboard/lenovo/t420/Kconfig M src/mainboard/lenovo/t420s/Kconfig M src/mainboard/lenovo/t520/Kconfig M src/mainboard/lenovo/x200/Kconfig M src/mainboard/lenovo/x201/Kconfig M src/mainboard/lenovo/x220/Kconfig M src/mainboard/supermicro/x11-lga1151-series/Kconfig M src/security/vboot/Kconfig M src/security/vboot/vbnv_cmos.c M src/vendorcode/google/chromeos/acpi/chromeos.asl 17 files changed, 19 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/38918/1
diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index e42cb3b..33ce503 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -212,7 +212,7 @@ vbnv = (struct lb_range *)lb_new_record(header); vbnv->tag = LB_TAG_VBNV; vbnv->size = sizeof(*vbnv); - vbnv->range_start = CONFIG_VBOOT_VBNV_OFFSET + 14; + vbnv->range_start = CONFIG_VBOOT_VBNV_OFFSET; vbnv->range_size = VBOOT_VBNV_BLOCK_SIZE; #endif } diff --git a/src/mainboard/emulation/qemu-i440fx/Kconfig b/src/mainboard/emulation/qemu-i440fx/Kconfig index 3c27b1e..fde47c2 100644 --- a/src/mainboard/emulation/qemu-i440fx/Kconfig +++ b/src/mainboard/emulation/qemu-i440fx/Kconfig @@ -38,7 +38,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2c + default 0x3a
config MAINBOARD_DIR string diff --git a/src/mainboard/emulation/qemu-q35/Kconfig b/src/mainboard/emulation/qemu-q35/Kconfig index ee430d0..a0a7175 100644 --- a/src/mainboard/emulation/qemu-q35/Kconfig +++ b/src/mainboard/emulation/qemu-q35/Kconfig @@ -37,7 +37,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2c + default 0x3a
config MAINBOARD_DIR string diff --git a/src/mainboard/google/kahlee/Kconfig b/src/mainboard/google/kahlee/Kconfig index 7d98d89..c538e12 100644 --- a/src/mainboard/google/kahlee/Kconfig +++ b/src/mainboard/google/kahlee/Kconfig @@ -107,7 +107,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2A + default 0x38
config CHROMEOS select LP_DEFCONFIG_OVERRIDE if PAYLOAD_DEPTHCHARGE diff --git a/src/mainboard/hp/z220_sff_workstation/Kconfig b/src/mainboard/hp/z220_sff_workstation/Kconfig index dc288d6..b07601a 100644 --- a/src/mainboard/hp/z220_sff_workstation/Kconfig +++ b/src/mainboard/hp/z220_sff_workstation/Kconfig @@ -28,7 +28,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config MAINBOARD_DIR string diff --git a/src/mainboard/lenovo/t400/Kconfig b/src/mainboard/lenovo/t400/Kconfig index deb6c8e..0cd4e5c 100644 --- a/src/mainboard/lenovo/t400/Kconfig +++ b/src/mainboard/lenovo/t400/Kconfig @@ -39,7 +39,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x82 + default 0x90
config FMDFILE string diff --git a/src/mainboard/lenovo/t410/Kconfig b/src/mainboard/lenovo/t410/Kconfig index 5d60565..744c62d 100644 --- a/src/mainboard/lenovo/t410/Kconfig +++ b/src/mainboard/lenovo/t410/Kconfig @@ -37,7 +37,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config FMDFILE string diff --git a/src/mainboard/lenovo/t420/Kconfig b/src/mainboard/lenovo/t420/Kconfig index d26ad17..342cb35 100644 --- a/src/mainboard/lenovo/t420/Kconfig +++ b/src/mainboard/lenovo/t420/Kconfig @@ -41,7 +41,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config FMDFILE string diff --git a/src/mainboard/lenovo/t420s/Kconfig b/src/mainboard/lenovo/t420s/Kconfig index 89f84fc..3cf721a 100644 --- a/src/mainboard/lenovo/t420s/Kconfig +++ b/src/mainboard/lenovo/t420s/Kconfig @@ -40,7 +40,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config FMDFILE string diff --git a/src/mainboard/lenovo/t520/Kconfig b/src/mainboard/lenovo/t520/Kconfig index 6d31be2..304ff84 100644 --- a/src/mainboard/lenovo/t520/Kconfig +++ b/src/mainboard/lenovo/t520/Kconfig @@ -40,7 +40,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config VARIANT_DIR string diff --git a/src/mainboard/lenovo/x200/Kconfig b/src/mainboard/lenovo/x200/Kconfig index 432c805..0d1b3f5 100644 --- a/src/mainboard/lenovo/x200/Kconfig +++ b/src/mainboard/lenovo/x200/Kconfig @@ -36,7 +36,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x82 + default 0x90
config FMDFILE string diff --git a/src/mainboard/lenovo/x201/Kconfig b/src/mainboard/lenovo/x201/Kconfig index a94d24e..9feb8da 100644 --- a/src/mainboard/lenovo/x201/Kconfig +++ b/src/mainboard/lenovo/x201/Kconfig @@ -36,7 +36,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config FMDFILE string diff --git a/src/mainboard/lenovo/x220/Kconfig b/src/mainboard/lenovo/x220/Kconfig index b20255e..4e93da7 100644 --- a/src/mainboard/lenovo/x220/Kconfig +++ b/src/mainboard/lenovo/x220/Kconfig @@ -39,7 +39,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config MAINBOARD_DIR string diff --git a/src/mainboard/supermicro/x11-lga1151-series/Kconfig b/src/mainboard/supermicro/x11-lga1151-series/Kconfig index e0f468c..674426d 100644 --- a/src/mainboard/supermicro/x11-lga1151-series/Kconfig +++ b/src/mainboard/supermicro/x11-lga1151-series/Kconfig @@ -56,7 +56,7 @@
config VBOOT_VBNV_OFFSET hex - default 0x2a + default 0x38
config FMDFILE string diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ea70e65..c534d7f 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -73,11 +73,11 @@
config VBOOT_VBNV_OFFSET hex - default 0x26 + default 0x34 depends on VBOOT_VBNV_CMOS help CMOS offset for VbNv data. This value must match cmos.layout - in the mainboard directory, minus 14 bytes for the RTC. + in the mainboard directory.
config VBOOT_VBNV_CMOS_BACKUP_TO_FLASH bool diff --git a/src/security/vboot/vbnv_cmos.c b/src/security/vboot/vbnv_cmos.c index 7758ef6..5cb1e96 100644 --- a/src/security/vboot/vbnv_cmos.c +++ b/src/security/vboot/vbnv_cmos.c @@ -62,7 +62,7 @@ int i;
for (i = 0; i < VBOOT_VBNV_BLOCK_SIZE; i++) - vbnv_copy[i] = cmos_read(CONFIG_VBOOT_VBNV_OFFSET + 14 + i); + vbnv_copy[i] = cmos_read(CONFIG_VBOOT_VBNV_OFFSET + i);
/* Verify contents before attempting a restore from backup storage. */ if (verify_vbnv(vbnv_copy)) @@ -76,7 +76,7 @@ int i;
for (i = 0; i < VBOOT_VBNV_BLOCK_SIZE; i++) - cmos_write(vbnv_copy[i], CONFIG_VBOOT_VBNV_OFFSET + 14 + i); + cmos_write(vbnv_copy[i], CONFIG_VBOOT_VBNV_OFFSET + i); }
void vbnv_init_cmos(uint8_t *vbnv_copy) diff --git a/src/vendorcode/google/chromeos/acpi/chromeos.asl b/src/vendorcode/google/chromeos/acpi/chromeos.asl index 4852600..29c04ef 100644 --- a/src/vendorcode/google/chromeos/acpi/chromeos.asl +++ b/src/vendorcode/google/chromeos/acpi/chromeos.asl @@ -76,7 +76,7 @@ Name(VNBV, Package() { // See src/vendorcode/google/chromeos/Kconfig // for the definition of these: - CONFIG_VBOOT_VBNV_OFFSET, + CONFIG_VBOOT_VBNV_OFFSET - 14, VBOOT_VBNV_BLOCK_SIZE }) Return(VNBV)
Hello Alexander Couzens, Patrick Rudolph, Aaron Durbin, Julius Werner, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38918
to look at the new patch set (#2).
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
vboot/Kconfig: Handle VBNV RTC offset in code
This way, programmers and reviewers don't have to make the calculation over and over in their heads.
Build tested Google/Aleena, showed no differences in the resulting binary.
Change-Id: I928457febd399526fa77108d12b804ca7e53f80b Signed-off-by: Nico Huber nico.h@gmx.de --- M src/lib/coreboot_table.c M src/mainboard/emulation/qemu-i440fx/Kconfig M src/mainboard/emulation/qemu-q35/Kconfig M src/mainboard/google/kahlee/Kconfig M src/mainboard/hp/z220_sff_workstation/Kconfig M src/mainboard/lenovo/t400/Kconfig M src/mainboard/lenovo/t410/Kconfig M src/mainboard/lenovo/t420/Kconfig M src/mainboard/lenovo/t420s/Kconfig M src/mainboard/lenovo/t520/Kconfig M src/mainboard/lenovo/x200/Kconfig M src/mainboard/lenovo/x201/Kconfig M src/mainboard/lenovo/x220/Kconfig M src/mainboard/supermicro/x11-lga1151-series/Kconfig M src/security/vboot/Kconfig M src/security/vboot/vbnv_cmos.c M src/vendorcode/google/chromeos/acpi/chromeos.asl 17 files changed, 19 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/38918/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38918 )
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38918/2/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/acpi/chromeos.asl:
https://review.coreboot.org/c/coreboot/+/38918/2/src/vendorcode/google/chrom... PS2, Line 79: CONFIG_VBOOT_VBNV_OFFSET - 14, Looks like this code would be added even if !VBOOT_VBNV_CMOS. So if cros checks for 0 here, it might be a problem.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38918 )
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38918 )
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
Patch Set 2: Code-Review+2
Attention is currently required from: Nico Huber. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38918 )
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
Patch Set 2: Code-Review+2
Attention is currently required from: Nico Huber. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38918 )
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2: rebase, please
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38918 )
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
Patch Set 2:
(1 comment)
File src/vendorcode/google/chromeos/acpi/chromeos.asl:
https://review.coreboot.org/c/coreboot/+/38918/comment/f7fd2563_723bc5e5 PS2, Line 79: CONFIG_VBOOT_VBNV_OFFSET - 14,
Looks like this code would be added even if !VBOOT_VBNV_CMOS. So if cros […]
I had to think twice about it to figure what my younger self meant here. The problem is that if VBOOT_VBNV_CMOS is disabled, we had a 0 here. But after this patch it would be an odd -14.
Maybe it's better to restart this work. First, figure out what this `14` is about. Why is C code always using that offset but ASL isn't? It's all not that easy with an unspecified ACPI API (or is it documented somewhere?).
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38918?usp=email )
Change subject: vboot/Kconfig: Handle VBNV RTC offset in code ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Martin L Roth (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)