Attention is currently required from: Martin Roth, Marshall Dawson, Arthur Heymans, Kyösti Mälkki, Yu-Ping Wu, Felix Held.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56122 )
Change subject: Makefile.inc: Add x86 bootblock as a separate target
......................................................................
Patch Set 14:
(3 comments)
File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56122/comment/e2bc5e60_e7692a80
PS9, Line 107: bootblock-options := $(TS_OPTIONS) $(TXTIBB)
> > Can't we just put the -b here as well (maybe if you use = instead of := )? Or is there no way to m […]
Thanks, I think that's cleaner.
File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56122/comment/397feb9a_03cf900a
PS11, Line 103: ifneq ($(CONFIG_UPDATE_IMAGE),y)
Shouldn't this also depend on TOP_ALIGNED_BOOTBLOCK? (And in that case maybe go in the arch-independent Makefile?)
https://review.coreboot.org/c/coreboot/+/56122/comment/4aebb9b0_7cc5edc9
PS11, Line 106: -$(CBFSTOOL) $< remove -n bootblock 2>/dev/null
Why are we removing something first?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56122
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4de9d7fedf1ae5a37a3310dd42eb07b44c030930
Gerrit-Change-Number: 56122
Gerrit-PatchSet: 14
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 05 Apr 2022 20:22:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63377 )
Change subject: Kconfig: Add an option to skip adding a cbfs bootblock on x86
......................................................................
Patch Set 4:
(1 comment)
File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/63377/comment/2767d593_62902e05
PS1, Line 1330: TOP_ALIGNED_CBFS_BOOTBLOCK
I think something like BOOTBLOCK_IN_CBFS would be a more straight-forward way to describe this?
--
To view, visit https://review.coreboot.org/c/coreboot/+/63377
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia42448f7e9dd0635c72857fbc1fab54508932721
Gerrit-Change-Number: 63377
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 05 Apr 2022 20:18:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Martin Roth, Yu-Ping Wu.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59132 )
Change subject: Makefile.inc: Generate master header and pointer as C structs
......................................................................
Patch Set 9:
(6 comments)
File src/arch/x86/bootblock.ld:
https://review.coreboot.org/c/coreboot/+/59132/comment/eea6e6e2_8fc93454
PS7, Line 77: . = 0xfffffffc;
: .header_pointer . : {
: KEEP(*(.header_pointer));
: }
> This needs to be smarter. It looks like the reset vector is wrong when header_pointer is missing.
The file is probably 4 bytes too short in that case? I think maybe this could do what you want:
. = 0xfffffffc;
.header_pointer . : {
KEEP(*(.header_pointer));
FILL(0xffffffff);
. = 4;
}
Then again, why would header_pointer be missing? Don't we always have it? Then this shouldn't be a problem.
edit: Okay, I guess that works too.
File src/lib/cbfs_master_header.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/42d2b82a_992623f2
PS9, Line 7: __attribute((used))
Same problem here as the other one.
https://review.coreboot.org/c/coreboot/+/59132/comment/927f5f73_c9c1bedf
PS9, Line 10: /* The offset and romsize fields within the master header are absolute
nit: https://doc.coreboot.org/contributing/coding_style.html#commentinghttps://review.coreboot.org/c/coreboot/+/59132/comment/6775785f_1eeeb8af
PS9, Line 11: relfect
typo
https://review.coreboot.org/c/coreboot/+/59132/comment/3154cfba_cd6cae21
PS9, Line 16: FMAP_SECTION_COREBOOT_START + FMAP_SECTION_COREBOOT_SIZE
Okay, I figured out how that memory-mapped offset thing in these headers works now (and it's honestly pretty horrible, we should try getting rid of that. It seems completely arbitrary which .fmd files define a FLASH@xxx and which don't. Most of our APIs that take a flash offset (e.g. boot_device_ro()) would not know how to deal with a memory-mapped offset like that. I think we only get away with that because we barely use these header constants anymore anyway). But then this line is a problem, because if it's using the memory-mapped offsets this would be a huge number. So you also need to subtract FMAP_SECTION_FLASH_START here again.
File src/lib/master_header_pointer.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/07d89e25_5f819cfd
PS7, Line 13: ATTRIBUTES uint32_t header_pointer =
I think an easier way to write this is
#if ENV_X86
__attribute__((__section__(".header_pointer")))
#endif
uint32_t header_pointer = ...;
--
To view, visit https://review.coreboot.org/c/coreboot/+/59132
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3ba01be7da1f09a8cac287751497c18cda97d293
Gerrit-Change-Number: 59132
Gerrit-PatchSet: 9
Gerrit-Owner: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Tue, 05 Apr 2022 20:15:57 +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, Patrick Rudolph.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56118 )
Change subject: soc/intel/common/block/fast_spi/Makefile.inc: Improve cosmetics
......................................................................
Patch Set 14: Verified+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/56118
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I41bbdabf7b846386651e64f4afb5b7b9fb38e1cb
Gerrit-Change-Number: 56118
Gerrit-PatchSet: 14
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Tue, 05 Apr 2022 20:14:10 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Fred Reitberger.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56117 )
Change subject: soc/amd/*/Makefile.inc: Do some cosmetics
......................................................................
Patch Set 14: Verified+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/56117
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iea2322ca1abd43900f3631b7965f07fed4235ca0
Gerrit-Change-Number: 56117
Gerrit-PatchSet: 14
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Comment-Date: Tue, 05 Apr 2022 20:08:32 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63377 )
Change subject: Kconfig: Add an option to skip adding a cbfs bootblock on x86
......................................................................
Patch Set 2: Verified+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/63377
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia42448f7e9dd0635c72857fbc1fab54508932721
Gerrit-Change-Number: 63377
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 05 Apr 2022 20:05:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Martin Roth, Marshall Dawson, Julius Werner, Kyösti Mälkki, Yu-Ping Wu, Felix Held.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56122 )
Change subject: Makefile.inc: Add x86 bootblock as a separate target
......................................................................
Patch Set 12: Verified+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/56122
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4de9d7fedf1ae5a37a3310dd42eb07b44c030930
Gerrit-Change-Number: 56122
Gerrit-PatchSet: 12
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 05 Apr 2022 20:05:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Julius Werner.
Hello build bot (Jenkins), Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/63378
to look at the new patch set (#4).
Change subject: Kconfig: Add an option to ommit a cbfs master header
......................................................................
Kconfig: Add an option to ommit a cbfs master header
The CBFS master header is legacy feature that has no internal use in
coreboot. Some payloads do depend on it however.
Change-Id: Ib3d5b32412db40df48c0ed4dd6216bdd9cb955e2
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/Kconfig
M src/lib/Makefile.inc
2 files changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/63378/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/63378
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib3d5b32412db40df48c0ed4dd6216bdd9cb955e2
Gerrit-Change-Number: 63378
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: newpatchset