Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
arch/x86: Remove ID_SECTION_OFFSET
The location is hardcoded inside flashrom at least and currently there is no platform that needs to locate it elsewhere.
Change-Id: I8348f2ac0cab969ab78ecb50a55de486eee0cf9b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/ppc64/Kconfig M src/arch/x86/Kconfig M src/arch/x86/id.S M src/cpu/x86/16bit/reset16.ld 4 files changed, 12 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47598/1
diff --git a/src/arch/ppc64/Kconfig b/src/arch/ppc64/Kconfig index 44dbb1d..546dbc8 100644 --- a/src/arch/ppc64/Kconfig +++ b/src/arch/ppc64/Kconfig @@ -17,3 +17,7 @@ config ARCH_RAMSTAGE_PPC64 bool select ARCH_PPC64 + +config ID_SECTION_OFFSET + hex + default 0x80 diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index b5e52e6..f4ddd20 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -179,10 +179,6 @@ hex default 0xfed00000 if !HPET_ADDRESS_OVERRIDE
-config ID_SECTION_OFFSET - hex - default 0x80 - # 64KiB default bootblock size config C_ENV_BOOTBLOCK_SIZE hex diff --git a/src/arch/x86/id.S b/src/arch/x86/id.S index 798b25d..f79952f 100644 --- a/src/arch/x86/id.S +++ b/src/arch/x86/id.S @@ -2,27 +2,18 @@
#include <build.h>
- .section ".id", "a", @progbits +.section ".id", "a", @progbits
- .globl __id_start -__id_start: ver: .asciz COREBOOT_VERSION vendor: .asciz CONFIG_MAINBOARD_VENDOR part: .asciz CONFIG_MAINBOARD_PART_NUMBER -.long __id_end + CONFIG_ID_SECTION_OFFSET - ver /* Reverse offset to the - *vendor id - */ -.long __id_end + CONFIG_ID_SECTION_OFFSET - vendor /* Reverse offset to the - * vendor id - */ -.long __id_end + CONFIG_ID_SECTION_OFFSET - part /* Reverse offset to the - * part number - */ -.long CONFIG_ROM_SIZE /* Size of this romimage */ - .globl __id_end
-__id_end: +.long - ver /* Reverse offset to the version */ +.long - vendor /* Reverse offset to the vendor id */ +.long - part /* Reverse offset to the part number */ +.long CONFIG_ROM_SIZE /* Size of this romimage */ + .previous diff --git a/src/cpu/x86/16bit/reset16.ld b/src/cpu/x86/16bit/reset16.ld index 1c7e40f..d356034 100644 --- a/src/cpu/x86/16bit/reset16.ld +++ b/src/cpu/x86/16bit/reset16.ld @@ -20,10 +20,11 @@ /* Trigger an error if I have an unuseable start address */ _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report.");
- . = (0xffffffff - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 1; + . = ID_SECTION; .id (.): { KEEP(*(.id)) } + ID_SECTION = 0xffffff80 - SIZEOF(.id);
#if CONFIG(CPU_INTEL_FIRMWARE_INTERFACE_TABLE) . = 0xffffffc0;
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 1: Code-Review+1
Hello build bot (Jenkins), Julius Werner, Arthur Heymans, ron minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47598
to look at the new patch set (#2).
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
arch/x86: Remove ID_SECTION_OFFSET
The location is hardcoded inside flashrom at least and currently there is no platform that needs to locate it elsewhere.
Change-Id: I8348f2ac0cab969ab78ecb50a55de486eee0cf9b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/ppc64/Kconfig M src/arch/x86/Kconfig M src/arch/x86/id.S M src/cpu/x86/16bit/reset16.ld 4 files changed, 19 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47598/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2:
See related CB:40376 CB:40377 and CB:47602.
A decision should be made if .id section can be dropped and compatibility with old flashrom (and FILO) builds sacrified with some schedule, 2022 at earliest perhaps. I think post-processing approach would allow removal of id.S independently of that.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2: Code-Review+2
Patch Set 2:
See related CB:40376 CB:40377 and CB:47602.
A decision should be made if .id section can be dropped and compatibility with old flashrom (and FILO) builds sacrified with some schedule, 2022 at earliest perhaps. I think post-processing approach would allow removal of id.S independently of that.
Any reason not to schedule that earlier? I would think that people update their OS/tools more often than their firmware? How old flashrom are we talking?
The proposal sounds good. What do you mean with post-processing approach?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+2
Patch Set 2:
See related CB:40376 CB:40377 and CB:47602.
A decision should be made if .id section can be dropped and compatibility with old flashrom (and FILO) builds sacrified with some schedule, 2022 at earliest perhaps. I think post-processing approach would allow removal of id.S independently of that.
Any reason not to schedule that earlier? I would think that people update their OS/tools more often than their firmware? How old flashrom are we talking?
From what I remember current flashrom uses .id section to match image against SMBIOS or DMI when invoked with -p internal.
The proposal sounds good. What do you mean with post-processing approach?
CB:47602, instead of filling in proper .id in linking state, reserve the needed 16byte .id reserve as empty and have cbfstool.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2:
See related CB:40376 CB:40377 and CB:47602.
A decision should be made if .id section can be dropped and compatibility with old flashrom (and FILO) builds sacrified with some schedule, 2022 at earliest perhaps. I think post-processing approach would allow removal of id.S independently of that.
Any reason not to schedule that earlier? I would think that people update their OS/tools more often than their firmware? How old flashrom are we talking?
The tools in our case are baked into the firmware (update process in FILO). No .id, no updates.
I'll have to look into the older discussions to figure out once again why we want to get rid of it at all.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2:
See related CB:40376 CB:40377 and CB:47602.
A decision should be made if .id section can be dropped and compatibility with old flashrom (and FILO) builds sacrified with some schedule, 2022 at earliest perhaps. I think post-processing approach would allow removal of id.S independently of that.
In CB:40377 it looked like we could settle for a simple file in CBFS instead. As that's simple enough to do even out-of-tree, I see no problem to remove the section, but also not much of a reason to.
For us, dropping it after 4.15 (in 12 months) should work. If I haven't branched another coreboot for our older platforms until then, it will likely never hap- pen. And even if, we'll have bigger problems than patching this back in ;)
Any reason not to schedule that earlier? I would think that people update their OS/tools more often than their firmware? How old flashrom are we talking?
The tools in our case are baked into the firmware (update process in FILO). No .id, no updates.
I'll have to look into the older discussions to figure out once again why we want to get rid of it at all.
Found this: https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld#...
They just wanted to save the space because the .id section wouldn't be readable anyway in a compressed bootblock. If I ever try to port coreboot to any Zen platform, remind me that I wanted to try without bootblock/romstage :)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2:
Patch Set 2:
See related CB:40376 CB:40377 and CB:47602.
A decision should be made if .id section can be dropped and compatibility with old flashrom (and FILO) builds sacrified with some schedule, 2022 at earliest perhaps. I think post-processing approach would allow removal of id.S independently of that.
In CB:40377 it looked like we could settle for a simple file in CBFS instead. As that's simple enough to do even out-of-tree, I see no problem to remove the section, but also not much of a reason to.
My motivation was to wipe out arch/x/id.S files that only contain strings and offsets. Maybe link lib/version.c to separate .text._ver_ such that for hexdumps strings would conveniently appear at start or end of binary?
When I talked about .id section I referred to the one in x86 bootblock. Keeping the offsets at 0xffffff70 but moving the strings would be my suggestion for flashrom compatibility.
I don't understand the purpose of .id for !ENV_X86 case in lib/program.ld. The version strings will appear at arbitrary location near start of binary, Are the offsets used for anything? Is the purpose to just force the strings to be present in binary even with decompressor/bootblock console disabled? Some of those builds are extremely tight fits already.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2:
I don't understand the purpose of .id for !ENV_X86 case in lib/program.ld. The version strings will appear at arbitrary location near start of binary, Are the offsets used for anything? Is the purpose to just force the strings to be present in binary even with decompressor/bootblock console disabled? Some of those builds are extremely tight fits already.
I think this was just done to try to match what x86 does as closely as possible, and serves no programmatic purpose. It does allow you to visually identify images when looking at them in a hexdump, since the strings always appear near the start of the image. But I agree it's kinda pointless and I'm also in favor of getting rid of these in general, and replacing them with an architecture-independent and extensible CBFS file.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47598/2/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/47598/2/src/cpu/x86/16bit/reset16.l... PS2, Line 27: ID_SECTION = 0xffffff80 - SIZEOF(.id); Just an unresolved comment, I am redoing some of this.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Julius Werner, Arthur Heymans, Aaron Durbin, ron minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47598
to look at the new patch set (#3).
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
arch/x86: Remove ID_SECTION_OFFSET
The location is hardcoded inside flashrom at least and currently there is no platform that needs to locate it elsewhere.
Change-Id: I8348f2ac0cab969ab78ecb50a55de486eee0cf9b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/ppc64/Kconfig M src/arch/x86/Kconfig M src/arch/x86/bootblock.ld M src/arch/x86/id.S 4 files changed, 17 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47598/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47598/3/src/arch/ppc64/Kconfig File src/arch/ppc64/Kconfig:
https://review.coreboot.org/c/coreboot/+/47598/3/src/arch/ppc64/Kconfig@21 PS3, Line 21: config ID_SECTION_OFFSET : hex : default 0x80 Weird. This symbol was not present on ppc since b86e96ab8cb190d0244fe442069fcef5e88ef68b
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47598/3/src/arch/ppc64/Kconfig File src/arch/ppc64/Kconfig:
https://review.coreboot.org/c/coreboot/+/47598/3/src/arch/ppc64/Kconfig@21 PS3, Line 21: config ID_SECTION_OFFSET : hex : default 0x80
Weird. […]
It was just defined 0 and nobody used the reverse offsets.
https://review.coreboot.org/c/coreboot/+/47598/2/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/47598/2/src/cpu/x86/16bit/reset16.l... PS2, Line 27: ID_SECTION = 0xffffff80 - SIZEOF(.id);
Just an unresolved comment, I am redoing some of this.
Done
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Julius Werner, Arthur Heymans, Aaron Durbin, ron minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47598
to look at the new patch set (#6).
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
arch/x86: Remove ID_SECTION_OFFSET
The location is hardcoded inside flashrom and FILO. Only two offsets are supported, 0x10 and 0x80.
Change-Id: I8348f2ac0cab969ab78ecb50a55de486eee0cf9b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/ppc64/Kconfig M src/arch/x86/Kconfig M src/arch/x86/bootblock.ld M src/arch/x86/id.S 4 files changed, 18 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47598/6
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
Patch Set 7: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47598 )
Change subject: arch/x86: Remove ID_SECTION_OFFSET ......................................................................
arch/x86: Remove ID_SECTION_OFFSET
The location is hardcoded inside flashrom and FILO. Only two offsets are supported, 0x10 and 0x80.
Change-Id: I8348f2ac0cab969ab78ecb50a55de486eee0cf9b Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47598 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/ppc64/Kconfig M src/arch/x86/Kconfig M src/arch/x86/bootblock.ld M src/arch/x86/id.S 4 files changed, 18 insertions(+), 20 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/arch/ppc64/Kconfig b/src/arch/ppc64/Kconfig index 44dbb1d..546dbc8 100644 --- a/src/arch/ppc64/Kconfig +++ b/src/arch/ppc64/Kconfig @@ -17,3 +17,7 @@ config ARCH_RAMSTAGE_PPC64 bool select ARCH_PPC64 + +config ID_SECTION_OFFSET + hex + default 0x80 diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 95c87da..4de8c96 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -186,10 +186,6 @@ hex default 0xfed00000 if !HPET_ADDRESS_OVERRIDE
-config ID_SECTION_OFFSET - hex - default 0x80 - # 64KiB default bootblock size config C_ENV_BOOTBLOCK_SIZE hex diff --git a/src/arch/x86/bootblock.ld b/src/arch/x86/bootblock.ld index 849addd..ad9c2ef 100644 --- a/src/arch/x86/bootblock.ld +++ b/src/arch/x86/bootblock.ld @@ -15,10 +15,11 @@ /* Trigger an error if I have an unusable start address */ _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report.");
- . = (0xffffffff - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 1; + . = _ID_SECTION; .id (.): { KEEP(*(.id)); } + _ID_SECTION = 0xffffff80 - SIZEOF(.id);
. = 0xffffffc0; .fit_pointer (.): { diff --git a/src/arch/x86/id.S b/src/arch/x86/id.S index a7b4be7..574a7dc 100644 --- a/src/arch/x86/id.S +++ b/src/arch/x86/id.S @@ -2,26 +2,23 @@
#include <build.h>
- .section ".id", "a", @progbits +.section ".id", "a", @progbits
- .globl __id_start -__id_start: ver: .asciz COREBOOT_VERSION vendor: .asciz CONFIG_MAINBOARD_VENDOR part: .asciz CONFIG_MAINBOARD_PART_NUMBER -.long __id_end + CONFIG_ID_SECTION_OFFSET - ver /* Reverse offset to the - *vendor id - */ -.long __id_end + CONFIG_ID_SECTION_OFFSET - vendor /* Reverse offset to the - * vendor id - */ -.long __id_end + CONFIG_ID_SECTION_OFFSET - part /* Reverse offset to the - * part number - */ -.long CONFIG_ROM_SIZE /* Size of this romimage */ - .globl __id_end
-__id_end: +#if ENV_X86_64 +.long 0xffffffff - ver + 1 /* Reverse offset to the version */ +.long 0xffffffff - vendor + 1 /* Reverse offset to the vendor id */ +.long 0xffffffff - part + 1 /* Reverse offset to the part number */ +#else +.long - ver /* Reverse offset to the version */ +.long - vendor /* Reverse offset to the vendor id */ +.long - part /* Reverse offset to the part number */ +#endif + +.long CONFIG_ROM_SIZE /* Size of this romimage */
-
Arthur Heymans (Code Review) -
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)