coreboot-gerrit March 2024

coreboot-gerrit@coreboot.org

1 participants
3668 discussions

[S] Change in coreboot[main]: soc/intel/xeon_sp/spr: Enable x86_64 support
by Patrick Rudolph (Code Review) 16 Mar '24

16 Mar '24

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Lean Sheng Tan, Nico Huber, Shuo Liu, Tim Chu. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81280?usp=email ) Change subject: soc/intel/xeon_sp/spr: Enable x86_64 support ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/xeon_sp/uncore_acpi.c: https://review.coreboot.org/c/coreboot/+/81280/comment/7c091216_638629bc : PS1, Line 473: } > Btw. does somebody know what this RMRR entry is about? A workaround for edk2? […] It does what UEFI native code does. However it looks like a workaround for some legacy feature. Since the allocated memory reported by acpi_create_dmar_rmrr() is never used, neither in coreboot nor in the UEFI native case, it might just be dead code? I guess it's needed for PS/2 emulation done in SMM using the XHCI controller. -- To view, visit https://review.coreboot.org/c/coreboot/+/81280?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: I2c5ed0339a9c2e9b088b16dbb4c19df98e796d65 Gerrit-Change-Number: 81280 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Sat, 16 Mar 2024 10:14:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: amdfwtool: Set the table size only for FWs
by Bao Zheng (Code Review) 16 Mar '24

16 Mar '24

Attention is currently required from: Bao Zheng, Felix Held, Fred Reitberger, Zheng Bao. Hello Felix Held, Fred Reitberger, Zheng Bao, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81255?usp=email to look at the new patch set (#7). The following approvals got outdated and were removed: Code-Review+2 by Felix Held, Verified+1 by build bot (Jenkins) The change is no longer submittable: Code-Review and Verified are unsatisfied now. Change subject: amdfwtool: Set the table size only for FWs ...................................................................... amdfwtool: Set the table size only for FWs The entry in the table has two categaries, file and pointer. For the pointer, it does not take table space. The ISH, PSP level 2, BIOS table are all the pointer type. So integration function only packs FWs located in folder amd_blobs. And only FWs increase the table size. So the table size is only set once. Later calls only update the count and fletcher. The table has a header at least, so the size can not be 0. The fill_dir_header can take the parameter count as 0, such PSP level 1 only with ISH-A and ISH-B. It doesn't have any file type entries. This actually reverts https://review.coreboot.org/c/coreboot/+/78274 and adds other changes. TEST=Identical test on all AMD SOC platform Change-Id: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M util/amdfwtool/amdfwtool.c 1 file changed, 38 insertions(+), 41 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/81255/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/81255?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: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Gerrit-Change-Number: 81255 Gerrit-PatchSet: 7 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Attention: Zheng Bao Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[XS] Change in coreboot[main]: drivers/intel/gma/acpi: Limit OpRegion size
by Patrick Rudolph (Code Review) 16 Mar '24

16 Mar '24

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81295?usp=email ) Change subject: drivers/intel/gma/acpi: Limit OpRegion size ...................................................................... drivers/intel/gma/acpi: Limit OpRegion size Limit the ACPI OpRegion to cover only MBOX3. This seems to fix BSOD errors seen on Windows 10/11 as reported at [1]. 1: https://ticket.coreboot.org/issues/327 Change-Id: Ia2affa799e5cd84c0a03330e0f78919755f0e8ac Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M src/drivers/intel/gma/acpi/configure_brightness_levels.asl 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/81295/1 diff --git a/src/drivers/intel/gma/acpi/configure_brightness_levels.asl b/src/drivers/intel/gma/acpi/configure_brightness_levels.asl index 3a097e3..888c241 100644 --- a/src/drivers/intel/gma/acpi/configure_brightness_levels.asl +++ b/src/drivers/intel/gma/acpi/configure_brightness_levels.asl @@ -11,7 +11,7 @@ { Name (_ADR, 0) - OperationRegion (OPRG, SystemMemory, ASLS, 0x2000) + OperationRegion (OPRG, SystemMemory, ASLS, 0x400) Field (OPRG, DWordAcc, NoLock, Preserve) { // OpRegion Header -- To view, visit https://review.coreboot.org/c/coreboot/+/81295?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: Ia2affa799e5cd84c0a03330e0f78919755f0e8ac Gerrit-Change-Number: 81295 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

1 0

[XS] Change in coreboot[main]: mb/google/nissa/var/anraggar: Assigned GPP_C0 as TouchScreen enable pin
by Qinghong Zeng (Code Review) 16 Mar '24

16 Mar '24

Qinghong Zeng has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81265?usp=email ) Change subject: mb/google/nissa/var/anraggar: Assigned GPP_C0 as TouchScreen enable pin ...................................................................... mb/google/nissa/var/anraggar: Assigned GPP_C0 as TouchScreen enable pin Modify it to GPP_C0 and enable only the touchscreen, which can effectively reduce the power consumption of the touchscreen during Suspend. BUG=b:304920262 TEST= touchscreen function workable Change-Id: Ia06209aa8303be4fc0669c5d6e5d7a06e8e9ab99 Signed-off-by: Qinghong Zeng <zengqinghong(a)huaqin.corp-partner.google.com> --- M src/mainboard/google/brya/variants/anraggar/overridetree.cb 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/81265/1 diff --git a/src/mainboard/google/brya/variants/anraggar/overridetree.cb b/src/mainboard/google/brya/variants/anraggar/overridetree.cb index 77ae2b6..8313919 100644 --- a/src/mainboard/google/brya/variants/anraggar/overridetree.cb +++ b/src/mainboard/google/brya/variants/anraggar/overridetree.cb @@ -239,7 +239,7 @@ register "generic.detect" = "1" register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_C1)" register "generic.reset_delay_ms" = "200" - register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C6)" + register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_C0)" register "generic.enable_delay_ms" = "12" register "generic.stop_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_C6)" register "generic.stop_off_delay_ms" = "200" -- To view, visit https://review.coreboot.org/c/coreboot/+/81265?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: Ia06209aa8303be4fc0669c5d6e5d7a06e8e9ab99 Gerrit-Change-Number: 81265 Gerrit-PatchSet: 1 Gerrit-Owner: Qinghong Zeng <zengqinghong(a)huaqin.corp-partner.google.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[main]: arch/riscv: add new SBI calls
by ron minnich (Code Review) 15 Mar '24

15 Mar '24

ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81294?usp=email ) Change subject: arch/riscv: add new SBI calls ...................................................................... arch/riscv: add new SBI calls This is just a start. We are playing catch up. 7 down, 70+ to go. Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> clang-formatted-by: Ronald G Minnich Change-Id: I5dac8613020e26ec74ac1c74158fc9791553693f --- M src/arch/riscv/sbi.c 1 file changed, 80 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/81294/1 diff --git a/src/arch/riscv/sbi.c b/src/arch/riscv/sbi.c index 8bf21b7..fd6d1d3 100644 --- a/src/arch/riscv/sbi.c +++ b/src/arch/riscv/sbi.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <console/console.h> #include <mcall.h> #include <stdint.h> #include <arch/exception.h> @@ -49,6 +50,52 @@ return 0; } +static void print_sbi_trap(uintptr_t trap) +{ + switch (trap) { + case SBI_SHUTDOWN: + printk(BIOS_EMERG, "SBI_SHUTDOWN\n"); + break; + case SBI_REMOTE_SFENCE_VMA_ASID: + printk(BIOS_EMERG, "SBI_REMOTE_SFENCE_VMA_ASID\n"); + break; + case SBI_REMOTE_SFENCE_VMA: + printk(BIOS_EMERG, "SBI_REMOTE_SFENCE_VMA\n"); + break; + case SBI_REMOTE_FENCE_I: + printk(BIOS_EMERG, "SBI_REMOTE_FENCE_I\n"); + break; + case SBI_SEND_IPI: + printk(BIOS_EMERG, "SBI_SEND_IPI\n"); + break; + case SBI_CLEAR_IPI: + printk(BIOS_EMERG, "SBI_CLEAR_IPI\n"); + break; + case SBI_CONSOLE_GETCHAR: + printk(BIOS_EMERG, "SBI_CONSOLE_GETCHAR\n"); + break; + case SBI_CONSOLE_PUTCHAR: + printk(BIOS_EMERG, "SBI_CONSOLE_PUTCHAR\n"); + break; + case SBI_SET_TIMER: + printk(BIOS_EMERG, "SBI_SET_TIMER\n"); + break; + case SBI_EXTENSION_ID: + printk(BIOS_EMERG, "SBI_EXTENSION_ID\n"); + break; + default: + printk(BIOS_EMERG, "%lx is an unknown SBI trap\n", trap); + break; + } +} + +/* These are the default sbi extension values. + * They can be overridden in the case of an older SoC like the FU[57]40 + * 1 is the most basic SBI, per the standard; per the standard, + * all the other values can be 0. + */ +int __weak sbi_features[] = {1, 0, 0, 0, 0, 0, 0}; + /* * sbi is triggered by the s-mode ecall * parameter : register a0 a1 a2 @@ -58,11 +105,24 @@ void handle_sbi(struct trapframe *tf) { uintptr_t ret = 0; + uintptr_t sbiret = 0; uintptr_t arg0 = tf->gpr[10]; __maybe_unused uintptr_t arg1 = tf->gpr[11]; - uintptr_t which = tf->gpr[17]; + uintptr_t fid = tf->gpr[16]; + uintptr_t eid = tf->gpr[17]; + uintptr_t retpc = read_csr(mepc) + 4; - switch (which) { + /* + * These are for when things are going seriously wrong. + * In practice we don't want them compiled in, + * hence the if(0). + */ + if (0) { + printk(BIOS_EMERG, "%s\n", __func__); + print_sbi_trap(eid); + } + + switch (eid) { case SBI_SET_TIMER: #if __riscv_xlen == 32 ret = sbi_set_timer(arg0 + ((uint64_t)arg1 << 32)); @@ -96,10 +156,25 @@ case SBI_SHUTDOWN: ret = send_ipi((uintptr_t *)arg0, IPI_SHUTDOWN); break; + case SBI_EXTENSION_ID: + /* zero is an allowed return value for most features, + * and the only required one is feature 0. + * So this test will return legal values + * for all possible values of fid. + */ + if (fid < ARRAY_SIZE(sbi_features)) + ret = sbi_features[fid]; + break; default: - ret = -SBI_ENOSYS; + printk(BIOS_EMERG, "SBI: %lx: ENOSYS\n", fid); + sbiret = -SBI_ENOSYS; break; } - tf->gpr[10] = ret; - write_csr(mepc, read_csr(mepc) + 4); + tf->gpr[10] = sbiret; + tf->gpr[11] = ret; + + if (0) + printk(BIOS_EMERG, "%s:returning [%lx,%lx] to %lx\n", __func__, sbiret, ret, retpc); + + write_csr(mepc, retpc); } -- To view, visit https://review.coreboot.org/c/coreboot/+/81294?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: I5dac8613020e26ec74ac1c74158fc9791553693f Gerrit-Change-Number: 81294 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[main]: WIP: arch/riscv: add test to set variable for menvcfg
by ron minnich (Code Review) 15 Mar '24

15 Mar '24

ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81293?usp=email ) Change subject: WIP: arch/riscv: add test to set variable for menvcfg ...................................................................... WIP: arch/riscv: add test to set variable for menvcfg there are many variant features on SoCs. This CL demonstrates a simple way to test feature existence with low effort. It correctly detects the menvcfg register. Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> Change-Id: If9f5db74d7467ab00044d20050531bcb511cce39 clang-formatted-by: Ronald G Minnich --- M src/arch/riscv/include/arch/encoding.h M src/arch/riscv/payload.c M src/arch/riscv/ramstage.S 3 files changed, 21 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/81293/1 diff --git a/src/arch/riscv/include/arch/encoding.h b/src/arch/riscv/include/arch/encoding.h index 4f01e5c..6ab38bb 100644 --- a/src/arch/riscv/include/arch/encoding.h +++ b/src/arch/riscv/include/arch/encoding.h @@ -800,6 +800,8 @@ #define CSR_MIE 0x304 #define CSR_MTVEC 0x305 #define CSR_MCOUNTEREN 0x306 +#define CSR_MENVCFG 0x30a +#define CSR_MENVCFGH 0x31a #define CSR_MSCRATCH 0x340 #define CSR_MEPC 0x341 #define CSR_MCAUSE 0x342 @@ -1292,6 +1294,8 @@ DECLARE_CSR(mie, CSR_MIE) DECLARE_CSR(mtvec, CSR_MTVEC) DECLARE_CSR(mcounteren, CSR_MCOUNTEREN) +DECLARE_CSR(menvcfg, CSR_MENVCFG) +DECLARE_CSR(menvcfgh, CSR_MENVCFGH) DECLARE_CSR(mscratch, CSR_MSCRATCH) DECLARE_CSR(mepc, CSR_MEPC) DECLARE_CSR(mcause, CSR_MCAUSE) diff --git a/src/arch/riscv/payload.c b/src/arch/riscv/payload.c index 7c6e0f4..636462f 100644 --- a/src/arch/riscv/payload.c +++ b/src/arch/riscv/payload.c @@ -11,6 +11,8 @@ #include <mcall.h> #include <vm.h> +uintptr_t has_menvcfg; + /* Run OpenSBI and let OpenSBI hand over control to the payload */ void run_payload_opensbi(struct prog *prog, void *fdt, struct prog *opensbi, int payload_mode) { @@ -68,6 +70,9 @@ */ close_pmp(); + printk(BIOS_EMERG, "has_menvcfg %ld\n", has_menvcfg); + if (has_menvcfg) { + } status = INSERT_FIELD(status, MSTATUS_MPP, PRV_S); /* Trap vector base address point to the payload */ write_csr(stvec, doit); diff --git a/src/arch/riscv/ramstage.S b/src/arch/riscv/ramstage.S index 954b155..0557d8c 100644 --- a/src/arch/riscv/ramstage.S +++ b/src/arch/riscv/ramstage.S @@ -35,6 +35,16 @@ #NOTE a1 contains FDT and should not be cluttered above call hls_init + # determine HART properties. It is easiest done here. + la t1, has_menvcfg + li t3, 1 + STORE x0, 0(t1) + la t0, 1f + csrw mtvec, t0 + csrs menvcfg, t0 + STORE t3, 0(t1) +1: + li a0, CONFIG_RISCV_WORKING_HARTID call smp_pause @@ -54,3 +64,5 @@ .weak exit_car exit_car: ret + +.globl has_menvcfg -- To view, visit https://review.coreboot.org/c/coreboot/+/81293?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: If9f5db74d7467ab00044d20050531bcb511cce39 Gerrit-Change-Number: 81293 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[main]: mb/google/brox: support ISH
by Li1 Feng (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: Ashish Kumar Mishra, Ashish Kumar Mishra, Bernardo Perez Priego, Karthik Ramasubramanian, Shelley Chen, Tanu Malhotra, Yuval Peress. Li1 Feng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80773?usp=email ) Change subject: mb/google/brox: support ISH ...................................................................... Patch Set 7: (1 comment) File src/mainboard/google/brox/variants/brox/overridetree.cb: https://review.coreboot.org/c/coreboot/+/80773/comment/65ac85d0_57cbc969 : PS5, Line 297: chip drivers/intel/ish : register "firmware_name" = ""brox_ish.bin"" : device generic 0 on : probe ISH ISH_ENABLE : end : end : chip drivers/intel/ish : register "add_acpi_dma_property" = "true" : device generic 0 on : probe STORAGE STORAGE_UFS : end : end > ISH FW for UFS support is different from ISH FW for full sensing capability. […] @Karthik done. -- To view, visit https://review.coreboot.org/c/coreboot/+/80773?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: Iadc5108c62737d27642a6948c00b5c122541aaba Gerrit-Change-Number: 80773 Gerrit-PatchSet: 7 Gerrit-Owner: Li1 Feng <li1.feng(a)intel.com> Gerrit-Reviewer: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-Reviewer: Ashish Kumar Mishra <ashish.k.mishra(a)intel.corp-partner.google.com> Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Tanu Malhotra <tanu.malhotra(a)intel.com> Gerrit-Reviewer: Yuval Peress <peress(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Yuval Peress <peress(a)google.com> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-Attention: Tanu Malhotra <tanu.malhotra(a)intel.com> Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.corp-partner.google.com> Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Sat, 16 Mar 2024 04:41:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tanu Malhotra <tanu.malhotra(a)intel.com> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: genbuild_h: Fix and harden major/minor version parsing
by Maximilian Brune (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: Martin L Roth, Nico Huber. Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81290?usp=email ) Change subject: genbuild_h: Fix and harden major/minor version parsing ...................................................................... Patch Set 1: (2 comments) File util/genbuild_h/genbuild_h.sh: https://review.coreboot.org/c/coreboot/+/81290/comment/7b85f4fe_b16ac903 : PS1, Line 39: MAJOR_VER="$(echo "${VERSION}" | sed -n 's/^0*$[0-9]*$\.0*$[0-9]*$.*/\1/p')" : MINOR_VER="$(echo "${VERSION}" | sed -n 's/^0*$[0-9]*$\.0*$[0-9]*$.*/\2/p')" > Would that be posix conform? The `-E` is posix compliant extended regex. `-r` would be the non posix compliant extended regex (although I am not sure if or how much they actually differ, but to be safe lets stay with `-E`). https://review.coreboot.org/c/coreboot/+/81290/comment/4c9ffbb4_b7cf235e : PS1, Line 40: * > Can you give an example? I wouldn't be sure how to do that and handle leading […] We only need the leading zero for the month since the year will never have one. But I forgot about the month, which may have a leading zero. But we could replace the `*` with a `+` for the month, because we will always have at least one number for the minor version (month). ``` 's/^([0-9]{2})\.0*([0-9]+).*/\2/p' ``` Regarding other version formats as described in the commit: The substitution that you have currently in place also does not work for the version tag described in the commit-msg ("v1.9308_26_0.0.22"), because your substitution requires a version number to be at the start (`^`) of the line, which means a `v` at the beginning is not valid. We would need something like this: ``` s/^v*0*\([0-9]... ``` But honestly I am not sure how far we want to stretch the support for people creating tags with their own custom version format, especially now after we have completely changed it. If it was up to me I would just force our format for the tags and let others deal with their special cases. -- To view, visit https://review.coreboot.org/c/coreboot/+/81290?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: Ie39381a8ef4b971556168b6996efeefe6adf2b14 Gerrit-Change-Number: 81290 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Comment-Date: Sat, 16 Mar 2024 03:11:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: genbuild_h: Fix and harden major/minor version parsing
by Nico Huber (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: Martin L Roth, Maximilian Brune. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81290?usp=email ) Change subject: genbuild_h: Fix and harden major/minor version parsing ...................................................................... Patch Set 1: (2 comments) File util/genbuild_h/genbuild_h.sh: https://review.coreboot.org/c/coreboot/+/81290/comment/38f42c7f_07e92d8a : PS1, Line 39: MAJOR_VER="$(echo "${VERSION}" | sed -n 's/^0*$[0-9]*$\.0*$[0-9]*$.*/\1/p')" : MINOR_VER="$(echo "${VERSION}" | sed -n 's/^0*$[0-9]*$\.0*$[0-9]*$.*/\2/p')" > we could gain some readability if we used extended regex `sed -E` and removed the `\` from the chara […] Would that be posix conform? https://review.coreboot.org/c/coreboot/+/81290/comment/4aa3eeed_bd2cec01 : PS1, Line 40: * > nit: we could be more explicit and write {2} instead of * (for year and for month) Can you give an example? I wouldn't be sure how to do that and handle leading zeros. Also, would that work with other version formats that people might use locally? (see commit that added the comment and `if` that I removed) -- To view, visit https://review.coreboot.org/c/coreboot/+/81290?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: Ie39381a8ef4b971556168b6996efeefe6adf2b14 Gerrit-Change-Number: 81290 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Comment-Date: Sat, 16 Mar 2024 02:28:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: acpi/acpi: mark CTBL coreboot table device as hidden
by Matt DeVillier (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: Cliff Huang, Felix Held, Lance Zhao, Matt DeVillier, Tim Wawrzynczak. Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81264?usp=email ) Change subject: acpi/acpi: mark CTBL coreboot table device as hidden ...................................................................... Patch Set 1: Code-Review-1 (1 comment) Patchset: PS1: this breaks attachment of coolstar's Windows drivers, which allow non-admin cbmem console access via ported cbmem util -- To view, visit https://review.coreboot.org/c/coreboot/+/81264?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: Ifaefeb662da33323460333d9ca9c0e8340720fd1 Gerrit-Change-Number: 81264 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sat, 16 Mar 2024 02:25:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2024