Change in coreboot[master]: arch/x86: Don't use .bss from car.ld if not running XIP

List overview All Threads
Download

newer

older

[S] Change in coreboot[master]:...

Arthur Heymans (Code Review)

30 Nov 2020 30 Nov '20

6:53 p.m.

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

arch/x86: Don't use .bss from car.ld if not running XIP

Some platform run early stages like romstage and verstage from CAR instead of XIP. This allows to link them like other arch inside the _program region.

Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/include/rules.h M src/lib/program.ld 4 files changed, 12 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48156/1

diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index 31670c2..7d7d3fc 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -36,6 +36,7 @@ mov $_STACK_TOP, %esp

/* clear .bss section as it is not shared */ +#if ENV_STAGE_XIP cld xor %eax, %eax movl $(_ebss), %ecx @@ -43,6 +44,7 @@ sub %edi, %ecx shrl $2, %ecx rep stosl +#endif

#if ((ENV_SEPARATE_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ || (ENV_ROMSTAGE && CONFIG(ROMSTAGE_DEBUG_SPINLOOP))) diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index c291efb..38128d1 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -62,6 +62,7 @@ * cbmem console. This is useful for clearing this area on a per-stage * basis when more than one stage uses cache-as-ram. */

+#if ENV_STAGE_HAS_CAR_XIP_BSS . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .; /* Allow global uninitialized variables for stages without CAR teardown. */ @@ -71,6 +72,7 @@ *(.sbss.*) . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _ebss = .; +#endif

#if ENV_ROMSTAGE && CONFIG(ASAN_IN_ROMSTAGE) _shadow_size = (_ebss - _car_region_start) >> 3; diff --git a/src/include/rules.h b/src/include/rules.h index 6ebb37e..f503914 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -261,14 +261,21 @@ #if ENV_X86 /* Indicates memory layout is determined with arch/x86/car.ld. */ #define ENV_CACHE_AS_RAM (ENV_ROMSTAGE_OR_BEFORE && !CONFIG(RESET_VECTOR_IN_RAM)) +/* Indicates if the stage runs XIP. */ +#define ENV_STAGE_XIP (ENV_CACHE_AS_RAM && \ + !((ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE) && CONFIG(NO_XIP_EARLY_STAGES))) /* No .data sections with execute-in-place from ROM. */ #define ENV_STAGE_HAS_DATA_SECTION !ENV_CACHE_AS_RAM #else /* Both .data and .bss, sometimes SRAM not DRAM. */ #define ENV_STAGE_HAS_DATA_SECTION 1 #define ENV_CACHE_AS_RAM 0 +#define ENV_STAGE_XIP 0 #endif

+/* Indicates if the stages uses the _bss region defined in arch/x86/car.ld */ +#define ENV_STAGE_HAS_CAR_XIP_BSS ENV_STAGE_XIP + /* Currently rmodules, ramstage and smm have heap. */ #define ENV_STAGE_HAS_HEAP_SECTION (ENV_RMODULE || ENV_RAMSTAGE || ENV_SMM)

diff --git a/src/lib/program.ld b/src/lib/program.ld index 3b6aa2e..ff3c672 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -103,7 +103,7 @@ } #endif

-#if !ENV_CACHE_AS_RAM +#if !ENV_STAGE_HAS_CAR_XIP_BSS .bss . : { . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .;

Attachments:

attachment.htm (text/html — 6.2 KB)

Show replies by date

build bot (Jenkins) (Code Review)

30 Nov 30 Nov

6:55 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h@266 PS1, Line 266: !((ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE) && CONFIG(NO_XIP_EARLY_STAGES))) line over 96 characters

Marshall Dawson (Code Review)

8:23 p.m.

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48156/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:

https://review.coreboot.org/c/coreboot/+/48156/1/src/arch/x86/assembly_entry... PS1, Line 39: ENV_STAGE_XIP Using this rule will eliminate clearing .bss on Picasso where we have CONFIG(RESET_VECTOR_IN_RAM). You can objdump romstage for a Mandolin build and check it.

Arthur Heymans (Code Review)

8:42 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48156/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:

https://review.coreboot.org/c/coreboot/+/48156/1/src/arch/x86/assembly_entry... PS1, Line 39: ENV_STAGE_XIP

...

Using this rule will eliminate clearing .bss on Picasso where we have CONFIG(RESET_VECTOR_IN_RAM). You can objdump romstage for a Mandolin build and check it.

Is it needed? When a program is actually loaded into CAR/dram the program loader should clear out .bss. It's only why when the program XIP, that the program is not loaded that we have to manually make sure .bss is cleared.

Marshall Dawson (Code Review)

1 Dec 1 Dec

7:02 p.m.

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1: Code-Review+1

(2 comments)

I'll hope to defer to someone using non-XIP and CAR for +2, however it looks OK to me now.

https://review.coreboot.org/c/coreboot/+/48156/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:

https://review.coreboot.org/c/coreboot/+/48156/1/src/arch/x86/assembly_entry... PS1, Line 39: ENV_STAGE_XIP

...

...
Using this rule will eliminate clearing .bss on Picasso where we have CONFIG(RESET_VECTOR_IN_RAM). […]

Oh, you're right. It's cleared in cbfs_prog_stage_load().

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h@276 PS1, Line 276: stages uses nit - maybe "stage uses".

Arthur Heymans (Code Review)

7:50 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

...

Patch Set 1: Code-Review+1

(2 comments)

I'll hope to defer to someone using non-XIP and CAR for +2, however it looks OK to me now.

Oh, like the following patches, it's tested on Intel Apollolake. I'll add that into the commit message.

Marshall Dawson (Code Review)

8:20 p.m.

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

...

Patch Set 1:

...
Patch Set 1: Code-Review+1

(2 comments)

I'll hope to defer to someone using non-XIP and CAR for +2, however it looks OK to me now.

Oh, like the following patches, it's tested on Intel Apollolake. I'll add that into the commit message.

I meant that I'm confident enough that I can +2 the stack, if necessary, although non-XIP CAR doesn't apply to anything I've worked on. My preference is that a stakeholder also review these changes, but let me know if you can't get someone.

Nico Huber (Code Review)

6 Dec 6 Dec

6:01 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h@266 PS1, Line 266: ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE Why only these? Why can't we just

#define ENV_STAGE_XIP (ENV_ROMSTAGE_OR_BEFORE && !CONFIG(NO_XIP_EARLY_STAGES))

independent of the arch?

Arthur Heymans (Code Review)

17 Dec 17 Dec

5:55 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h@266 PS1, Line 266: ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE

...

Why only these? Why can't we just
#define ENV_STAGE_XIP (ENV_ROMSTAGE_OR_BEFORE && !CONFIG(NO_XIP_EARLY_STAGES))
independent of the arch?

Bootblock is still XIP when NO_XIP_EARLY_STAGES is selected.

Nico Huber (Code Review)

10:13 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h@266 PS1, Line 266: ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE

...

...
Why only these? Why can't we just […]

I thought all these targets (non-x86 and APL) load the bootblock into SRAM?

Kyösti Mälkki (Code Review)

10:21 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

This will increase bootblock binary if it is not compressed. Also, .text._start, _start16bit will move further away from .reset so you may want to wait for .init stuff on x86 bootblocks to land first.

Kyösti Mälkki (Code Review)

10:25 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

...

Patch Set 1:

This will increase bootblock binary if it is not compressed. Also, .text._start, _start16bit will move further away from .reset so you may want to wait for .init stuff on x86 bootblocks to land first.

NVM, should have visited the code more thorougly.

Nico Huber (Code Review)

10:29 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h@266 PS1, Line 266: ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE

...

I thought all these targets (non-x86 and APL) load the bootblock into SRAM?

I'm starting to remember, APL's SRAM mimics ROM. So that could indeed be called XIP. But that seems more like a peculiarity then what the Kconfigs try to express.

Nico Huber (Code Review)

10:38 p.m.

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 1:

(1 comment)

...

NVM, should have visited the code more thorougly.

If I understand correctly, only CAR && !XIP environments are directly affected by this change, iow. Apollo Lake?

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/1/src/include/rules.h@266 PS1, Line 266: ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE

...

I'm starting to remember, APL's SRAM mimics ROM. So that could indeed […]

If we want to cover the bootblock, that could be more explicit:

(ENV_CACHE_AS_RAM && (ENV_BOOTBLOCK || !CONFIG(NO_XIP_EARLY_STAGES))

Arthur Heymans (Code Review)

8 Apr 8 Apr

2:25 p.m.

Attention is currently required from: Furquan Shaikh. Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Julius Werner,

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

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

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

arch/x86: Don't use .bss from car.ld if not running XIP

Some platform run early stages like romstage and verstage from CAR instead of XIP. This allows to link them like other arch inside the _program region.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48156/2

build bot (Jenkins) (Code Review)

2:25 p.m.

Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 2:

(1 comment)

File src/include/rules.h:

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145879): https://review.coreboot.org/c/coreboot/+/48156/comment/5ed6f7e5_83d0d532 PS2, Line 272: !((ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE) && CONFIG(NO_XIP_EARLY_STAGES))) line over 96 characters

Kyösti Mälkki (Code Review)

4:26 p.m.

Attention is currently required from: Furquan Shaikh, Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 2:

(1 comment)

File src/arch/x86/assembly_entry.S:

https://review.coreboot.org/c/coreboot/+/48156/comment/f3423c80_5c680cae PS2, Line 39: #if ENV_STAGE_XIP Remember non-car AMD write_resume_eip? It sort of suggests that for S3 resume path first stage would not be reloaded from SPI flash, but I am not sure.

If you create combined bootblock+romstage, S3 resume path will need to clear .bss.

Kyösti Mälkki (Code Review)

4:35 p.m.

Attention is currently required from: Furquan Shaikh, Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 2:

(1 comment)

File src/arch/x86/assembly_entry.S:

https://review.coreboot.org/c/coreboot/+/48156/comment/ce0d9acf_e47ad1fe PS2, Line 39: #if ENV_STAGE_XIP

...

Remember non-car AMD write_resume_eip? It sort of suggests that for S3 resume path first stage would […]

Same question for .data, although I don't see how one could re-initialise it. So .data becomes persistent across S3 suspend for bootblock(+romstage) ?

Arthur Heymans (Code Review)

4:50 p.m.

Attention is currently required from: Furquan Shaikh, Kyösti Mälkki. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 2:

(1 comment)

File src/arch/x86/assembly_entry.S:

https://review.coreboot.org/c/coreboot/+/48156/comment/45a7040c_72b78bab PS2, Line 39: #if ENV_STAGE_XIP

...

Same question for .data, although I don't see how one could re-initialise it. So .data becomes persistent across S3 suspend for bootblock(+romstage) ?

The resume path already clears .bss.

For the data section it looks like we would need to save it somewhere and restore it on the S3 path? Assuming we want a .data section in the first stage of course.

Arthur Heymans (Code Review)

2 Jun 2 Jun

12:01 p.m.

Attention is currently required from: Furquan Shaikh, Kyösti Mälkki. Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Julius Werner,

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

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

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

arch/x86: Don't use .bss from car.ld if not running XIP

Some platform run early stages like romstage and verstage from CAR instead of XIP. This allows to link them like other arch inside the _program region. This make in place LZ4 decompression possible as it needs a bit of extra place to extract the code which is now provided by the .bss.

Tested on up/squared (Intel APL) & google/vilboz (AMD Picasso).

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48156/3

Arthur Heymans (Code Review)

12:06 p.m.

Attention is currently required from: Nico Huber, Furquan Shaikh, Marshall Dawson, Kyösti Mälkki. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

Patch Set 3:

(3 comments)

File src/arch/x86/assembly_entry.S:

https://review.coreboot.org/c/coreboot/+/48156/comment/7c2847c9_350ec9b4 PS2, Line 39: #if ENV_STAGE_XIP

...

...
Same question for .data, although I don't see how one could re-initialise it. So .data becomes persistent across S3 suspend for bootblock(+romstage) ?

The resume path already clears .bss.

For the data section it looks like we would need to save it somewhere and restore it on the S3 path? Assuming we want a .data section in the first stage of course.

Done. Now this only targets ROMSTAGE & VERSTAGE.

File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/comment/d6eadfad_f0979837 PS1, Line 266: ENV_ROMSTAGE || ENV_SEPARATE_VERSTAGE

...

If we want to cover the bootblock, that could be more explicit: […]

Done

https://review.coreboot.org/c/coreboot/+/48156/comment/e7bf0871_5fa44c1b PS1, Line 276: stages uses

...

nit - maybe "stage uses".

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Thu, 02 Jun 2022 10:06:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Marshall Dawson marshalldawson3rd@gmail.com Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Comment-In-Reply-To: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-MessageType: comment

Arthur Heymans (Code Review)

12:08 p.m.

Attention is currently required from: Nico Huber, Furquan Shaikh, Marshall Dawson, Kyösti Mälkki. Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Julius Werner,

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

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

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

arch/x86: Don't use .bss from car.ld if not running XIP

Tested on up/squared (Intel APL) & google/vilboz (AMD Picasso).

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48156/4

Sean Rhodes (Code Review)

20 Sep 20 Sep

9:09 p.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Nico Huber, Furquan Shaikh, Marshall Dawson, Arthur Heymans, Kyösti Mälkki.

Sean Rhodes has uploaded a new patch set (#6) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

arch/x86: Only use .bss from car.ld when running XIP

Tested on up/squared (Intel APL) & google/vilboz (AMD Picasso).

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48156/6

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-MessageType: newpatchset

build bot (Jenkins) (Code Review)

9:10 p.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Nico Huber, Furquan Shaikh, Marshall Dawson, Arthur Heymans, Kyösti Mälkki.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

Patch Set 7:

(2 comments)

File src/include/rules.h:

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159055): https://review.coreboot.org/c/coreboot/+/48156/comment/a82b05c3_b230af44 PS7, Line 279: #define ENV_SEPARATE_BSS ENV_CACHE_AS_RAM && (ENV_BOOTBLOCK || !CONFIG(NO_XIP_EARLY_STAGES)) line length of 99 exceeds 96 columns

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159055): https://review.coreboot.org/c/coreboot/+/48156/comment/2c2d55d9_85498269 PS7, Line 279: #define ENV_SEPARATE_BSS ENV_CACHE_AS_RAM && (ENV_BOOTBLOCK || !CONFIG(NO_XIP_EARLY_STAGES)) Macros with complex values should be enclosed in parentheses

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Tue, 20 Sep 2022 19:10:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

build bot (Jenkins) (Code Review)

10:18 p.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Sean Rhodes, Nico Huber, Furquan Shaikh, Marshall Dawson, Arthur Heymans, Kyösti Mälkki.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

Patch Set 8:

(1 comment)

File src/include/rules.h:

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159061): https://review.coreboot.org/c/coreboot/+/48156/comment/3665ef59_f61b8e3c PS8, Line 279: #define ENV_SEPARATE_BSS (ENV_CACHE_AS_RAM && (ENV_BOOTBLOCK || !CONFIG(NO_XIP_EARLY_STAGES))) line length of 101 exceeds 96 columns

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Sean Rhodes sean@starlabs.systems Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Tue, 20 Sep 2022 20:18:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Arthur Heymans (Code Review)

21 Sep 21 Sep

7:17 a.m.

New subject: [S] Change in coreboot[master]: arch/x86: Don't use .bss from car.ld if not running XIP

Attention is currently required from: Sean Rhodes, Nico Huber, Furquan Shaikh, Marshall Dawson, Arthur Heymans, Kyösti Mälkki.

Hello build bot (Jenkins), Sean Rhodes, Furquan Shaikh, Marshall Dawson, Tim Wawrzynczak, Julius Werner,

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

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

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

Change subject: arch/x86: Don't use .bss from car.ld if not running XIP ......................................................................

arch/x86: Don't use .bss from car.ld if not running XIP

Tested on up/squared (Intel APL) & google/vilboz (AMD Picasso).

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48156/9

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 9 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Sean Rhodes sean@starlabs.systems Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-MessageType: newpatchset

Arthur Heymans (Code Review)

7:25 a.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Sean Rhodes, Nico Huber, Furquan Shaikh, Marshall Dawson, Arthur Heymans, Kyösti Mälkki.

Hello build bot (Jenkins), Sean Rhodes, Furquan Shaikh, Marshall Dawson, Tim Wawrzynczak, Julius Werner,

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

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

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

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

arch/x86: Only use .bss from car.ld when running XIP

Tested on up/squared (Intel APL).

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48156/10

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 10 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Sean Rhodes sean@starlabs.systems Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-MessageType: newpatchset

build bot (Jenkins) (Code Review)

7:25 a.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Sean Rhodes, Nico Huber, Furquan Shaikh, Marshall Dawson, Arthur Heymans, Kyösti Mälkki.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

Patch Set 10:

(1 comment)

File src/include/rules.h:

Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-159084): https://review.coreboot.org/c/coreboot/+/48156/comment/8678dcfd_4cbe00be PS10, Line 279: #define ENV_SEPARATE_BSS (ENV_CACHE_AS_RAM && (ENV_BOOTBLOCK || !CONFIG(NO_XIP_EARLY_STAGES))) line length of 101 exceeds 96 columns

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 10 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Sean Rhodes sean@starlabs.systems Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Wed, 21 Sep 2022 05:25:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Sean Rhodes (Code Review)

9:38 a.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Nico Huber, Furquan Shaikh, Marshall Dawson, Arthur Heymans, Kyösti Mälkki.

Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

Patch Set 10: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 10 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Wed, 21 Sep 2022 07:38:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Sean Rhodes (Code Review)

16 Oct 16 Oct

9:58 p.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Nico Huber, Furquan Shaikh, Marshall Dawson, Matt DeVillier, Tim Wawrzynczak, Arthur Heymans, Kyösti Mälkki.

Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

Patch Set 10:

(2 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48156/comment/271e464f_d1b68291 PS4, Line 7: Don't use .bss from car.ld if not running XIP

...

suggestion: […]

Done

Patchset:

PS4:

...

Mind if I adjust for the comments?

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 10 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Matt DeVillier matt.devillier@gmail.com Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Nico Huber nico.h@gmx.de Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Matt DeVillier matt.devillier@gmail.com Gerrit-Attention: Tim Wawrzynczak inforichland@gmail.com Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Sun, 16 Oct 2022 19:58:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sean Rhodes sean@starlabs.systems Comment-In-Reply-To: Tim Wawrzynczak inforichland@gmail.com Gerrit-MessageType: comment

Nico Huber (Code Review)

18 Oct 18 Oct

10:38 a.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Attention is currently required from: Furquan Shaikh, Marshall Dawson, Matt DeVillier, Tim Wawrzynczak, Julius Werner, Arthur Heymans, Kyösti Mälkki.

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

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

Patch Set 10: Code-Review+1

(1 comment)

File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/48156/comment/0d18c842_d7adfbee PS4, Line 279: #define ENV_SEPARATE_BSS (ENV_CACHE_AS_RAM && !((ENV_ROMSTAGE \

...

I think equivalent but a bit cleaner would be […]

Looks done.

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 10 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Matt DeVillier matt.devillier@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Attention: Matt DeVillier matt.devillier@gmail.com Gerrit-Attention: Tim Wawrzynczak inforichland@gmail.com Gerrit-Attention: Julius Werner jwerner@chromium.org Gerrit-Attention: Arthur Heymans arthur@aheymans.xyz Gerrit-Attention: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Tue, 18 Oct 2022 08:38:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Julius Werner jwerner@chromium.org Gerrit-MessageType: comment

Felix Held (Code Review)

20 Oct 20 Oct

4:44 p.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

arch/x86: Only use .bss from car.ld when running XIP

Tested on up/squared (Intel APL).

Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/48156 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Sean Rhodes sean@starlabs.systems Reviewed-by: Nico Huber nico.h@gmx.de --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/include/rules.h M src/lib/program.ld 4 files changed, 30 insertions(+), 1 deletion(-)

Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, but someone else must approve Sean Rhodes: Looks good to me, approved

diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index 79d6e19..869acc8 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -34,6 +34,7 @@ mov $_STACK_TOP, %esp

/* clear .bss section as it is not shared */ +#if ENV_SEPARATE_BSS cld xor %eax, %eax movl $(_ebss), %ecx @@ -41,6 +42,7 @@ sub %edi, %ecx shrl $2, %ecx rep stosl +#endif

#if ((ENV_SEPARATE_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ || (ENV_ROMSTAGE && CONFIG(ROMSTAGE_DEBUG_SPINLOOP))) diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 132937f..47afd78 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -59,6 +59,7 @@ * cbmem console. This is useful for clearing this area on a per-stage * basis when more than one stage uses cache-as-ram. */

+#if ENV_SEPARATE_BSS . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .; /* Allow global uninitialized variables for stages without CAR teardown. */ @@ -69,6 +70,7 @@ . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _ebss = .; RECORD_SIZE(bss) +#endif

#if ENV_ROMSTAGE && CONFIG(ASAN_IN_ROMSTAGE) _shadow_size = (_ebss - _car_region_start) >> 3; diff --git a/src/include/rules.h b/src/include/rules.h index b2f2142..de33a78 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -275,6 +275,9 @@ #define ENV_CACHE_AS_RAM 0 #endif

+/* Indicates if the stage uses the _bss region defined in arch/x86/car.ld */ +#define ENV_SEPARATE_BSS (ENV_CACHE_AS_RAM && (ENV_BOOTBLOCK || !CONFIG(NO_XIP_EARLY_STAGES))) + /* Currently rmodules, ramstage and smm have heap. */ #define ENV_HAS_HEAP_SECTION (ENV_RMODULE || ENV_RAMSTAGE || ENV_SMM)

diff --git a/src/lib/program.ld b/src/lib/program.ld index 8db7ddc..67f685f 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -116,7 +116,7 @@ } #endif

-#if !ENV_CACHE_AS_RAM +#if !ENV_SEPARATE_BSS .bss . : { . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .;

9elements QA (Code Review)

7:38 p.m.

New subject: [S] Change in coreboot[master]: arch/x86: Only use .bss from car.ld when running XIP

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48156 )

Change subject: arch/x86: Only use .bss from car.ld when running XIP ......................................................................

Patch Set 11:

Automatic boot test returned (PASS/FAIL/TOTAL): 15 / 1 / 16

PASS: x86_32 "Hermes CFL" , build config PRODRIVE_HERMES_ and payload TianoCore_UefiPayloadPkg : https://lava.9esec.io/r/128961 PASS: x86_32 "Hermes CFL" , build config PRODRIVE_HERMES and payload TianoCore_UefiPayloadPkg : https://lava.9esec.io/r/128960 PASS: x86_32 "ThinkPad T500" , build config LENOVO_T500 and payload SeaBIOS : https://lava.9esec.io/r/128959 PASS: x86_32 "HP Z220 SFF Workstation" , build config HP_Z220_SFF_WORKSTATION and payload LinuxBoot_BB_kexec : https://lava.9esec.io/r/128958 PASS: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload TianoCore : https://lava.9esec.io/r/128957 PASS: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload SeaBIOS : https://lava.9esec.io/r/128956 PASS: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload TianoCore : https://lava.9esec.io/r/128955 PASS: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload SeaBIOS : https://lava.9esec.io/r/128954 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35_SMM_TSEG and payload TianoCore : https://lava.9esec.io/r/128953 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35_SMM_TSEG and payload SeaBIOS : https://lava.9esec.io/r/128952 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35 and payload TianoCore : https://lava.9esec.io/r/128951 PASS: x86_32 "QEMU x86 q35/ich9" , build config EMULATION_QEMU_X86_Q35 and payload SeaBIOS : https://lava.9esec.io/r/128950 FAIL: x86_64 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_X86_64 and payload SeaBIOS : https://lava.9esec.io/r/128949 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ASAN and payload SeaBIOS : https://lava.9esec.io/r/128948 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ and payload SeaBIOS : https://lava.9esec.io/r/128947 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX and payload SeaBIOS : https://lava.9esec.io/r/128946

Please note: This test is under development and might not be accurate at all!

-- To view, visit https://review.coreboot.org/c/coreboot/+/48156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6cf51f943dde5f642d75ba4c5d3be520dc56370a Gerrit-Change-Number: 48156 Gerrit-PatchSet: 11 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Matt DeVillier matt.devillier@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Comment-Date: Thu, 20 Oct 2022 17:38:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

765

days inactive

1454

days old

coreboot-gerrit@coreboot.org

32 comments

8 participants

tags (0)

participants (8)

9elements QA (Code Review)
Arthur Heymans (Code Review)
build bot (Jenkins) (Code Review)
Felix Held (Code Review)
Kyösti Mälkki (Code Review)
Marshall Dawson (Code Review)
Nico Huber (Code Review)
Sean Rhodes (Code Review)