Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
soc/intel/common/block/cpu: Refactor init_cpus function
This patch makes init_cpus function external so that it can be used in below scenarios:
1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude this call if user has selected CONFIG_USE_INTEL_FSP_MP_INIT) 2. coreboot would like to take APs control back after FSP-S has done with MP initialization based on user select CONFIG_USE_INTEL_FSP_MP_INIT
Also make sure post_cpus_init function is getting executed unconditionally to update MTTR snapshot on all cores.
Change-Id: Idc03090360f34df074b33ba0fced2d192edf068a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/common/block/include/intelblocks/mp_init.h 2 files changed, 29 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44076/1
diff --git a/src/soc/intel/common/block/cpu/mp_init.c b/src/soc/intel/common/block/cpu/mp_init.c index 90bae16..4f27e52 100644 --- a/src/soc/intel/common/block/cpu/mp_init.c +++ b/src/soc/intel/common/block/cpu/mp_init.c @@ -125,14 +125,19 @@ *parallel = 1; }
-static void init_cpus(void *unused) +/* + * Perform BSP and AP initialization + * This function can be called in below cases + * 1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude + * this call if user has selected CONFIG_USE_INTEL_FSP_MP_INIT) + * 2. coreboot would like to take APs control back after FSP-S has done with MP + * initialization based on user select CONFIG_USE_INTEL_FSP_MP_INIT + */ +void init_cpus(void) { struct device *dev = dev_find_path(NULL, DEVICE_PATH_CPU_CLUSTER); assert(dev != NULL);
- if (CONFIG(USE_INTEL_FSP_MP_INIT)) - return; - microcode_patch = intel_microcode_find(); intel_microcode_load_unlocked(microcode_patch);
@@ -140,6 +145,15 @@ soc_init_cpus(dev->link_list); }
+static void coreboot_init_cpus(void *unused) +{ + if (CONFIG(USE_INTEL_FSP_MP_INIT)) + return; + + init_cpus(); +} + + static void wrapper_x86_setup_mtrrs(void *unused) { x86_setup_mtrrs_with_detect(); @@ -148,9 +162,6 @@ /* Ensure to re-program all MTRRs based on DRAM resource settings */ static void post_cpus_init(void *unused) { - if (CONFIG(USE_INTEL_FSP_MP_INIT)) - return; - if (mp_run_on_all_cpus(&wrapper_x86_setup_mtrrs, NULL) < 0) printk(BIOS_ERR, "MTRR programming failure\n");
@@ -158,6 +169,6 @@ }
/* Do CPU MP Init before FSP Silicon Init */ -BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, init_cpus, NULL); +BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, coreboot_init_cpus, NULL); BOOT_STATE_INIT_ENTRY(BS_WRITE_TABLES, BS_ON_EXIT, post_cpus_init, NULL); BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, post_cpus_init, NULL); diff --git a/src/soc/intel/common/block/include/intelblocks/mp_init.h b/src/soc/intel/common/block/include/intelblocks/mp_init.h index e03d8bd..615449f 100644 --- a/src/soc/intel/common/block/include/intelblocks/mp_init.h +++ b/src/soc/intel/common/block/include/intelblocks/mp_init.h @@ -69,6 +69,16 @@ void get_microcode_info(const void **microcode, int *parallel);
/* + * Perform BSP and AP initialization + * This function can be called in below cases + * 1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude + * this call if user has selected CONFIG_USE_INTEL_FSP_MP_INIT) + * 2. coreboot would like to take APs control back after FSP-S has done with MP + * initialization based on user select CONFIG_USE_INTEL_FSP_MP_INIT + */ +void init_cpus(void); + +/* * SoC Overrides * * All new SoC must implement below functionality for ramstage.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44076/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/1/src/soc/intel/common/block/... PS1, Line 150: if (CONFIG(USE_INTEL_FSP_MP_INIT)) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/44076/1/src/soc/intel/common/block/... PS1, Line 150: if (CONFIG(USE_INTEL_FSP_MP_INIT)) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/44076/1/src/soc/intel/common/block/... PS1, Line 151: return; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/44076/1/src/soc/intel/common/block/... PS1, Line 151: return; please, no spaces at the start of a line
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44076
to look at the new patch set (#2).
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
soc/intel/common/block/cpu: Refactor init_cpus function
This patch makes init_cpus function external so that it can be used in below scenarios:
1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude this call if user has selected USE_INTEL_FSP_MP_INIT) 2. coreboot would like to take APs control back after FSP-S has done with MP initialization based on user select USE_INTEL_FSP_MP_INIT
Also make sure post_cpus_init function is getting executed unconditionally to update MTTR snapshot on all cores.
Change-Id: Idc03090360f34df074b33ba0fced2d192edf068a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/common/block/include/intelblocks/mp_init.h 2 files changed, 29 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44076/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44076/2//COMMIT_MSG@9 PS2, Line 9: This patch makes init_cpus function external so that it can be used in below nit: wrap lines at 72 characters
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44076
to look at the new patch set (#3).
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
soc/intel/common/block/cpu: Refactor init_cpus function
This patch makes init_cpus function external so that it can be used in below scenarios:
1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude this call if user has selected USE_INTEL_FSP_MP_INIT) 2. coreboot would like to take APs control back after FSP-S has done with MP initialization based on user select USE_INTEL_FSP_MP_INIT
Also make sure post_cpus_init function is getting executed unconditionally to update MTTR snapshot on all cores.
Change-Id: Idc03090360f34df074b33ba0fced2d192edf068a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/common/block/include/intelblocks/mp_init.h 2 files changed, 29 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44076/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44076/2//COMMIT_MSG@9 PS2, Line 9: This patch makes init_cpus function external so that it can be used in below
nit: wrap lines at 72 characters
Ack
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44076
to look at the new patch set (#4).
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
soc/intel/common/block/cpu: Refactor init_cpus function
This patch makes init_cpus function external so that it can be used in below scenarios:
1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude this call if user has selected USE_INTEL_FSP_MP_INIT) 2. coreboot would like to take APs control back after FSP-S has done with MP initialization based on user select USE_INTEL_FSP_MP_INIT
Also make sure post_cpus_init function is getting executed unconditionally to update MTTR snapshot on all cores.
Change-Id: Idc03090360f34df074b33ba0fced2d192edf068a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/common/block/include/intelblocks/mp_init.h 2 files changed, 28 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44076/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 4: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG@12 PS4, Line 12: During When
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG@13 PS4, Line 13: BS_DEV_INIT_CHIPS (exclude this call if user has selected nit: add three spaces at the beginning of these lines to align with the rest:
1. When coreboot ... BS_DEV_INIT_CHIPS (exclude ... USE_INTEL_FSP_MP_INIT) 2. coreboot would like to ... with MP initialization
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG@19 PS4, Line 19: MTTR MTRR
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44076
to look at the new patch set (#5).
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
soc/intel/common/block/cpu: Refactor init_cpus function
This patch makes init_cpus function external so that it can be used in below scenarios:
1. When coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude this call if user has selected USE_INTEL_FSP_MP_INIT) 2. coreboot would like to take APs control back after FSP-S has done with MP initialization based on user select USE_INTEL_FSP_MP_INIT
Also make sure post_cpus_init function is getting executed unconditionally to update MTRR snapshot on all cores.
Change-Id: Idc03090360f34df074b33ba0fced2d192edf068a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/common/block/include/intelblocks/mp_init.h 2 files changed, 28 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44076/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG@12 PS4, Line 12: During
When
Ack
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG@13 PS4, Line 13: BS_DEV_INIT_CHIPS (exclude this call if user has selected
nit: add three spaces at the beginning of these lines to align with the rest: […]
Ack
https://review.coreboot.org/c/coreboot/+/44076/4//COMMIT_MSG@19 PS4, Line 19: MTTR
MTRR
Ack
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked Why do you need to load Microcodes after FSP-S has done MPinit?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked
Why do you need to load Microcodes after FSP-S has done MPinit?
I believe we have checks inside "src/cpu/intel/microcode/microcode.c" as below to skip if ucode is already loaded. So calling is harmless
vim src/cpu/intel/microcode/microcode.c +59 /* No use loading the same revision. */ if (current_rev == m->rev) { printk(BIOS_INFO, "microcode: Update skipped, already up-to-date\n"); return; }
And from log, i could see it skipping the same.
microcode: Update skipped, already up-to-date
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked
I believe we have checks inside "src/cpu/intel/microcode/microcode. […]
Then, we might as well skip it when using FSP MP init, right?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked
Then, we might as well skip it when using FSP MP init, right?
yes, we are calling that function bt as ucode is already loaded on all cores hence this additional step got skipped. Do you recommend to move this ucode loading into line 152 below inside coreboot_init_cpus() ?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked
yes, we are calling that function bt as ucode is already loaded on all cores hence this additional s […]
For example
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked
For example
void init_cpus(void) { struct device *dev = dev_find_path(NULL, DEVICE_PATH_CPU_CLUSTER); assert(dev != NULL);
if (dev && dev->link_list) soc_init_cpus(dev->link_list); }
static void coreboot_init_cpus(void *unused) { if (CONFIG(USE_INTEL_FSP_MP_INIT)) return;
microcode_patch = intel_microcode_find(); intel_microcode_load_unlocked(microcode_patch);
init_cpus(); }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked
void init_cpus(void) […]
Yes, this should work.
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Aamir Bohra, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44076
to look at the new patch set (#6).
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
soc/intel/common/block/cpu: Refactor init_cpus function
This patch makes init_cpus function external so that it can be used in below scenarios:
1. When coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude this call if user has selected USE_INTEL_FSP_MP_INIT) 2. coreboot would like to take APs control back after FSP-S has done with MP initialization based on user select USE_INTEL_FSP_MP_INIT
Also make sure post_cpus_init function is getting executed unconditionally to update MTRR snapshot on all cores.
Change-Id: Idc03090360f34df074b33ba0fced2d192edf068a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/common/block/include/intelblocks/mp_init.h 2 files changed, 27 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44076/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/5/src/soc/intel/common/block/... PS5, Line 142: intel_microcode_load_unlocked
Yes, this should work.
Ack
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... PS6, Line 136: init_cpus Just curious: Does this have to be done at a certain point after FSP-S init? I am thinking if it would be better to just make the call to init_cpus() be done from different boot state depending upon `USE_INTEL_FSP_MP_INIT`
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... PS6, Line 149: : microcode_patch = intel_microcode_find(); : intel_microcode_load_unlocked(microcode_patch); I know this change wasn't made here but since we are revisiting this path - how is microcode loading handled in case of FSP init? Isn't that required to be done at some point in coreboot?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... PS6, Line 136: init_cpus
Just curious: Does this have to be done at a certain point after FSP-S init? I am thinking if it wou […]
yes can be done as well, bt wondering if user would like to run some multithreded programming immediately right after FSP-S return. in those case MP's won't be available if we are running using BS machine.
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... PS6, Line 149: : microcode_patch = intel_microcode_find(); : intel_microcode_load_unlocked(microcode_patch);
I know this change wasn't made here but since we are revisiting this path - how is microcode loading […]
valid point, FSP might not need to update ucode as we don't have SGX enable request bt if it does then there would be problem hence cb should provide ucode as part of UPD (MicrocodeRegionBase)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6:
@Furquan anything on this CL?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... PS6, Line 136: init_cpus
yes can be done as well, bt wondering if user would like to run some multithreded programming immedi […]
Sounds good. We can go ahead with this.
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... PS6, Line 149: : microcode_patch = intel_microcode_find(); : intel_microcode_load_unlocked(microcode_patch);
valid point, FSP might not need to update ucode as we don't have SGX enable request bt if it does th […]
I was actually referring to coreboot applying the microcode update. I believe currently it does not happen in case where USE_INTEL_FSP_MP_INIT is selected?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44076/6/src/soc/intel/common/block/... PS6, Line 149: : microcode_patch = intel_microcode_find(); : intel_microcode_load_unlocked(microcode_patch);
I was actually referring to coreboot applying the microcode update. […]
I believe we have checks inside "src/cpu/intel/microcode/microcode.c" as below to skip if ucode is already loaded. in case FSP is doing MP init, attempting to do ucocde update inside CB, i could see "microcode: Update skipped, already up-to-date" msg in log, hence remove the redundant call inside init_cpus()
vim src/cpu/intel/microcode/microcode.c +59 /* No use loading the same revision. */
if (current_rev == m->rev) { printk(BIOS_INFO, "microcode: Update skipped, already up-to-date\n"); return; }
And from log, i could see it skipping the same.
microcode: Update skipped, already up-to-date
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44076 )
Change subject: soc/intel/common/block/cpu: Refactor init_cpus function ......................................................................
soc/intel/common/block/cpu: Refactor init_cpus function
This patch makes init_cpus function external so that it can be used in below scenarios:
1. When coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude this call if user has selected USE_INTEL_FSP_MP_INIT) 2. coreboot would like to take APs control back after FSP-S has done with MP initialization based on user select USE_INTEL_FSP_MP_INIT
Also make sure post_cpus_init function is getting executed unconditionally to update MTRR snapshot on all cores.
Change-Id: Idc03090360f34df074b33ba0fced2d192edf068a Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44076 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/common/block/include/intelblocks/mp_init.h 2 files changed, 27 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/src/soc/intel/common/block/cpu/mp_init.c b/src/soc/intel/common/block/cpu/mp_init.c index 1d6205c..5306431 100644 --- a/src/soc/intel/common/block/cpu/mp_init.c +++ b/src/soc/intel/common/block/cpu/mp_init.c @@ -127,19 +127,32 @@ *parallel = 1; }
-static void init_cpus(void *unused) +/* + * Perform BSP and AP initialization + * This function can be called in below cases: + * 1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude + * this call if user has selected USE_INTEL_FSP_MP_INIT). + * 2. coreboot would like to take APs control back after FSP-S has done with MP + * initialization based on user select USE_INTEL_FSP_MP_INIT. + */ +void init_cpus(void) { struct device *dev = dev_find_path(NULL, DEVICE_PATH_CPU_CLUSTER); assert(dev != NULL);
+ if (dev && dev->link_list) + soc_init_cpus(dev->link_list); +} + +static void coreboot_init_cpus(void *unused) +{ if (CONFIG(USE_INTEL_FSP_MP_INIT)) return;
microcode_patch = intel_microcode_find(); intel_microcode_load_unlocked(microcode_patch);
- if (dev && dev->link_list) - soc_init_cpus(dev->link_list); + init_cpus(); }
static void wrapper_x86_setup_mtrrs(void *unused) @@ -150,9 +163,6 @@ /* Ensure to re-program all MTRRs based on DRAM resource settings */ static void post_cpus_init(void *unused) { - if (CONFIG(USE_INTEL_FSP_MP_INIT)) - return; - if (mp_run_on_all_cpus(&wrapper_x86_setup_mtrrs, NULL) < 0) printk(BIOS_ERR, "MTRR programming failure\n");
@@ -160,6 +170,6 @@ }
/* Do CPU MP Init before FSP Silicon Init */ -BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, init_cpus, NULL); +BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, coreboot_init_cpus, NULL); BOOT_STATE_INIT_ENTRY(BS_WRITE_TABLES, BS_ON_EXIT, post_cpus_init, NULL); BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, post_cpus_init, NULL); diff --git a/src/soc/intel/common/block/include/intelblocks/mp_init.h b/src/soc/intel/common/block/include/intelblocks/mp_init.h index 10dd19b..6220b76 100644 --- a/src/soc/intel/common/block/include/intelblocks/mp_init.h +++ b/src/soc/intel/common/block/include/intelblocks/mp_init.h @@ -70,6 +70,16 @@ void get_microcode_info(const void **microcode, int *parallel);
/* + * Perform BSP and AP initialization + * This function can be called in below cases + * 1. During coreboot is doing MP initialization as part of BS_DEV_INIT_CHIPS (exclude + * this call if user has selected USE_INTEL_FSP_MP_INIT) + * 2. coreboot would like to take APs control back after FSP-S has done with MP + * initialization based on user select USE_INTEL_FSP_MP_INIT + */ +void init_cpus(void); + +/* * SoC Overrides * * All new SoC must implement below functionality for ramstage.
-
Aamir Bohra (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Patrick Rudolph (Code Review)
-
Subrata Banik (Code Review)