Attention is currently required from: Arthur Heymans, Julius Werner, Maximilian Brune, Nico Huber, Subrata Banik.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80776?usp=email )
Change subject: lib: Declare heap in assembly
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Patchset:
PS2:
Getting identical binaries for Google/Rex0 with this or the revert.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80776?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I67cb5ce886fda313e0720b0bc7c6e66e4aae45fa
Gerrit-Change-Number: 80776
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 28 Feb 2024 15:33:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Arthur Heymans has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/80776?usp=email )
Change subject: lib: Declare heap in assembly
......................................................................
lib: Declare heap in assembly
Some linkers like LLD don't set up the NOBITS flag on the .heap section
when just declaring that region in the linker script. This would have
the program loader initialize the heap, which is not desirable for
performance reasons. Also if the cbfs file would not be compressed it
would be huge as the default heap is 1M.
This fixes commit 99bf23c9e73ca, which broke setups with a relocatable
ramstage as the heap size was not accounted for when allocating memory
in cbmem. The alloc code could trash higher cbmem entries in that case.
Change-Id: I67cb5ce886fda313e0720b0bc7c6e66e4aae45fa
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/lib/Makefile.mk
A src/lib/heap.S
M src/lib/program.ld
3 files changed, 20 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/80776/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/80776?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I67cb5ce886fda313e0720b0bc7c6e66e4aae45fa
Gerrit-Change-Number: 80776
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Held, Jason Nien, Martin Roth, Paul Menzel.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80711?usp=email )
Change subject: mb/google/skyrim/var/skyrim: Hide fingerprint reader from Windows OS
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80711/comment/27131998_0216f8f0 :
PS1, Line 9: is needed
> Sorry, I do not understand this. […]
the FPR can't be used outside of ChromeOS, there's no support in Linux even.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80711?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ia700aa4ccd478bc734db012e1419e566a5dcf493
Gerrit-Change-Number: 80711
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 28 Feb 2024 15:23:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Caveh Jalali, Julius Werner, Subrata Banik.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80775?usp=email )
Change subject: Revert "lib: Explicitly declare heap as NOLOAD"
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> revision 3 of https://review.coreboot.org/c/coreboot/+/80735/3 did it differently and that should work. I just quit because the ARM syntax is slightly different, but I figured it out.
>
> Feel free to try CB:80735
Eh I meant CB:80776
--
To view, visit https://review.coreboot.org/c/coreboot/+/80775?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d49e2dc49cd2935a9d8023c989869ec9558039e
Gerrit-Change-Number: 80775
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Wed, 28 Feb 2024 15:18:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Caveh Jalali, Julius Werner, Subrata Banik.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80775?usp=email )
Change subject: Revert "lib: Explicitly declare heap as NOLOAD"
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
revision 3 of https://review.coreboot.org/c/coreboot/+/80735/3 did it differently and that should work. I just quit because the ARM syntax is slightly different, but I figured it out.
Feel free to try CB:80735
--
To view, visit https://review.coreboot.org/c/coreboot/+/80775?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d49e2dc49cd2935a9d8023c989869ec9558039e
Gerrit-Change-Number: 80775
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 28 Feb 2024 15:17:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80776?usp=email )
Change subject: lib: Declare heap in assembly
......................................................................
lib: Declare heap in assembly
Some linkers like LLD don't set up the NOBITS flag on the .heap section
when just declaring that region in the linker script. This would have
the program loader initialize the heap, which is not desirable for
performance reasons. Also if the cbfs file would not be compressed it
would be huge as the default heap is 1M.
This fixes commit 99bf23c9e7, which broke setups with a relocatable
ramstage as the heap size was not accounted for when allocating memory
in cbmem. The alloc code could trash higher cbmem entries in that case.
Change-Id: I67cb5ce886fda313e0720b0bc7c6e66e4aae45fa
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/lib/Makefile.mk
A src/lib/heap.S
M src/lib/program.ld
3 files changed, 20 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/80776/1
diff --git a/src/lib/Makefile.mk b/src/lib/Makefile.mk
index e22fd08..9409cb0 100644
--- a/src/lib/Makefile.mk
+++ b/src/lib/Makefile.mk
@@ -138,6 +138,7 @@
ramstage-y += memchr.c
ramstage-y += memcmp.c
ramstage-y += malloc.c
+ramstage-y += heap.S
ramstage-y += dimm_info_util.c
ramstage-y += delay.c
ramstage-y += fallback_boot.c
diff --git a/src/lib/heap.S b/src/lib/heap.S
new file mode 100644
index 0000000..dc29940
--- /dev/null
+++ b/src/lib/heap.S
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ * Declare the heap in an assembly files as some linkers (LLD) don't set
+ * the NOBITS section flag on non default sections.
+ */
+#if ENV_ARM
+.section ".heap", "wa", %nobits
+#else
+.section .heap, "wa", @nobits
+#endif
+
+.global _heap, _eheap
+_heap:
+.skip CONFIG_HEAP_SIZE
+_eheap:
diff --git a/src/lib/program.ld b/src/lib/program.ld
index 6d72d9e..bdcadca 100644
--- a/src/lib/program.ld
+++ b/src/lib/program.ld
@@ -131,12 +131,11 @@
#endif
#if ENV_HAS_HEAP_SECTION
-.heap . (NOLOAD) : {
+.heap . : {
. = ALIGN(ARCH_POINTER_ALIGN_SIZE);
- _heap = .;
- . += CONFIG_HEAP_SIZE;
+ *(.heap)
+ *(.heap.*)
. = ALIGN(ARCH_POINTER_ALIGN_SIZE);
- _eheap = .;
RECORD_SIZE(heap)
}
#endif
--
To view, visit https://review.coreboot.org/c/coreboot/+/80776?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I67cb5ce886fda313e0720b0bc7c6e66e4aae45fa
Gerrit-Change-Number: 80776
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Attention is currently required from: Arthur Heymans, Caveh Jalali, Julius Werner, Subrata Banik.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80775?usp=email )
Change subject: Revert "lib: Explicitly declare heap as NOLOAD"
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I suspect the code relocating this in cbmem is unaware of the heap and overwrites cbmem by using the […]
This was also my conclusion. rmodule_cbfs_allocator() uses the `memlen` from the
stage-header attribute, which is filled with the `memsz` from the PT_LOAD header,
which doesn't include the heap anymore.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80775?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d49e2dc49cd2935a9d8023c989869ec9558039e
Gerrit-Change-Number: 80775
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 28 Feb 2024 15:07:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Caveh Jalali, Julius Werner, Subrata Banik.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80775?usp=email )
Change subject: Revert "lib: Explicitly declare heap as NOLOAD"
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
I suspect the code relocating this in cbmem is unaware of the heap and overwrites cbmem by using the heap.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80775?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d49e2dc49cd2935a9d8023c989869ec9558039e
Gerrit-Change-Number: 80775
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Wed, 28 Feb 2024 14:56:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Caveh Jalali, Julius Werner, Subrata Banik.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80775?usp=email )
Change subject: Revert "lib: Explicitly declare heap as NOLOAD"
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Haven't fully figured out what went wrong yet. The `memsz` in the program […]
I guess one solution would be to alter the calculation of `mem_end` in
parse_elf_to_stage(). So it would keep the bigger program size in the
stage attribute. But I'm not fluent enough in ELF to say what would
be the best way to figure the correct `mem_end`.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80775?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d49e2dc49cd2935a9d8023c989869ec9558039e
Gerrit-Change-Number: 80775
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 28 Feb 2024 14:53:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Caveh Jalali, Julius Werner, Subrata Banik.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80775?usp=email )
Change subject: Revert "lib: Explicitly declare heap as NOLOAD"
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
Haven't fully figured out what went wrong yet. The `memsz` in the program
header seems to lose the 1MiB heap, i.e. it's not allocated? Which seems wrong.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80775?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d49e2dc49cd2935a9d8023c989869ec9558039e
Gerrit-Change-Number: 80775
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 28 Feb 2024 14:37:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment