coreboot-gerrit January 2024

coreboot-gerrit@coreboot.org

1 participants
3043 discussions

[S] Change in coreboot[main]: [RFC] region: Hide struct region members
by Nico Huber (Code Review) 11 Jan '24

11 Jan '24

Attention is currently required from: Jakub Czapiga, Julius Werner, Philipp Hug, ron minnich. Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79907?usp=email ) Change subject: [RFC] region: Hide struct region members ...................................................................... [RFC] region: Hide struct region members We cannot make `struct region` opaque for several reasons (size needs to be known for local objects, a lot of inline API functions, ...). However, we still want to encourage using the high-level region API instead of directly accessing and manipulating the struct members. This patch tries to achieve this by renaming the struct members. Only if REGION_INTERNAL_STRUCTURES is defined before including <region.h>, the usual member names are used. Change-Id: I713be9cf0bab4c2e21113b55e7229ab50f06c6cf Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M src/arch/arm/fit_payload.c M src/arch/arm64/fit_payload.c M src/arch/riscv/fit_payload.c M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/lib/fit_payload.c M tests/commonlib/region-test.c M tests/lib/cbfs-lookup-test.c M tests/lib/fmap-test.c 9 files changed, 26 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/79907/1 diff --git a/src/arch/arm/fit_payload.c b/src/arch/arm/fit_payload.c index 19f46a8..84c6dbb 100644 --- a/src/arch/arm/fit_payload.c +++ b/src/arch/arm/fit_payload.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ +#define REGION_INTERNAL_STRUCTURES /* FIXME: should use the high-level region api */ #include <bootmem.h> #include <program_loading.h> #include <fit.h> diff --git a/src/arch/arm64/fit_payload.c b/src/arch/arm64/fit_payload.c index f8ae16a..f6d6386 100644 --- a/src/arch/arm64/fit_payload.c +++ b/src/arch/arm64/fit_payload.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ +#define REGION_INTERNAL_STRUCTURES /* FIXME: should use the high-level region api */ #include <commonlib/bsd/compression.h> #include <console/console.h> #include <bootmem.h> diff --git a/src/arch/riscv/fit_payload.c b/src/arch/riscv/fit_payload.c index bfa0c98..c879b9e 100644 --- a/src/arch/riscv/fit_payload.c +++ b/src/arch/riscv/fit_payload.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ +#define REGION_INTERNAL_STRUCTURES /* FIXME: should use the high-level region api */ #include <commonlib/bsd/compression.h> #include <console/console.h> #include <bootmem.h> diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index a730c58..4fdbb80 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -73,9 +73,17 @@ ssize_t (*eraseat)(const struct region_device *, size_t, size_t); }; +#ifdef REGION_INTERNAL_STRUCTURES +#define _region_offset offset +#define _region_size size +#else +#define _region_offset _region_internal_only__offset +#define _region_size _region_internal_only__size +#endif + struct region { - size_t offset; - size_t size; + size_t _region_offset; + size_t _region_size; }; struct region_device { @@ -89,15 +97,15 @@ .root = NULL, \ .ops = (ops_), \ .region = { \ - .offset = (offset_), \ - .size = (size_), \ + (offset_), \ + (size_), \ }, \ } static inline struct region region_create(size_t offset, size_t size) { assert(offset + size - 1 >= offset); - return (struct region){ .offset = offset, .size = size }; + return (struct region){ ._region_offset = offset, ._region_size = size }; } static inline int region_create_untrusted( @@ -114,14 +122,17 @@ static inline size_t region_offset(const struct region *r) { - return r->offset; + return r->_region_offset; } static inline size_t region_sz(const struct region *r) { - return r->size; + return r->_region_size; } +#undef _region_offset +#undef _region_size + static inline unsigned long long region_end(const struct region *r) { return region_offset(r) + region_sz(r); diff --git a/src/commonlib/region.c b/src/commonlib/region.c index 4153f0a..96c70fa 100644 --- a/src/commonlib/region.c +++ b/src/commonlib/region.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#define REGION_INTERNAL_STRUCTURES #include <commonlib/helpers.h> #include <commonlib/region.h> #include <stdint.h> diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index 21bc4e8..64ab4fb 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#define REGION_INTERNAL_STRUCTURES /* FIXME: should use the high-level region api */ #include <commonlib/bsd/compression.h> #include <console/console.h> #include <bootmem.h> diff --git a/tests/commonlib/region-test.c b/tests/commonlib/region-test.c index 3280482..fd82fa6 100644 --- a/tests/commonlib/region-test.c +++ b/tests/commonlib/region-test.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#define REGION_INTERNAL_STRUCTURES #include <commonlib/region.h> #include <string.h> #include <tests/test.h> diff --git a/tests/lib/cbfs-lookup-test.c b/tests/lib/cbfs-lookup-test.c index 7562868..8e46e73 100644 --- a/tests/lib/cbfs-lookup-test.c +++ b/tests/lib/cbfs-lookup-test.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#define REGION_INTERNAL_STRUCTURES #include <assert.h> #include <cbfs.h> #include <commonlib/bsd/cbfs_mdata.h> diff --git a/tests/lib/fmap-test.c b/tests/lib/fmap-test.c index 1cc8246..43c046f 100644 --- a/tests/lib/fmap-test.c +++ b/tests/lib/fmap-test.c @@ -5,6 +5,7 @@ #include <tests/test.h> +#define REGION_INTERNAL_STRUCTURES #include <fmap.h> #include <commonlib/region.h> -- To view, visit https://review.coreboot.org/c/coreboot/+/79907?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: I713be9cf0bab4c2e21113b55e7229ab50f06c6cf Gerrit-Change-Number: 79907 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: ron minnich <rminnich(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[main]: [RFC] region: Allow region_end() to return 64-bit numbers
by Nico Huber (Code Review) 11 Jan '24

11 Jan '24

Attention is currently required from: Jérémy Compostella. Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79906?usp=email ) Change subject: [RFC] region: Allow region_end() to return 64-bit numbers ...................................................................... [RFC] region: Allow region_end() to return 64-bit numbers We most likely operate on regions that end exactly at the 4GiB boundary (e.g. the boot device on x86). The current code seem to work fine even if this overflows the `size_t` return value of region_end(). Though, to be accurate, we should allow this value. Change-Id: Id261d72c58659f180ca8e5cb6c4bcfc3855ad903 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M src/commonlib/include/commonlib/region.h M src/cpu/x86/smm/smm_module_loader.c M util/cbfstool/cbfstool.c 3 files changed, 5 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/79906/1 diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 8de3c5d..a730c58 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -122,7 +122,7 @@ return r->size; } -static inline size_t region_end(const struct region *r) +static inline unsigned long long region_end(const struct region *r) { return region_offset(r) + region_sz(r); } @@ -154,7 +154,7 @@ return region_offset(region_device_region(rdev)); } -static inline size_t region_device_end(const struct region_device *rdev) +static inline unsigned long long region_device_end(const struct region_device *rdev) { return region_end(region_device_region(rdev)); } diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 39b36e61..235ef5c 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -348,7 +348,7 @@ static void print_region(const char *name, const struct region region) { - printk(BIOS_DEBUG, "%-12s [0x%zx-0x%zx]\n", name, region_offset(&region), + printk(BIOS_DEBUG, "%-12s [0x%zx-0x%llx]\n", name, region_offset(&region), region_end(&region)); } diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 10d6501..0719318 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -421,7 +421,7 @@ for (int j = i + 1; j < mmap_window_table_size; j++) { if (region_overlap(&mmap_window_table[i].flash_space, &mmap_window_table[j].flash_space)) { - ERROR("Flash space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", + ERROR("Flash space windows (base=0x%zx, limit=0x%llx) and (base=0x%zx, limit=0x%llx) overlap!\n", region_offset(&mmap_window_table[i].flash_space), region_end(&mmap_window_table[i].flash_space), region_offset(&mmap_window_table[j].flash_space), @@ -431,7 +431,7 @@ if (region_overlap(&mmap_window_table[i].host_space, &mmap_window_table[j].host_space)) { - ERROR("Host space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n", + ERROR("Host space windows (base=0x%zx, limit=0x%llx) and (base=0x%zx, limit=0x%llx) overlap!\n", region_offset(&mmap_window_table[i].flash_space), region_end(&mmap_window_table[i].flash_space), region_offset(&mmap_window_table[j].flash_space), -- To view, visit https://review.coreboot.org/c/coreboot/+/79906?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: Id261d72c58659f180ca8e5cb6c4bcfc3855ad903 Gerrit-Change-Number: 79906 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: newchange

1 0

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

11 Jan '24

Attention is currently required from: Jérémy Compostella. Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79905?usp=email ) Change subject: [RFC] region: Introduce region_create() functions ...................................................................... [RFC] region: Introduce region_create() functions This introduces two new functions to create region objects. They allow us to check for integer overflows (region_create_untrusted()) or assert their absence (region_create()). Again, FIT payload support is left out, as it doesn't use the region API (only the struct). Change-Id: I4ae3e6274c981c9ab4fb1263c2a72fa68ef1c32b Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M src/commonlib/include/commonlib/region.h M src/cpu/x86/smm/smm_module_loader.c M src/drivers/spi/spi_flash.c M src/include/cpu/x86/smm.h M src/lib/fmap.c M util/cbfstool/cbfstool.c M util/cbfstool/cse_serger.c 7 files changed, 77 insertions(+), 46 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/79905/1 diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 25efcc8..8de3c5d 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -94,10 +94,20 @@ }, \ } -/* Helper to dynamically initialize region device. */ -void region_device_init(struct region_device *rdev, - const struct region_device_ops *ops, size_t offset, - size_t size); +static inline struct region region_create(size_t offset, size_t size) +{ + assert(offset + size - 1 >= offset); + return (struct region){ .offset = offset, .size = size }; +} + +static inline int region_create_untrusted( + struct region *r, unsigned long long offset, unsigned long long size) +{ + if (offset > SIZE_MAX || size > SIZE_MAX || offset + size - 1 < offset) + return -1; + *r = region_create(offset, size); + return 0; +} /* Return 1 if child is subregion of parent, else 0. */ int region_is_subregion(const struct region *p, const struct region *c); @@ -123,6 +133,11 @@ (region_offset(r1) < region_end(r2)); } +/* Helper to dynamically initialize region device. */ +void region_device_init(struct region_device *rdev, + const struct region_device_ops *ops, size_t offset, + size_t size); + static inline const struct region *region_device_region( const struct region_device *rdev) { diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index d965593..39b36e61 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -113,11 +113,10 @@ const size_t segment_number = i / cpus_per_segment; cpus[i].smbase = smbase - SMM_CODE_SEGMENT_SIZE * segment_number - needed_ss_size * (i % cpus_per_segment); - cpus[i].stub_code.offset = cpus[i].smbase + SMM_ENTRY_OFFSET; - cpus[i].stub_code.size = stub_size; - cpus[i].ss.offset = cpus[i].smbase + SMM_CODE_SEGMENT_SIZE - - params->cpu_save_state_size; - cpus[i].ss.size = params->cpu_save_state_size; + cpus[i].stub_code = region_create(cpus[i].smbase + SMM_ENTRY_OFFSET, stub_size); + cpus[i].ss = region_create( + cpus[i].smbase + SMM_CODE_SEGMENT_SIZE - params->cpu_save_state_size, + params->cpu_save_state_size); cpus[i].active = 1; } @@ -425,7 +424,7 @@ if (rmodule_parse(&_binary_smm_start, &smi_handler)) return -1; - const struct region smram = { .offset = smram_base, .size = smram_size }; + const struct region smram = region_create(smram_base, smram_size); const uintptr_t smram_top = region_end(&smram); const size_t stm_size = @@ -433,8 +432,7 @@ if (CONFIG(STM)) { struct region stm = {}; - stm.offset = smram_top - stm_size; - stm.size = stm_size; + stm = region_create(smram_top - stm_size, stm_size); if (append_and_check_region(smram, stm, region_list, "STM")) return -1; printk(BIOS_DEBUG, "MSEG size 0x%x\n", CONFIG_MSEG_SIZE); @@ -446,10 +444,7 @@ const uintptr_t handler_base = ALIGN_DOWN(smram_top - stm_size - handler_size, handler_alignment); - struct region handler = { - .offset = handler_base, - .size = handler_size - }; + struct region handler = region_create(handler_base, handler_size); if (append_and_check_region(smram, handler, region_list, "HANDLER")) return -1; @@ -470,10 +465,8 @@ return -1; } - struct region stacks = { - .offset = smram_base, - .size = params->num_concurrent_save_states * CONFIG_SMM_MODULE_STACK_SIZE - }; + struct region stacks = region_create(smram_base, + params->num_concurrent_save_states * CONFIG_SMM_MODULE_STACK_SIZE); printk(BIOS_DEBUG, "\n"); if (append_and_check_region(smram, stacks, region_list, "stacks")) return -1; diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index 42952df..11597c6 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -610,12 +610,12 @@ int spi_flash_is_write_protected(const struct spi_flash *flash, const struct region *region) { - struct region flash_region = { 0 }; + struct region flash_region; if (!flash || !region) return -1; - flash_region.size = flash->size; + flash_region = region_create(0, flash->size); if (!region_is_subregion(&flash_region, region)) return -1; @@ -633,13 +633,13 @@ const struct region *region, const enum spi_flash_status_reg_lockdown mode) { - struct region flash_region = { 0 }; + struct region flash_region; int ret; if (!flash) return -1; - flash_region.size = flash->size; + flash_region = region_create(0, flash->size); if (!region_is_subregion(&flash_region, region)) return -1; @@ -755,12 +755,12 @@ const enum ctrlr_prot_type type) { const struct spi_ctrlr *ctrlr; - struct region flash_region = { 0 }; + struct region flash_region; if (!flash) return -1; - flash_region.size = flash->size; + flash_region = region_create(0, flash->size); if (!region_is_subregion(&flash_region, region)) return -1; diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index dfb27cd..1c49842 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -137,7 +137,10 @@ /* Returns true if the memory pointed to overlaps with SMM reserved memory. */ static inline bool smm_points_to_smram(const void *ptr, const size_t len) { - const struct region r = {(uintptr_t)ptr, len}; + struct region r; + + if (region_create_untrusted(&r, (uintptr_t)ptr, len)) + return true; /* Play it safe and pretend it overlaps if we can't tell. */ return smm_region_overlaps_handler(&r); } diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 75c5a9f..80fb0b2 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -199,8 +199,7 @@ printk(BIOS_DEBUG, "FMAP: area %s found @ %x (%d bytes)\n", name, le32toh(area->offset), le32toh(area->size)); - ar->offset = le32toh(area->offset); - ar->size = le32toh(area->size); + *ar = region_create(le32toh(area->offset), le32toh(area->size)); rdev_munmap(&fmrd, area); diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index e11cfbc..10d6501 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -324,19 +324,25 @@ static int mmap_window_table_size; static struct mmap_window mmap_window_table[MMAP_MAX_WINDOWS]; -static void add_mmap_window(size_t flash_offset, size_t host_offset, - size_t window_size) +static int add_mmap_window(unsigned long flash_offset, unsigned long host_offset, unsigned long window_size) { if (mmap_window_table_size >= MMAP_MAX_WINDOWS) { ERROR("Too many memory map windows\n"); - return; + return 1; } - mmap_window_table[mmap_window_table_size].flash_space.offset = flash_offset; - mmap_window_table[mmap_window_table_size].host_space.offset = host_offset; - mmap_window_table[mmap_window_table_size].flash_space.size = window_size; - mmap_window_table[mmap_window_table_size].host_space.size = window_size; + if (region_create_untrusted( + &mmap_window_table[mmap_window_table_size].flash_space, + flash_offset, window_size) || + region_create_untrusted( + &mmap_window_table[mmap_window_table_size].host_space, + host_offset, window_size)) { + ERROR("Invalid mmap window size %zu.\n", window_size); + return 1; + } + mmap_window_table_size++; + return 0; } @@ -377,7 +383,9 @@ return 1; } - add_mmap_window(mmap_args.flash_base, mmap_args.mmap_base, mmap_args.mmap_size); + if (add_mmap_window(mmap_args.flash_base, mmap_args.mmap_base, mmap_args.mmap_size)) + return 1; + return 0; } @@ -403,7 +411,8 @@ * maximum of 16MiB. If the window is smaller than 16MiB, the SPI flash window is mapped * at the top of the host window just below 4G. */ - add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size); + if (add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size)) + return false; } else { /* * Check provided memory map diff --git a/util/cbfstool/cse_serger.c b/util/cbfstool/cse_serger.c index f53b390..9b776e6 100644 --- a/util/cbfstool/cse_serger.c +++ b/util/cbfstool/cse_serger.c @@ -820,15 +820,22 @@ return 0; } -static void parse_region(struct region *r, char *arg) +static int parse_region(struct region *r, char *arg) { char *tok; tok = strtok(arg, ":"); - r->offset = strtol(tok, NULL, 0); + unsigned long offset = strtol(tok, NULL, 0); tok = strtok(NULL, ":"); - r->size = strtol(tok, NULL, 0); + unsigned long size = strtol(tok, NULL, 0); + + if (region_create_untrusted(r, offset, size)) { + ERROR("Invalid region: %lx:%lx\n", offset, size); + return -1; + } + + return 0; } static struct command { @@ -991,19 +998,24 @@ params.partition_type = atoi(optarg); break; case LONGOPT_BP1: - parse_region(&params.layout_regions[BP1], optarg); + if (parse_region(&params.layout_regions[BP1], optarg)) + return 1; break; case LONGOPT_BP2: - parse_region(&params.layout_regions[BP2], optarg); + if (parse_region(&params.layout_regions[BP2], optarg)) + return 1; break; case LONGOPT_BP3: - parse_region(&params.layout_regions[BP3], optarg); + if (parse_region(&params.layout_regions[BP3], optarg)) + return 1; break; case LONGOPT_BP4: - parse_region(&params.layout_regions[BP4], optarg); + if (parse_region(&params.layout_regions[BP4], optarg)) + return 1; break; case LONGOPT_DATA: - parse_region(&params.layout_regions[DP], optarg); + if (parse_region(&params.layout_regions[DP], optarg)) + return 1; break; case LONGOPT_BP1_FILE: params.layout_files[BP1] = optarg; -- 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: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[main]: tree: Use accessor functions for struct region fields
by Nico Huber (Code Review) 11 Jan '24

11 Jan '24

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79904?usp=email ) Change subject: tree: Use accessor functions for struct region fields ...................................................................... tree: Use accessor functions for struct region fields Always use the high-level API region_offset() and region_sz() functions. This excludes the internal `region.c` code as well as unit tests. FIT payload support was also skipped, as it seems it never tried to use the API and would need a bigger overhaul. Change-Id: Iaae116a1ab2da3b2ea2a5ebcd0c300b238582834 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M src/cpu/x86/smm/smm_module_loader.c M src/lib/fmap.c M util/cbfstool/bpdt_formats/bpdt_1_6.c M util/cbfstool/bpdt_formats/bpdt_1_7.c 4 files changed, 23 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/79904/1 diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index f827e7b..d965593 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -363,7 +363,7 @@ { unsigned int region_counter = 0; for (; region_counter < SMM_REGIONS_ARRAY_SIZE; region_counter++) - if (region_list[region_counter].size == 0) + if (region_sz(&region_list[region_counter]) == 0) break; if (region_counter >= SMM_REGIONS_ARRAY_SIZE) { diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 8901ff7..75c5a9f 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -235,15 +235,15 @@ if (area == NULL) return -1; - if ((ar->offset != le32toh(area->offset)) || - (ar->size != le32toh(area->size))) { + if (region_offset(ar) != le32toh(area->offset) || + region_sz(ar) != le32toh(area->size)) { rdev_munmap(&fmrd, area); offset += sizeof(struct fmap_area); continue; } printk(BIOS_DEBUG, "FMAP: area (%zx, %zx) found, named %s\n", - ar->offset, ar->size, area->name); + region_offset(ar), region_sz(ar), area->name); memcpy(name, area->name, FMAP_STRLEN); @@ -253,7 +253,7 @@ } printk(BIOS_DEBUG, "FMAP: area (%zx, %zx) not found\n", - ar->offset, ar->size); + region_offset(ar), region_sz(ar)); return -1; } diff --git a/util/cbfstool/bpdt_formats/bpdt_1_6.c b/util/cbfstool/bpdt_formats/bpdt_1_6.c index c5cedd3..477a58c 100644 --- a/util/cbfstool/bpdt_formats/bpdt_1_6.c +++ b/util/cbfstool/bpdt_formats/bpdt_1_6.c @@ -149,14 +149,14 @@ if (!l) return NULL; - l->data_offset = r[DP].offset; - l->data_size = r[DP].size; - l->bp1_offset = r[BP1].offset; - l->bp1_size = r[BP1].size; - l->bp2_offset = r[BP2].offset; - l->bp2_size = r[BP2].size; - l->bp3_offset = r[BP3].offset; - l->bp3_size = r[BP3].size; + l->data_offset = region_offset(&r[DP]); + l->data_size = region_sz(&r[DP]); + l->bp1_offset = region_offset(&r[BP1]); + l->bp1_size = region_sz(&r[BP1]); + l->bp2_offset = region_offset(&r[BP2]); + l->bp2_size = region_sz(&r[BP2]); + l->bp3_offset = region_offset(&r[BP3]); + l->bp3_size = region_sz(&r[BP3]); l->checksum = 0; /* unused */ return l; diff --git a/util/cbfstool/bpdt_formats/bpdt_1_7.c b/util/cbfstool/bpdt_formats/bpdt_1_7.c index 9badc82..7e42a7b 100644 --- a/util/cbfstool/bpdt_formats/bpdt_1_7.c +++ b/util/cbfstool/bpdt_formats/bpdt_1_7.c @@ -196,16 +196,16 @@ l->size = sizeof(struct cse_layout) - sizeof(l->rom_bypass); l->redundancy = 0; l->checksum = 0; - l->data_offset = r[DP].offset; - l->data_size = r[DP].size; - l->bp1_offset = r[BP1].offset; - l->bp1_size = r[BP1].size; - l->bp2_offset = r[BP2].offset; - l->bp2_size = r[BP2].size; - l->bp3_offset = r[BP3].offset; - l->bp3_size = r[BP3].size; - l->bp4_offset = r[BP4].offset; - l->bp4_size = r[BP4].size; + l->data_offset = region_offset(&r[DP]); + l->data_size = region_sz(&r[DP]); + l->bp1_offset = region_offset(&r[BP1]); + l->bp1_size = region_sz(&r[BP1]); + l->bp2_offset = region_offset(&r[BP2]); + l->bp2_size = region_sz(&r[BP2]); + l->bp3_offset = region_offset(&r[BP3]); + l->bp3_size = region_sz(&r[BP3]); + l->bp4_offset = region_offset(&r[BP4]); + l->bp4_size = region_sz(&r[BP4]); l->bp5_offset = 0; l->bp5_size = 0; l->temp_base_addr = 0; -- To view, visit https://review.coreboot.org/c/coreboot/+/79904?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: Iaae116a1ab2da3b2ea2a5ebcd0c300b238582834 Gerrit-Change-Number: 79904 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newchange

1 0

[L] Change in coreboot[main]: mb/google/poppy: Use chipset dt reference names
by Felix Singer (Code Review) 11 Jan '24

11 Jan '24

Attention is currently required from: Martin L Roth, Marvin Evers. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79439?usp=email ) Change subject: mb/google/poppy: Use chipset dt reference names ...................................................................... Patch Set 6: (3 comments) File src/mainboard/google/poppy/variants/baseboard/devicetree.cb: https://review.coreboot.org/c/coreboot/+/79439/comment/9b45a4b9_5b12ee5d : PS4, Line 254: > The commit message doesn't suggest that lines are removed and no lines were removed from the other f […] Done https://review.coreboot.org/c/coreboot/+/79439/comment/f1dedd30_5296b41d : PS4, Line 379: device pci 1f.1 on end # P2SB : > Same here. Please add these lines again. Done PS4: > Please indent the on keyword with tabs, and also in the other files. See the previous patches. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/79439?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: I22bcde2dea726f47f8d64a762ca147efde0b610d Gerrit-Change-Number: 79439 Gerrit-PatchSet: 6 Gerrit-Owner: Marvin Evers <marvin.n.evers(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-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Marvin Evers <marvin.n.evers(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Comment-Date: Thu, 11 Jan 2024 18:19:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: mb/pcengines/apu2/Kconfig: select HUDSON_FADT_LEGACY_DEVICES
by Arthur Heymans (Code Review) 11 Jan '24

11 Jan '24

Attention is currently required from: Felix Held, Michał Żygowski, Piotr Król. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79894?usp=email ) Change subject: mb/pcengines/apu2/Kconfig: select HUDSON_FADT_LEGACY_DEVICES ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/79894?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: I8872b8c3d6e0610630ba17a0fccdcf8cebb1d3c0 Gerrit-Change-Number: 79894 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Thu, 11 Jan 2024 18:17:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: sb/amd/pi/hudson/Kconfig: replace HUDSON_LEGACY_FREE option
by Arthur Heymans (Code Review) 11 Jan '24

11 Jan '24

Attention is currently required from: Felix Held, Matt DeVillier, Michał Żygowski, Piotr Król. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79893?usp=email ) Change subject: sb/amd/pi/hudson/Kconfig: replace HUDSON_LEGACY_FREE option ...................................................................... Patch Set 1: Code-Review+2 (1 comment) File src/southbridge/amd/pi/hudson/fadt.c: https://review.coreboot.org/c/coreboot/+/79893/comment/55100c4c_1930477f : PS1, Line 38: fadt->iapc_boot_arch = ACPI_FADT_LEGACY_FREE; /* See table 5-10 */ : if (CONFIG(HUDSON_FADT_LEGACY_DEVICES)) : fadt->iapc_boot_arch |= ACPI_FADT_LEGACY_DEVICES; : if (CONFIG(HUDSON_FADT_8042)) : fadt->iapc_boot_arch |= ACPI_FADT_8042; I'm curious whether the compiler creates code with one write. If you really want it in one write you could use "ACPI_FADT_LEGACY_FREE | CONFIG(HUDSON_FADT_LEGACY_DEVICES) ? ACPI_FADT_LEGACY_DEVICES : 0 ... -- To view, visit https://review.coreboot.org/c/coreboot/+/79893?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: Id4ff85630c90fb2ae8c8826bbc9049a08668210d Gerrit-Change-Number: 79893 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Thu, 11 Jan 2024 18:17:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: mb/pcengines/apu2/BiosCallOuts: don't have binaryPI clear LPC decodes
by Felix Held (Code Review) 11 Jan '24

11 Jan '24

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/79852?usp=email ) Change subject: mb/pcengines/apu2/BiosCallOuts: don't have binaryPI clear LPC decodes ...................................................................... mb/pcengines/apu2/BiosCallOuts: don't have binaryPI clear LPC decodes Tell binaryPI to not disable the LPC decodes for the IO ports used by the serial ports on the Super I/O chip during the AmdInitReset binaryPI entry point. Checked the Stoneyridge binaryPI source code which is closely enough related to be reasonable sure that this option only controls which LPC decode bits get cleared and won't have any other side effects. TEST=Now the full console output from the APU2 board gets printed on the serial console. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Suggested-by: Krystian Hebel <krystian.hebel(a)3mdeb.com> Change-Id: I91ef4423bd7bf6c1d7a175336f0f89479f2cde02 Reviewed-on: https://review.coreboot.org/c/coreboot/+/79852 Reviewed-by: Paul Menzel <paulepanter(a)mailbox.org> Reviewed-by: Krystian Hebel <krystian.hebel(a)3mdeb.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/mainboard/pcengines/apu2/BiosCallOuts.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Krystian Hebel: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve build bot (Jenkins): Verified diff --git a/src/mainboard/pcengines/apu2/BiosCallOuts.c b/src/mainboard/pcengines/apu2/BiosCallOuts.c index efd88d8..c78dda6 100644 --- a/src/mainboard/pcengines/apu2/BiosCallOuts.c +++ b/src/mainboard/pcengines/apu2/BiosCallOuts.c @@ -45,7 +45,7 @@ { printk(BIOS_DEBUG, "Fch OEM config in INIT RESET "); //FchParams_reset->EcChannel0 = TRUE; /* logical devicd 3 */ - FchParams->LegacyFree = CONFIG(HUDSON_LEGACY_FREE); + FchParams->LegacyFree = 0; /* don't clear LPC IO decodes for serial console */ FchParams->FchReset.SataEnable = hudson_sata_enable(); FchParams->FchReset.IdeEnable = hudson_ide_enable(); FchParams->FchReset.Xhci0Enable = CONFIG(HUDSON_XHCI_ENABLE); -- To view, visit https://review.coreboot.org/c/coreboot/+/79852?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: I91ef4423bd7bf6c1d7a175336f0f89479f2cde02 Gerrit-Change-Number: 79852 Gerrit-PatchSet: 4 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[main]: Documentation: Start administrator handbook
by Patrick Georgi (Code Review) 11 Jan '24

11 Jan '24

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79901?usp=email ) Change subject: Documentation: Start administrator handbook ...................................................................... Documentation: Start administrator handbook Let's spread the work of maintaining various of our services, but to achieve that, we need to document what needs to be done. Change-Id: I87021ee62d18fa464f70351ea8bad732889d55f1 Signed-off-by: Patrick Georgi <patrick(a)coreboot.org> --- A Documentation/infrastructure/admin.md M Documentation/infrastructure/index.md 2 files changed, 53 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/79901/1 diff --git a/Documentation/infrastructure/admin.md b/Documentation/infrastructure/admin.md new file mode 100644 index 0000000..c6fa781 --- /dev/null +++ b/Documentation/infrastructure/admin.md @@ -0,0 +1,52 @@ +# Operating our services + +## Mailing list moderation + +Our [mailing lists] experience the same barrage of spam mails than any +other email address. We do have a spam filter in front of it, and +since the lists require registration, spam ends up in the moderation +queue. But not only spam ends up there, sometimes users send inquiries +without registering first. It's a custom of the project to let these +through, so that such emails can be discussed. This requires manual +intervention. + +This section describes the tasks related to mailing list management. + +### Registration + +To participate in mailing list moderation, you need to become a list +moderator or owner. This is up for the existing owners to handle and +if you want to contribute in that area, it might be best to bring it +up at the leadership meeting. + +After gaining leadership approval, list admins can add you to the +appropriate group in the [mailing list backend] by selecting the list, +then User / group-name, and add your email address there. + +### Regular tasks + +Most of our lists are auto-subscribing, so users can register +themselves and finish the process by responding to the double-opt-in +email. Some lists are manually managed though. The [mailing list +backend] shows the number of open subscription requests for these +lists on the mailing list's main page. + +It also provides a list of held messages, where they can be accepted, +rejected or dropped. Spam should be dropped, that's clear. Emails with +huge attachments (e.g. screenshots) should be rejected, which gives +you an opportunity to explain the reason (in case of large +attachments, something like "Please re-send without attachments, offer +the files through some other mechanism please: Our emails are +distributed to hundreds of readers, and sending the files to everybody +is inconsiderate of traffic and storage constraints.") + +Legit emails (often simple requests of the form "is this or that +supported") can be accepted, which means they'll the sent out. + +If you notice recurring spam sources (e.g. marketers) you can put them +on the [global ban list] to filter them out across all lists. It takes +entries in regular expression format. + +[mailing lists]: https://mail.coreboot.org/hyperkitty/ +[mailing list backend]: https://mail.coreboot.org/postorius/ +[global ban list]: https://mail.coreboot.org/postorius/bans/ diff --git a/Documentation/infrastructure/index.md b/Documentation/infrastructure/index.md index 744809f..6748637 100644 --- a/Documentation/infrastructure/index.md +++ b/Documentation/infrastructure/index.md @@ -5,7 +5,7 @@ ## Services * [Project services](services.md) - +* [Administrator's handbook](admin.md) ## Jenkins builders and builds * [Setting up Jenkins build machines](builders.md) -- To view, visit https://review.coreboot.org/c/coreboot/+/79901?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: I87021ee62d18fa464f70351ea8bad732889d55f1 Gerrit-Change-Number: 79901 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <patrick(a)coreboot.org> Gerrit-MessageType: newchange

1 0

[XS] Change in coreboot[main]: mb/starlabs/starbook/rpl: Enable C1e
by Felix Held (Code Review) 11 Jan '24

11 Jan '24

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/79379?usp=email ) Change subject: mb/starlabs/starbook/rpl: Enable C1e ...................................................................... mb/starlabs/starbook/rpl: Enable C1e Signed-off-by: Sean Rhodes <sean(a)starlabs.systems> Change-Id: I3a317d031e71f86afc50b229d1b97197552f4fa9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/79379 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> --- M src/mainboard/starlabs/starbook/variants/rpl/devicetree.cb 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: Matt DeVillier: Looks good to me, approved build bot (Jenkins): Verified diff --git a/src/mainboard/starlabs/starbook/variants/rpl/devicetree.cb b/src/mainboard/starlabs/starbook/variants/rpl/devicetree.cb index 1eaab18..160656d 100644 --- a/src/mainboard/starlabs/starbook/variants/rpl/devicetree.cb +++ b/src/mainboard/starlabs/starbook/variants/rpl/devicetree.cb @@ -5,6 +5,7 @@ # FSP Silicon register "eist_enable" = "1" + register "enable_c1e" = "1" register "cnvi_bt_core" = "1" register "cnvi_bt_audio_offload" = "1" -- To view, visit https://review.coreboot.org/c/coreboot/+/79379?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: I3a317d031e71f86afc50b229d1b97197552f4fa9 Gerrit-Change-Number: 79379 Gerrit-PatchSet: 3 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2024