summaryrefslogtreecommitdiff
path: root/src/soc/intel/meteorlake
diff options
context:
space:
mode:
authorSubrata Banik <subratabanik@google.com>2022-12-09 13:38:26 +0530
committerFelix Held <felix-coreboot@felixheld.de>2022-12-19 14:14:11 +0000
commit848c37da42572df8930bb9651c70a2fab91ba131 (patch)
tree678fc6970a1b9fe0b9f0749fae4ef8bd14cd44ef /src/soc/intel/meteorlake
parentf251a6a439e70d7003ee6a5fbfae79eab3971e75 (diff)
soc/intel/meteorlake: Remove dependency of FSP-S CpuMpPei Module
This patch fixes a hidden issue present inside FSP-S while coreboot decides to skip performing MP initialization by overriding FSP-S UPDs as below: 1. CpuMpPpi ------> Passing `NULL` as coreboot assume FSP don't need to use coreboot wrapper for performing any operation over APs. 2. SkipMpInit -----> Set `1` to let FSP know that coreboot decided to skip FSP running CPU feature programming. Unfortunately, the assumption of coreboot is not aligned with FSP when it comes to the behaviour of `CpuMpPpi` UPD. FSP assumes ownership of the APs (Application Processors) upon passing `NULL` pointer to the `CpuMpPpi` FSP-S UPD. FSP-S creates its own infrastructure code after seeing the CpuMpPpi UPD is set to `NULL`. FSP requires the CpuMpPei module, file name `UefiCpuPkg/CpuMpPei/CpuMpPei.c`, function name `InitializeCpuMpWorker` to perform those additional initialization which is not relevant for the coreboot upon selecting the SkipMpInit UPD to 1 (a.k.a avoid running CPU feature programming on APs). Additionally, FSP-S binary size has increased by ~30KB (irrespective of being compressed) with the inclusion of the CpuMpPei module, which is eventually not meaningful for coreboot. Hence, this patch selects `MP_SERVICES_PPI_V2_NOOP` config unconditionally to ensure pass a valid pointer to the `CpuMpPpi` UPD and avoid APs getting hijacked by FSP while coreboot decides to set SkipMpInit UPD. Ideally, FSP should have avoided all AP related operations when coreboot requested FSP to skip MP init by overriding required UPDs. TEST=Able to drop CpuMpPei Module from FSP and boot to Chrome OS on Google/Redrix, Kano, Taeko devices with SkipMpInit=1. Without this patch: Here is the CPU AP logs coming from the EDK2 (open-source) [UefiCpuPkg/CpuMpPei/CpuMpPei.c] when coreboot sets `NULL` to the CpuMpPpi UPD. [SPEW ] Loading PEIM EDADEB9D-DDBA-48BD-9D22-C1C169C8C5C6 [SPEW ] Loading PEIM at 0x00076F9A000 EntryPoint=0x00076FA24E2 CpuMpPei.efi PROGRESS CODE: V03020002 I0 [SPEW ] Register PPI Notify: F894643D-C449-42D1-8EA8-85BDD8C65BDE [SPEW ] Notify: PPI Guid: F894643D-C449-42D1-8EA8-85BDD8C65BDE, Peim notify entry point: 76FA0239 AP Loop Mode is 2 GetMicrocodePatchInfoFromHob: Microcode patch cache HOB is not found. CPU[0000]: Microcode revision = 00000000, expected = 00000000 [SPEW ] Register PPI Notify: 8F9D4825-797D-48FC-8471-845025792EF6 Does not find any stored CPU BIST information from PPI! APICID - 0x00000000, BIST - 0x00000000 [SPEW ] Install PPI: 9E9F374B-8F16-4230-9824-5846EE766A97 [SPEW ] Install PPI: 5CB9CB3D-31A4-480C-9498-29D269BACFBA [SPEW ] Install PPI: EE16160A-E8BE-47A6-820A-C6900DB0250A PROGRESS CODE: V03020003 I0 With this patch: No instance of `CpuMpPei` has been found in the AP UART log with FSP debug enabled. This patch is backported from commit 8409f156d588e74932924ae8aac69478a4b6388e (soc/intel/alderlake: Remove dependency of FSP-S CpuMpPei Module) Change-Id: I7d9fb37ca1cd4bf325edc951ee7293e459fa2ea4 Signed-off-by: Subrata Banik <subratabanik@google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/70600 Reviewed-by: Eric Lai <eric_lai@quanta.corp-partner.google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Kapil Porwal <kapilporwal@google.com> Reviewed-by: Ivy Jian <ivy.jian@quanta.corp-partner.google.com>
Diffstat (limited to 'src/soc/intel/meteorlake')
-rw-r--r--src/soc/intel/meteorlake/Kconfig7
-rw-r--r--src/soc/intel/meteorlake/fsp_params.c28
2 files changed, 21 insertions, 14 deletions
diff --git a/src/soc/intel/meteorlake/Kconfig b/src/soc/intel/meteorlake/Kconfig
index ab66211eec..919f08cbac 100644
--- a/src/soc/intel/meteorlake/Kconfig
+++ b/src/soc/intel/meteorlake/Kconfig
@@ -339,6 +339,13 @@ config MTL_USE_FSP_MP_INIT
config MTL_USE_COREBOOT_MP_INIT
bool "Use coreboot MP init"
+ # FSP assumes ownership of the APs (Application Processors)
+ # upon passing `NULL` pointer to the CpuMpPpi FSP-S UPD.
+ # Hence, select `MP_SERVICES_PPI_V2_NOOP` config to pass a valid
+ # pointer to the CpuMpPpi UPD with FSP_UNSUPPORTED type APIs.
+ # This will protect APs from getting hijacked by FSP while coreboot
+ # decides to set SkipMpInit UPD.
+ select MP_SERVICES_PPI_V2_NOOP
select RELOAD_MICROCODE_PATCH
help
Upon selection, coreboot performs MP Initialization that includes feature programming.
diff --git a/src/soc/intel/meteorlake/fsp_params.c b/src/soc/intel/meteorlake/fsp_params.c
index 816a704790..a624025591 100644
--- a/src/soc/intel/meteorlake/fsp_params.c
+++ b/src/soc/intel/meteorlake/fsp_params.c
@@ -144,21 +144,21 @@ static void fill_fsps_microcode_params(FSP_S_CONFIG *s_cfg,
static void fill_fsps_cpu_params(FSP_S_CONFIG *s_cfg,
const struct soc_intel_meteorlake_config *config)
{
- if (CONFIG(MTL_USE_FSP_MP_INIT)) {
- /*
- * Fill `2nd microcode loading FSP UPD` if FSP is running CPU feature
- * programming.
- */
+ /*
+ * FIXME: FSP assumes ownership of the APs (Application Processors)
+ * upon passing `NULL` pointer to the CpuMpPpi FSP-S UPD.
+ * Hence, pass a valid pointer to the CpuMpPpi UPD unconditionally.
+ * This would avoid APs from getting hijacked by FSP while coreboot
+ * decides to set SkipMpInit UPD.
+ */
+ s_cfg->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data();
+
+ /*
+ * Fill `2nd microcode loading FSP UPD` if FSP is running CPU feature
+ * programming.
+ */
+ if (CONFIG(MTL_USE_FSP_MP_INIT))
fill_fsps_microcode_params(s_cfg, config);
- /*
- * Use FSP running MP PPI services to perform CPU feature programming
- * if Kconfig is enabled
- */
- s_cfg->CpuMpPpi = (uintptr_t)mp_fill_ppi_services_data();
- } else {
- /* Use coreboot native driver to perform MP init by default */
- s_cfg->CpuMpPpi = (uintptr_t)NULL;
- }
}