coreboot-gerrit February 2022

coreboot-gerrit@coreboot.org

1 participants
7066 discussions

Change in coreboot[master]: util/chromeos/crosfirmware: format with shfmt
by Matt DeVillier (Code Review) 04 Feb '22

04 Feb '22

Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/61617 ) Change subject: util/chromeos/crosfirmware: format with shfmt ...................................................................... util/chromeos/crosfirmware: format with shfmt Clean up formatting using shfmt Change-Id: I46ce84668bfb4ea3df179317e2848b6bb75d8d5c Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M util/chromeos/crosfirmware.sh 1 file changed, 36 insertions(+), 42 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/61617/1 diff --git a/util/chromeos/crosfirmware.sh b/util/chromeos/crosfirmware.sh index e022f69..6c0f553 100755 --- a/util/chromeos/crosfirmware.sh +++ b/util/chromeos/crosfirmware.sh @@ -28,92 +28,87 @@ exit_if_uninstalled "unzip" "unzip" } -get_inventory() -{ +get_inventory() { _conf=$1 _url=https://dl.google.com/dl/edgedl/chromeos/recovery/recovery.conf echo "Downloading recovery image inventory..." - curl -s "$_url" > $_conf + curl -s "$_url" >$_conf } -download_image() -{ +download_image() { _url=$1 _file=$2 echo "Downloading recovery image" - curl "$_url" > "$_file.zip" + curl "$_url" >"$_file.zip" echo "Decompressing recovery image" unzip -q "$_file.zip" rm "$_file.zip" } -extract_partition() -{ +extract_partition() { NAME=$1 FILE=$2 ROOTFS=$3 _bs=1024 echo "Extracting ROOT-A partition" - ROOTP=$( printf "unit\nB\nprint\nquit\n" | \ - parted $FILE 2>/dev/null | grep $NAME ) + ROOTP=$(printf "unit\nB\nprint\nquit\n" | + parted $FILE 2>/dev/null | grep $NAME) if [ "$ROOTP" == "" ]; then # Automatic extraction failed, likely due to parted detecting # overlapping partitions. Fall back to using fdisk and assume # ROOT-A is partition #3 echo "(Extracting via parted failed; falling back to fdisk)" - _ssize=$(printf "p q" | fdisk $FILE | grep "Sector size" | \ + _ssize=$(printf "p q" | fdisk $FILE | grep "Sector size" | cut -f2 -d: | cut -f2 -d ' ') - _start=$(printf "p q" | fdisk $FILE | grep "bin3" | tr -s ' ' | \ + _start=$(printf "p q" | fdisk $FILE | grep "bin3" | tr -s ' ' | cut -f2 -d ' ') - _nsec=$(printf "p q" | fdisk $FILE | grep "bin3" | tr -s ' ' | \ + _nsec=$(printf "p q" | fdisk $FILE | grep "bin3" | tr -s ' ' | cut -f4 -d ' ') START=$(($_ssize * $_start)) SIZE=$(($_ssize * $_nsec)) else - START=$(( $( echo $ROOTP | cut -f2 -d\ | tr -d "B" ) )) - SIZE=$(( $( echo $ROOTP | cut -f4 -d\ | tr -d "B" ) )) + START=$(($(echo $ROOTP | cut -f2 -d\ | tr -d "B"))) + SIZE=$(($(echo $ROOTP | cut -f4 -d\ | tr -d "B"))) fi - dd if=$FILE of=$ROOTFS bs=$_bs skip=$(( $START / $_bs )) \ - count=$(( $SIZE / $_bs )) > /dev/null 2>&1 + dd if=$FILE of=$ROOTFS bs=$_bs skip=$(($START / $_bs)) \ + count=$(($SIZE / $_bs)) >/dev/null 2>&1 } -extract_shellball() -{ +extract_shellball() { ROOTFS=$1 SHELLBALL=$2 echo "Extracting chromeos-firmwareupdate" - printf "cd /usr/sbin\ndump chromeos-firmwareupdate $SHELLBALL\nquit" | \ - debugfs $ROOTFS > /dev/null 2>&1 + printf "cd /usr/sbin\ndump chromeos-firmwareupdate $SHELLBALL\nquit" | + debugfs $ROOTFS >/dev/null 2>&1 } -extract_coreboot() -{ +extract_coreboot() { _shellball=$1 - _unpacked=$( mktemp -d ) + _unpacked=$(mktemp -d) echo "Extracting coreboot image" - if ! sh $_shellball --unpack $_unpacked > /dev/null 2>&1; then - sh $_shellball --sb_extract $_unpacked > /dev/null 2>&1 + if ! sh $_shellball --unpack $_unpacked >/dev/null 2>&1; then + sh $_shellball --sb_extract $_unpacked >/dev/null 2>&1 fi if [ -d $_unpacked/models/ ]; then - _version=$( cat $_unpacked/VERSION | grep -e Model.*$_board -A5 \ - | grep "BIOS (RW) version:" | cut -f2 -d: | tr -d \ ) + _version=$(cat $_unpacked/VERSION | grep -e Model.*$_board -A5 | + grep "BIOS (RW) version:" | cut -f2 -d: | tr -d \ ) if [ "$_version" == "" ]; then - _version=$( cat $_unpacked/VERSION | grep -e Model.*$_board -A5 \ - | grep "BIOS version:" | cut -f2 -d: | tr -d \ ) + _version=$(cat $_unpacked/VERSION | grep -e Model.*$_board -A5 | + grep "BIOS version:" | cut -f2 -d: | tr -d \ ) fi _bios_image=$(grep "IMAGE_MAIN" $_unpacked/models/$_board/setvars.sh | cut -f2 -d\") else - _version=$( cat $_unpacked/VERSION | grep BIOS\ version: | \ - cut -f2 -d: | tr -d \ ) + _version=$(cat $_unpacked/VERSION | grep BIOS\ version: | + cut -f2 -d: | tr -d \ ) _bios_image=bios.bin fi if cp $_unpacked/$_bios_image coreboot-$_version.bin; then @@ -123,8 +118,7 @@ rm $_shellball } -do_one_board() -{ +do_one_board() { _board=$1 _url=$2 _file=$3 @@ -147,25 +141,25 @@ exit_if_dependencies_are_missing if [ "$BOARD" == "all" ]; then - CONF=$( mktemp ) + CONF=$(mktemp) get_inventory $CONF - grep ^name= $CONF| while read _line; do - name=$( echo $_line | cut -f2 -d= ) + grep ^name= $CONF | while read _line; do + name=$(echo $_line | cut -f2 -d=) echo Processing board $name - eval $( grep -v hwid= $CONF | grep -A11 "$_line" | \ - grep '$url=\|file=$' ) - BOARD=$( echo $url | cut -f3 -d_ ) + eval $(grep -v hwid= $CONF | grep -A11 "$_line" | + grep '$url=\|file=$') + BOARD=$(echo $url | cut -f3 -d_) do_one_board $BOARD $url $file done rm "$CONF" elif [ "$BOARD" != "" ]; then - CONF=$( mktemp ) + CONF=$(mktemp) get_inventory $CONF echo Processing board $BOARD - eval $( grep -i $BOARD -A8 $CONF | grep '$url=\|file=$' ) + eval $(grep -i $BOARD -A8 $CONF | grep '$url=\|file=$') do_one_board $BOARD $url $file rm "$CONF" -- To view, visit https://review.coreboot.org/c/coreboot/+/61617 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I46ce84668bfb4ea3df179317e2848b6bb75d8d5c Gerrit-Change-Number: 61617 Gerrit-PatchSet: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: util/chromeos/crosfirmware: Handle "broken" recovery images
by Matt DeVillier (Code Review) 04 Feb '22

04 Feb '22

Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/61616 ) Change subject: util/chromeos/crosfirmware: Handle "broken" recovery images ...................................................................... util/chromeos/crosfirmware: Handle "broken" recovery images Several recovery images for newer ChromeOS boards fail in extract_partition() due to parted detecting that there are overlapping partitions, and therefore failing to print the partition layout (this is potentially a parted bug; requries further investigation). To work around this, fall back to using fdisk, making the assumption that ROOT-A is always partition #3, and calculate the partition start and size using the sector size. Test: successfully extract coreboot firmware images from recovery images which previously failed to extract (fizz, octopus, volteer). Change-Id: I03234170ba0544af9eb0879253f0a8e0e7bf33f5 Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M util/chromeos/crosfirmware.sh 1 file changed, 18 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/61616/1 diff --git a/util/chromeos/crosfirmware.sh b/util/chromeos/crosfirmware.sh index f912753..e022f69 100755 --- a/util/chromeos/crosfirmware.sh +++ b/util/chromeos/crosfirmware.sh @@ -61,11 +61,26 @@ ROOTP=$( printf "unit\nB\nprint\nquit\n" | \ parted $FILE 2>/dev/null | grep $NAME ) - START=$(( $( echo $ROOTP | cut -f2 -d\ | tr -d "B" ) )) - SIZE=$(( $( echo $ROOTP | cut -f4 -d\ | tr -d "B" ) )) + if [ "$ROOTP" == "" ]; then + # Automatic extraction failed, likely due to parted detecting + # overlapping partitions. Fall back to using fdisk and assume + # ROOT-A is partition #3 + echo "(Extracting via parted failed; falling back to fdisk)" + _ssize=$(printf "p q" | fdisk $FILE | grep "Sector size" | \ + cut -f2 -d: | cut -f2 -d ' ') + _start=$(printf "p q" | fdisk $FILE | grep "bin3" | tr -s ' ' | \ + cut -f2 -d ' ') + _nsec=$(printf "p q" | fdisk $FILE | grep "bin3" | tr -s ' ' | \ + cut -f4 -d ' ') + START=$(($_ssize * $_start)) + SIZE=$(($_ssize * $_nsec)) + else + START=$(( $( echo $ROOTP | cut -f2 -d\ | tr -d "B" ) )) + SIZE=$(( $( echo $ROOTP | cut -f4 -d\ | tr -d "B" ) )) + fi dd if=$FILE of=$ROOTFS bs=$_bs skip=$(( $START / $_bs )) \ - count=$(( $SIZE / $_bs )) > /dev/null + count=$(( $SIZE / $_bs )) > /dev/null 2>&1 } extract_shellball() -- To view, visit https://review.coreboot.org/c/coreboot/+/61616 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I03234170ba0544af9eb0879253f0a8e0e7bf33f5 Gerrit-Change-Number: 61616 Gerrit-PatchSet: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: util/chromeos/crosfirmware: Fix handling of newer boards
by Matt DeVillier (Code Review) 04 Feb '22

04 Feb '22

Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/61615 ) Change subject: util/chromeos/crosfirmware: Fix handling of newer boards ...................................................................... util/chromeos/crosfirmware: Fix handling of newer boards Wile historically there was a unique recovery image for each Chrome OS board/HWID (with matching names), this is no longer the case. Now, multiple boards share a single recovery image, so adjust how the proper recovery image is determined, and how the coreboot image is extracted from it. Test: successfully extract coreboot images for older 1:1 boards (e.g. CAVE) and newer 1:N boards (e.g. DROBIT) Change-Id: If478aa6eadea3acf3ee9d4c5fa266acd72c99b7a Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M util/chromeos/crosfirmware.sh 1 file changed, 21 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/61615/1 diff --git a/util/chromeos/crosfirmware.sh b/util/chromeos/crosfirmware.sh index 25c88fa..f912753 100755 --- a/util/chromeos/crosfirmware.sh +++ b/util/chromeos/crosfirmware.sh @@ -84,13 +84,28 @@ _unpacked=$( mktemp -d ) echo "Extracting coreboot image" - sh $_shellball --sb_extract $_unpacked > /dev/null + if ! sh $_shellball --unpack $_unpacked > /dev/null 2>&1; then + sh $_shellball --sb_extract $_unpacked > /dev/null 2>&1 + fi - _version=$( cat $_unpacked/VERSION | grep BIOS\ version: | \ + if [ -d $_unpacked/models/ ]; then + _version=$( cat $_unpacked/VERSION | grep -e Model.*$_board -A5 \ + | grep "BIOS (RW) version:" | cut -f2 -d: | tr -d \ ) + if [ "$_version" == "" ]; then + _version=$( cat $_unpacked/VERSION | grep -e Model.*$_board -A5 \ + | grep "BIOS version:" | cut -f2 -d: | tr -d \ ) + fi + _bios_image=$(grep "IMAGE_MAIN" $_unpacked/models/$_board/setvars.sh | cut -f2 -d\") + else + _version=$( cat $_unpacked/VERSION | grep BIOS\ version: | \ cut -f2 -d: | tr -d \ ) - - cp $_unpacked/bios.bin coreboot-$_version.bin - rm -r "$_unpacked" + _bios_image=bios.bin + fi + if cp $_unpacked/$_bios_image coreboot-$_version.bin; then + echo "Extracted coreboot-$_version.bin" + fi + rm -rf "$_unpacked" + rm $_shellball } do_one_board() @@ -135,7 +150,7 @@ get_inventory $CONF echo Processing board $BOARD - eval $( grep $BOARD $CONF | grep '$url=\|file=$' ) + eval $( grep -i $BOARD -A8 $CONF | grep '$url=\|file=$' ) do_one_board $BOARD $url $file rm "$CONF" -- To view, visit https://review.coreboot.org/c/coreboot/+/61615 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If478aa6eadea3acf3ee9d4c5fa266acd72c99b7a Gerrit-Change-Number: 61615 Gerrit-PatchSet: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: mb/google/brya/var/nivviks: Disable LTE-related GPIOs based on fw_config
by Reka Norman (Code Review) 04 Feb '22

04 Feb '22

Attention is currently required from: Reka Norman. Hello Reka Norman, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/61614 to review the following change. Change subject: mb/google/brya/var/nivviks: Disable LTE-related GPIOs based on fw_config ...................................................................... mb/google/brya/var/nivviks: Disable LTE-related GPIOs based on fw_config If the LTE USB DB is not connected, disable the LTE-related GPIOs. BUG=b:197479026 TEST=abuild -a -x -c max -p none -t google/brya -b nivviks Signed-off-by: Reka Norman <rekanorman(a)google.com> Change-Id: I86251d8ad58d82ff2112ac5f2dfafdabbff4c76f --- M src/mainboard/google/brya/variants/nivviks/Makefile.inc A src/mainboard/google/brya/variants/nivviks/fw_config.c 2 files changed, 29 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/61614/1 diff --git a/src/mainboard/google/brya/variants/nivviks/Makefile.inc b/src/mainboard/google/brya/variants/nivviks/Makefile.inc index defb592..4e12843 100644 --- a/src/mainboard/google/brya/variants/nivviks/Makefile.inc +++ b/src/mainboard/google/brya/variants/nivviks/Makefile.inc @@ -4,4 +4,5 @@ romstage-y += gpio.c romstage-y += memory.c +ramstage-$(CONFIG_FW_CONFIG) += fw_config.c ramstage-y += gpio.c diff --git a/src/mainboard/google/brya/variants/nivviks/fw_config.c b/src/mainboard/google/brya/variants/nivviks/fw_config.c new file mode 100644 index 0000000..c475ca3 --- /dev/null +++ b/src/mainboard/google/brya/variants/nivviks/fw_config.c @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <baseboard/gpio.h> +#include <bootstate.h> +#include <console/console.h> +#include <fw_config.h> + +static const struct pad_config lte_disable_pads[] = { + /* A8 : WWAN_RF_DISABLE_ODL */ + PAD_NC(GPP_A8, NONE), + /* D6 : WWAN_EN */ + PAD_NC(GPP_D6, NONE), + /* F12 : WWAN_RST_L */ + PAD_NC(GPP_F12, NONE), + /* H19 : SOC_I2C_SUB_INT_ODL */ + PAD_NC(GPP_H19, NONE), + /* H23 : WWAN_SAR_DETECT_ODL */ + PAD_NC(GPP_H23, NONE), +}; + +static void fw_config_handle(void *unused) +{ + if (!fw_config_probe(FW_CONFIG(DB_USB, DB_1C_LTE))) { + printk(BIOS_INFO, "Disable LTE-related GPIO pins.\n"); + gpio_configure_pads(lte_disable_pads, ARRAY_SIZE(lte_disable_pads)); + } +} +BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_ENTRY, fw_config_handle, NULL); -- To view, visit https://review.coreboot.org/c/coreboot/+/61614 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I86251d8ad58d82ff2112ac5f2dfafdabbff4c76f Gerrit-Change-Number: 61614 Gerrit-PatchSet: 1 Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Reka Norman <rekanorman(a)google.com> Gerrit-Attention: Reka Norman <rekanorman(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: security/vboot/secdata_tpm: Set up space for widevine counter info
by Julius Werner (Code Review) 04 Feb '22

04 Feb '22

Attention is currently required from: Andrey Pronin, Raul Rangel, Mohan Viswanathan, Name of user not set #1004133, Jon Murphy, Paul Menzel, Rob Barnes, Yu-Ping Wu, Karthik Ramasubramanian. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61521 ) Change subject: security/vboot/secdata_tpm: Set up space for widevine counter info ...................................................................... Patch Set 4: (1 comment) Patchset: PS3: > Julius, […] Okay, I'm not sure why this needs to be so complicated but I'll trust you on that I guess. Are there any possible issues from the counters and the info space going out of sync due to random power loss while writing them? That would be one of my concerns about creating so many different spaces that depend on each other. -- To view, visit https://review.coreboot.org/c/coreboot/+/61521 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I80de9024195452a235cbed73e21410f848c91722 Gerrit-Change-Number: 61521 Gerrit-PatchSet: 4 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Andrey Pronin <apronin(a)google.com> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Mohan Viswanathan Gerrit-CC: Name of user not set #1004133 Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Andrey Pronin <apronin(a)google.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Mohan Viswanathan Gerrit-Attention: Name of user not set #1004133 Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 04 Feb 2022 02:29:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Mohan Viswanathan Comment-In-Reply-To: Name of user not set #1004133 Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: console: Add compile-time fast path when only CBMEM console is used
by Julius Werner (Code Review) 04 Feb '22

04 Feb '22

Julius Werner has uploaded a new patch set (#3). ( https://review.coreboot.org/c/coreboot/+/61613 ) Change subject: console: Add compile-time fast path when only CBMEM console is used ...................................................................... console: Add compile-time fast path when only CBMEM console is used A common use case when running coreboot on production systems is that only the CBMEM console (the one with the least impact on boot speed) is enabled. In this case, some of the code in the console subsystem has no effect. Due to the way it's all genericized over multiple consoles and tied together with function pointers, not all of this can be compile-time eliminated automatically, so this patch adds a little helper to facilitate that. This results in roughly 200 (compressed) bytes of savings per stage on an arm64 system. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I1d5b8bda80d02a13ee0b7835e0805c4319fd21d8 --- M src/console/console.c M src/console/printk.c M src/include/console/console.h 3 files changed, 20 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/61613/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/61613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1d5b8bda80d02a13ee0b7835e0805c4319fd21d8 Gerrit-Change-Number: 61613 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: console: Add compile-time fast path when only CBMEM console is used
by Julius Werner (Code Review) 04 Feb '22

04 Feb '22

Julius Werner has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/61613 ) Change subject: console: Add compile-time fast path when only CBMEM console is used ...................................................................... console: Add compile-time fast path when only CBMEM console is used A common use case when running coreboot on production systems is that only the CBMEM console (the one with the least impact on boot speed) is enabled. In this case, some of the code in the console subsystem has no effect. Due to the way it's all genericized over multiple consoles and tied together with function pointers, not all of this can be compile-time eliminated automatically, so this patch adds a little helper to facilitate that. This results in roughly 200 (compressed) bytes of savings per stage on an arm64 system. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I1d5b8bda80d02a13ee0b7835e0805c4319fd21d8 --- M src/console/console.c M src/console/init.c M src/console/printk.c M src/include/console/console.h 4 files changed, 22 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/61613/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/61613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1d5b8bda80d02a13ee0b7835e0805c4319fd21d8 Gerrit-Change-Number: 61613 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: console: Add ANSI escape sequences for highlighting
by Julius Werner (Code Review) 04 Feb '22

04 Feb '22

1 0

Change in coreboot[master]: console: Add loglevel prefix to interactive consoles
by Julius Werner (Code Review) 04 Feb '22

04 Feb '22

Attention is currently required from: Raul Rangel, Nico Huber, Patrick Georgi, Kyösti Mälkki. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61306 ) Change subject: console: Add loglevel prefix to interactive consoles ...................................................................... Patch Set 5: (3 comments) File src/console/printk.c: https://review.coreboot.org/c/coreboot/+/61306/comment/fd65199e_06322cfc PS5, Line 80: console_interactive_tx_byte > Might be cleaner to add a `static interactive_printk(fmt, args)`. […] Yeah... well you can't build a va_list from scratch in C (and you shouldn't try to fake it with a pointer because the format can be architecture-dependent), but I can write another vararg wrapper just for this here. I guess it does look a bit cleaner... I'm just worried because each of these things adds a little bit of overhead again and I was trying to be explicitly lightweight with my changes to such a central piece of code. Putting the vtxprintf() here means it parses the format string every time even if there's no interactive console to print to. I guess if you do have an interactive console, that level of overhead wouldn't matter. And if you don't, it would be best if I could compile-time eliminate all of this anyway. I'll do that in a follow-up patch. https://review.coreboot.org/c/coreboot/+/61306/comment/dfdde803_8d092248 PS5, Line 80: console_interactive_tx_byte > Might be cleaner to add a `static interactive_printk(fmt, args)`. […] Done https://review.coreboot.org/c/coreboot/+/61306/comment/98e98463_2d7f47fc PS5, Line 85: > nit: Do we need two spaces? This was my personal preference choice after playing around with various distances... I feel that with one space, the prefixes visually merge too much into the log text and it makes it harder to read the messages you're actually interested in without getting distracted by the clutter on the left. I'm open to discuss alternatives but please run it and look at the output first so you know how it looks in practice. (If anything, I'd suggest rather going up to 3 spaces than back down to 1, though. I don't want to make the distance too large since I want to be conscious of people with small terminals, too.) -- To view, visit https://review.coreboot.org/c/coreboot/+/61306 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic83413475400821f8097ef1819a293ee8926bb0b Gerrit-Change-Number: 61306 Gerrit-PatchSet: 5 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Fri, 04 Feb 2022 02:05:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: console: Add compile-time fast path when only CBMEM console is used
by Julius Werner (Code Review) 04 Feb '22

04 Feb '22

Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/61613 ) Change subject: console: Add compile-time fast path when only CBMEM console is used ...................................................................... console: Add compile-time fast path when only CBMEM console is used A common use case when running coreboot on production systems is that only the CBMEM console (the one with the least impact on boot speed) is enabled. In this case, some of the code in the console subsystem has no effect. Due to the way it's all genericized over multiple consoles and tied together with function pointers, not all of this can be compile-time eliminated automatically, so this patch adds a little helper to facilitate that. This results in roughly 200 (compressed) bytes of savings per stage on an arm64 system. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I1d5b8bda80d02a13ee0b7835e0805c4319fd21d8 --- M src/console/console.c M src/console/init.c M src/console/printk.c M src/include/console/console.h 4 files changed, 23 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/61613/1 diff --git a/src/console/console.c b/src/console/console.c index 5b8a872..f37f120 100644 --- a/src/console/console.c +++ b/src/console/console.c @@ -11,6 +11,8 @@ #include <console/flash.h> #include <console/system76_ec.h> +/* Note: when adding a new console, make sure you update the definition of + HAS_ONLY_FAST_CONSOLES in <console.h>! */ void console_hw_init(void) { __cbmemc_init(); diff --git a/src/console/init.c b/src/console/init.c index 4f93997..da8d8f7 100644 --- a/src/console/init.c +++ b/src/console/init.c @@ -37,6 +37,9 @@ if (log_level < 0) return CONSOLE_LOG_NONE; + if (HAS_ONLY_FAST_CONSOLES) + return CONSOLE_LOG_FAST; + if (msg_level <= log_level) return CONSOLE_LOG_ALL; diff --git a/src/console/printk.c b/src/console/printk.c index d24dc9db..8db2c45 100644 --- a/src/console/printk.c +++ b/src/console/printk.c @@ -67,6 +67,8 @@ }; }; +#define LOG_FAST(state) (HAS_ONLY_FAST_CONSOLES || ((state).level == CONSOLE_LOG_FAST)) + static void wrap_interactive_printf(const char *fmt, ...) { va_list args; @@ -83,11 +85,11 @@ LOG_FAST mode, just write the marker to CBMC and exit -- the rest of this function implements the LOG_ALL case. */ unsigned char marker = BIOS_LOG_LEVEL_TO_MARKER(state.level); - if (state.speed == CONSOLE_LOG_FAST) { + if (LOG_FAST(state)) { __cbmemc_tx_byte(marker); return; } - console_stored_tx_byte(marker); + console_stored_tx_byte(marker, NULL); /* Interactive consoles get a `[DEBUG] ` style readable prefix, and potentially an escape sequence for highlighting. */ @@ -98,7 +100,7 @@ static void line_end(union log_state state) { - if (CONFIG(CONSOLE_USE_ANSI_ESCAPES) && state.speed != CONSOLE_LOG_FAST) + if (CONFIG(CONSOLE_USE_ANSI_ESCAPES) && !LOG_FAST(state)) wrap_interactive_printf(BIOS_LOG_ESCAPE_RESET); } @@ -115,7 +117,7 @@ line_started = true; } - if (state.speed == CONSOLE_LOG_FAST) + if (LOG_FAST(state)) __cbmemc_tx_byte(byte); else console_tx_byte(byte); @@ -138,7 +140,7 @@ console_time_run(); i = vtxprintf(wrap_putchar, fmt, args, state.as_ptr); - if (state.speed != CONSOLE_LOG_FAST) + if (LOG_FAST(state)) console_tx_flush(); console_time_stop(); diff --git a/src/include/console/console.h b/src/include/console/console.h index 5546aa9..e4090af 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -61,7 +61,18 @@ long console_time_get_and_reset(void); void console_time_report(void); +/* + * "Fast" basically means only the CBMEM console right now. This is used to still + * print debug messages there when loglevel disables the other consoles. It is also + * used to compile-time eliminate code paths that only affect "interactive" consoles + * (which are all "slow") when none of those are enabled. + */ enum { CONSOLE_LOG_NONE = 0, CONSOLE_LOG_FAST, CONSOLE_LOG_ALL }; +#define HAS_ONLY_FAST_CONSOLES !(CONFIG(SPKMODEM) || CONFIG(CONSOLE_QEMU_DEBUGCON) || \ + CONFIG(CONSOLE_SERIAL) || CONFIG(CONSOLE_NE2K) || CONFIG(CONSOLE_USB) || \ + CONFIG(EM100PRO_SPI_CONSOLE) || CONFIG(CONSOLE_SPI_FLASH) || \ + CONFIG(CONSOLE_SYSTEM76_EC)) + #else static inline int get_log_level(void) { return -1; } static inline void console_init(void) {} -- To view, visit https://review.coreboot.org/c/coreboot/+/61613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1d5b8bda80d02a13ee0b7835e0805c4319fd21d8 Gerrit-Change-Number: 61613 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2022