Change in coreboot[master]: [WIP] arch/x86: Use intermediate .id section

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] arch/x86: Use intermediate .id section
......................................................................

[WIP] arch/x86: Use intermediate .id section

The strings in id.S are the same as those defined in
lib/version.c. Eliminating the calculation of reverse
offsets allows the references across linker sections and
dropping the duplicate strings in id.S.

TBD: util/cbfstool needs to post-process and invert
the offsets in .id section.

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/id.S
1 file changed, 4 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/47602/1

diff --git a/src/arch/x86/id.S b/src/arch/x86/id.S
index 4ff397d..545232a 100644
--- a/src/arch/x86/id.S
+++ b/src/arch/x86/id.S
@@ -4,23 +4,9 @@

.section ".id", "a", @progbits

-ver:
-	.asciz COREBOOT_VERSION
-vendor:
-	.asciz CONFIG_MAINBOARD_VENDOR
-part:
-	.asciz CONFIG_MAINBOARD_PART_NUMBER
-
-#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 */
+.long 	coreboot_version	/* Reverse offset to the version */
+.long	mainboard_vendor	/* Reverse offset to the vendor id */
+.long	mainboard_part_number	/* Reverse offset to the part number */
+.long	CONFIG_ROM_SIZE		/* Size of this romimage */

.previous

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] arch/x86: Use intermediate .id section
......................................................................

Patch Set 1:

Now I wonder, could we turn it around? let `version.c` point to
the strings in `id.S`. Would that make things easier? assuming
the goal is to get rid of the duplication.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] arch/x86: Use intermediate .id section
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Now I wonder, could we turn it around? let `version.c` point to
> the strings in `id.S`. Would that make things easier? assuming
> the goal is to get rid of the duplication.

Well, starting with amd/picasso, bootblock binary is not stored at end of coreboot.rom so solving that would need cbfstool change anyways.

Change in coreboot[master]: lib/version: Use .id section and remove id.S files

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47602

to look at the new patch set (#2).

Change subject: lib/version: Use .id section and remove id.S files
......................................................................

lib/version: Use .id section and remove id.S files

Selected strings from lib/version.c are moved from .text to .id.keep
section. Pointers to such strings are added in .flashrom_id in
structured form such that cbfstool can extract version information
from the final ELF object.

For arch/arm[64], the offsets to board identification strings and
CONFIG_ROM_SIZE inside .id were never really used; it was only a
convenience to have the strings appear near the start of image.
Order of strings is now undefined.

TBD: For arch/x86 util/cbfstool needs to post-process and invert
the offsets in .flashrom_id section.

TBD: Check amd/picasso memlayout.

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
---
M src/arch/arm/Makefile.inc
D src/arch/arm/id.S
M src/arch/arm64/Makefile.inc
D src/arch/arm64/id.S
M src/arch/x86/Makefile.inc
M src/arch/x86/bootblock.ld
D src/arch/x86/id.S
M src/lib/Makefile.inc
A src/lib/flash_id.c
M src/lib/program.ld
M src/lib/version.c
11 files changed, 43 insertions(+), 87 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/47602/2

Change in coreboot[master]: lib/version: Use .id section and remove id.S files

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47602

to look at the new patch set (#3).

Change subject: lib/version: Use .id section and remove id.S files
......................................................................

lib/version: Use .id section and remove id.S files

Selected strings from lib/version.c are moved from .rodata to .id.keep
section. Pointers to such strings are added in .flashrom_id in
structured form such that cbfstool can extract version information
from the final ELF object.

For arch/arm[64], the offsets to board identification strings and
CONFIG_ROM_SIZE inside .id were never really used; it was only a
convenience to have the strings appear near the start of image.
Order of strings is now undefined.

TBD: For arch/x86 util/cbfstool needs to post-process and invert
the offsets in .flashrom_id section.

TBD: Check amd/picasso memlayout.

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
---
M src/arch/arm/Makefile.inc
D src/arch/arm/id.S
M src/arch/arm64/Makefile.inc
D src/arch/arm64/id.S
M src/arch/x86/Makefile.inc
M src/arch/x86/bootblock.ld
D src/arch/x86/id.S
M src/lib/Makefile.inc
A src/lib/flash_id.c
M src/lib/program.ld
M src/lib/version.c
11 files changed, 47 insertions(+), 86 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/47602/3

Change in coreboot[master]: lib/version: Use .id section and remove id.S files

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: lib/version: Use .id section and remove id.S files
......................................................................

Patch Set 3:

(5 comments)

https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG@17 
PS3, Line 17: Order of strings is now undefined.
This would be easy to fix if you just used three separate sections...

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc 
File src/lib/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc@225 
PS3, Line 225: bootblock-y += flash_id.c
If this is only used on x86 please put it in arch/x86.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c 
File src/lib/flash_id.c:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@17 
PS3, Line 17: 	union __ptr version;
Are these even needed here? I'm not 100% sure what you're trying to do but it seems that you ultimately expect readers to just use the reverse_offset anyway, right (so it's backwards-compatible)? So what are these pointers good for? (Since they're in the runtime address space they're not going to be useful to an external tool parsing the file anyway.)

If you just have these here so cbfstool can use them to calculate the reverse offsets, I don't think you need that. cbfstool gets the whole unstripped ELF as input so it should be able to just find the location of those strings in the symbol table.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@26 
PS3, Line 26: 	.magic = 0xdeadbeef,
Is this needed/useful for anything? If you want a real magic that can actually be searched and found by other tools, please pick something other than deadbeef which is way too overused (e.g. 0xCB1DD1BC or something like that).

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld 
File src/lib/program.ld:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld@18 
PS3, Line 18: #if !(ENV_X86 && ENV_BOOTBLOCK)
I think this is problematic because now you're using this section name for stuff that is linked into every stage, but the linker script only explicitly mentions it in the bootblock. For all other stages it will be an orphaned section and the linker will basically just stuff it wherever it feels like (I think that's usually at the end but better not to rely on that).

I think the simplest solution is to just it up here for all stages -- it doesn't really matter where exactly it goes. You just need to use ENV_BOOTBLOCK to control the KEEP(). (Also, please keep the existing condition with the decompressor... when the bootblock is compressed these should be put into the decompressor stub instead if the point is to see them in a hex editor.)

Change in coreboot[master]: lib/version: Use .id section and remove id.S files

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: lib/version: Use .id section and remove id.S files
......................................................................

Patch Set 3:

(5 comments)

https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG@17 
PS3, Line 17: Order of strings is now undefined.
> This would be easy to fix if you just used three separate sections...

True but not necessary if the strings are for hexdump convenience only and the reverse-offsets get filled by some other method.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc 
File src/lib/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc@225 
PS3, Line 225: bootblock-y += flash_id.c
> If this is only used on x86 please put it in arch/x86.

If we wanted an approach where cbfstool can access the version strings within the ELF, this could be all-y. I know there are alternative and maybe simpler solutions with separate FMAP headers(?) and CBFS textfiles.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c 
File src/lib/flash_id.c:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@17 
PS3, Line 17: 	union __ptr version;
> Are these even needed here? I'm not 100% sure what you're trying to do but it seems that you ultimately expect readers to just use the reverse_offset anyway, right (so it's backwards-compatible)? So what are these pointers good for? (Since they're in the runtime address space they're not going to be useful to an external tool parsing the file anyway.)

I was anticipating maintaining backwards compatibility for flashrom is required and the reverse offsets below filled in. Parsing the ELF, one can still translate the runtime addresses to file offsets.
> If you just have these here so cbfstool can use them to calculate the reverse offsets, I don't think you need that. cbfstool gets the whole unstripped ELF as input so it should be able to just find the location of those strings in the symbol table.

I only saw bootblock.bin for cbfstool input file, so this would need to change. Even if ELF .strtab (?) was parsed, I don't know if a dependency between a variable name in coreboot proper and util/cbfstool would be allowed. So the approach I was taking is definining a serialized struct with a magic tag.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@26 
PS3, Line 26: 	.magic = 0xdeadbeef,
> Is this needed/useful for anything? If you want a real magic that can actually be searched and found […]

Different magic for sure, if implemented.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld 
File src/lib/program.ld:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld@18 
PS3, Line 18: #if !(ENV_X86 && ENV_BOOTBLOCK)
> I think this is problematic because now you're using this section name for stuff that is linked into every stage, but the linker script only explicitly mentions it in the bootblock. For all other stages it will be an orphaned section and the linker will basically just stuff it wherever it feels like (I think that's usually at the end but better not to rely on that).

Did you read this as !ENV_X86 && ENV_BOOTBLOCK ?

Only x86 bootblocks were excluded here because they traditionally had these strings near the end of coreboot.rom.
> I think the simplest solution is to just it up here for all stages -- it doesn't really matter where exactly it goes. You just need to use ENV_BOOTBLOCK to control the KEEP(). (Also, please keep the existing condition with the decompressor... when the bootblock is compressed these should be put into the decompressor stub instead if the point is to see them in a hex editor.)

I need to rebase on CB:46834 to move .id.keep for x86 bootblocks. I would use KEEP() for every stage here if cbfstool would parse the ELF for these strings.

Change in coreboot[master]: lib/version: Use .id section and remove id.S files

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: lib/version: Use .id section and remove id.S files
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc 
File src/lib/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc@225 
PS3, Line 225: bootblock-y += flash_id.c
> If we wanted an approach where cbfstool can access the version strings within the ELF, this could be […]

Yeah, I don't think we would want to build further upon this, I think the separate CBFS file solution is so much better (extensible, easier to parse and doesn't waste bytes in loaded images). If there's a strong desire to maintain the existing .id section for backwards-compatibility, I think this patch is fine to do just that. But I don't think we should expand it to other architectures.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c 
File src/lib/flash_id.c:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@17 
PS3, Line 17: 	union __ptr version;
> > Are these even needed here? I'm not 100% sure what you're trying to do but it seems that you ultim […]

Oh right, I forgot bootblocks were added as bins. You could do it in the Makefile while it's still an ELF rather than in cbfstool, I guess? For example override the `bootblock.raw.bin: bootblock.raw.elf` rule for x86, nm|grep the symbol addresses from $<, do some math, dd conv=notrunc them into $@ to the well-known offsets (from the end of the file). Maybe too complicated to save the bytes of a few pointers, idk.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld 
File src/lib/program.ld:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld@18 
PS3, Line 18: #if !(ENV_X86 && ENV_BOOTBLOCK)
> Did you read this as !ENV_X86 && ENV_BOOTBLOCK ?

Oh, yes I did, sorry. But that does add the KEEP() to all stages and I don't see why we'd want that? Why would cbfstool need to parse this out of each stage separately? Feels like just wasting space...

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47602

to look at the new patch set (#4).

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

[WIP] lib/version: Remove .id section and id.S files

For arch/arm[64], the offsets to board identification strings and
CONFIG_ROM_SIZE inside .id were never really used; it was only a
convenience to have the strings appear near the start of image.

TBD: For arch/x86 util/cbfstool needs to fill in the reverse
offsets in .id section for backwards compatibility

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
---
M Makefile.inc
M src/arch/arm/Makefile.inc
D src/arch/arm/id.S
M src/arch/arm64/Makefile.inc
D src/arch/arm64/id.S
M src/arch/x86/Makefile.inc
M src/arch/x86/bootblock.ld
D src/arch/x86/id.S
M src/lib/program.ld
9 files changed, 11 insertions(+), 82 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/47602/4

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

Patch Set 4:

(4 comments)

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc 
File src/lib/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc@225 
PS3, Line 225: bootblock-y += flash_id.c
> Yeah, I don't think we would want to build further upon this, I think the separate CBFS file solutio […]

Dropped in favor of cbfs-file containing the strings.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c 
File src/lib/flash_id.c:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@17 
PS3, Line 17: 	union __ptr version;
> Oh right, I forgot bootblocks were added as bins. […]

Dropped in favor of cbfs-file containing the strings.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@26 
PS3, Line 26: 	.magic = 0xdeadbeef,
> Different magic for sure, if implemented.

Dropped in favor of cbfs-file containing the strings.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld 
File src/lib/program.ld:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld@18 
PS3, Line 18: #if !(ENV_X86 && ENV_BOOTBLOCK)
> > Did you read this as !ENV_X86 && ENV_BOOTBLOCK ? […]

If the only actual requirement is for the strings from (now removed) id.S files to appear uncompressed in the FMAP region COREBOOT, that is fulfilled with the separate cbfs-file added in a later patchset. To ever maintain the strings with a KEEP(*(.id)) seems waste of space to me now.

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

Patch Set 4: Code-Review+2

(3 comments)

https://review.coreboot.org/c/coreboot/+/47602/4/Makefile.inc 
File Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47602/4/Makefile.inc@533 
PS4, Line 533: idstrings
nit: Should this also be called idsection? I'm somewhat confused why there are two different names. (Or maybe we should just call it build_info or something like that?)

https://review.coreboot.org/c/coreboot/+/47602/4/Makefile.inc@534 
PS4, Line 534: COREBOOT_VERSION: $(KERNELVERSION)
Should put quotes around these just in case.

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld 
File src/lib/program.ld:

https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld@18 
PS3, Line 18: #if !(ENV_X86 && ENV_BOOTBLOCK)
> If the only actual requirement is for the strings from (now removed) id. […]

I don't think there's really a hard requirement in any way. The practical effect of this used to be that you can see what kind of image it is at the top of the file in a hexdump (because the BOOTBLOCK section is normally the first for Arm platforms). But I don't think it's a big deal, there are plenty of other ways to figure out what an image is, and I'm fine with removing it completely too.

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47602

to look at the new patch set (#5).

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

[WIP] lib/version: Remove .id section and id.S files

For arch/arm[64], the offsets to board identification strings and
CONFIG_ROM_SIZE inside .id were never really used; it was only a
convenience to have the strings appear near the start of image.

TBD: For arch/x86 util/cbfstool needs to fill in the reverse
offsets in .id section for backwards compatibility

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
---
M Makefile.inc
M src/arch/arm/Makefile.inc
D src/arch/arm/id.S
M src/arch/arm64/Makefile.inc
D src/arch/arm64/id.S
M src/arch/x86/Makefile.inc
M src/arch/x86/bootblock.ld
D src/arch/x86/id.S
M src/lib/program.ld
9 files changed, 11 insertions(+), 82 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/47602/5

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG@17 
PS3, Line 17: Order of strings is now undefined.
> True but not necessary if the strings are for hexdump convenience only and the reverse-offsets get f […]

Done

https://review.coreboot.org/c/coreboot/+/47602/4/Makefile.inc 
File Makefile.inc:

https://review.coreboot.org/c/coreboot/+/47602/4/Makefile.inc@533 
PS4, Line 533: idstrings
> nit: Should this also be called idsection? I'm somewhat confused why there are two different names. […]

Done

https://review.coreboot.org/c/coreboot/+/47602/4/Makefile.inc@534 
PS4, Line 534: COREBOOT_VERSION: $(KERNELVERSION)
> Should put quotes around these just in case.

Done

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47602

to look at the new patch set (#6).

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

[WIP] lib/version: Remove .id section and id.S files

For arch/arm[64], the offsets to board identification strings and
CONFIG_ROM_SIZE inside .id were never really used; it was only a
convenience to have the strings appear near the start of image.

TBD: For arch/x86 util/cbfstool needs to fill in the reverse
offsets in .id section for backwards compatibility

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
---
M Makefile.inc
M src/arch/arm/Makefile.inc
D src/arch/arm/id.S
M src/arch/arm64/Makefile.inc
D src/arch/arm64/id.S
M src/arch/x86/Makefile.inc
M src/arch/x86/bootblock.ld
D src/arch/x86/id.S
M src/lib/program.ld
9 files changed, 11 insertions(+), 82 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/47602/6

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: [WIP] lib/version: Remove .id section and id.S files

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: [WIP] lib/version: Remove .id section and id.S files
......................................................................

Patch Set 6:

Well it turned out backwards flashrom compatibility needs null-terminated strings.
And FILO has assumptions for the vendor and part number strings to be next to each other.

So I cannot really touch the x86 part at all.

Change in coreboot[master]: arch/arm: Replace .id section with build_info in CBFS

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47602

to look at the new patch set (#7).

Change subject: arch/arm: Replace .id section with build_info in CBFS
......................................................................

arch/arm: Replace .id section with build_info in CBFS

For arch/arm[64], the offsets to board identification strings and
CONFIG_ROM_SIZE inside .id were never really used; it was only a
convenience to have the strings appear near the start of image.

Add the same strings in an uncompressed file in CBFS.

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
---
M Makefile.inc
M src/arch/arm/Makefile.inc
D src/arch/arm/id.S
M src/arch/arm64/Makefile.inc
D src/arch/arm64/id.S
M src/lib/program.ld
6 files changed, 10 insertions(+), 54 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/47602/7

Change in coreboot[master]: arch/arm: Replace .id section with build_info in CBFS

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: arch/arm: Replace .id section with build_info in CBFS
......................................................................

Patch Set 7: Code-Review+2
> Well it turned out backwards flashrom compatibility needs null-terminated strings.

Weren't they with your new patch? Normal .rodata from version.c should also be null-terminated.
> And FILO has assumptions for the vendor and part number strings to be next to each other.

Okay, that's unfortunate. I mean, there are still ways to merge this with version.c... e.g. you could have an explicitly named linker section for each string to make sure they appear in the right order, or define them in version.c as `CONFIG_VERSION "\0" CONFIG_MAINBOARD_VENDOR "\0" CONFIG_MAINBOARD_PART_NUMBER` and then get the individual components with sizeof() math. But that starts becoming so complicated to follow that it's maybe not really an improvement anymore.

Change in coreboot[master]: arch/arm: Replace .id section with build_info in CBFS

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: arch/arm: Replace .id section with build_info in CBFS
......................................................................

Patch Set 8: Code-Review+2

Change in coreboot[master]: arch/arm: Replace .id section with build_info in CBFS

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47602 )

Change subject: arch/arm: Replace .id section with build_info in CBFS
......................................................................

arch/arm: Replace .id section with build_info in CBFS

For arch/arm[64], the offsets to board identification strings and
CONFIG_ROM_SIZE inside .id were never really used; it was only a
convenience to have the strings appear near the start of image.

Add the same strings in an uncompressed file in CBFS.

Change-Id: I35d3312336e9c66d657d2ca619cf30fd79e18fd4
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47602
Reviewed-by: Arthur Heymans 
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M Makefile.inc
M src/arch/arm/Makefile.inc
D src/arch/arm/id.S
M src/arch/arm64/Makefile.inc
D src/arch/arm64/id.S
M src/lib/program.ld
6 files changed, 10 insertions(+), 54 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved
  Arthur Heymans: Looks good to me, approved

diff --git a/Makefile.inc b/Makefile.inc
index 90c2494..1212dae 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -530,6 +530,12 @@
 build-dirs $(objcbfs) $(objgenerated):
    mkdir -p $(objcbfs) $(objgenerated)

+$(obj)/build_info:
+	@echo 'COREBOOT_VERSION: $(call strip_quotes,$(KERNELVERSION))' > $@.tmp
+	@echo 'MAINBOARD_VENDOR: $(call strip_quotes,$(CONFIG_MAINBOARD_VENDOR))' >> $@.tmp
+	@echo 'MAINBOARD_PART_NUMBER: $(call strip_quotes,$(CONFIG_MAINBOARD_PART_NUMBER))' >> $@.tmp
+	mv $@.tmp $@
+
 #######################################################################
 # Build the tools
 CBFSTOOL:=$(objutil)/cbfstool/cbfstool
@@ -1208,6 +1214,10 @@
 revision-file := $(obj)/build.h
 revision-type := raw

+cbfs-files-y += build_info
+build_info-file := $(obj)/build_info
+build_info-type := raw
+
 BOOTSPLASH_SUFFIX=$(suffix $(call strip_quotes,$(CONFIG_BOOTSPLASH_FILE)))
 cbfs-files-$(CONFIG_BOOTSPLASH_IMAGE) += bootsplash$(BOOTSPLASH_SUFFIX)
 bootsplash$(BOOTSPLASH_SUFFIX)-file := $(call strip_quotes,$(CONFIG_BOOTSPLASH_FILE))
diff --git a/src/arch/arm/Makefile.inc b/src/arch/arm/Makefile.inc
index 3d35914..63367bb 100644
--- a/src/arch/arm/Makefile.inc
+++ b/src/arch/arm/Makefile.inc
@@ -27,11 +27,6 @@

ifeq ($(CONFIG_ARCH_BOOTBLOCK_ARM),y)

-decompressor-y += id.S
-bootblock-y += id.S
-$(call src-to-obj,decompressor,$(dir)/id.S): $(obj)/build.h
-$(call src-to-obj,bootblock,$(dir)/id.S): $(obj)/build.h
-
 decompressor-y += boot.c
 bootblock-y += boot.c
 decompressor-y += div0.c
diff --git a/src/arch/arm/id.S b/src/arch/arm/id.S
deleted file mode 100644
index a8f7a79..0000000
--- a/src/arch/arm/id.S
+++ /dev/null
@@ -1,21 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include <build.h>
-
-	.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 - ver  /* Reverse offset to the vendor id */
-.long __id_end - vendor  /* Reverse offset to the vendor id */
-.long __id_end - part    /* Reverse offset to the part number */
-.long CONFIG_ROM_SIZE                               /* Size of this romimage */
-	.globl __id_end
-
-__id_end:
diff --git a/src/arch/arm64/Makefile.inc b/src/arch/arm64/Makefile.inc
index 44517cb..6b49743 100644
--- a/src/arch/arm64/Makefile.inc
+++ b/src/arch/arm64/Makefile.inc
@@ -26,10 +26,6 @@
 bootblock-y += div0.c
 decompressor-y += eabi_compat.c
 bootblock-y += eabi_compat.c
-decompressor-y += id.S
-bootblock-y += id.S
-$(call src-to-obj,decompressor,$(dir)/id.S): $(obj)/build.h
-$(call src-to-obj,bootblock,$(dir)/id.S): $(obj)/build.h

decompressor-$(CONFIG_ARM64_USE_ARCH_TIMER) += arch_timer.c
 bootblock-$(CONFIG_ARM64_USE_ARCH_TIMER) += arch_timer.c
diff --git a/src/arch/arm64/id.S b/src/arch/arm64/id.S
deleted file mode 100644
index a8f7a79..0000000
--- a/src/arch/arm64/id.S
+++ /dev/null
@@ -1,21 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include <build.h>
-
-	.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 - ver  /* Reverse offset to the vendor id */
-.long __id_end - vendor  /* Reverse offset to the vendor id */
-.long __id_end - part    /* Reverse offset to the part number */
-.long CONFIG_ROM_SIZE                               /* Size of this romimage */
-	.globl __id_end
-
-__id_end:
diff --git a/src/lib/program.ld b/src/lib/program.ld
index 94ba409..d419ab6 100644
--- a/src/lib/program.ld
+++ b/src/lib/program.ld
@@ -15,9 +15,6 @@
    _text = .;
    *(.text._start);
    *(.text.stage_entry);
-#if !ENV_X86 && (ENV_DECOMPRESSOR || ENV_BOOTBLOCK && !CONFIG(COMPRESS_BOOTBLOCK))
-	KEEP(*(.id));
-#endif
    KEEP(*(.metadata_hash_anchor));
    *(.text);
    *(.text.*);