coreboot-gerrit March 2024

coreboot-gerrit@coreboot.org

1 participants
3668 discussions

[L] Change in coreboot[main]: arch/riscv: remove misaligned load/store/fetch handling
by ron minnich (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Philipp Hug. ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81416?usp=email ) Change subject: arch/riscv: remove misaligned load/store/fetch handling ...................................................................... arch/riscv: remove misaligned load/store/fetch handling Testing on the unmatched shows the code no longer works completely correctly; Linux has taken over the handling of misaligments anyway, because handling it in firmware, with the growing complexity of the ISA and the awkward way in which it has to be handled, is more trouble than its worth. Plus, we don't WANT misalignment handled, magically, in firmware: the cost of getting it wrong is high (as I've spent a month learning); the performance is terrible; and most toolchains know to avoid unaligned load/store on RISC-V anyway. But, mostly, if alignment problems exist, *we need to know*, and if they're handled invisibly in firmware, we don't. The problem with invisble handling was shown a while back in the Go toolchain: runtime had a small error, such that many misaligned load/store were happening, and it was not discovered for some time. Had a trap been directed to kernel or user on misalignment, the problem would have been known immediately, not after many months. (The error, btw, was masking the address with 3, not 7, to detect misalignment; an easy mistake!). But, this code does not work any more any way, and it's not worth fixing. Remove it. Change-Id: I84a8d433ed2f50745686a8c109d101e8718f2a46 Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> --- M src/arch/riscv/Makefile.mk D src/arch/riscv/misaligned.c M src/arch/riscv/trap_handler.c M src/arch/riscv/virtual_memory.c 4 files changed, 6 insertions(+), 265 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/81416/1 diff --git a/src/arch/riscv/Makefile.mk b/src/arch/riscv/Makefile.mk index 056a460..d3e5284 100644 --- a/src/arch/riscv/Makefile.mk +++ b/src/arch/riscv/Makefile.mk @@ -52,7 +52,6 @@ all-y += trap_util.S all-y += trap_handler.c all-y += fp_asm.S -all-y += misaligned.c all-y += sbi.c all-y += mcall.c all-y += virtual_memory.c diff --git a/src/arch/riscv/misaligned.c b/src/arch/riscv/misaligned.c deleted file mode 100644 index 151ed99..0000000 --- a/src/arch/riscv/misaligned.c +++ /dev/null @@ -1,260 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <vm.h> -#include <arch/exception.h> -#include <commonlib/helpers.h> -#include <types.h> - -/* these functions are defined in src/arch/riscv/fp_asm.S */ -#if defined(__riscv_flen) -#if __riscv_flen >= 32 -extern void read_f32(int regnum, uint32_t *v); -extern void write_f32(int regnum, uint32_t *v); -#endif // __riscv_flen >= 32 -#if __riscv_flen >= 64 -extern void read_f64(int regnum, uint64_t *v); -extern void write_f64(int regnum, uint64_t *v); -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) - -/* This union makes it easy to read multibyte types by byte operations. */ -union endian_buf { - uint8_t b[8]; - uint16_t h[4]; - uint32_t w[2]; - uint64_t d[1]; - uintptr_t v; -}; - -/* This struct hold info of load/store instruction */ -struct memory_instruction_info { - /* opcode/mask used to identify instruction, - * (instruction_val) & mask == opcode */ - uint32_t opcode; - uint32_t mask; - /* reg_shift/reg_mask/reg_addition used to get register number - * ((instruction_val >> reg_shift) & reg_mask) + reg_addition */ - unsigned int reg_shift; - unsigned int reg_mask; - unsigned int reg_addition; - unsigned int is_fp : 1; /* mark as a float operation */ - unsigned int is_load : 1; /* mark as a load operation */ - unsigned int width : 8; /* Record the memory width of the operation */ - unsigned int sign_extend : 1; /* mark need to be sign extended */ -}; - -static struct memory_instruction_info insn_info[] = { -#if __riscv_xlen == 128 - { 0x00002000, 0x0000e003, 2, 7, 8, 0, 1, 16, 1}, // C.LQ -#else - { 0x00002000, 0x0000e003, 2, 7, 8, 1, 1, 8, 0}, // C.FLD -#endif - { 0x00004000, 0x0000e003, 2, 7, 8, 0, 1, 4, 1}, // C.LW -#if __riscv_xlen == 32 - { 0x00006000, 0x0000e003, 2, 7, 8, 1, 1, 4, 0}, // C.FLW -#else - { 0x00006000, 0x0000e003, 2, 7, 8, 0, 1, 8, 1}, // C.LD -#endif - -#if __riscv_xlen == 128 - { 0x0000a000, 0x0000e003, 2, 7, 8, 0, 0, 16, 0}, // C.SQ -#else - { 0x0000a000, 0x0000e003, 2, 7, 8, 1, 0, 8, 0}, // C.FSD -#endif - { 0x0000c000, 0x0000e003, 2, 7, 8, 0, 0, 4, 0}, // C.SW -#if __riscv_xlen == 32 - { 0x0000e000, 0x0000e003, 2, 7, 8, 1, 0, 4, 0}, // C.FSW -#else - { 0x0000e000, 0x0000e003, 2, 7, 8, 0, 0, 8, 0}, // C.SD -#endif - -#if __riscv_xlen == 128 - { 0x00002002, 0x0000e003, 7, 15, 0, 0, 1, 16, 1}, // C.LQSP -#else - { 0x00002002, 0x0000e003, 7, 15, 0, 1, 1, 8, 0}, // C.FLDSP -#endif - { 0x00004002, 0x0000e003, 7, 15, 0, 0, 1, 4, 1}, // C.LWSP -#if __riscv_xlen == 32 - { 0x00006002, 0x0000e003, 7, 15, 0, 1, 1, 4, 0}, // C.FLWSP -#else - { 0x00006002, 0x0000e003, 7, 15, 0, 0, 1, 8, 1}, // C.LDSP -#endif - -#if __riscv_xlen == 128 - { 0x0000a002, 0x0000e003, 2, 15, 0, 0, 0, 16, 0}, // C.SQSP -#else - { 0x0000a002, 0x0000e003, 2, 15, 0, 1, 0, 8, 0}, // C.FSDSP -#endif - { 0x0000c002, 0x0000e003, 2, 15, 0, 0, 0, 4, 0}, // C.SWSP -#if __riscv_xlen == 32 - { 0x0000e002, 0x0000e003, 2, 15, 0, 1, 0, 4, 0}, // C.FSWSP -#else - { 0x0000e002, 0x0000e003, 2, 15, 0, 0, 0, 8, 0}, // C.SDSP -#endif - - { 0x00000003, 0x0000707f, 7, 15, 0, 0, 1, 1, 1}, // LB - { 0x00001003, 0x0000707f, 7, 15, 0, 0, 1, 2, 1}, // LH - { 0x00002003, 0x0000707f, 7, 15, 0, 0, 1, 4, 1}, // LW -#if __riscv_xlen > 32 - { 0x00003003, 0x0000707f, 7, 15, 0, 0, 1, 8, 1}, // LD -#endif - { 0x00004003, 0x0000707f, 7, 15, 0, 0, 1, 1, 0}, // LBU - { 0x00005003, 0x0000707f, 7, 15, 0, 0, 1, 2, 0}, // LHU - { 0x00006003, 0x0000707f, 7, 15, 0, 0, 1, 4, 0}, // LWU - - { 0x00000023, 0x0000707f, 20, 15, 0, 0, 0, 1, 0}, // SB - { 0x00001023, 0x0000707f, 20, 15, 0, 0, 0, 2, 0}, // SH - { 0x00002023, 0x0000707f, 20, 15, 0, 0, 0, 4, 0}, // SW -#if __riscv_xlen > 32 - { 0x00003023, 0x0000707f, 20, 15, 0, 0, 0, 8, 0}, // SD -#endif - -#if defined(__riscv_flen) -#if __riscv_flen >= 32 - { 0x00002007, 0x0000707f, 7, 15, 0, 1, 1, 4, 0}, // FLW - { 0x00003007, 0x0000707f, 7, 15, 0, 1, 1, 8, 0}, // FLD -#endif // __riscv_flen >= 32 - -#if __riscv_flen >= 64 - { 0x00002027, 0x0000707f, 20, 15, 0, 1, 0, 4, 0}, // FSW - { 0x00003027, 0x0000707f, 20, 15, 0, 1, 0, 8, 0}, // FSD -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) -}; - -static struct memory_instruction_info *match_instruction(uintptr_t insn) -{ - int i; - for (i = 0; i < ARRAY_SIZE(insn_info); i++) - if ((insn_info[i].mask & insn) == insn_info[i].opcode) - return &(insn_info[i]); - return NULL; -} - -static enum cb_err fetch_16bit_instruction(uintptr_t vaddr, uintptr_t *insn, int *size) -{ - uint16_t ins = mprv_read_mxr_u16((uint16_t *)vaddr); - if (EXTRACT_FIELD(ins, 0x3) != 3) { - *insn = ins; - *size = 2; - return CB_SUCCESS; - } - return CB_ERR; -} - -static enum cb_err fetch_32bit_instruction(uintptr_t vaddr, uintptr_t *insn, int *size) -{ - uint32_t l = (uint32_t)mprv_read_mxr_u16((uint16_t *)vaddr + 0); - uint32_t h = (uint32_t)mprv_read_mxr_u16((uint16_t *)vaddr + 1); - uint32_t ins = (h << 16) | l; - if ((EXTRACT_FIELD(ins, 0x3) == 3) && - (EXTRACT_FIELD(ins, 0x1c) != 0x7)) { - *insn = ins; - *size = 4; - return CB_SUCCESS; - } - return CB_ERR; -} - -void handle_misaligned(struct trapframe *tf) -{ - uintptr_t insn = 0; - union endian_buf buff; - int insn_size = 0; - - /* try to fetch 16/32 bits instruction */ - if (fetch_16bit_instruction(tf->epc, &insn, &insn_size) < 0) { - if (fetch_32bit_instruction(tf->epc, &insn, &insn_size) < 0) { - redirect_trap(); - return; - } - } - - /* matching instruction */ - struct memory_instruction_info *match = match_instruction(insn); - - if (!match) { - redirect_trap(); - return; - } - - int regnum; - regnum = ((insn >> match->reg_shift) & match->reg_mask); - regnum = regnum + match->reg_addition; - buff.v = 0; - if (match->is_load) { - /* load operation */ - - /* reading from memory by bytes prevents misaligned - * memory access */ - for (int i = 0; i < match->width; i++) { - uint8_t *addr = (uint8_t *)(tf->badvaddr + i); - buff.b[i] = mprv_read_u8(addr); - } - - /* sign extend for signed integer loading */ - if (match->sign_extend) - if (buff.v >> (8 * match->width - 1)) - buff.v |= -1 << (8 * match->width); - - /* write to register */ - if (match->is_fp) { - int done = 0; -#if defined(__riscv_flen) -#if __riscv_flen >= 32 - /* single-precision floating-point */ - if (match->width == 4) { - write_f32(regnum, buff.w); - done = 1; - } -#endif // __riscv_flen >= 32 -#if __riscv_flen >= 64 - /* double-precision floating-point */ - if (match->width == 8) { - write_f64(regnum, buff.d); - done = 1; - } -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) - if (!done) - redirect_trap(); - } else { - tf->gpr[regnum] = buff.v; - } - } else { - /* store operation */ - - /* reading from register */ - if (match->is_fp) { - int done = 0; -#if defined(__riscv_flen) -#if __riscv_flen >= 32 - if (match->width == 4) { - read_f32(regnum, buff.w); - done = 1; - } -#endif // __riscv_flen >= 32 -#if __riscv_flen >= 64 - if (match->width == 8) { - read_f64(regnum, buff.d); - done = 1; - } -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) - if (!done) - redirect_trap(); - } else { - buff.v = tf->gpr[regnum]; - } - - /* writing to memory by bytes prevents misaligned - * memory access */ - for (int i = 0; i < match->width; i++) { - uint8_t *addr = (uint8_t *)(tf->badvaddr + i); - mprv_write_u8(addr, buff.b[i]); - } - } - - /* return to where we came from */ - write_csr(mepc, read_csr(mepc) + insn_size); -} diff --git a/src/arch/riscv/trap_handler.c b/src/arch/riscv/trap_handler.c index 172dd45..b99f9bc 100644 --- a/src/arch/riscv/trap_handler.c +++ b/src/arch/riscv/trap_handler.c @@ -179,7 +179,6 @@ } switch (tf->cause) { - case CAUSE_MISALIGNED_FETCH: case CAUSE_FETCH_ACCESS: case CAUSE_BREAKPOINT: case CAUSE_LOAD_ACCESS: @@ -192,11 +191,12 @@ case CAUSE_SUPERVISOR_ECALL: handle_sbi(tf); return; + case CAUSE_MISALIGNED_FETCH: case CAUSE_MISALIGNED_LOAD: case CAUSE_MISALIGNED_STORE: print_trap_information(tf); - handle_misaligned(tf); - return; + printk(BIOS_EMERG, "coreboot no longer does misaliged load/store/fetch\n"); + break; case CAUSE_ILLEGAL_INSTRUCTION: ill(tf); return; diff --git a/src/arch/riscv/virtual_memory.c b/src/arch/riscv/virtual_memory.c index e739f03..8f958e5 100644 --- a/src/arch/riscv/virtual_memory.c +++ b/src/arch/riscv/virtual_memory.c @@ -13,8 +13,10 @@ * 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 +static u32 delegate = 0 | (1 << CAUSE_MISALIGNED_FETCH) + | (1 << CAUSE_MISALIGNED_STORE) + | (1 << CAUSE_MISALIGNED_LOAD) | (1 << CAUSE_FETCH_ACCESS) | (1 << CAUSE_BREAKPOINT) | (1 << CAUSE_LOAD_ACCESS) -- To view, visit https://review.coreboot.org/c/coreboot/+/81416?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: I84a8d433ed2f50745686a8c109d101e8718f2a46 Gerrit-Change-Number: 81416 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[main]: arch/riscv: linuxcheck framework which runs from ramstage
by ron minnich (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Arthur Heymans, Maximilian Brune, Philipp Hug. Hello Arthur Heymans, Maximilian Brune, Philipp Hug, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81367?usp=email to look at the new patch set (#7). Change subject: arch/riscv: linuxcheck framework which runs from ramstage ...................................................................... arch/riscv: linuxcheck framework which runs from ramstage Errors in Linux booting are notoriously hard to debug. This change adds a linuxcheck framework that allows testing of things Linux (and other kernels) needs, without having to debug Linux. If LINUXCHECK is set during menuconfig, the linuxcheck function will be called, in S mode, from the ramstage. That function then runs code, in S mode, that tests coreboot M mode code such as SBI. This is extremely handy: because linuxcheck is built into the ramstage, it allows calling any ramstage function, e.g. printk, if required; at the same time, the function is running in S mode, so it is very easy to test SBI calls, instruction emulation, and so on. Note that, if CONFIG_LINUXCHECK is set, the PMPs should enable RWX permissions (instead of R permissions) on the ramstage. For now, until we get the rest of the debugging done, we unconditionally set the permissions. This is a very powerful approach that could also form the core of an SBI fuzzing suite; unlike the currently proposed one, which is based on the KVM fuzzer, this code would not require a Linux kernel to test. It is very light weight. This is also better than the older linuxcheck payload, since it does not need libpayload or seperate compilation. It reduces the testing of SBI and other trap code to its simplest form, and allows to focus on testing SBI, not fighting build systems and large complex kernels. Change-Id: I7d19147b9df57c63ec7301da243fd5541e9952a7 Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> --- M src/arch/riscv/Kconfig M src/arch/riscv/Makefile.mk M src/arch/riscv/include/arch/cpu.h A src/arch/riscv/linuxcheck.c M src/arch/riscv/payload.c 5 files changed, 117 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/81367/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/81367?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: I7d19147b9df57c63ec7301da243fd5541e9952a7 Gerrit-Change-Number: 81367 Gerrit-PatchSet: 7 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: arch/x86/bootblock.ld: Align the base of bootblock downwards
by Jérémy Compostella (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Arthur Heymans, Kyösti Mälkki, Patrick Rudolph. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80346?usp=email ) Change subject: arch/x86/bootblock.ld: Align the base of bootblock downwards ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/80346?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: I78c6ba6e250ded3f04b12cd0c20b18cb653a1506 Gerrit-Change-Number: 80346 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Thu, 21 Mar 2024 21:29:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: soc/amd/noncar: Increase bootblock size from 64K to 128K
by Arthur Heymans (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Martin L Roth. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80348?usp=email ) Change subject: soc/amd/noncar: Increase bootblock size from 64K to 128K ...................................................................... Patch Set 7: (1 comment) Patchset: PS7: > can we change the size only when compiling for 64-bit? this will affect boot time because bootblock is decompressed by the PSP vs the x86. Maybe it's possible to do something similar to the general x86 bootblock and top align the code. That reduce size on both cases and use only as much memory as needed. I'll look into that. -- To view, visit https://review.coreboot.org/c/coreboot/+/80348?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: I23f176d63d3c303b13331a77ad5ac6c7a19073d3 Gerrit-Change-Number: 80348 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 21 Mar 2024 21:03:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin L Roth <gaumless(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: soc/amd/noncar: Increase bootblock size from 64K to 128K
by Martin L Roth (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Arthur Heymans, Felix Held, Fred Reitberger, Jason Glenesk. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80348?usp=email ) Change subject: soc/amd/noncar: Increase bootblock size from 64K to 128K ...................................................................... Patch Set 7: (1 comment) Patchset: PS7: can we change the size only when compiling for 64-bit? this will affect boot time because bootblock is decompressed by the PSP vs the x86. -- To view, visit https://review.coreboot.org/c/coreboot/+/80348?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: I23f176d63d3c303b13331a77ad5ac6c7a19073d3 Gerrit-Change-Number: 80348 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 21 Mar 2024 20:54:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: cpu/x86: Link page tables in stage if possible
by Arthur Heymans (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Jérémy Compostella, Matt DeVillier, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80337?usp=email ) Change subject: cpu/x86: Link page tables in stage if possible ...................................................................... Patch Set 13: (9 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/80337/comment/6bcb54bc_6bbb9575 : PS13, Line 9: 32 to 64 > 32-bits to 64-bits Done https://review.coreboot.org/c/coreboot/+/80337/comment/162b91e6_06e455b4 : PS13, Line 9: back and forward > "backward and forward" or "back and forth" Done https://review.coreboot.org/c/coreboot/+/80337/comment/cb0d347f_54493a29 : PS13, Line 10: tables > table Done https://review.coreboot.org/c/coreboot/+/80337/comment/7ef79674_e5cc382b : PS13, Line 10: 32bit > 32-bits Done https://review.coreboot.org/c/coreboot/+/80337/comment/78decd16_8a6be729 : PS13, Line 14: cbfs > CBFS Done https://review.coreboot.org/c/coreboot/+/80337/comment/6b7d10c1_c2325e34 : PS13, Line 22: used > using Done https://review.coreboot.org/c/coreboot/+/80337/comment/03af8c6b_3c6d025e : PS13, Line 26: cbfs > CBFS Done https://review.coreboot.org/c/coreboot/+/80337/comment/ce1c17f0_acf831bd : PS13, Line 28: TESTED > TEST: <...> I don't understand this. I'm stating that I tested it on the following platforms with the test being reaching a payload. TEST seems to indicate what a valid test would be for this code without implying that I actually tested it. File src/cpu/x86/64bit/pt.S: https://review.coreboot.org/c/coreboot/+/80337/comment/87b30431_fe7f6e6e : PS13, Line 21: .align 4096 > wouldn't it make sense to change the alignment depending on the configuration to prevent waste of space when page tables are in CBFS ? No space is wasted as this is the first entry of the .rodata section. 0 is always aligned -- To view, visit https://review.coreboot.org/c/coreboot/+/80337?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: Ied54b66b930187cba5fbc578a81ed5859a616562 Gerrit-Change-Number: 80337 Gerrit-PatchSet: 13 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 21 Mar 2024 20:52:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: arch/x86/bootblock.ld: Align the base of bootblock downwards
by Martin L Roth (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Arthur Heymans, Jérémy Compostella, Kyösti Mälkki, Patrick Rudolph. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80346?usp=email ) Change subject: arch/x86/bootblock.ld: Align the base of bootblock downwards ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80346?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: I78c6ba6e250ded3f04b12cd0c20b18cb653a1506 Gerrit-Change-Number: 80346 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Thu, 21 Mar 2024 20:51:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: cpu/x86: Link page tables in stage if possible
by Arthur Heymans (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Arthur Heymans, Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Patrick Rudolph. Hello Felix Held, Fred Reitberger, Jason Glenesk, Jérémy Compostella, Matt DeVillier, Patrick Rudolph, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80337?usp=email to look at the new patch set (#15). Change subject: cpu/x86: Link page tables in stage if possible ...................................................................... cpu/x86: Link page tables in stage if possible When switching back and forth between 32 to 64 bit mode, for example to call a 32-bits FSP or to call the payload, new page tables in the respective stage will be linked. The advantages of this approach are: - No need to determine a good place for page tables in CBFS that does not overlap. - Works with non memory mapped flash (however all coreboot targets currently do support this) - If later stages can use their own page tables which fits better with the vboot RO/RW flow A disadvantage is that it increases the stage size. This could be improved upon by using 1G pages and generating the pages at runtime. Note: qemu cannot have the page tables in the RO boot medium and needs to relocate them at runtime. This is why keeping the existing code with page tables in CBFS is done for now. TESTED on google/vilbox and qemu/q35: boots to payload Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Change-Id: Ied54b66b930187cba5fbc578a81ed5859a616562 --- M src/arch/x86/Kconfig M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S M src/cpu/x86/64bit/Makefile.mk M src/cpu/x86/64bit/entry64.inc M src/cpu/x86/64bit/mode_switch.S M src/cpu/x86/64bit/mode_switch2.S M src/cpu/x86/64bit/pt.S M src/mainboard/emulation/qemu-i440fx/Kconfig M src/mainboard/emulation/qemu-q35/Kconfig M src/soc/amd/common/block/cpu/noncar/pre_c.S M src/soc/intel/common/block/cpu/car/cache_as_ram.S 13 files changed, 28 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/80337/15 -- To view, visit https://review.coreboot.org/c/coreboot/+/80337?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: Ied54b66b930187cba5fbc578a81ed5859a616562 Gerrit-Change-Number: 80337 Gerrit-PatchSet: 15 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: cpu/x86: Link page tables in stage if possible
by Arthur Heymans (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Arthur Heymans, Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Patrick Rudolph. Hello Felix Held, Fred Reitberger, Jason Glenesk, Jérémy Compostella, Matt DeVillier, Patrick Rudolph, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80337?usp=email to look at the new patch set (#14). Change subject: cpu/x86: Link page tables in stage if possible ...................................................................... cpu/x86: Link page tables in stage if possible When switching back and forth between 32 to 64 bit mode, for example to call a 32-bits FSP or to call the payload, new page tables in the respective stage will be linked. The advantages of this approach are: - No need to determine a good place for page tables in CBFS that does not overlap. - Works with non memory mapped flash (however all coreboot targets currently do support this) - If later stages can use their own page tables which fits better with the vboot RO/RW flow A disadvantage is that it increases the stage size. This could be improved upon by using 1G pages and generating the pages at runtime. Note: qemu cannot have the page tables in the RO boot medium and needs to relocate them at runtime. This is why keeping the existing code with page tables in cbfs is done for now. TESTED on google/vilbox and qemu/q35: boots to payload Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Change-Id: Ied54b66b930187cba5fbc578a81ed5859a616562 --- M src/arch/x86/Kconfig M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S M src/cpu/x86/64bit/Makefile.mk M src/cpu/x86/64bit/entry64.inc M src/cpu/x86/64bit/mode_switch.S M src/cpu/x86/64bit/mode_switch2.S M src/cpu/x86/64bit/pt.S M src/mainboard/emulation/qemu-i440fx/Kconfig M src/mainboard/emulation/qemu-q35/Kconfig M src/soc/amd/common/block/cpu/noncar/pre_c.S M src/soc/intel/common/block/cpu/car/cache_as_ram.S 13 files changed, 28 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/80337/14 -- To view, visit https://review.coreboot.org/c/coreboot/+/80337?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: Ied54b66b930187cba5fbc578a81ed5859a616562 Gerrit-Change-Number: 80337 Gerrit-PatchSet: 14 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[main]: cpu/x86/Kconfig: Mark 64bit support as stable
by Martin L Roth (Code Review) 21 Mar '24

21 Mar '24

Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Fred Reitberger, Jason Glenesk, Julius Werner, Matt DeVillier. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80338?usp=email ) Change subject: cpu/x86/Kconfig: Mark 64bit support as stable ...................................................................... Patch Set 13: Code-Review+2 (1 comment) Patchset: PS13: yay! -- To view, visit https://review.coreboot.org/c/coreboot/+/80338?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: Ifcf0a1a5097a2d7c064bb709ec0b09ebee13a47d Gerrit-Change-Number: 80338 Gerrit-PatchSet: 13 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 21 Mar 2024 20:49:59 +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