coreboot-gerrit March 2024

coreboot-gerrit@coreboot.org

1 participants
3668 discussions

[M] Change in coreboot[main]: vc/amd/opensil/stub: add stub MPIO driver
by Matt DeVillier (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Felix Held, Martin L Roth, Varshit Pandya. Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81100?usp=email ) Change subject: vc/amd/opensil/stub: add stub MPIO driver ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/81100?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: Ib4f5c232859b9abcd10bfa5c21e2f2c3a70b4b0e Gerrit-Change-Number: 81100 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 08 Mar 2024 17:55:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: arch/riscv: use PMP
by ron minnich (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Philipp Hug. ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81152?usp=email ) Change subject: arch/riscv: use PMP ...................................................................... arch/riscv: use PMP With this change, a simple S-mode payload works: 1: li a7, 1 li a0, 48 ecall j 1b Without this change, it will not work. Linux also boots with this on the command line: clk_ignore_unused Resolving that problem will require a separate patch. Getting this to build on RV32 required changes to the API, as it was incorrect. In RV32, PMP entries are 34 bits. Hence, the setup_pmp needed to accept u64. So, uinptr_t can not be used, as on 32 bits they are only 32 bit numbers. The internal API uses uintptr_t, but the exported API uses u64, so external code does not have to think about right shifts on base and size. Another issue is that there is not requirement that PMP address registers implement all bits. So, for example, on the SiFive Fu740, only 32 bits are implemented, which means 34 bits of address, which means 16 GiB. Further, on the SiFive FU740, the PMPcfg MUST be written first, or the PMP address will be changed when written. Errors are detected: an error in base and size will result in a BIOS_EMERG print, but not a panic. Boots not bricks if possible. There are small changes to the internal API to reduce stack pressure: there's no need to have two pmpcfg_t on the stack when one will do. Change-Id: I8d7dd171ee69e83f3b904df38c7e2d36cc46a62e Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> --- M src/arch/riscv/include/arch/pmp.h M src/arch/riscv/payload.c M src/arch/riscv/pmp.c 3 files changed, 136 insertions(+), 55 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/81152/1 diff --git a/src/arch/riscv/include/arch/pmp.h b/src/arch/riscv/include/arch/pmp.h index b25fc96..f98adf7 100644 --- a/src/arch/riscv/include/arch/pmp.h +++ b/src/arch/riscv/include/arch/pmp.h @@ -14,7 +14,13 @@ /* reset PMP setting */ void reset_pmp(void); -/* set up PMP record */ -void setup_pmp(uintptr_t base, uintptr_t size, uintptr_t flags); +/* + * set up PMP record + * reminder: base and size are 34-bits on RV32. + */ +void setup_pmp(u64 base, u64 size, u8 flags); + +/* write the last PMP record, i.e. the "default" case. */ +void close_pmp(void); #endif /* __RISCV_PMP_H__ */ diff --git a/src/arch/riscv/payload.c b/src/arch/riscv/payload.c index a7a7dcb..a80b305 100644 --- a/src/arch/riscv/payload.c +++ b/src/arch/riscv/payload.c @@ -1,21 +1,18 @@ /* SPDX-License-Identifier: GPL-2.0-only */ #include <delay.h> +#include <cbmem.h> #include <program_loading.h> #include <stdint.h> #include <arch/boot.h> #include <arch/encoding.h> +#include <arch/pmp.h> #include <arch/smp/atomic.h> #include <console/console.h> #include <mcall.h> #include <vm.h> #include <arch/pmp.h> -int pmp_entries_num(void) -{ - return 16; -} - /* 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) { @@ -61,6 +58,7 @@ void (*doit)(int hart_id, void *fdt) = prog_entry(prog); int hart_id = read_csr(mhartid); uintptr_t status = read_csr(mstatus); + extern void *_text, *_estack; status = INSERT_FIELD(status, MSTATUS_MPIE, 0); mdelay(1000 * hart_id); @@ -69,11 +67,31 @@ case RISCV_PAYLOAD_MODE_S: mstatus_init(); reset_pmp(); - // setup_pmp(0x84000000, 0xc0000000-0x84000000, PMP_R | PMP_W | PMP_X); - setup_pmp(0x20000000, 1<<10, 0); - setup_pmp(0x80000000, 1<<12, 0); - setup_pmp(0, 0xffffffffffffffff+1, PMP_R | PMP_W | PMP_X); status = 0; + /* + * Set up a PMP to protect coreboot, then close the PMPs. + * If a mainboard or SoC needs other ranges + * set up, they should do so before this point, + * as close_pmp puts in a "match all" entry, and + * PMPs are processed in linear order. + */ + + /* + * On this code path, coreboot is providing the coreboot SBI, and must + * protect the ramstage, from _text to _estack, from S and U + * modes. Because the number of PMP registers may be very + * small, make this an NAPOT area. The linker scripts + * should round _text and _estack to 4K. + */ + setup_pmp((u64)(uintptr_t) _text, + (u64)(uintptr_t) _estack - (u64)(uintptr_t) _text, 0); + + /* + * All pmp operations should be finished when close_pmp is called. + * Presently, this requirement is not enforced. + */ + close_pmp(); + status = INSERT_FIELD(status, MSTATUS_MPP, PRV_S); /* Trap vector base address point to the payload */ diff --git a/src/arch/riscv/pmp.c b/src/arch/riscv/pmp.c index 68dcc0f..08b3493 100644 --- a/src/arch/riscv/pmp.c +++ b/src/arch/riscv/pmp.c @@ -28,6 +28,12 @@ /* This variable is used to record which entries have been used. */ static uintptr_t pmp_entry_used_mask; +/* The architectural spec says that up to 16 PMP entries are available. */ +int pmp_entries_num(void) +{ + return 16; +} + /* helper function used to read pmpcfg[idx] */ static uintptr_t read_pmpcfg(int idx) { @@ -96,19 +102,20 @@ new = (old & ~((uintptr_t)0xff << shift)) | ((cfg & 0xff) << shift); write_csr(pmpcfg0, new); - printk(BIOS_EMERG, "%s: cfg %lx old %lx new %lx\n", __func__, cfg, old, new); + printk(BIOS_INFO, "%s(%d, %lx) = %lx\n", __func__, idx, cfg, read_csr(pmpcfg0)); break; case 1: old = read_csr(pmpcfg2); new = (old & ~((uintptr_t)0xff << shift)) | ((cfg & 0xff) << shift); write_csr(pmpcfg2, new); + printk(BIOS_INFO, "%s(%d, %lx) = %lx\n", __func__, idx, cfg, read_csr(pmpcfg2)); break; } #endif if (read_pmpcfg(idx) != cfg) { - printk(BIOS_EMERG, "%s: read(%d) is %lx, not %lx\n", __func__, idx, read_pmpcfg(idx), cfg); - //die("write pmpcfg failure!"); + printk(BIOS_WARNING, "%s: PMPcfg%d: Wrote %lx, read %lx\n", __func__, idx, cfg, read_pmpcfg(idx)); + die("PMPcfg write failed"); } } @@ -205,50 +212,64 @@ write_csr(pmpaddr15, val); break; } - if (read_pmpaddr(idx) != val){ - printk(BIOS_EMERG, "%s: read(%d) is %lx, not %lx\n", __func__, idx, read_pmpaddr(idx), val); - //die("write pmpaddr failure"); + + printk(BIOS_INFO, "%s(%d, %lx) = %lx\n", __func__, idx, val, read_pmpaddr(idx)); + /* The PMP is not required to return what we wrote. On some SoC, many bits are cleared. */ + if (read_pmpaddr(idx) != val) { + printk(BIOS_WARNING, "%s: PMPaddr%d: Wrote %lx, read %lx\n", __func__, + idx, val, read_pmpaddr(idx)); } } +/* Generate a PMP configuration for all memory */ +static void generate_pmp_all(pmpcfg_t *p) +{ + p->cfg = PMP_NAPOT | PMP_R | PMP_W | PMP_X; + p->previous_address = 0; + p->address = (uintptr_t) -1; +} + /* Generate a PMP configuration of type NA4/NAPOT */ -static pmpcfg_t generate_pmp_napot( - uintptr_t base, uintptr_t size, uintptr_t flags) +static void generate_pmp_napot(pmpcfg_t *p, uintptr_t base, uintptr_t size, u8 flags) { - pmpcfg_t p; flags = flags & (PMP_R | PMP_W | PMP_X | PMP_L); - if (size > GRANULE) { - p.cfg = flags | PMP_NAPOT; - p.previous_address = 0; - p.address = (base + (size / 2 - 1)) >> PMP_SHIFT; - return p; - } - - p.cfg = flags | PMP_NA4; - p.previous_address = 0; - p.address = (base + (size / 2 - 1)) >> PMP_SHIFT; - return p; + p->cfg = flags | (size > GRANULE ? PMP_NAPOT : PMP_NA4); + p->previous_address = 0; + p->address = (base + (size / 2 - 1)); } /* Generate a PMP configuration of type TOR */ -static pmpcfg_t generate_pmp_range( - uintptr_t base, uintptr_t size, uintptr_t flags) +static void generate_pmp_range(pmpcfg_t *p, uintptr_t base, uintptr_t size, u8 flags) { - pmpcfg_t p; flags = flags & (PMP_R | PMP_W | PMP_X | PMP_L); - p.cfg = flags | PMP_TOR; - p.previous_address = base >> PMP_SHIFT; - p.address = (base + size) >> PMP_SHIFT; - return p; + p->cfg = flags | PMP_TOR; + p->previous_address = base; + p->address = (base + size); } -/* Generate a PMP configuration */ -static pmpcfg_t generate_pmp(uintptr_t base, uintptr_t size, uintptr_t flags) +/* + * Generate a PMP configuration. + * reminder: base and size are 34 bit numbers on RV32. + */ +static int generate_pmp(pmpcfg_t *p, u64 base, u64 size, u8 flags) { - if (IS_POWER_OF_2(size) && (size >= 4) && ((base & (size - 1)) == 0)) - return generate_pmp_napot(base, size, flags); - else - return generate_pmp_range(base, size, flags); + uintptr_t b = (uintptr_t) base >> PMP_SHIFT, s = (uintptr_t) size >> PMP_SHIFT; +#if __riscv_xlen == 32 + /* verify that base + size fits in 34 bits */ + if ((base + size - 1) >> 34) { + printk(BIOS_EMERG, "%s: base (%llx) + size (%llx) - 1 is more than 34 bits\n", + __func__, base, size); + return 1; + } +#endif + if (base == (u64)-1) { + generate_pmp_all(p); + } else if (IS_POWER_OF_2(size) && (size >= 4) && ((base & (size - 1)) == 0)) { + generate_pmp_napot(p, b, s, flags); + } else { + generate_pmp_range(p, b, s, flags); + } + return 0; } /* @@ -289,38 +310,74 @@ /* reset PMP setting */ void reset_pmp(void) { - printk(BIOS_EMERG, "%s: start\n", __func__); for (int i = 0; i < pmp_entries_num(); i++) { if (read_pmpcfg(i) & PMP_L) - die("Some PMP configurations are locked " - "and cannot be reset!"); + die("Some PMP configurations are locked and cannot be reset!"); write_pmpcfg(i, 0); write_pmpaddr(i, 0); } - printk(BIOS_EMERG, "%s: done\n", __func__); } -/* set up PMP record */ -void setup_pmp(uintptr_t base, uintptr_t size, uintptr_t flags) +/* + * set up PMP record + * Why are these u64 and not uintptr_t? + * because, per the spec: + * The Sv32 page-based virtual-memory scheme described in Section 4.3 + * supports 34-bit physical addresses for RV32, so the PMP scheme must + * support addresses wider than XLEN for RV32. + * Yes, in RV32, these are 34-bit numbers. + * Rather than require every future user of these to remember that, + * this ABI is 64 bits. + * generate_pmp will check for out of range values. + */ +void setup_pmp(u64 base, u64 size, u8 flags) { pmpcfg_t p; int is_range, n; - printk(BIOS_EMERG, "%s: start base %lx size %lx flags %lx\n", __func__, base, size, flags); - p = generate_pmp(base, size, flags); + if (generate_pmp(&p, base, size, flags)) + return; + is_range = ((p.cfg & PMP_A) == PMP_TOR); n = find_empty_pmp_entry(is_range); + /* + * NOTE! you MUST write the cfg register first, or on (e.g.) + * the SiFive FU740, it will not take all the bits. + * This is different than QEMU. NASTY! + */ + write_pmpcfg(n, p.cfg); + write_pmpaddr(n, p.address); if (is_range) write_pmpaddr(n - 1, p.previous_address); - printk(BIOS_EMERG, "%s: p.cfg is %lx\n", __func__, p.cfg); - write_pmpcfg(n, p.cfg); mask_pmp_entry_used(n); if (is_range) mask_pmp_entry_used(n - 1); - printk(BIOS_EMERG, "%s: done\n", __func__); } +/* + * close_pmp will "close" the pmp. + * This consists of adding the "match every address" entry. + * This should be the last pmp function that is called. + * Because we can not be certain that there is not some reason for it + * NOT to be last, we do not check -- perhaps, later, a check would + * make sense, but, for now, we do not check. + * If previous code has used up all pmp entries, print a warning + * and continue. + * The huge constant for the memory size may seem a bit odd here. + * Recall that PMP is to protect a *limited* number of M mode + * memory ranges from S and U modes. Therefore, the last range + * entry should cover all possible addresses, up to + * an architectural limit. It is entirely acceptable + * for it to cover memory that does not exist -- PMP + * protects M mode, nothing more. + * Think of this range as the final catch-all else + * in an if-then-else. + */ +void close_pmp(void) +{ + setup_pmp((u64)-1, 0, PMP_R|PMP_W|PMP_X); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/81152?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8d7dd171ee69e83f3b904df38c7e2d36cc46a62e Gerrit-Change-Number: 81152 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: checkpoint commit
by ron minnich (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Philipp Hug. ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81131?usp=email ) Change subject: checkpoint commit ...................................................................... checkpoint commit Change-Id: I013663c7b68abf7b3b8a9dd89d9a585445fee3f1 Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> --- M src/arch/riscv/boot.c M src/arch/riscv/payload.c M src/arch/riscv/pmp.c M src/arch/riscv/virtual_memory.c 4 files changed, 42 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/81131/1 diff --git a/src/arch/riscv/boot.c b/src/arch/riscv/boot.c index ea22774..ebdee88 100644 --- a/src/arch/riscv/boot.c +++ b/src/arch/riscv/boot.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <delay.h> #include <program_loading.h> #include <vm.h> #include <arch/boot.h> @@ -50,6 +51,11 @@ void arch_prog_run(struct prog *prog) { struct arch_prog_run_args args = {}; + int hart_id = HLS()->hart_id; + + for (;hart_id != CONFIG_RISCV_WORKING_HARTID;) { + mdelay(1000); + } args.prog = prog; @@ -63,5 +69,6 @@ args.opensbi = &sbi; } + printk(BIOS_EMERG, "resuming hart %d\n", hart_id); smp_resume((void (*)(void *))do_arch_prog_run, &args); } diff --git a/src/arch/riscv/payload.c b/src/arch/riscv/payload.c index 3c92c0b..a7a7dcb 100644 --- a/src/arch/riscv/payload.c +++ b/src/arch/riscv/payload.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <delay.h> #include <program_loading.h> #include <stdint.h> #include <arch/boot.h> @@ -62,13 +63,17 @@ uintptr_t status = read_csr(mstatus); status = INSERT_FIELD(status, MSTATUS_MPIE, 0); + mdelay(1000 * hart_id); + printk(BIOS_EMERG, "%s: hart %d\n", __func__, hart_id); switch (payload_mode) { case RISCV_PAYLOAD_MODE_S: mstatus_init(); - //reset_pmp(); + reset_pmp(); // setup_pmp(0x84000000, 0xc0000000-0x84000000, PMP_R | PMP_W | PMP_X); - setup_pmp(0, (u64)256 * 1024 * 1024 * 1024, PMP_R | PMP_W | PMP_X); - //status = 0; + setup_pmp(0x20000000, 1<<10, 0); + setup_pmp(0x80000000, 1<<12, 0); + setup_pmp(0, 0xffffffffffffffff+1, PMP_R | PMP_W | PMP_X); + status = 0; status = INSERT_FIELD(status, MSTATUS_MPP, PRV_S); /* Trap vector base address point to the payload */ @@ -81,6 +86,7 @@ /* disable M-Mode interrupt */ write_csr(mie, 0); + if (0) { long menvcfg_val = read_csr(menvcfg); menvcfg_val |= ENVCFG_CBZE; menvcfg_val |= ENVCFG_CBCFE; @@ -91,6 +97,7 @@ printk(BIOS_DEBUG, "run_payload() envcfg: 0x%lx\n", menvcfg_val); write_csr(menvcfg, menvcfg_val); + } /* save stack to mscratch so trap_entry can use that as exception stack */ write_csr(mscratch, MACHINE_STACK_TOP()); @@ -108,6 +115,7 @@ } printk(BIOS_DEBUG, "run_payload(%p, %p) mstatus: 0x%lx\n", doit, fdt, status); + mdelay(5000); write_csr(mstatus, status); write_csr(mepc, doit); diff --git a/src/arch/riscv/pmp.c b/src/arch/riscv/pmp.c index ee39ac4..68dcc0f 100644 --- a/src/arch/riscv/pmp.c +++ b/src/arch/riscv/pmp.c @@ -96,6 +96,7 @@ new = (old & ~((uintptr_t)0xff << shift)) | ((cfg & 0xff) << shift); write_csr(pmpcfg0, new); + printk(BIOS_EMERG, "%s: cfg %lx old %lx new %lx\n", __func__, cfg, old, new); break; case 1: old = read_csr(pmpcfg2); @@ -105,8 +106,10 @@ break; } #endif - if (read_pmpcfg(idx) != cfg) - die("write pmpcfg failure!"); + if (read_pmpcfg(idx) != cfg) { + printk(BIOS_EMERG, "%s: read(%d) is %lx, not %lx\n", __func__, idx, read_pmpcfg(idx), cfg); + //die("write pmpcfg failure!"); + } } /* helper function used to read pmpaddr[idx] */ @@ -202,8 +205,10 @@ write_csr(pmpaddr15, val); break; } - if (read_pmpaddr(idx) != val) - die("write pmpaddr failure"); + if (read_pmpaddr(idx) != val){ + printk(BIOS_EMERG, "%s: read(%d) is %lx, not %lx\n", __func__, idx, read_pmpaddr(idx), val); + //die("write pmpaddr failure"); + } } /* Generate a PMP configuration of type NA4/NAPOT */ @@ -212,7 +217,14 @@ { pmpcfg_t p; flags = flags & (PMP_R | PMP_W | PMP_X | PMP_L); - p.cfg = flags | (size > GRANULE ? PMP_NAPOT : PMP_NA4); + if (size > GRANULE) { + p.cfg = flags | PMP_NAPOT; + p.previous_address = 0; + p.address = (base + (size / 2 - 1)) >> PMP_SHIFT; + return p; + } + + p.cfg = flags | PMP_NA4; p.previous_address = 0; p.address = (base + (size / 2 - 1)) >> PMP_SHIFT; return p; @@ -277,6 +289,7 @@ /* reset PMP setting */ void reset_pmp(void) { + printk(BIOS_EMERG, "%s: start\n", __func__); for (int i = 0; i < pmp_entries_num(); i++) { if (read_pmpcfg(i) & PMP_L) die("Some PMP configurations are locked " @@ -284,6 +297,7 @@ write_pmpcfg(i, 0); write_pmpaddr(i, 0); } + printk(BIOS_EMERG, "%s: done\n", __func__); } /* set up PMP record */ @@ -292,6 +306,7 @@ pmpcfg_t p; int is_range, n; + printk(BIOS_EMERG, "%s: start base %lx size %lx flags %lx\n", __func__, base, size, flags); p = generate_pmp(base, size, flags); is_range = ((p.cfg & PMP_A) == PMP_TOR); @@ -300,9 +315,12 @@ write_pmpaddr(n, p.address); if (is_range) write_pmpaddr(n - 1, p.previous_address); + printk(BIOS_EMERG, "%s: p.cfg is %lx\n", __func__, p.cfg); write_pmpcfg(n, p.cfg); mask_pmp_entry_used(n); if (is_range) mask_pmp_entry_used(n - 1); + printk(BIOS_EMERG, "%s: done\n", __func__); } + diff --git a/src/arch/riscv/virtual_memory.c b/src/arch/riscv/virtual_memory.c index 9f51618..dbfbf49 100644 --- a/src/arch/riscv/virtual_memory.c +++ b/src/arch/riscv/virtual_memory.c @@ -53,6 +53,7 @@ // Enable all user/supervisor-mode counters write_csr(mcounteren, 7); + return; write_csr(scounteren, 7); // Enable FPU, if available -- To view, visit https://review.coreboot.org/c/coreboot/+/81131?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I013663c7b68abf7b3b8a9dd89d9a585445fee3f1 Gerrit-Change-Number: 81131 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: arch/riscv: Add SMP support for exception handler
by ron minnich (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Philipp Hug, Xiang W. Hello Xiang W, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/81130?usp=email to review the following change. Change subject: arch/riscv: Add SMP support for exception handler ...................................................................... arch/riscv: Add SMP support for exception handler Change-Id: Ia1f97b82e329f6358061072f98278cf56b503618 Signed-off-by: Xiang Wang <merle(a)hardenedlinux.org> --- M src/arch/riscv/payload.c M src/arch/riscv/trap_util.S 2 files changed, 118 insertions(+), 96 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/81130/1 diff --git a/src/arch/riscv/payload.c b/src/arch/riscv/payload.c index 5c47b1e..3c92c0b 100644 --- a/src/arch/riscv/payload.c +++ b/src/arch/riscv/payload.c @@ -6,6 +6,7 @@ #include <arch/encoding.h> #include <arch/smp/atomic.h> #include <console/console.h> +#include <mcall.h> #include <vm.h> #include <arch/pmp.h> @@ -91,6 +92,8 @@ write_csr(menvcfg, menvcfg_val); + /* save stack to mscratch so trap_entry can use that as exception stack */ + write_csr(mscratch, MACHINE_STACK_TOP()); break; case RISCV_PAYLOAD_MODE_M: status = INSERT_FIELD(status, MSTATUS_MPP, PRV_M); diff --git a/src/arch/riscv/trap_util.S b/src/arch/riscv/trap_util.S index d7b1250..f5a048f 100644 --- a/src/arch/riscv/trap_util.S +++ b/src/arch/riscv/trap_util.S @@ -7,109 +7,121 @@ #include <mcall.h> .macro restore_regs - # restore x registers - LOAD x1,1*REGBYTES(a0) - LOAD x2,2*REGBYTES(a0) - LOAD x3,3*REGBYTES(a0) - LOAD x4,4*REGBYTES(a0) - LOAD x5,5*REGBYTES(a0) - LOAD x6,6*REGBYTES(a0) - LOAD x7,7*REGBYTES(a0) - LOAD x8,8*REGBYTES(a0) - LOAD x9,9*REGBYTES(a0) - LOAD x11,11*REGBYTES(a0) - LOAD x12,12*REGBYTES(a0) - LOAD x13,13*REGBYTES(a0) - LOAD x14,14*REGBYTES(a0) - LOAD x15,15*REGBYTES(a0) - LOAD x16,16*REGBYTES(a0) - LOAD x17,17*REGBYTES(a0) - LOAD x18,18*REGBYTES(a0) - LOAD x19,19*REGBYTES(a0) - LOAD x20,20*REGBYTES(a0) - LOAD x21,21*REGBYTES(a0) - LOAD x22,22*REGBYTES(a0) - LOAD x23,23*REGBYTES(a0) - LOAD x24,24*REGBYTES(a0) - LOAD x25,25*REGBYTES(a0) - LOAD x26,26*REGBYTES(a0) - LOAD x27,27*REGBYTES(a0) - LOAD x28,28*REGBYTES(a0) - LOAD x29,29*REGBYTES(a0) - LOAD x30,30*REGBYTES(a0) - LOAD x31,31*REGBYTES(a0) - # restore a0 last - LOAD x10,10*REGBYTES(a0) + # restore x registers + LOAD x1, 1 * REGBYTES(sp) + LOAD x3, 3 * REGBYTES(sp) + LOAD x4, 4 * REGBYTES(sp) + LOAD x5, 5 * REGBYTES(sp) + LOAD x6, 6 * REGBYTES(sp) + LOAD x7, 7 * REGBYTES(sp) + LOAD x8, 8 * REGBYTES(sp) + LOAD x9, 9 * REGBYTES(sp) + LOAD x10, 10 * REGBYTES(sp) + LOAD x11, 11 * REGBYTES(sp) + LOAD x12, 12 * REGBYTES(sp) + LOAD x13, 13 * REGBYTES(sp) + LOAD x14, 14 * REGBYTES(sp) + LOAD x15, 15 * REGBYTES(sp) + LOAD x16, 16 * REGBYTES(sp) + LOAD x17, 17 * REGBYTES(sp) + LOAD x18, 18 * REGBYTES(sp) + LOAD x19, 19 * REGBYTES(sp) + LOAD x20, 20 * REGBYTES(sp) + LOAD x21, 21 * REGBYTES(sp) + LOAD x22, 22 * REGBYTES(sp) + LOAD x23, 23 * REGBYTES(sp) + LOAD x24, 24 * REGBYTES(sp) + LOAD x25, 25 * REGBYTES(sp) + LOAD x26, 26 * REGBYTES(sp) + LOAD x27, 27 * REGBYTES(sp) + LOAD x28, 28 * REGBYTES(sp) + LOAD x29, 29 * REGBYTES(sp) + LOAD x30, 30 * REGBYTES(sp) + LOAD x31, 31 * REGBYTES(sp) +.endm - - .endm .macro save_tf - # save gprs - STORE x1,1*REGBYTES(x2) - STORE x3,3*REGBYTES(x2) - STORE x4,4*REGBYTES(x2) - STORE x5,5*REGBYTES(x2) - STORE x6,6*REGBYTES(x2) - STORE x7,7*REGBYTES(x2) - STORE x8,8*REGBYTES(x2) - STORE x9,9*REGBYTES(x2) - STORE x10,10*REGBYTES(x2) - STORE x11,11*REGBYTES(x2) - STORE x12,12*REGBYTES(x2) - STORE x13,13*REGBYTES(x2) - STORE x14,14*REGBYTES(x2) - STORE x15,15*REGBYTES(x2) - STORE x16,16*REGBYTES(x2) - STORE x17,17*REGBYTES(x2) - STORE x18,18*REGBYTES(x2) - STORE x19,19*REGBYTES(x2) - STORE x20,20*REGBYTES(x2) - STORE x21,21*REGBYTES(x2) - STORE x22,22*REGBYTES(x2) - STORE x23,23*REGBYTES(x2) - STORE x24,24*REGBYTES(x2) - STORE x25,25*REGBYTES(x2) - STORE x26,26*REGBYTES(x2) - STORE x27,27*REGBYTES(x2) - STORE x28,28*REGBYTES(x2) - STORE x29,29*REGBYTES(x2) - STORE x30,30*REGBYTES(x2) - STORE x31,31*REGBYTES(x2) + # save general purpose registers + # no point in saving x0 since it is always 0 + STORE x1, 1 * REGBYTES(sp) + # x2 is our stack pointer and is saved further below + STORE x3, 3 * REGBYTES(sp) + STORE x4, 4 * REGBYTES(sp) + STORE x5, 5 * REGBYTES(sp) + STORE x6, 6 * REGBYTES(sp) + STORE x7, 7 * REGBYTES(sp) + STORE x8, 8 * REGBYTES(sp) + STORE x9, 9 * REGBYTES(sp) + STORE x10, 10 * REGBYTES(sp) + STORE x11, 11 * REGBYTES(sp) + STORE x12, 12 * REGBYTES(sp) + STORE x13, 13 * REGBYTES(sp) + STORE x14, 14 * REGBYTES(sp) + STORE x15, 15 * REGBYTES(sp) + STORE x16, 16 * REGBYTES(sp) + STORE x17, 17 * REGBYTES(sp) + STORE x18, 18 * REGBYTES(sp) + STORE x19, 19 * REGBYTES(sp) + STORE x20, 20 * REGBYTES(sp) + STORE x21, 21 * REGBYTES(sp) + STORE x22, 22 * REGBYTES(sp) + STORE x23, 23 * REGBYTES(sp) + STORE x24, 24 * REGBYTES(sp) + STORE x25, 25 * REGBYTES(sp) + STORE x26, 26 * REGBYTES(sp) + STORE x27, 27 * REGBYTES(sp) + STORE x28, 28 * REGBYTES(sp) + STORE x29, 29 * REGBYTES(sp) + STORE x30, 30 * REGBYTES(sp) + STORE x31, 31 * REGBYTES(sp) - # get sr, epc, badvaddr, cause - csrrw t0,mscratch,x0 - csrr s0,mstatus - csrr t1,mepc - csrr t2,mtval - csrr t3,mcause - STORE t0,2*REGBYTES(x2) - STORE s0,32*REGBYTES(x2) - STORE t1,33*REGBYTES(x2) - STORE t2,34*REGBYTES(x2) - STORE t3,35*REGBYTES(x2) + # get sr, epc, badvaddr, cause + csrr t0, mscratch + bnez t0, 1f # t0 == 0, trap come from coreboot + # t0 != 0, t0 is saved old sp + add t0, sp, MENTRY_FRAME_SIZE +1: + csrr s0, mstatus + csrr t1, mepc + csrr t2, mtval + csrr t3, mcause + STORE t0, 2 * REGBYTES(sp) + STORE s0, 32 * REGBYTES(sp) + STORE t1, 33 * REGBYTES(sp) + STORE t2, 34 * REGBYTES(sp) + STORE t3, 35 * REGBYTES(sp) - # get faulting insn, if it wasn't a fetch-related trap - li x5,-1 - STORE x5,36*REGBYTES(x2) + # get faulting insn, if it wasn't a fetch-related trap + li x5, -1 + STORE x5, 36 * REGBYTES(sp) +.endm - .endm - -.globl estack - .text - - .global trap_entry - .align 2 # four byte alignment, as required by mtvec + .text + .global trap_entry + .align 2 # four byte alignment, as required by mtvec trap_entry: - csrw mscratch, sp + # mscratch is initialized to 0 + # when exiting coreboot, write sp to mscratch + # before jumping to m-mode firmware we always set trap vector to the entry point of the + # payload and we don't care about mscratch anymore. mscratch is only ever used as + # exception stack if whatever coreboot jumps to is in s-mode. + #TODO we could check MPP field in mstatus to see if come from unpriviledged code. That + # way we could still use mscratch for other purposes inside the code base. + #TODO In case we got called from s-mode firmware we need to protect our stack and trap + # handler with a PMP region. + csrrw sp, mscratch, sp + # sp == 0 => trap came from coreboot + # sp != 0 => trap came from s-mode payload + bnez sp, 1f + csrrw sp, mscratch, sp +1: + addi sp, sp, -MENTRY_FRAME_SIZE + save_tf - # SMP isn't supported yet, to avoid overwriting the same stack with different - # harts that handle traps at the same time. - # someday this gets fixed. - //csrr sp, mhartid - csrr sp, 0xf14 -.Lsmp_hang: - bnez sp, .Lsmp_hang + mv a0, sp + jal trap_handler +#if 0 # Use a different stack than in the main context, to avoid overwriting # stack data. # TODO: Maybe use the old stack pointer (plus an offset) instead. But only if @@ -130,6 +142,13 @@ trap_return: csrr a0, mscratch +#endif restore_regs - # go back to the previous mode + addi sp, sp, MENTRY_FRAME_SIZE + + # restore original stack pointer (either sp or mscratch) + csrrw sp, mscratch, sp + bnez sp, 1f + csrrw sp, mscratch, sp +1: mret -- To view, visit https://review.coreboot.org/c/coreboot/+/81130?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia1f97b82e329f6358061072f98278cf56b503618 Gerrit-Change-Number: 81130 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Reviewer: Xiang W <wxjstz(a)126.com> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Xiang W <wxjstz(a)126.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: WIP: Working on enabling coreboot SBI again
by ron minnich (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Philipp Hug. Hello Philipp Hug, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/81129?usp=email to review the following change. Change subject: WIP: Working on enabling coreboot SBI again ...................................................................... WIP: Working on enabling coreboot SBI again DO NOT MERGE Change-Id: I5a9b648f9ad1a4b01ec69178f770419b37bb1377 Signed-off-by: Philipp Hug <philipp(a)hug.cx> --- M src/arch/riscv/Makefile.mk M src/arch/riscv/boot.c M src/arch/riscv/payload.c 3 files changed, 55 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/81129/1 diff --git a/src/arch/riscv/Makefile.mk b/src/arch/riscv/Makefile.mk index 1131bd5..0931068 100644 --- a/src/arch/riscv/Makefile.mk +++ b/src/arch/riscv/Makefile.mk @@ -125,6 +125,7 @@ ramstage-y += tables.c ramstage-y += payload.c ramstage-y += fit_payload.c +ramstage-y += pmp.c $(eval $(call create_class_compiler,rmodules,riscv)) diff --git a/src/arch/riscv/boot.c b/src/arch/riscv/boot.c index 6f744d3..ea22774 100644 --- a/src/arch/riscv/boot.c +++ b/src/arch/riscv/boot.c @@ -35,9 +35,11 @@ // tell OpenSBI to switch to Supervisor mode before jumping to payload run_payload_opensbi(prog, fdt, args->opensbi, RISCV_PAYLOAD_MODE_S); } else { + printk(BIOS_DEBUG, "running payload"); run_payload(prog, fdt, RISCV_PAYLOAD_MODE_S); } } else { + printk(BIOS_DEBUG, "running entry"); void (*doit)(int hart_id, void *fdt, void *arg) = prog_entry(prog); doit(hart_id, fdt, prog_entry_arg(prog)); } diff --git a/src/arch/riscv/payload.c b/src/arch/riscv/payload.c index ee2ee8e..5c47b1e 100644 --- a/src/arch/riscv/payload.c +++ b/src/arch/riscv/payload.c @@ -7,6 +7,12 @@ #include <arch/smp/atomic.h> #include <console/console.h> #include <vm.h> +#include <arch/pmp.h> + +int pmp_entries_num(void) +{ + return 16; +} /* 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) @@ -30,6 +36,23 @@ run_opensbi(hart_id, fdt, prog_entry(opensbi), prog_entry(prog), payload_mode); } +#define ENVCFG_STCE (1ULL << 63) +#define ENVCFG_STCE (1ULL << 63) +#define ENVCFG_STCE (1ULL << 63) +#define ENVCFG_STCE (1ULL << 63) +#define ENVCFG_STCE (1ULL << 63) + +#define ENVCFG_STCE (1ULL << 63) +#define ENVCFG_PBMTE (1ULL << 62) +#define ENVCFG_CBZE (1UL << 7) +#define ENVCFG_CBCFE (1UL << 6) +#define ENVCFG_CBIE_SHIFT 4 +#define ENVCFG_CBIE (3UL) << ENVCFG_CBIE_SHIFT) +#define ENVCFG_CBIE_ILL 0UL +#define ENVCFG_CBIE_FLUS 1UL +#define ENVCFG_CBIE_INV 3UL +#define ENVCFG_FIOM 1UL + /* Runs the payload without OpenSBI integration */ void run_payload(struct prog *prog, void *fdt, int payload_mode) { @@ -40,13 +63,34 @@ switch (payload_mode) { case RISCV_PAYLOAD_MODE_S: + mstatus_init(); + //reset_pmp(); + // setup_pmp(0x84000000, 0xc0000000-0x84000000, PMP_R | PMP_W | PMP_X); + setup_pmp(0, (u64)256 * 1024 * 1024 * 1024, PMP_R | PMP_W | PMP_X); + //status = 0; status = INSERT_FIELD(status, MSTATUS_MPP, PRV_S); + /* Trap vector base address point to the payload */ write_csr(stvec, doit); /* disable S-Mode interrupt */ write_csr(sie, 0); /* disable MMU */ write_csr(satp, 0); + + /* disable M-Mode interrupt */ + write_csr(mie, 0); + + long menvcfg_val = read_csr(menvcfg); + menvcfg_val |= ENVCFG_CBZE; + menvcfg_val |= ENVCFG_CBCFE; + menvcfg_val |= ENVCFG_CBIE_INV << ENVCFG_CBIE_SHIFT; + menvcfg_val |= ENVCFG_PBMTE; + + menvcfg_val |= ENVCFG_STCE; + printk(BIOS_DEBUG, "run_payload() envcfg: 0x%lx\n", menvcfg_val); + + write_csr(menvcfg, menvcfg_val); + break; case RISCV_PAYLOAD_MODE_M: status = INSERT_FIELD(status, MSTATUS_MPP, PRV_M); @@ -59,12 +103,14 @@ die("wrong privilege level for payload"); break; } + + printk(BIOS_DEBUG, "run_payload(%p, %p) mstatus: 0x%lx\n", doit, fdt, status); + write_csr(mstatus, status); write_csr(mepc, doit); - asm volatile( - "mv a0, %0\n\t" - "mv a1, %1\n\t" - "mret" ::"r"(hart_id), - "r"(fdt) - : "a0", "a1"); + asm volatile("mv a0, %0\n\t" + "mv a1, %1\n\t" + "mret" ::"r"(hart_id), + "r"(fdt) + : "a0", "a1"); } -- To view, visit https://review.coreboot.org/c/coreboot/+/81129?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5a9b648f9ad1a4b01ec69178f770419b37bb1377 Gerrit-Change-Number: 81129 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: WIP: arch/riscv: Enable FPU, vector extensions, timer and counters
by ron minnich (Code Review) 08 Mar '24

08 Mar '24

ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81128?usp=email ) Change subject: WIP: arch/riscv: Enable FPU, vector extensions, timer and counters ...................................................................... WIP: arch/riscv: Enable FPU, vector extensions, timer and counters Enable on all harts, if available Change-Id: I54be0467654ee7a445d61e4724dc58ee86b62d6f --- M src/arch/riscv/include/arch/encoding.h M src/arch/riscv/ramstage.S M src/arch/riscv/virtual_memory.c 3 files changed, 39 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/81128/1 diff --git a/src/arch/riscv/include/arch/encoding.h b/src/arch/riscv/include/arch/encoding.h index 4f01e5c..b6a1b36 100644 --- a/src/arch/riscv/include/arch/encoding.h +++ b/src/arch/riscv/include/arch/encoding.h @@ -12,10 +12,11 @@ #define MSTATUS_HPIE 0x00000040 #define MSTATUS_MPIE 0x00000080 #define MSTATUS_SPP 0x00000100 -#define MSTATUS_HPP 0x00000600 +#define MSTATUS_VS 0x00000600 #define MSTATUS_MPP 0x00001800 #define MSTATUS_FS 0x00006000 #define MSTATUS_XS 0x00018000 + #define MSTATUS_MPRV 0x00020000 #define MSTATUS_SUM 0x00040000 #define MSTATUS_MXR 0x00080000 diff --git a/src/arch/riscv/ramstage.S b/src/arch/riscv/ramstage.S index 954b155..693ef6c 100644 --- a/src/arch/riscv/ramstage.S +++ b/src/arch/riscv/ramstage.S @@ -35,8 +35,6 @@ #NOTE a1 contains FDT and should not be cluttered above call hls_init - li a0, CONFIG_RISCV_WORKING_HARTID - call smp_pause # initialize entry of interrupt/exception la t0, trap_entry @@ -45,9 +43,16 @@ # clear any pending interrupts csrwi mip, 0 - call exit_car # set up the mstatus register call mstatus_init + + # only continue with working hart, other harts will be resumed, + # when smp_resume is called, code below won't be executed on those + # harts + li a0, CONFIG_RISCV_WORKING_HARTID + call smp_pause + + call exit_car tail main # These codes need to be implemented on a specific SoC diff --git a/src/arch/riscv/virtual_memory.c b/src/arch/riscv/virtual_memory.c index 43e3d70..9f51618 100644 --- a/src/arch/riscv/virtual_memory.c +++ b/src/arch/riscv/virtual_memory.c @@ -13,23 +13,32 @@ * to M-mode). In practice, this variable has been a lifesaver. It is * still not quite determined which delegation might by unallowed by * the spec so for now we enumerate and set them all. */ -static int delegate = 0 - | (1 << CAUSE_MISALIGNED_FETCH) - | (1 << CAUSE_FETCH_ACCESS) - | (1 << CAUSE_ILLEGAL_INSTRUCTION) - | (1 << CAUSE_BREAKPOINT) - | (1 << CAUSE_LOAD_ACCESS) - | (1 << CAUSE_STORE_ACCESS) - | (1 << CAUSE_USER_ECALL) - | (1 << CAUSE_FETCH_PAGE_FAULT) - | (1 << CAUSE_LOAD_PAGE_FAULT) - | (1 << CAUSE_STORE_PAGE_FAULT) - ; +static int delegate = 0 | (1 << CAUSE_MISALIGNED_FETCH) | (1 << CAUSE_FETCH_ACCESS) | + (1 << CAUSE_ILLEGAL_INSTRUCTION) | (1 << CAUSE_BREAKPOINT) | + (1 << CAUSE_LOAD_ACCESS) | (1 << CAUSE_STORE_ACCESS) | + (1 << CAUSE_USER_ECALL) | (1 << CAUSE_FETCH_PAGE_FAULT) | + (1 << CAUSE_LOAD_PAGE_FAULT) | (1 << CAUSE_STORE_PAGE_FAULT); void mstatus_init(void) { + // initialize mstatus to defined value + uintptr_t mstatus_val = 0; + + // enable FPU, if available + if (supports_extension('D') || supports_extension('F')) { + mstatus_val |= MSTATUS_FS; + } + + // enable vector extensions + if (supports_extension('V')) { + mstatus_val |= MSTATUS_VS; + } + + // update mstatus CSR + write_csr(mstatus, mstatus_val); + // clear any pending timer interrupts. - clear_csr(mip, MIP_STIP | MIP_SSIP); + clear_csr(mip, MIP_STIP | MIP_SSIP); // enable machine and supervisor timer and // all other supervisor interrupts. @@ -38,10 +47,16 @@ // Delegate supervisor timer and other interrupts to supervisor mode, // if supervisor mode is supported. if (supports_extension('S')) { - set_csr(mideleg, MIP_STIP | MIP_SSIP); + set_csr(mideleg, MIP_STIP | MIP_SSIP | MIP_SEIP); set_csr(medeleg, delegate); } // Enable all user/supervisor-mode counters write_csr(mcounteren, 7); + write_csr(scounteren, 7); + + // Enable FPU, if available + if (supports_extension('D') || supports_extension('F')) { + write_csr(fcsr, 0); + } } -- To view, visit https://review.coreboot.org/c/coreboot/+/81128?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I54be0467654ee7a445d61e4724dc58ee86b62d6f Gerrit-Change-Number: 81128 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-CC: Philipp Hug <philipp(a)hug.cx> Gerrit-MessageType: newchange

1 0

[XS] Change in coreboot[main]: arch/x86/include/arch: Add feature check macros
by Jérémy Compostella (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Jincheng Li, Shuo Liu. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81133?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: arch/x86/include/arch: Add feature check macros ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: Do you have code using them ? Adding declaration which are not used at all is a bit confusing. File src/arch/x86/include/arch/cpu.h: https://review.coreboot.org/c/coreboot/+/81133/comment/26d407d1_d8b90918 : PS1, Line 54: #define CPUID_FEATURE_HTT (1 << 28) The typo fix should in a separate clean-up patch. Also I recommend use the BIT macro instead of the manual bit shift construction. -- To view, visit https://review.coreboot.org/c/coreboot/+/81133?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: I014c25ced1dbe21f35486f8305b1de7669e932d0 Gerrit-Change-Number: 81133 Gerrit-PatchSet: 1 Gerrit-Owner: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Comment-Date: Fri, 08 Mar 2024 17:43:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: util/docker/Makefile: Create Documentation/_build for docker-build-docs
by Nicholas Chin (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Martin L Roth, Nicholas Chin. Hello Felix Singer, Martin L Roth, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81127?usp=email to look at the new patch set (#3). Change subject: util/docker/Makefile: Create Documentation/_build for docker-build-docs ...................................................................... util/docker/Makefile: Create Documentation/_build for docker-build-docs If the host directory of a bind mount does not exist, Docker will create it. However, the newly created directory will be owned by root due to the Docker service running within a root context. The docker command in the recipe for docker-build-docs binds Documentation/_build to /data-out within the container, so if it doesn't already exist, the documentation builder will be unable to copy the HTML output into /data-out since it runs with the same UID and GID as the host user. By creating, if necessary, the _build directory before the `docker run` command, there should always be an existing directory owned by the host user for docker to bind /data-out to (ignoring the case of an existing _build directory the current user does not have permission to write to), avoiding the issue where it cannot write the output. TEST: make -C util/docker docker-build-docs completes without issues with and without an existing Documentation/_build directory Change-Id: I6be9bc1fdca48f4d924f5c07cc261189ab6862fd Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> --- M util/docker/Makefile 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/81127/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/81127?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: I6be9bc1fdca48f4d924f5c07cc261189ab6862fd Gerrit-Change-Number: 81127 Gerrit-PatchSet: 3 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[main]: [RFC] region: Introduce region_create() functions
by Nico Huber (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Andrey Petrov, Julius Werner, Jérémy Compostella, Martin L Roth, Maximilian Brune, Ronak Kanabar. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79905?usp=email ) Change subject: [RFC] region: Introduce region_create() functions ...................................................................... Patch Set 6: (1 comment) File src/commonlib/include/commonlib/region.h: https://review.coreboot.org/c/coreboot/+/79905/comment/ea8d5ef7_42ac85c3 : PS1, Line 106: offset > SIZE_MAX || size > SIZE_MAX > > Hmmm... well, okay, if you feel strongly about it I guess it doesn't really hurt. […] Max, any more thoughts on this? I'd like to finish this patch series. Didn't want to rush things, though, in case you want to suggest another implementation for callers with untrusted input. -- To view, visit https://review.coreboot.org/c/coreboot/+/79905?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: I4ae3e6274c981c9ab4fb1263c2a72fa68ef1c32b Gerrit-Change-Number: 79905 Gerrit-PatchSet: 6 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Fri, 08 Mar 2024 17:36:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: util/docker/Makefile: Create Documentation/_build for docker-build-docs
by Felix Singer (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Martin L Roth, Nicholas Chin. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81127?usp=email ) Change subject: util/docker/Makefile: Create Documentation/_build for docker-build-docs ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/81127?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: I6be9bc1fdca48f4d924f5c07cc261189ab6862fd Gerrit-Change-Number: 81127 Gerrit-PatchSet: 2 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Fri, 08 Mar 2024 17:29:47 +0000 Gerrit-HasComments: No 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