Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
soc/intel/alderlake: Update CPU microcode patch base address/size
This patch updates CPU microcode patch base address/size to FSP-S UPD to have second microcode patch loaded successfully to enable Mcheck flow.
TEST=Able to reach beyond PC6 without any MCE.
Change-Id: I936816e3173dbcdf82b2b16b465f6b4ed5d90335 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/alderlake/fsp_params.c 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/48847/1
diff --git a/src/soc/intel/alderlake/fsp_params.c b/src/soc/intel/alderlake/fsp_params.c index a21ca4a..6de8649 100644 --- a/src/soc/intel/alderlake/fsp_params.c +++ b/src/soc/intel/alderlake/fsp_params.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <assert.h> +#include <cbfs.h> #include <console/console.h> #include <device/device.h> #include <device/pci.h> @@ -89,6 +90,8 @@ void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd) { int i; + const struct microcode *microcode_file; + size_t microcode_len; FSP_S_CONFIG *params = &supd->FspsConfig;
struct device *dev; @@ -99,6 +102,14 @@ /* Parse device tree and enable/disable Serial I/O devices */ parse_devicetree(params);
+ microcode_file = cbfs_map("cpu_microcode_blob.bin", µcode_len); + + if ((microcode_file != NULL) && (microcode_len != 0)) { + /* Update CPU Microcode patch base address/size */ + params->MicrocodeRegionBase = (uint32_t)microcode_file; + params->MicrocodeRegionSize = (uint32_t)microcode_len; + } + /* Load VBT before devicetree-specific config. */ params->GraphicsConfigPtr = (uintptr_t)vbt_get();
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow What is "second microcode patch"?
And why is this required? Coreboot already does the microcode loading as part of CPU init.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1: Code-Review+1
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112: nit: Should there be a additional check whether UCODE is loaded successfully or not?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
What is "second microcode patch"? […]
on ADL, BIOS should do the second ucode patch load(To Enable MCHECK)
[InitializeMicrocode]: Microcode Region Address = FF41E9D0, Size = 194560 [InitializeMicrocode]: Find a potential patch! LoadMicrocode: Before load, revision = 0x8200000A LoadMicrocode: After load, revision = 0x8200000A ReloadMicrocodePatch: Second patch load success.
without the 2nd microcode loading Mcheck flow won't finished successfuly
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
nit: Should there be a additional check whether UCODE is loaded successfully or not?
2nd microcode loading is part of FSP doing MP init process, we are just assigning the UPD.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
on ADL, BIOS should do the second ucode patch load(To Enable MCHECK) […]
Is it different from older platforms?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
Is it different from older platforms?
At what point in the boot sequence is this expected? Why can't coreboot do this instead of relying on FSP for it.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
At what point in the boot sequence is this expected? Why can't coreboot do this instead of relying o […]
Are we potentially missing this from TGL as well? We have rarely, if ever, used the FSP's ucode loading facility in the past, coreboot typically handles the "2nd microcode load" (apart from the FIT load)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
Are we potentially missing this from TGL as well? We have rarely, if ever, used the FSP's ucode load […]
Mcheck flow is different than previous platform.
In past SGX was the reason to load the second patch load.
Bios should load the 2nd ucode after bios done indication is set and before bios cpl is sent to pcu.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Sridhar Siricilla, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48847
to look at the new patch set (#2).
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
soc/intel/alderlake: Update CPU microcode patch base address/size
This patch updates CPU microcode patch base address/size to FSP-S UPD to have second microcode patch loaded successfully to enable Mcheck flow.
This is new feature requirement for ADL as per new Mcheck initialization flow.
TEST=Able to reach beyond PC6 without any MCE.
Change-Id: I936816e3173dbcdf82b2b16b465f6b4ed5d90335 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/alderlake/fsp_params.c 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/48847/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
Mcheck flow is different than previous platform.
Is this documented somewhere? Can you please provide some reference?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
Mcheck flow is different than previous platform. […]
I see something in the core/uncore bios spec section 3.4.4 about an MCHECK flow after the post-mem ucode load
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
I see something in the core/uncore bios spec section 3.4. […]
yes, i believe thats the document, i have also started pulling CPU EDS folks to capture on more details in EDS.
Can you please allow me to merge this CL, without this S0ix is broken
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
Furquan, Tim, Angel: Any further review on this ?
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
2nd microcode loading is part of FSP doing MP init process, we are just assigning the UPD.
What if FSP(UCODE loader) fails to load UCODE? I think coreboot should check if UCODE is loaded succesfully or nor. If UCODE load fails, coreboot should trigger CrOS Recovery mode. I think we should have a separate discussion on this. For now, let this patch be in.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
What if FSP(UCODE loader) fails to load UCODE?
Consider this is 2nd time patching just needed for mcheck init flow as coreboot always use FIT way of loading ucode and this is not related to ucode update flow. In API mode if FSP fails to load ucode then FSP don't care because this is not chronical error do it will notify bootloader. But i believe this could be part of FSP info hob where FSP can dump this information and bootloader can parse to know the exact failure.
I think coreboot should check if UCODE is loaded succesfully or nor.
I think you are referring to mcheck init is successful or not, not the ucode load because ucode load would be always successful as part of FIT loading process
If UCODE load fails, coreboot should trigger CrOS Recovery mode. I think we should have a separate discussion on this.
2nd ucode load failure don't cause recovery rather there should be some information pass through between FSP and bootloader so that we avoid debugging such weird issue.
For now, let this patch be in.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
What if FSP(UCODE loader) fails to load UCODE? […]
But this 2nd load can load different ucode than is available in FIT; that's the whole point of doing the 2nd load, isn't it? Until we enable the in-the-field ucode update, the ucode in FIT is RO, so this is how we ship install new ucode; it might be good to have some kind of indication from FSP (I guess that means parsing a new HOB) if the ucode update was successful or not, but that's probably going to be a bigger effort, I'll file a bug and CC you two on it.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
But this 2nd load can load different ucode than is available in FIT; that's the whole point of doing […]
Filing the bug, but according to the core/uncore bios spec the ucode version read from the BIOS_SIGN MSR will appear as X-1 (instead of X) if the mcheck flow fails, so perhaps we could use that instead?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
Filing the bug, but according to the core/uncore bios spec the ucode version read from the BIOS_SIGN […]
This is good point, let me try that and add a warning msg without bothering FSP
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48847
to look at the new patch set (#3).
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
soc/intel/alderlake: Update CPU microcode patch base address/size
This patch updates CPU microcode patch base address/size to FSP-S UPD to have second microcode patch loaded successfully to enable Mcheck flow.
This is new feature requirement for ADL as per new Mcheck initialization flow.
BUG=b:176551651 TEST=Able to reach beyond PC6 without any MCE.
Change-Id: I936816e3173dbcdf82b2b16b465f6b4ed5d90335 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/alderlake/fsp_params.c 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/48847/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
This is good point, let me try that and add a warning msg without bothering FSP
updated the bug details. kindly refer Z and Z-1 is now obsolete hence can't be used to check the mcheck is done or not
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48847/1/src/soc/intel/alderlake/fsp... PS1, Line 112:
updated the bug details. […]
Thanks, let's go ahead with the CL for now and continue discussion on the bug.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
yes, i believe thats the document, i have also started pulling CPU EDS folks to capture on more deta […]
continuing discussion on the bug
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48847/1//COMMIT_MSG@9 PS1, Line 9: This patch updates CPU microcode patch base address/size to FSP-S : UPD to have second microcode patch loaded successfully to enable : Mcheck flow
continuing discussion on the bug
Thanks Tim
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
Patch Set 3: Code-Review+1
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48847 )
Change subject: soc/intel/alderlake: Update CPU microcode patch base address/size ......................................................................
soc/intel/alderlake: Update CPU microcode patch base address/size
This patch updates CPU microcode patch base address/size to FSP-S UPD to have second microcode patch loaded successfully to enable Mcheck flow.
This is new feature requirement for ADL as per new Mcheck initialization flow.
BUG=b:176551651 TEST=Able to reach beyond PC6 without any MCE.
Change-Id: I936816e3173dbcdf82b2b16b465f6b4ed5d90335 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48847 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Aamir Bohra aamir.bohra@intel.com Reviewed-by: Sridhar Siricilla sridhar.siricilla@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/alderlake/fsp_params.c 1 file changed, 11 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aamir Bohra: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Sridhar Siricilla: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/alderlake/fsp_params.c b/src/soc/intel/alderlake/fsp_params.c index a21ca4a..6de8649 100644 --- a/src/soc/intel/alderlake/fsp_params.c +++ b/src/soc/intel/alderlake/fsp_params.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <assert.h> +#include <cbfs.h> #include <console/console.h> #include <device/device.h> #include <device/pci.h> @@ -89,6 +90,8 @@ void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd) { int i; + const struct microcode *microcode_file; + size_t microcode_len; FSP_S_CONFIG *params = &supd->FspsConfig;
struct device *dev; @@ -99,6 +102,14 @@ /* Parse device tree and enable/disable Serial I/O devices */ parse_devicetree(params);
+ microcode_file = cbfs_map("cpu_microcode_blob.bin", µcode_len); + + if ((microcode_file != NULL) && (microcode_len != 0)) { + /* Update CPU Microcode patch base address/size */ + params->MicrocodeRegionBase = (uint32_t)microcode_file; + params->MicrocodeRegionSize = (uint32_t)microcode_len; + } + /* Load VBT before devicetree-specific config. */ params->GraphicsConfigPtr = (uintptr_t)vbt_get();