coreboot-gerrit April 2022

coreboot-gerrit@coreboot.org

1 participants
5879 discussions

Change in coreboot[master]: cpu/x86/64bit: Generate static page tables from an assembly file
by Felix Held (Code Review) 25 Apr '22

25 Apr '22

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/63725 ) Change subject: cpu/x86/64bit: Generate static page tables from an assembly file ...................................................................... cpu/x86/64bit: Generate static page tables from an assembly file This removes the need for a tool to generate simple identity pages. Future patches will link this page table directly into the stages on some platforms so having an assembly file makes a lot of sense. This also optimizes the size of the page of each 4K page by placing the PDPE_table below the PDE. Change-Id: Ia1e31b701a2584268c85d327bf139953213899e3 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/63725 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> --- M Documentation/arch/x86/index.md M src/cpu/x86/64bit/Makefile.inc A src/cpu/x86/64bit/pt.S D util/pgtblgen/Makefile.inc D util/pgtblgen/description.md D util/pgtblgen/pgtblgen.c 6 files changed, 48 insertions(+), 181 deletions(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved diff --git a/Documentation/arch/x86/index.md b/Documentation/arch/x86/index.md index a30c5e2..c7115b4 100644 --- a/Documentation/arch/x86/index.md +++ b/Documentation/arch/x86/index.md @@ -26,9 +26,7 @@ * A stage can install new page tables in RAM ## Page tables -Page tables are generated by a tool in `util/pgtblgen/pgtblgen`. It writes -the page tables to a file which is then included into the CBFS as file called -`pagetables`. +A `pagetables` cbfs file is generated based on an assembly file. To generate the static page tables it must know the physical address where to place the file. diff --git a/src/cpu/x86/64bit/Makefile.inc b/src/cpu/x86/64bit/Makefile.inc index 721e620..1895498 100644 --- a/src/cpu/x86/64bit/Makefile.inc +++ b/src/cpu/x86/64bit/Makefile.inc @@ -5,3 +5,15 @@ romstage-y += mode_switch.S postcar-y += mode_switch.S ramstage-y += mode_switch.S + +# Add --defsym=_start=0 to suppress a linker warning. +$(objcbfs)/pt: $(dir)/pt.S + $(CC_bootblock) $(CFLAGS_bootblock) $(CPPFLAGS_bootblock) -o $@.tmp $< -Wl,--section-start=.rodata=$(CONFIG_ARCH_X86_64_PGTBL_LOC),--defsym=_start=0 + $(OBJCOPY_ramstage) -Obinary -j .rodata $@.tmp $@ + rm $@.tmp + +cbfs-files-y += pagetables +pagetables-file := $(objcbfs)/pt +pagetables-type := raw +pagetables-compression := none +pagetables-COREBOOT-position := $(CONFIG_ARCH_X86_64_PGTBL_LOC) diff --git a/src/cpu/x86/64bit/pt.S b/src/cpu/x86/64bit/pt.S new file mode 100644 index 0000000..b105528 --- /dev/null +++ b/src/cpu/x86/64bit/pt.S @@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* + * For reference see "AMD64 Architecture Programmer's Manual Volume 2", + * Document 24593-Rev. 3.31-July 2019 Chapter 5.3.4 + * + * Page table attributes: WB, User+Supervisor, Present, Writeable, Accessed, Dirty + */ + +.section .rodata +#define _PRES (1ULL << 0) +#define _RW (1ULL << 1) +#define _US (1ULL << 2) +#define _A (1ULL << 5) +#define _D (1ULL << 6) +#define _PS (1ULL << 7) +#define _GEN_DIR(a) (_PRES + _RW + _US + _A + (a)) +#define _GEN_PAGE(a) (_PRES + _RW + _US + _PS + _A + _D + (a)) + +.global PM4LE +.align 32 +PM4LE: +.quad _GEN_DIR(PDPE_table) + +.align 4096 +PDE_tables: /* identity map 2MiB pages */ +.rept 2048 +.quad _GEN_PAGE(0x200000 * ((. - PDE_tables) >> 3)) +.endr + +.align 4096 +PDPE_table: /* Point to PDE */ +.rept 4 +.quad _GEN_DIR(PDE_tables + 4096 * ((. - PDPE_table) >> 3)) +.endr diff --git a/util/pgtblgen/Makefile.inc b/util/pgtblgen/Makefile.inc deleted file mode 100644 index 08fb620..0000000 --- a/util/pgtblgen/Makefile.inc +++ /dev/null @@ -1,19 +0,0 @@ -ifeq ($(CONFIG_ARCH_BOOTBLOCK_X86_64),y) - -PGTBLGEN:= $(obj)/pgtblgen - -cbfs-files-y += pagetables -pagetables-file := $(obj)/mainboard/$(MAINBOARDDIR)/pagetables -pagetables-type := raw -pagetables-compression := none -pagetables-COREBOOT-position := $(CONFIG_ARCH_X86_64_PGTBL_LOC) - -$(obj)/mainboard/$(MAINBOARDDIR)/pagetables: $(PGTBLGEN) $(obj)/config.h - printf " TOOL Creating page tables\n" - $(PGTBLGEN) -b $(CONFIG_ARCH_X86_64_PGTBL_LOC) -a x86_64 -o $@ - -$(PGTBLGEN): util/pgtblgen/pgtblgen.c - printf " MAKE Creating PGTBLGEN tool\n" - $(HOSTCC) -std=c99 $< -I$(obj) -o $@ - -endif diff --git a/util/pgtblgen/description.md b/util/pgtblgen/description.md deleted file mode 100644 index f2e4763..0000000 --- a/util/pgtblgen/description.md +++ /dev/null @@ -1 +0,0 @@ -Generates page tables based on fixed physical address. `C` diff --git a/util/pgtblgen/pgtblgen.c b/util/pgtblgen/pgtblgen.c deleted file mode 100644 index 7ed98b2..0000000 --- a/util/pgtblgen/pgtblgen.c +++ /dev/null @@ -1,158 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <stdio.h> -#include <stdlib.h> -#include <unistd.h> -#include <string.h> -#include <sys/types.h> -#include <sys/stat.h> -#include <fcntl.h> -#include <errno.h> -#include <inttypes.h> -#include <getopt.h> - -static void usage(char *argv[]) -{ - printf("usage: %s -b <addr> -a <arch> -o <file>\n", argv[0]); - printf(" -a\t architecture. Supported: x86_64\n"); - printf(" -b\t base address\n"); - printf(" -o\t the file to write to\n"); - printf(" -h\t show this help text\n"); -} - -/* - * For reference see "AMD64 ArchitectureProgrammer's Manual Volume 2", - * Document 24593-Rev. 3.31-July 2019 Chapter 5.3.4 - * - * Page table attributes: WB, User+Supervisor, Present, Writeable, Accessed, Dirty - */ -#define _PRES (1ULL << 0) -#define _RW (1ULL << 1) -#define _US (1ULL << 2) -#define _A (1ULL << 5) -#define _D (1ULL << 6) -#define _PS (1ULL << 7) -#define _GEN_DIR(a) (_PRES | _RW | _US | _A | (a)) -#define _GEN_PAGE(a) (_PRES | _RW | _US | _PS | _A | _D | (a)) - -/* - * Generate x86_64 page tables. - * The page tables needs to be placed at @base_address, and identity map - * the first @size_gib GiB of physical memory. - */ -static int gen_pgtbl_x86_64(const uint64_t base_address, - const size_t size_gib, - void **out_buf, - size_t *out_size) -{ - uint64_t *entry; - - if (!out_size || !out_buf) - return 1; - - *out_size = (size_gib + 2) * 4096; - *out_buf = malloc(*out_size); - if (!*out_buf) - return 1; - - memset(*out_buf, 0, *out_size); - entry = (uint64_t *)*out_buf; - - /* Generate one PM4LE entry - point to PDPE */ - entry[0] = _GEN_DIR(base_address + 4096); - entry += 512; - - /* PDPE table - point to PDE */ - for (size_t i = 0; i < size_gib; i++) - entry[i] = _GEN_DIR(base_address + 4096 * (i + 2)); - entry += 512; - - /* PDE tables - identity map 2MiB pages */ - for (size_t g = 0; g < size_gib; g++) { - for (size_t i = 0; i < 512; i++) { - uint64_t addr = ((1ULL << (12 + 9)) * i) | ((1ULL << (12 + 9 + 9)) * g); - entry[i] = _GEN_PAGE(addr); - } - entry += 512; - } - - return 0; -} - -int main(int argc, char *argv[]) -{ - int ret = 1; - uint64_t base_address = 0; - char *filename = NULL; - char *arch = NULL; - void *buf = NULL; - size_t buf_size = 0; - int c; - - while ((c = getopt(argc, argv, "ho:a:b:")) != -1) - switch (c) { - case '?': /* falltrough */ - case 'h': - usage(argv); - return 0; - case 'o': - filename = optarg; - break; - case 'a': - arch = optarg; - break; - case 'b': - base_address = strtoull(optarg, NULL, 0); - break; - default: - break; - } - - if (!filename) { - fprintf(stderr, "E: Missing filename.\n"); - goto done; - } - if (!arch) { - fprintf(stderr, "E: Missing architecture.\n"); - goto done; - } else if (strcmp(arch, "x86_64") != 0) { - fprintf(stderr, "E: Unsupported architecture.\n"); - goto done; - } - if (base_address & 4095) { - fprintf(stderr, "E: Base address not 4 KiB aligned\n"); - goto done; - } - - /* FIXME: Identity map 4GiB for now, increase if necessary */ - if (strcmp(arch, "x86_64") == 0) - ret = gen_pgtbl_x86_64(base_address, 4, &buf, &buf_size); - - if (ret) { - fprintf(stderr, "Failed to generate page tables\n"); - goto done; - } - - // write the table - FILE *fd = fopen(filename, "wb"); - if (!fd) { - fprintf(stderr, "%s open failed: %s\n", filename, strerror(errno)); - goto done; - } - - if (fwrite(buf, 1, buf_size, fd) != buf_size) { - fprintf(stderr, "%s write failed: %s\n", filename, strerror(errno)); - fclose(fd); - goto done; - } - - if (fclose(fd)) { - fprintf(stderr, "%s close failed: %s\n", filename, strerror(errno)); - goto done; - } - - ret = 0; -done: - free(buf); - return ret; -} -- To view, visit https://review.coreboot.org/c/coreboot/+/63725 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia1e31b701a2584268c85d327bf139953213899e3 Gerrit-Change-Number: 63725 Gerrit-PatchSet: 6 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: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: src: Remove unused <cf9_reset.h>
by Elyes Haouas (Code Review) 25 Apr '22

25 Apr '22

Attention is currently required from: Patrick Rudolph. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60803 ) Change subject: src: Remove unused <cf9_reset.h> ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: it needs rebase -- To view, visit https://review.coreboot.org/c/coreboot/+/60803 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I093d8412e14ce81b462fb9a7ccb3a2a93ae760a6 Gerrit-Change-Number: 60803 Gerrit-PatchSet: 2 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Martin L Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 25 Apr 2022 13:59:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/x86/64bit: Add a separate Makefile.inc
by Felix Held (Code Review) 25 Apr '22

25 Apr '22

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/63758 ) Change subject: cpu/x86/64bit: Add a separate Makefile.inc ...................................................................... cpu/x86/64bit: Add a separate Makefile.inc Follow-up patches will add more to this makefile. Change-Id: I8da6265b4c810e39a67f5ec27e26eeb26e3679a4 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/63758 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> Reviewed-by: Lean Sheng Tan <sheng.tan(a)9elements.com> --- A src/cpu/x86/64bit/Makefile.inc M src/cpu/x86/Makefile.inc 2 files changed, 9 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved Lean Sheng Tan: Looks good to me, approved diff --git a/src/cpu/x86/64bit/Makefile.inc b/src/cpu/x86/64bit/Makefile.inc new file mode 100644 index 0000000..721e620 --- /dev/null +++ b/src/cpu/x86/64bit/Makefile.inc @@ -0,0 +1,7 @@ +bootblock-y += mode_switch.S +ifneq ($(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK),y) +verstage-y += mode_switch.S +endif +romstage-y += mode_switch.S +postcar-y += mode_switch.S +ramstage-y += mode_switch.S diff --git a/src/cpu/x86/Makefile.inc b/src/cpu/x86/Makefile.inc index b970528..dffc2b1 100644 --- a/src/cpu/x86/Makefile.inc +++ b/src/cpu/x86/Makefile.inc @@ -3,8 +3,8 @@ subdirs-y += pae subdirs-$(CONFIG_HAVE_SMI_HANDLER) += smm subdirs-$(CONFIG_UDELAY_TSC) += tsc - -all-$(CONFIG_ARCH_ALL_STAGES_X86_64) += 64bit/mode_switch.S +# Use ARCH_BOOTBLOCK_X86_64 as a proxy for knowing if 64bit is going to be used +subdirs-$(CONFIG_ARCH_BOOTBLOCK_X86_64) += 64bit subdirs-$(CONFIG_PARALLEL_MP) += name ramstage-$(CONFIG_PARALLEL_MP) += mp_init.c -- To view, visit https://review.coreboot.org/c/coreboot/+/63758 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8da6265b4c810e39a67f5ec27e26eeb26e3679a4 Gerrit-Change-Number: 63758 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/intel/common: Add support to control CSE firmware update
by Felix Held (Code Review) 25 Apr '22

25 Apr '22

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/62715 ) Change subject: soc/intel/common: Add support to control CSE firmware update ...................................................................... soc/intel/common: Add support to control CSE firmware update The patch adds support to control CSE Lite firmware update dynamically. In order to disable the CSE firmware update functionality, offset 0xf00 in the coreboot binary be updated with 0x1. Run below command on the binary to disable CSE firmwar update printf '\x01' | dd of=image-brya4es.serial.bin bs=1 seek=3840 count=1 conv=notrunc BUG=b:153410586 TEST=Verified CSE firmware update functionality is not getting triggered after updating the offset:0xF00 in the coreboot binary. ........................ CB Logs ...................................... [DEBUG] prev_sleep_state 5 [DEBUG] cse_lite: Number of partitions = 3 [DEBUG] cse_lite: Current partition = RW [DEBUG] cse_lite: Next partition = RW [DEBUG] cse_lite: Flags = 0x3 [DEBUG] cse_lite: RO version = 16.0.15.1752 (Status=0x0, Start=0x2000, End=0x19bfff) [DEBUG] cse_lite: RW version = 16.0.15.1752 (Status=0x0, Start=0x205000, End=0x439fff) rt_debug: pre_mem_debug.cse_fw_update_disable=1 [DEBUG] Boot Count incremented to 956 ....................................................................... Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Change-Id: I9f234b142191eb83137d5d83f21e890e1cb828ba Reviewed-on: https://review.coreboot.org/c/coreboot/+/62715 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Rizwan Qureshi <rizwan.qureshi(a)intel.com> --- M src/soc/intel/common/basecode/debug/debug_feature.c M src/soc/intel/common/basecode/include/intelbasecode/debug_feature.h M src/soc/intel/common/block/cse/cse_lite.c 3 files changed, 30 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Rizwan Qureshi: Looks good to me, approved diff --git a/src/soc/intel/common/basecode/debug/debug_feature.c b/src/soc/intel/common/basecode/debug/debug_feature.c index 42cf992..1c55f0b 100644 --- a/src/soc/intel/common/basecode/debug/debug_feature.c +++ b/src/soc/intel/common/basecode/debug/debug_feature.c @@ -10,7 +10,8 @@ #define SI_DESC_REGION_SZ 4096 struct pre_mem_ft { - uint8_t reserved[64]; + uint8_t cse_fw_update_disable; /* Byte location: 0xF00 */ + uint8_t reserved[63]; }; static struct pre_mem_ft pre_mem_debug; @@ -18,6 +19,14 @@ _Static_assert(sizeof(struct pre_mem_ft) % 64 == 0 && sizeof(struct pre_mem_ft) <= 256, "sizeof(struct pre_mem_ft) must be a multiple of 64 bytes and up to 256 bytes"); +bool is_debug_cse_fw_update_disable(void) +{ + printk(BIOS_DEBUG, "rt_debug: pre_mem_debug.cse_fw_update_disable=%d\n", + pre_mem_debug.cse_fw_update_disable); + + return pre_mem_debug.cse_fw_update_disable == 1; +} + uint8_t pre_mem_debug_init(void) { if (spi_flash_read(boot_device_spi_flash(), PRE_MEM_FEATURE_CTRL_OFFSET, diff --git a/src/soc/intel/common/basecode/include/intelbasecode/debug_feature.h b/src/soc/intel/common/basecode/include/intelbasecode/debug_feature.h index fabf27d..51e3f4d 100644 --- a/src/soc/intel/common/basecode/include/intelbasecode/debug_feature.h +++ b/src/soc/intel/common/basecode/include/intelbasecode/debug_feature.h @@ -5,6 +5,9 @@ #include <types.h> +/* Check if CSE firmware update is enabled or not */ +bool is_debug_cse_fw_update_disable(void); + /* * Reads OEM Section area in the Descriptor Region and * populates pre_mem_debug structure. diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c index ca6d7d3..83f5eb1 100644 --- a/src/soc/intel/common/block/cse/cse_lite.c +++ b/src/soc/intel/common/block/cse/cse_lite.c @@ -1,15 +1,16 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <arch/cpu.h> #include <console/console.h> #include <cbfs.h> #include <commonlib/region.h> #include <fmap.h> #include <intelblocks/cse.h> #include <intelblocks/cse_layout.h> +#include <intelbasecode/debug_feature.h> #include <security/vboot/vboot_common.h> #include <security/vboot/misc.h> #include <soc/intel/common/reset.h> -#include <arch/cpu.h> #define BPDT_HEADER_SZ sizeof(struct bpdt_header) #define BPDT_ENTRY_SZ sizeof(struct bpdt_entry) @@ -663,6 +664,17 @@ return true; } +static bool is_cse_fw_update_enabled(void) +{ + if (!CONFIG(SOC_INTEL_CSE_RW_UPDATE)) + return false; + + if (CONFIG(SOC_INTEL_COMMON_BASECODE_DEBUG_FEATURE)) + return !is_debug_cse_fw_update_disable(); + + return true; +} + static enum csme_failure_reason cse_update_rw(const struct cse_bp_info *cse_bp_info, const void *cse_cbfs_rw, const size_t cse_blob_sz, struct region_device *target_rdev) @@ -1079,10 +1091,11 @@ cse_trigger_vboot_recovery(CSE_LITE_SKU_DATA_WIPE_ERROR); /* - * If SOC_INTEL_CSE_RW_UPDATE is defined , then trigger CSE firmware update. The driver - * triggers recovery if CSE CBFS RW metadata or CSE CBFS RW blob is not available. + * cse firmware update is skipped if SOC_INTEL_CSE_RW_UPDATE is not defined and + * runtime debug control flag is not enabled. The driver triggers recovery if CSE CBFS + * RW metadata or CSE CBFS RW blob is not available. */ - if (CONFIG(SOC_INTEL_CSE_RW_UPDATE)) { + if (is_cse_fw_update_enabled()) { uint8_t rv; rv = cse_fw_update(&cse_bp_info.bp_info); if (rv) 7 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. -- To view, visit https://review.coreboot.org/c/coreboot/+/62715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f234b142191eb83137d5d83f21e890e1cb828ba Gerrit-Change-Number: 62715 Gerrit-PatchSet: 11 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)gmail.com> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/intel/common: Fix buggy code tries to access DESC region
by Felix Held (Code Review) 25 Apr '22

25 Apr '22

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/63734 ) Change subject: soc/intel/common: Fix buggy code tries to access DESC region ...................................................................... soc/intel/common: Fix buggy code tries to access DESC region The patch fixes the buggy code which tries to access the Descriptor Region. The existing code doesn't use correct APIs to access the Descriptor Region. Hence, error message is getting displayed during the boot. BUG=b:229003612 TEST=Build and verify no errors seen while accessing the Descriptor Region. Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Change-Id: Ib144cc0845b7527e5a3032529b0802f961944b87 Reviewed-on: https://review.coreboot.org/c/coreboot/+/63734 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/soc/intel/common/basecode/debug/debug_feature.c 1 file changed, 3 insertions(+), 18 deletions(-) Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved diff --git a/src/soc/intel/common/basecode/debug/debug_feature.c b/src/soc/intel/common/basecode/debug/debug_feature.c index 5cdbaf7..42cf992 100644 --- a/src/soc/intel/common/basecode/debug/debug_feature.c +++ b/src/soc/intel/common/basecode/debug/debug_feature.c @@ -1,10 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ -#include <boot_device.h> -#include <commonlib/region.h> #include <intelbasecode/debug_feature.h> #include <console/console.h> -#include <fmap.h> +#include <spi_flash.h> #define SI_DESC_OEM_SECTION_OFFSET 0xF00 #define PRE_MEM_FEATURE_CTRL_OFFSET SI_DESC_OEM_SECTION_OFFSET @@ -22,21 +20,8 @@ uint8_t pre_mem_debug_init(void) { - struct region_device desc_rdev; - const struct region_device *boot_device = boot_device_ro(); - - if (!boot_device) { - printk(BIOS_ERR, "Failed to get RW boot device\n"); - return 1; - } - - if (rdev_chain(&desc_rdev, boot_device, 0, SI_DESC_REGION_SZ)) { - printk(BIOS_ERR, "Failed to get description region device\n"); - return 1; - } - - if (rdev_readat(&desc_rdev, &pre_mem_debug, PRE_MEM_FEATURE_CTRL_OFFSET, - PRE_MEM_FEATURE_CTRL_SZ) != PRE_MEM_FEATURE_CTRL_SZ) { + if (spi_flash_read(boot_device_spi_flash(), PRE_MEM_FEATURE_CTRL_OFFSET, + PRE_MEM_FEATURE_CTRL_SZ, &pre_mem_debug)) { printk(BIOS_ERR, "Failed to read Descriptor Region from SPI Flash\n"); return 1; } -- To view, visit https://review.coreboot.org/c/coreboot/+/63734 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib144cc0845b7527e5a3032529b0802f961944b87 Gerrit-Change-Number: 63734 Gerrit-PatchSet: 7 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/amd/sabrina: Modify start address of PSP verstage
by Felix Held (Code Review) 25 Apr '22

25 Apr '22

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/63732 ) Change subject: soc/amd/sabrina: Modify start address of PSP verstage ...................................................................... soc/amd/sabrina: Modify start address of PSP verstage PSP verstage can start at address 0 and use 200KB of PSP SRAM for execution. Modify both the PSP SRAM start address and size for use by PSP verstage. BUG=b:220848544 TEST=Build Skyrim BIOS image with PSP verstage enabled. Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Change-Id: I73e13b82faa0f443570a0c839e7699a79bdae024 Reviewed-on: https://review.coreboot.org/c/coreboot/+/63732 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> --- M src/soc/amd/sabrina/include/soc/psp_verstage_addr.h 1 file changed, 5 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved diff --git a/src/soc/amd/sabrina/include/soc/psp_verstage_addr.h b/src/soc/amd/sabrina/include/soc/psp_verstage_addr.h index 6636ea1..c8f07c9 100644 --- a/src/soc/amd/sabrina/include/soc/psp_verstage_addr.h +++ b/src/soc/amd/sabrina/include/soc/psp_verstage_addr.h @@ -6,12 +6,12 @@ #define AMD_SABRINA_PSP_VERSTAGE_ADDR_H /* - * Start of available space is 0x36000 and this is where the + * Start of available space is 0x0 and this is where the * header for the user app (verstage) must be mapped. - * Size is 0x14000 bytes + * Size is 208KB */ -#define PSP_SRAM_START 0x26000 -#define PSP_SRAM_SIZE (148K) +#define PSP_SRAM_START 0x0 +#define PSP_SRAM_SIZE (208K) #define VERSTAGE_START PSP_SRAM_START /* @@ -19,7 +19,7 @@ * and make the size a multiple of 4k */ -#define PSP_VERSTAGE_STACK_START 0x41000 +#define PSP_VERSTAGE_STACK_START 0x2a000 #define PSP_VERSTAGE_STACK_SIZE (40K) #endif /* AMD_SABRINA_PSP_VERSTAGE_ADDR_H */ 2 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. -- To view, visit https://review.coreboot.org/c/coreboot/+/63732 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I73e13b82faa0f443570a0c839e7699a79bdae024 Gerrit-Change-Number: 63732 Gerrit-PatchSet: 5 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.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: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Avinash Alevoor Gerrit-CC: Mohan Viswanathan Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: mb/google/skyrim: Prepare for enabling PSP verstage
by Felix Held (Code Review) 25 Apr '22

25 Apr '22

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/63731 ) Change subject: mb/google/skyrim: Prepare for enabling PSP verstage ...................................................................... mb/google/skyrim: Prepare for enabling PSP verstage Add various verstage init functions to prepare for enable PSP verstage. BUG=None TEST=Build Skyrim BIOS image with PSP verstage enabled. Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Change-Id: I0d0dba05d4d083e2c6860078676e59cf8f487c87 Reviewed-on: https://review.coreboot.org/c/coreboot/+/63731 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> --- M src/mainboard/google/skyrim/Makefile.inc M src/mainboard/google/skyrim/variants/baseboard/Makefile.inc A src/mainboard/google/skyrim/verstage.c 3 files changed, 40 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved diff --git a/src/mainboard/google/skyrim/Makefile.inc b/src/mainboard/google/skyrim/Makefile.inc index f76a961..3a91cff 100644 --- a/src/mainboard/google/skyrim/Makefile.inc +++ b/src/mainboard/google/skyrim/Makefile.inc @@ -8,6 +8,8 @@ ramstage-y += ec.c ramstage-$(CONFIG_CHROMEOS) += chromeos.c +verstage-$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += verstage.c + subdirs-y += variants/baseboard subdirs-y += variants/$(VARIANT_DIR) diff --git a/src/mainboard/google/skyrim/variants/baseboard/Makefile.inc b/src/mainboard/google/skyrim/variants/baseboard/Makefile.inc index 7706e50..2864241 100644 --- a/src/mainboard/google/skyrim/variants/baseboard/Makefile.inc +++ b/src/mainboard/google/skyrim/variants/baseboard/Makefile.inc @@ -7,5 +7,6 @@ ramstage-y += tpm_tis.c verstage-y += tpm_tis.c +verstage-$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += gpio.c smm-y += gpio.c diff --git a/src/mainboard/google/skyrim/verstage.c b/src/mainboard/google/skyrim/verstage.c new file mode 100644 index 0000000..8aaf70c --- /dev/null +++ b/src/mainboard/google/skyrim/verstage.c @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <amdblocks/acpimmio.h> +#include <amdblocks/gpio.h> +#include <arch/io.h> +#include <baseboard/variants.h> +#include <psp_verstage.h> +#include <security/vboot/vboot_common.h> +#include <soc/southbridge.h> + +void verstage_mainboard_early_init(void) +{ + const struct soc_amd_gpio *gpios, *override_gpios; + size_t num_gpios, override_num_gpios; + + variant_early_gpio_table(&gpios, &num_gpios); + variant_early_override_gpio_table(&override_gpios, &override_num_gpios); + gpio_configure_pads_with_override(gpios, num_gpios, override_gpios, override_num_gpios); +} + +void verstage_mainboard_espi_init(void) +{ + const struct soc_amd_gpio *gpios; + size_t num_gpios; + + variant_espi_gpio_table(&gpios, &num_gpios); + gpio_configure_pads(gpios, num_gpios); +} + +void verstage_mainboard_tpm_init(void) +{ + const struct soc_amd_gpio *gpios; + size_t num_gpios; + + variant_tpm_gpio_table(&gpios, &num_gpios); + gpio_configure_pads(gpios, num_gpios); +} 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. -- To view, visit https://review.coreboot.org/c/coreboot/+/63731 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0d0dba05d4d083e2c6860078676e59cf8f487c87 Gerrit-Change-Number: 63731 Gerrit-PatchSet: 4 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: soc/intel/cmn/cse: Enforce CSE disabling
by Sridhar Siricilla (Code Review) 25 Apr '22

25 Apr '22

Attention is currently required from: SRIDHAR SIRICILLA, Subrata Banik, Tim Wawrzynczak, Angel Pons, Arthur Heymans, Eric Lai. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63822 ) Change subject: soc/intel/cmn/cse: Enforce CSE disabling ...................................................................... Patch Set 3: (1 comment) File src/soc/intel/common/block/cse/cse.c: https://review.coreboot.org/c/coreboot/+/63822/comment/a362a067_ca8d5f6e PS1, Line 1206: vboot_recovery_mode_enabled() && > > Ack […] >Also, looking for your response to avoid 2 questions. if we can remove CSE Lite check from cse_eop.c as well ? EOP is different case from hiding ME Devices when CSE's current operation mode is soft temporary disable mode. an you please refer section#3.4.9.4 in ADL CSME 16.0 BIOS section for recommendation? -- To view, visit https://review.coreboot.org/c/coreboot/+/63822 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I77c254195895a93a5606adee8b6f43d8b7100848 Gerrit-Change-Number: 63822 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: SRIDHAR SIRICILLA Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: SRIDHAR SIRICILLA Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Mon, 25 Apr 2022 13:41:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: include/device/i2c_simple: add i2c_read_eeprom_bytes function
by Fred Reitberger (Code Review) 25 Apr '22

25 Apr '22

Attention is currently required from: Raul Rangel, Julius Werner, Kyösti Mälkki, Felix Held. Fred Reitberger has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63791 ) Change subject: include/device/i2c_simple: add i2c_read_eeprom_bytes function ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I think "read_eeprom" is a risky name choice. […] Would something like a generic write_read_bytes() name make more sense where you pass in a byte array and length for the address portion? Then read_bytes() could be refactored to use this more generic function. -- To view, visit https://review.coreboot.org/c/coreboot/+/63791 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I224e434bb2654aabef6302c1525112e44c4b21fa Gerrit-Change-Number: 63791 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 25 Apr 2022 11:56:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/dedede/beadrix: Update DPTF setting
by Teddy Shih (Code Review) 25 Apr '22

25 Apr '22

Teddy Shih has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63824 ) Change subject: mb/google/dedede/beadrix: Update DPTF setting ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/63824 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I34c1298dc8412121f8688842bb8d69d7fafa46f6 Gerrit-Change-Number: 63824 Gerrit-PatchSet: 1 Gerrit-Owner: Teddy Shih <teddyshih(a)ami.corp-partner.google.com> Gerrit-Reviewer: Teddy Shih <teddyshih(a)ami.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eddy Lu <eddylu(a)ami.corp-partner.google.com> Gerrit-CC: Raymond Chung <raymondchung(a)ami.corp-partner.google.com> Gerrit-Comment-Date: Mon, 25 Apr 2022 11:51:09 +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 April 2022