coreboot-gerrit April 2021

coreboot-gerrit@coreboot.org

1 participants
6269 discussions

Change in coreboot[master]: decompressor: Add CBFS_VERIFICATION support
by Julius Werner (Code Review) 06 Apr '21

06 Apr '21

Attention is currently required from: Martin Roth, Furquan Shaikh. Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh, Aaron Durbin, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/52085 to look at the new patch set (#2). Change subject: decompressor: Add CBFS_VERIFICATION support ...................................................................... decompressor: Add CBFS_VERIFICATION support CBFS_VERIFICATION requires the CBFS metadata hash anchor to be linked into an uncompressed stage, but for platforms using COMPRESS_BOOTBLOCK, this is only the decompressor stage. The first CBFS accesses are made in the bootblock stage after decompression, so if we want to make CBFS_VERIFICATION work on those platforms, we have to pass the metadata hash anchor from the decompressor into the bootblock. This patch does just that. (Note that this relies on the decompressor data remaining valid in memory for as long as the metadata hash anchor is needed. This is always true even for OVERLAP_DECOMPRESSOR_ROMSTAGE() situations because the FMAP and CBFS metadata necessarily need to have finished verification before a new stage could be loaded.) Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I2e6d7384cfb8339a24369eb6c01fc12f911c974e --- M src/include/bootblock_common.h M src/include/metadata_hash.h M src/lib/Kconfig.cbfs_verification M src/lib/Makefile.inc M src/lib/bootblock.c M src/lib/decompressor.c M src/lib/metadata_hash.c 7 files changed, 42 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/52085/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/52085 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2e6d7384cfb8339a24369eb6c01fc12f911c974e Gerrit-Change-Number: 52085 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: decompressor: Add CBFS_VERIFICATION support
by Julius Werner (Code Review) 06 Apr '21

06 Apr '21

Attention is currently required from: Martin Roth, Furquan Shaikh. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52085 ) Change subject: decompressor: Add CBFS_VERIFICATION support ...................................................................... Patch Set 1: (1 comment) File src/lib/metadata_hash.c: https://review.coreboot.org/c/coreboot/+/52085/comment/0a03237c_e6726680 PS1, Line 33: return &anchor_ptr->cbfs_hash; > I know it's not likely (and much badness would have to occur), but do you think it's worth doing che […] Yeah... I had that at first and then I wanted to simplify it further. I can put it back in if you want. -- To view, visit https://review.coreboot.org/c/coreboot/+/52085 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2e6d7384cfb8339a24369eb6c01fc12f911c974e Gerrit-Change-Number: 52085 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Comment-Date: Mon, 05 Apr 2021 23:09:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: *x86: Support x2apic mode
by Wonkyu Kim (Code Review) 06 Apr '21

06 Apr '21

1 0

Change in coreboot[master]: *x86: Support x2apic mode
by Wonkyu Kim (Code Review) 06 Apr '21

06 Apr '21

1 0

Change in coreboot[master]: fsp2_0: Replace fspld->get_destination() callback with CBFS allocator
by Julius Werner (Code Review) 06 Apr '21

06 Apr '21

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/52082 ) Change subject: fsp2_0: Replace fspld->get_destination() callback with CBFS allocator ...................................................................... fsp2_0: Replace fspld->get_destination() callback with CBFS allocator The Intel FSP 2.0 driver contains a custom construct that basically serves the same purpose as the new CBFS allocator concept: a callback function to customize placement of a loaded CBFS file whose size is initially unknown. This patch removes the existing implementation and replaces it with a CBFS allocator. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I0b7b446a0d2af87ec337fb80ad54f2d404e69668 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52082 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Aaron Durbin <adurbin(a)chromium.org> Reviewed-by: Furquan Shaikh <furquan(a)google.com> --- M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c 4 files changed, 27 insertions(+), 89 deletions(-) Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h index 253b74a..6583b64 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/util.h +++ b/src/drivers/intel/fsp2_0/include/fsp/util.h @@ -4,6 +4,7 @@ #define _FSP2_0_UTIL_H_ #include <boot/coreboot_tables.h> +#include <cbfs.h> #include <commonlib/region.h> #include <arch/cpu.h> #include <fsp/api.h> @@ -111,11 +112,8 @@ /* fsp_prog object will have region_device initialized to final * load location in memory. */ struct prog fsp_prog; - /* Fill in destination location given final load size. Return 0 on - * success, < 0 on error. */ - int (*get_destination)(const struct fsp_load_descriptor *fspld, - void **dest, size_t final_load_size, - const struct region_device *source); + /* CBFS allocator to place loaded FSP. NULL to map flash directly. */ + cbfs_allocator_t alloc; /* Optional argument to be utilized by get_destination() callback. */ void *arg; }; diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 0ea5c08..bbc26bc 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -339,40 +339,20 @@ fsp_debug_after_memory_init(status); } -static int fspm_get_dest(const struct fsp_load_descriptor *fspld, void **dest, - size_t size, const struct region_device *source) +static void *fspm_allocator(void *arg, size_t size, const union cbfs_mdata *unused) { + const struct fsp_load_descriptor *fspld = arg; struct fspm_context *context = fspld->arg; - struct fsp_header *hdr = &context->header; struct memranges *memmap = &context->memmap; - uintptr_t fspm_begin; - uintptr_t fspm_end; - - if (CONFIG(FSP_M_XIP)) { - if (fsp_validate_component(hdr, source) != CB_SUCCESS) - return -1; - - *dest = rdev_mmap_full(source); - if ((uintptr_t)*dest != hdr->image_base) { - printk(BIOS_CRIT, "FSPM XIP base does not match: %p vs %p\n", - (void *)(uintptr_t)hdr->image_base, *dest); - return -1; - } - /* Since the component is XIP it's already in the address space. - Thus, there's no need to rdev_munmap(). */ - return 0; - } /* Non XIP FSP-M uses FSP-M address */ - fspm_begin = (uintptr_t)CONFIG_FSP_M_ADDR; - fspm_end = fspm_begin + size; + uintptr_t fspm_begin = (uintptr_t)CONFIG_FSP_M_ADDR; + uintptr_t fspm_end = fspm_begin + size; if (check_region_overlap(memmap, "FSPM", fspm_begin, fspm_end) != CB_SUCCESS) - return -1; + return NULL; - *dest = (void *)fspm_begin; - - return 0; + return (void *)fspm_begin; } void fsp_memory_init(bool s3wake) @@ -381,12 +361,15 @@ struct fspm_context context; struct fsp_load_descriptor fspld = { .fsp_prog = PROG_INIT(PROG_REFCODE, CONFIG_FSP_M_CBFS), - .get_destination = fspm_get_dest, .arg = &context, }; struct fsp_header *hdr = &context.header; struct memranges *memmap = &context.memmap; + /* For FSP-M XIP we leave alloc NULL to get a direct mapping to flash. */ + if (!CONFIG(FSP_M_XIP)) + fspld.alloc = fspm_allocator; + elog_boot_notify(s3wake); /* Build up memory map of romstage address space including CAR. */ @@ -399,6 +382,10 @@ if (fsp_load_component(&fspld, hdr) != CB_SUCCESS) die("FSPM not available or failed to load!\n"); + if (CONFIG(FSP_M_XIP) && (uintptr_t)prog_start(&fspld.fsp_prog) != hdr->image_base) + die("FSPM XIP base does not match: %p vs %p\n", + (void *)(uintptr_t)hdr->image_base, prog_start(&fspld.fsp_prog)); + timestamp_add_now(TS_BEFORE_INITRAM); do_fsp_memory_init(&context, s3wake); diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index c9898f5..6326d53 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -184,22 +184,16 @@ post_code(POST_FSP_MULTI_PHASE_SI_INIT_EXIT); } -static int fsps_get_dest(const struct fsp_load_descriptor *fspld, void **dest, - size_t size, const struct region_device *source) +static void *fsps_allocator(void *arg_unused, size_t size, const union cbfs_mdata *mdata_unused) { - *dest = cbmem_add(CBMEM_ID_REFCODE, size); - - if (*dest == NULL) - return -1; - - return 0; + return cbmem_add(CBMEM_ID_REFCODE, size); } void fsps_load(void) { struct fsp_load_descriptor fspld = { .fsp_prog = PROG_INIT(PROG_REFCODE, CONFIG_FSP_S_CBFS), - .get_destination = fsps_get_dest, + .alloc = fsps_allocator, }; struct prog *fsps = &fspld.fsp_prog; static int load_done; diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c index 9831e64..33bfc11 100644 --- a/src/drivers/intel/fsp2_0/util.c +++ b/src/drivers/intel/fsp2_0/util.c @@ -139,65 +139,24 @@ return false; } -static void *fsp_get_dest_and_load(struct fsp_load_descriptor *fspld, size_t size, - const struct region_device *source_rdev, - uint32_t compression_algo) -{ - void *dest; - - if (fspld->get_destination(fspld, &dest, size, source_rdev) < 0) { - printk(BIOS_ERR, "FSP Destination not obtained.\n"); - return NULL; - } - - /* Don't load when executing in place. */ - if (fspm_xip()) - return dest; - - if (cbfs_load_and_decompress(source_rdev, 0, region_device_sz(source_rdev), - dest, size, compression_algo) != size) { - printk(BIOS_ERR, "Failed to load FSP component.\n"); - return NULL; - } - - /* Don't allow FSP-M relocation. */ - if (fspm_env()) - return dest; - - if (fsp_component_relocate((uintptr_t)dest, dest, size) < 0) { - printk(BIOS_ERR, "Unable to relocate FSP component!\n"); - return NULL; - } - - return dest; -} - /* Load the FSP component described by fsp_load_descriptor from cbfs. The FSP * header object will be validated and filled in on successful load. */ enum cb_err fsp_load_component(struct fsp_load_descriptor *fspld, struct fsp_header *hdr) { - struct cbfsf file_desc; - uint32_t compression_algo; size_t output_size; void *dest; - struct region_device source_rdev, prog_rdev; + struct region_device prog_rdev; struct prog *fsp_prog = &fspld->fsp_prog; - if (fspld->get_destination == NULL) + dest = cbfs_alloc(prog_name(fsp_prog), fspld->alloc, fspld, &output_size); + if (!dest) return CB_ERR; - if (cbfs_boot_locate(&file_desc, prog_name(fsp_prog), &fsp_prog->cbfs_type) < 0) + /* Don't allow FSP-M relocation. */ + if (!fspm_env() && fsp_component_relocate((uintptr_t)dest, dest, output_size) < 0) { + printk(BIOS_ERR, "Unable to relocate FSP component!\n"); return CB_ERR; - - if (cbfsf_decompression_info(&file_desc, &compression_algo, &output_size) < 0) - return CB_ERR; - - cbfs_file_data(&source_rdev, &file_desc); - - dest = fsp_get_dest_and_load(fspld, output_size, &source_rdev, compression_algo); - - if (dest == NULL) - return CB_ERR; + } prog_set_area(fsp_prog, dest, output_size); -- To view, visit https://review.coreboot.org/c/coreboot/+/52082 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0b7b446a0d2af87ec337fb80ad54f2d404e69668 Gerrit-Change-Number: 52082 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: cbfs: Make `mdata` argument to cbfs_allocator_t const
by Julius Werner (Code Review) 06 Apr '21

06 Apr '21

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/52081 ) Change subject: cbfs: Make `mdata` argument to cbfs_allocator_t const ...................................................................... cbfs: Make `mdata` argument to cbfs_allocator_t const Right before CB:49334 was submitted, I changed the signature of cbfs_allocator_t function pointers to include another argument passing in the already loaded CBFS metadata (to allow for the rare edge case of allocators needing to read CBFS attributes). This interface is not meant to be able to modify the passed-in metadata, so to clarify that and prevent potential errors, we should declare the argument const. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I7e3756490b9ad7ded91268c61797cef36c4118ee Reviewed-on: https://review.coreboot.org/c/coreboot/+/52081 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Aaron Durbin <adurbin(a)chromium.org> Reviewed-by: Furquan Shaikh <furquan(a)google.com> --- M src/include/cbfs.h M src/lib/cbfs.c M src/lib/rmodule.c 3 files changed, 6 insertions(+), 6 deletions(-) Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved diff --git a/src/include/cbfs.h b/src/include/cbfs.h index b89a13d..37bac30 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -67,7 +67,7 @@ * attributes). Must return a pointer to space of the requested size where the file data should * be loaded, or NULL to make the operation fail. */ -typedef void *(*cbfs_allocator_t)(void *arg, size_t size, union cbfs_mdata *mdata); +typedef void *(*cbfs_allocator_t)(void *arg, size_t size, const union cbfs_mdata *mdata); static inline size_t cbfs_load(const char *name, void *buf, size_t size); static inline size_t cbfs_ro_load(const char *name, void *buf, size_t size); @@ -183,9 +183,9 @@ void *buf; size_t buf_size; }; -void *_cbfs_default_allocator(void *arg, size_t size, union cbfs_mdata *unused); +void *_cbfs_default_allocator(void *arg, size_t size, const union cbfs_mdata *unused); -void *_cbfs_cbmem_allocator(void *arg, size_t size, union cbfs_mdata *unused); +void *_cbfs_cbmem_allocator(void *arg, size_t size, const union cbfs_mdata *unused); /********************************************************************************************** * INLINE IMPLEMENTATIONS * diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index fbf4531..65bb721 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -369,7 +369,7 @@ return loc; } -void *_cbfs_default_allocator(void *arg, size_t size, union cbfs_mdata *unused) +void *_cbfs_default_allocator(void *arg, size_t size, const union cbfs_mdata *unused) { struct _cbfs_default_allocator_arg *darg = arg; if (size > darg->buf_size) @@ -377,7 +377,7 @@ return darg->buf; } -void *_cbfs_cbmem_allocator(void *arg, size_t size, union cbfs_mdata *unused) +void *_cbfs_cbmem_allocator(void *arg, size_t size, const union cbfs_mdata *unused) { return cbmem_add((uintptr_t)arg, size); } diff --git a/src/lib/rmodule.c b/src/lib/rmodule.c index ac9eb0b..31bf141 100644 --- a/src/lib/rmodule.c +++ b/src/lib/rmodule.c @@ -192,7 +192,7 @@ } static void *rmodule_cbfs_allocator(void *rsl_arg, size_t unused, - union cbfs_mdata *mdata) + const union cbfs_mdata *mdata) { struct rmod_stage_load *rsl = rsl_arg; -- To view, visit https://review.coreboot.org/c/coreboot/+/52081 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7e3756490b9ad7ded91268c61797cef36c4118ee Gerrit-Change-Number: 52081 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: vc/amd/fsp/cezanne: update UPD headers
by Raul Rangel (Code Review) 06 Apr '21

06 Apr '21

Attention is currently required from: Furquan Shaikh, Marshall Dawson, Paul Menzel, Matt Papageorge, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51806 ) Change subject: vc/amd/fsp/cezanne: update UPD headers ...................................................................... Patch Set 9: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/51806/comment/5ab48413_7ff20f2c PS9, Line 19: TODO Tested on guybrush and it works! -- To view, visit https://review.coreboot.org/c/coreboot/+/51806 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idca235029bf2e68d403230d55308820cab61a6c0 Gerrit-Change-Number: 51806 Gerrit-PatchSet: 9 Gerrit-Owner: Matt Papageorge <matthewpapa07(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> 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: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Matt Papageorge <matthewpapa07(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 05 Apr 2021 22:57:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/*acpi: create acpi table for x2apic mode
by Wonkyu Kim (Code Review) 06 Apr '21

06 Apr '21

Attention is currently required from: Nico Huber, Furquan Shaikh, Subrata Banik, Raj Astekar, Patrick Rudolph. Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51724 ) Change subject: src/*acpi: create acpi table for x2apic mode ...................................................................... Patch Set 7: (2 comments) File src/acpi/acpi.c: https://review.coreboot.org/c/coreboot/+/51724/comment/0882efc4_66d7b3da PS6, Line 165: if (apic_ids[index] < 0xff) > No need for further indentation, this could be an `else if`. Done https://review.coreboot.org/c/coreboot/+/51724/comment/e52ae021_568c6e9e PS6, Line 170: current += acpi_create_madt_lx2apic( : (acpi_madt_lx2apic_t *)current, : index, apic_ids[index]); > This already uses the lx2apic function. Please explain what is wrong […] Tested it again and it's working without change. I thought we should not mix. Remove this file change. -- To view, visit https://review.coreboot.org/c/coreboot/+/51724 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9399d30b686b55d86806f5db4110bf4a80fe459b Gerrit-Change-Number: 51724 Gerrit-PatchSet: 7 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 05 Apr 2021 22:53:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: *x86: Support x2apic mode
by Wonkyu Kim (Code Review) 06 Apr '21

06 Apr '21

Attention is currently required from: Nico Huber, Furquan Shaikh, Jamie Ryu, Paul Menzel, Angel Pons, Subrata Banik, Kyösti Mälkki, Raj Astekar, Aaron Durbin, Patrick Rudolph. Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51723 ) Change subject: *x86: Support x2apic mode ...................................................................... Patch Set 6: (7 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/51723/comment/eb102ea9_360d426a PS6, Line 10: LAPIC_BASE_MSR(LAPIC_BASE_MSR_X2APIC_MODE) > Please add a space before the (. Done https://review.coreboot.org/c/coreboot/+/51723/comment/45393bb9_41f8e1a5 PS6, Line 11: implment > implement Done https://review.coreboot.org/c/coreboot/+/51723/comment/3b35836a_9b5f8981 PS6, Line 21: cat /proc/cpuinfo | grep "apicid" > I don't like the grep-only approach, especially when I want to change what I'm grepping for. […] Done File src/cpu/x86/lapic/lapic_cpu_init.c: https://review.coreboot.org/c/coreboot/+/51723/comment/36b1cbd5_94bce1af PS6, Line 355: LAPIC_DM_INIT | LAPIC_INT_LEVELTRIG, id); > No need to break the line here. Done File src/cpu/x86/mp_init.c: https://review.coreboot.org/c/coreboot/+/51723/comment/401400a8_8d9f0d09 PS6, Line 458: } else { > Or even better, add more functions. e.g. start_aps_lapic(), start_aps_lx2apic(). […] Done https://review.coreboot.org/c/coreboot/+/51723/comment/58e6ed7b_22bfe339 PS6, Line 678: } else { > Simpler: […] Done File src/include/cpu/x86/lapic_def.h: https://review.coreboot.org/c/coreboot/+/51723/comment/509113ea_b23a1ae1 PS6, Line 99: #define X2APIC_LAPIC_ID (X2APIC_MSR_BASE_ADDRESS | (LAPIC_ID >> 4)) > These definitions are misplaced (they're not fields in LAPIC_TDCR, not even LAPIC registers) Done -- To view, visit https://review.coreboot.org/c/coreboot/+/51723 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0bb729b0521fb9dc38b7981014755daeaf9ca817 Gerrit-Change-Number: 51723 Gerrit-PatchSet: 6 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 05 Apr 2021 22:34:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: *x86: Support x2apic mode
by Wonkyu Kim (Code Review) 06 Apr '21

06 Apr '21

Attention is currently required from: Furquan Shaikh, Jamie Ryu, Wonkyu Kim, Paul Menzel, Angel Pons, Subrata Banik, Kyösti Mälkki, Raj Astekar, Aaron Durbin, Patrick Rudolph. Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Ravishankar Sarawadi, Paul Menzel, Subrata Banik, Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Lance Zhao, Nico Huber, Jamie Ryu, Srinidhi N Kaushik, Raj Astekar, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/51723 to look at the new patch set (#7). Change subject: *x86: Support x2apic mode ...................................................................... *x86: Support x2apic mode Implement x2apic mode as existing code only supports apic mode. Use info from LAPIC_BASE_MSR (LAPIC_BASE_MSR_X2APIC_MODE) to check if apic mode or x2apic mode and implement x2apic mode according to x2apic specfication. Reference: https://software.intel.com/content/www/us/en/develop/download/ intel-64-architecture-x2apic-specification.html BUG=None BRANCH=None TEST=boot to OS and check apic mode cat /proc/cpuinfo | grep "apicid" ex) can see apicid bigger than 255 apicid : 256 apicid : 260 Signed-off-by: Wonkyu Kim <wonkyu.kim(a)intel.com> Change-Id: I0bb729b0521fb9dc38b7981014755daeaf9ca817 --- M src/arch/x86/cpu.c M src/cpu/x86/lapic/lapic.c M src/cpu/x86/lapic/lapic_cpu_init.c M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/lapic.h M src/include/cpu/x86/lapic_def.h M src/include/cpu/x86/smm.h 8 files changed, 227 insertions(+), 116 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/51723/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/51723 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0bb729b0521fb9dc38b7981014755daeaf9ca817 Gerrit-Change-Number: 51723 Gerrit-PatchSet: 7 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2021