Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31486
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
soc/amd/common: Refactor AmdCreateStruct() use
AmdCreateStruct() and AmdReleaseStruct() are equally bad when it comes to lack of correct function declarations for definitions found in vendorcode binaryPI/AGESA.c.
Replace these with calls that go through the common module_dispatch() functions.
Change-Id: I611bcbe2a71fb65c8eb759a9dc74cbd9cb74136e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/pi/agesawrapper.c M src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c 2 files changed, 70 insertions(+), 100 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/31486/1
diff --git a/src/soc/amd/common/block/pi/agesawrapper.c b/src/soc/amd/common/block/pi/agesawrapper.c index f09fc5d..1c6c0aa 100644 --- a/src/soc/amd/common/block/pi/agesawrapper.c +++ b/src/soc/amd/common/block/pi/agesawrapper.c @@ -66,41 +66,60 @@ return module_dispatch(AMD_LATE_RUN_AP_TASK, StdHeader); }
-static void *create_struct(AMD_INTERFACE_PARAMS *interface_struct) +static void *amd_create_struct(AMD_INTERFACE_PARAMS *aip, + AGESA_STRUCT_NAME func, void *buf, size_t len) { AMD_CONFIG_PARAMS *StdHeader; + AGESA_STATUS status;
/* Should clone entire StdHeader here. */ - interface_struct->StdHeader.CalloutPtr = &GetBiosCallout; + memset(aip, 0, sizeof(*aip)); + aip->StdHeader.CalloutPtr = &GetBiosCallout;
- AGESA_STATUS status = AmdCreateStruct(interface_struct); + /* If we provide the buffer, API expects it to have + StdHeader already filled. */ + if (buf != NULL && len >= sizeof(*StdHeader)) { + memcpy(buf, &aip->StdHeader, sizeof(*StdHeader)); + aip->AllocationMethod = ByHost; + aip->NewStructPtr = buf; + aip->NewStructSize = len; + } else { + if (ENV_ROMSTAGE) + aip->AllocationMethod = PreMemHeap; + if (ENV_RAMSTAGE) + aip->AllocationMethod = PostMemDram; + } + + aip->AgesaFunctionName = func; + status = module_dispatch(AMD_CREATE_STRUCT, &aip->StdHeader);
if (status != AGESA_SUCCESS) { printk(BIOS_ERR, "Error: AmdCreateStruct() for 0x%x returned 0x%x. " "Proper system initialization may not be possible.\n", - interface_struct->AgesaFunctionName, status); + aip->AgesaFunctionName, status); }
- if (!interface_struct->NewStructPtr) /* Avoid NULL pointer usage */ + if (!aip->NewStructPtr) die("No AGESA structure created");
- StdHeader = interface_struct->NewStructPtr; - StdHeader->Func = interface_struct->AgesaFunctionName; + StdHeader = aip->NewStructPtr; + StdHeader->Func = aip->AgesaFunctionName; return StdHeader; }
+static AGESA_STATUS amd_release_struct(AMD_INTERFACE_PARAMS *aip) +{ + return module_dispatch(AMD_RELEASE_STRUCT, &aip->StdHeader); +} + static AGESA_STATUS amd_init_reset(void) { AGESA_STATUS status; AMD_RESET_PARAMS _ResetParams; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_RESET, - .AllocationMethod = ByHost, - .NewStructSize = sizeof(AMD_RESET_PARAMS), - .NewStructPtr = &_ResetParams, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_RESET_PARAMS *ResetParams = create_struct(&AmdParamStruct); + AMD_RESET_PARAMS *ResetParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_RESET, &_ResetParams, sizeof(AMD_RESET_PARAMS));
SetFchResetParams(&ResetParams->FchInterface);
@@ -108,19 +127,17 @@ status = amd_dispatch(ResetParams); timestamp_add_now(TS_AGESA_INIT_RESET_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct); return status; }
static AGESA_STATUS amd_init_early(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_EARLY, - .AllocationMethod = PreMemHeap, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_EARLY_PARAMS *EarlyParams = create_struct(&AmdParamStruct); + AMD_EARLY_PARAMS *EarlyParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_EARLY, NULL, 0);
soc_customize_init_early(EarlyParams); OemCustomizeInitEarly(EarlyParams); @@ -129,7 +146,7 @@ status = amd_dispatch(EarlyParams); timestamp_add_now(TS_AGESA_INIT_EARLY_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -170,12 +187,10 @@ static AGESA_STATUS amd_init_post(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_POST, - .AllocationMethod = PreMemHeap, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_POST_PARAMS *PostParams = create_struct(&AmdParamStruct); + AMD_POST_PARAMS *PostParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_POST, NULL, 0);
PostParams->MemConfig.UmaMode = CONFIG_GFXUMA ? UMA_AUTO : UMA_NONE; PostParams->MemConfig.UmaSize = 0; @@ -215,7 +230,7 @@
print_init_post_settings(PostParams);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -223,12 +238,10 @@ static AGESA_STATUS amd_init_env(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_ENV, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_ENV_PARAMS *EnvParams = create_struct(&AmdParamStruct); + AMD_ENV_PARAMS *EnvParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_ENV, NULL, 0);
SetFchEnvParams(&EnvParams->FchInterface); SetNbEnvParams(&EnvParams->GnbEnvConfiguration); @@ -237,7 +250,7 @@ status = amd_dispatch(EnvParams); timestamp_add_now(TS_AGESA_INIT_ENV_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -271,15 +284,13 @@ static AGESA_STATUS amd_init_mid(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_MID, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
/* Enable MMIO on AMD CPU Address Map Controller */ amd_initcpuio();
- AMD_MID_PARAMS *MidParams = create_struct(&AmdParamStruct); + AMD_MID_PARAMS *MidParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_MID, NULL, 0);
SetFchMidParams(&MidParams->FchInterface); SetNbMidParams(&MidParams->GnbMidConfiguration); @@ -288,7 +299,7 @@ status = amd_dispatch(MidParams); timestamp_add_now(TS_AGESA_INIT_MID_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -296,16 +307,14 @@ static AGESA_STATUS amd_init_late(void) { AGESA_STATUS Status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_LATE, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
/* * NOTE: if not call amdcreatestruct, the initializer * (AmdInitLateInitializer) would not be called. */ - AMD_LATE_PARAMS *LateParams = create_struct(&AmdParamStruct); + AMD_LATE_PARAMS *LateParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_LATE, NULL, 0);
const struct device *dev = pcidev_path_on_root(IOMMU_DEVFN); if (dev && dev->enabled) { @@ -334,19 +343,17 @@ AcpiSlit, AcpiWheaMce, AcpiWheaCmc, AcpiAlib, AcpiIvrs, __func__);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct); return Status; }
static AGESA_STATUS amd_init_rtb(void) { AGESA_STATUS Status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_RTB, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_RTB_PARAMS *RtbParams = create_struct(&AmdParamStruct); + AMD_RTB_PARAMS *RtbParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_RTB, NULL, 0);
timestamp_add_now(TS_AGESA_INIT_RTB_START); Status = amd_dispatch(RtbParams); @@ -358,7 +365,7 @@ RtbParams->S3DataBlock.VolatileStorageSize)) printk(BIOS_ERR, "S3 data not saved, resuming impossible\n");
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return Status; } @@ -366,13 +373,11 @@ static AGESA_STATUS amd_init_resume(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_RESUME, - .AllocationMethod = PreMemHeap, - }; + AMD_INTERFACE_PARAMS AmdParamStruct; size_t nv_size;
- AMD_RESUME_PARAMS *InitResumeParams = create_struct(&AmdParamStruct); + AMD_RESUME_PARAMS *InitResumeParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_RESUME, NULL, 0);
get_s3nv_info(&InitResumeParams->S3DataBlock.NvStorage, &nv_size); InitResumeParams->S3DataBlock.NvStorageSize = nv_size; @@ -381,7 +386,7 @@ status = amd_dispatch(InitResumeParams); timestamp_add_now(TS_AGESA_INIT_RESUME_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -390,17 +395,13 @@ { AGESA_STATUS Status; AMD_S3LATE_PARAMS _S3LateParams; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_S3LATE_RESTORE, - .AllocationMethod = ByHost, - .NewStructSize = sizeof(AMD_S3LATE_PARAMS), - .NewStructPtr = &_S3LateParams, - }; + AMD_INTERFACE_PARAMS AmdParamStruct; size_t vol_size;
amd_initcpuio();
- AMD_S3LATE_PARAMS *S3LateParams = create_struct(&AmdParamStruct); + AMD_S3LATE_PARAMS *S3LateParams = amd_create_struct(&AmdParamStruct, + AMD_S3LATE_RESTORE, &_S3LateParams, sizeof(AMD_S3LATE_PARAMS));
get_s3vol_info(&S3LateParams->S3DataBlock.VolatileStorage, &vol_size); S3LateParams->S3DataBlock.VolatileStorageSize = vol_size; @@ -409,7 +410,7 @@ Status = amd_dispatch(S3LateParams); timestamp_add_now(TS_AGESA_S3_LATE_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return Status; } @@ -418,15 +419,11 @@ { AGESA_STATUS Status; AMD_S3FINAL_PARAMS _S3FinalParams; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_S3FINAL_RESTORE, - .AllocationMethod = ByHost, - .NewStructSize = sizeof(AMD_S3FINAL_PARAMS), - .NewStructPtr = &_S3FinalParams, - }; + AMD_INTERFACE_PARAMS AmdParamStruct; size_t vol_size;
- AMD_S3FINAL_PARAMS *S3FinalParams = create_struct(&AmdParamStruct); + AMD_S3FINAL_PARAMS *S3FinalParams = amd_create_struct(&AmdParamStruct, + AMD_S3FINAL_RESTORE, &_S3FinalParams, sizeof(AMD_S3FINAL_PARAMS));
get_s3vol_info(&S3FinalParams->S3DataBlock.VolatileStorage, &vol_size); S3FinalParams->S3DataBlock.VolatileStorageSize = vol_size; @@ -435,7 +432,7 @@ Status = amd_dispatch(S3FinalParams); timestamp_add_now(TS_AGESA_S3_FINAL_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return Status; } diff --git a/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c b/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c index e17cedc..da89ce8 100644 --- a/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c +++ b/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c @@ -45,30 +45,3 @@ CONST UINT32 ModuleSignature = MODULE_SIGNATURE; CONST CHAR8 ModuleIdentifier[8] = AGESA_ID;
-/********************************************************************** - * Interface call: AmdCreateStruct - **********************************************************************/ -AGESA_STATUS -AmdCreateStruct ( - IN OUT AMD_INTERFACE_PARAMS *InterfaceParams - ) -{ - MODULE_ENTRY Dispatcher = agesa_get_dispatcher(); - InterfaceParams->StdHeader.Func = AMD_CREATE_STRUCT; - if (!Dispatcher) return AGESA_UNSUPPORTED; - return Dispatcher(InterfaceParams); -} - -/********************************************************************** - * Interface call: AmdReleaseStruct - **********************************************************************/ -AGESA_STATUS -AmdReleaseStruct ( - IN OUT AMD_INTERFACE_PARAMS *InterfaceParams - ) -{ - MODULE_ENTRY Dispatcher = agesa_get_dispatcher(); - InterfaceParams->StdHeader.Func = AMD_RELEASE_STRUCT; - if (!Dispatcher) return AGESA_UNSUPPORTED; - return Dispatcher(InterfaceParams); -}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31486/1/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/1/src/soc/amd/common/block/pi/agesawra... PS1, Line 426: AMD_S3FINAL_RESTORE, &_S3FinalParams, sizeof(AMD_S3FINAL_PARAMS)); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31486/2/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/2/src/soc/amd/common/block/pi/agesawra... PS2, Line 426: AMD_S3FINAL_RESTORE, &_S3FinalParams, sizeof(AMD_S3FINAL_PARAMS)); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31486/3/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/3/src/soc/amd/common/block/pi/agesawra... PS3, Line 426: AMD_S3FINAL_RESTORE, &_S3FinalParams, sizeof(AMD_S3FINAL_PARAMS)); line over 80 characters
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 3: Code-Review+1
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31486/4/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/4/src/soc/amd/common/block/pi/agesawra... PS4, Line 426: AMD_S3FINAL_RESTORE, &_S3FinalParams, sizeof(AMD_S3FINAL_PARAMS)); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 425: AMD_S3FINAL_RESTORE, &_S3FinalParams, sizeof(AMD_S3FINAL_PARAMS)); line over 80 characters
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 68: aip Why aip? Initial name, interface_struct, was descriptive (though long), I can't figure out why aip.
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 111: return module_dispatch(AMD_RELEASE_STRUCT, &aip->StdHeader); Original code replaced AGESA_BOUNDS_CHK with AGESA_UNSUPPORTED. However, a bounds check means that there's no create_struct/release_struct for the particular function, so it would already have failed to create the structure and, and would die at AMD_INIT_EARLY. This is going out of AMD initial intention, but I don't think it's harmful.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 68: aip
Why aip? Initial name, interface_struct, was descriptive (though long), I can't figure out why aip.
Consistent with src/drivers/amd/agesa/state_machine.c; ultimately there should be only one AGESA v8 implementation for this amd_create_struct().
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 111: return module_dispatch(AMD_RELEASE_STRUCT, &aip->StdHeader);
Original code replaced AGESA_BOUNDS_CHK with AGESA_UNSUPPORTED. […]
Can you elaborate what change you see and where in source code, where did this replace originate happen?
The return value of AmdReleaseStruct() call was never evaluated.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 5: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 68: aip
Consistent with src/drivers/amd/agesa/state_machine. […]
Unfortunately that's not true. AGESA varies depending on Carrizo/Bristol, FP4/FT4, and the associated southbridge. There's one AGESA for stoneyridge, another for Merlin falcon, and so on. But I'll accept your reasons.
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 111: return module_dispatch(AMD_RELEASE_STRUCT, &aip->StdHeader);
Can you elaborate what change you see and where in source code, where did this replace originate hap […]
The return of is internal to AGESA. That said, look agesa.c line 58 to see where it's replaced by unsupported. The die actually happens in line 102 above (it does not happens in amd_init_reset because at init reset the physical pointer is created ByHost. I have seen this myself in a project with SOC merlinfalcon, where the returned status was a bounds check and it died at amd_init_early. So I know there will be no ill effect simply because it'll die on a situation where it could cause such problem.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
soc/amd/common: Refactor AmdCreateStruct() use
AmdCreateStruct() and AmdReleaseStruct() are equally bad when it comes to lack of correct function declarations for definitions found in vendorcode binaryPI/AGESA.c.
Replace these with calls that go through the common module_dispatch() functions.
Change-Id: I611bcbe2a71fb65c8eb759a9dc74cbd9cb74136e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31486 Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/pi/agesawrapper.c M src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c 2 files changed, 70 insertions(+), 100 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Richard Spiegel: Looks good to me, approved
diff --git a/src/soc/amd/common/block/pi/agesawrapper.c b/src/soc/amd/common/block/pi/agesawrapper.c index 5f58346..da6de00 100644 --- a/src/soc/amd/common/block/pi/agesawrapper.c +++ b/src/soc/amd/common/block/pi/agesawrapper.c @@ -65,41 +65,60 @@ return module_dispatch(AMD_LATE_RUN_AP_TASK, StdHeader); }
-static void *create_struct(AMD_INTERFACE_PARAMS *interface_struct) +static void *amd_create_struct(AMD_INTERFACE_PARAMS *aip, + AGESA_STRUCT_NAME func, void *buf, size_t len) { AMD_CONFIG_PARAMS *StdHeader; + AGESA_STATUS status;
/* Should clone entire StdHeader here. */ - interface_struct->StdHeader.CalloutPtr = &GetBiosCallout; + memset(aip, 0, sizeof(*aip)); + aip->StdHeader.CalloutPtr = &GetBiosCallout;
- AGESA_STATUS status = AmdCreateStruct(interface_struct); + /* If we provide the buffer, API expects it to have + StdHeader already filled. */ + if (buf != NULL && len >= sizeof(*StdHeader)) { + memcpy(buf, &aip->StdHeader, sizeof(*StdHeader)); + aip->AllocationMethod = ByHost; + aip->NewStructPtr = buf; + aip->NewStructSize = len; + } else { + if (ENV_ROMSTAGE) + aip->AllocationMethod = PreMemHeap; + if (ENV_RAMSTAGE) + aip->AllocationMethod = PostMemDram; + } + + aip->AgesaFunctionName = func; + status = module_dispatch(AMD_CREATE_STRUCT, &aip->StdHeader);
if (status != AGESA_SUCCESS) { printk(BIOS_ERR, "Error: AmdCreateStruct() for 0x%x returned 0x%x. " "Proper system initialization may not be possible.\n", - interface_struct->AgesaFunctionName, status); + aip->AgesaFunctionName, status); }
- if (!interface_struct->NewStructPtr) /* Avoid NULL pointer usage */ + if (!aip->NewStructPtr) die("No AGESA structure created");
- StdHeader = interface_struct->NewStructPtr; - StdHeader->Func = interface_struct->AgesaFunctionName; + StdHeader = aip->NewStructPtr; + StdHeader->Func = aip->AgesaFunctionName; return StdHeader; }
+static AGESA_STATUS amd_release_struct(AMD_INTERFACE_PARAMS *aip) +{ + return module_dispatch(AMD_RELEASE_STRUCT, &aip->StdHeader); +} + static AGESA_STATUS amd_init_reset(void) { AGESA_STATUS status; AMD_RESET_PARAMS _ResetParams; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_RESET, - .AllocationMethod = ByHost, - .NewStructSize = sizeof(AMD_RESET_PARAMS), - .NewStructPtr = &_ResetParams, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_RESET_PARAMS *ResetParams = create_struct(&AmdParamStruct); + AMD_RESET_PARAMS *ResetParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_RESET, &_ResetParams, sizeof(AMD_RESET_PARAMS));
SetFchResetParams(&ResetParams->FchInterface);
@@ -107,19 +126,17 @@ status = amd_dispatch(ResetParams); timestamp_add_now(TS_AGESA_INIT_RESET_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct); return status; }
static AGESA_STATUS amd_init_early(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_EARLY, - .AllocationMethod = PreMemHeap, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_EARLY_PARAMS *EarlyParams = create_struct(&AmdParamStruct); + AMD_EARLY_PARAMS *EarlyParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_EARLY, NULL, 0);
soc_customize_init_early(EarlyParams); OemCustomizeInitEarly(EarlyParams); @@ -128,7 +145,7 @@ status = amd_dispatch(EarlyParams); timestamp_add_now(TS_AGESA_INIT_EARLY_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -169,12 +186,10 @@ static AGESA_STATUS amd_init_post(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_POST, - .AllocationMethod = PreMemHeap, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_POST_PARAMS *PostParams = create_struct(&AmdParamStruct); + AMD_POST_PARAMS *PostParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_POST, NULL, 0);
PostParams->MemConfig.UmaMode = CONFIG(GFXUMA) ? UMA_AUTO : UMA_NONE; PostParams->MemConfig.UmaSize = 0; @@ -214,7 +229,7 @@
print_init_post_settings(PostParams);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -222,12 +237,10 @@ static AGESA_STATUS amd_init_env(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_ENV, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_ENV_PARAMS *EnvParams = create_struct(&AmdParamStruct); + AMD_ENV_PARAMS *EnvParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_ENV, NULL, 0);
SetFchEnvParams(&EnvParams->FchInterface); SetNbEnvParams(&EnvParams->GnbEnvConfiguration); @@ -236,7 +249,7 @@ status = amd_dispatch(EnvParams); timestamp_add_now(TS_AGESA_INIT_ENV_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -270,15 +283,13 @@ static AGESA_STATUS amd_init_mid(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_MID, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
/* Enable MMIO on AMD CPU Address Map Controller */ amd_initcpuio();
- AMD_MID_PARAMS *MidParams = create_struct(&AmdParamStruct); + AMD_MID_PARAMS *MidParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_MID, NULL, 0);
SetFchMidParams(&MidParams->FchInterface); SetNbMidParams(&MidParams->GnbMidConfiguration); @@ -287,7 +298,7 @@ status = amd_dispatch(MidParams); timestamp_add_now(TS_AGESA_INIT_MID_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -295,16 +306,14 @@ static AGESA_STATUS amd_init_late(void) { AGESA_STATUS Status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_LATE, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
/* * NOTE: if not call amdcreatestruct, the initializer * (AmdInitLateInitializer) would not be called. */ - AMD_LATE_PARAMS *LateParams = create_struct(&AmdParamStruct); + AMD_LATE_PARAMS *LateParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_LATE, NULL, 0);
const struct device *dev = pcidev_path_on_root(IOMMU_DEVFN); if (dev && dev->enabled) { @@ -333,19 +342,17 @@ AcpiSlit, AcpiWheaMce, AcpiWheaCmc, AcpiAlib, AcpiIvrs, __func__);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct); return Status; }
static AGESA_STATUS amd_init_rtb(void) { AGESA_STATUS Status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_RTB, - .AllocationMethod = PostMemDram, - }; + AMD_INTERFACE_PARAMS AmdParamStruct;
- AMD_RTB_PARAMS *RtbParams = create_struct(&AmdParamStruct); + AMD_RTB_PARAMS *RtbParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_RTB, NULL, 0);
timestamp_add_now(TS_AGESA_INIT_RTB_START); Status = amd_dispatch(RtbParams); @@ -357,7 +364,7 @@ RtbParams->S3DataBlock.VolatileStorageSize)) printk(BIOS_ERR, "S3 data not saved, resuming impossible\n");
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return Status; } @@ -365,13 +372,11 @@ static AGESA_STATUS amd_init_resume(void) { AGESA_STATUS status; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_INIT_RESUME, - .AllocationMethod = PreMemHeap, - }; + AMD_INTERFACE_PARAMS AmdParamStruct; size_t nv_size;
- AMD_RESUME_PARAMS *InitResumeParams = create_struct(&AmdParamStruct); + AMD_RESUME_PARAMS *InitResumeParams = amd_create_struct(&AmdParamStruct, + AMD_INIT_RESUME, NULL, 0);
get_s3nv_info(&InitResumeParams->S3DataBlock.NvStorage, &nv_size); InitResumeParams->S3DataBlock.NvStorageSize = nv_size; @@ -380,7 +385,7 @@ status = amd_dispatch(InitResumeParams); timestamp_add_now(TS_AGESA_INIT_RESUME_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return status; } @@ -389,17 +394,13 @@ { AGESA_STATUS Status; AMD_S3LATE_PARAMS _S3LateParams; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_S3LATE_RESTORE, - .AllocationMethod = ByHost, - .NewStructSize = sizeof(AMD_S3LATE_PARAMS), - .NewStructPtr = &_S3LateParams, - }; + AMD_INTERFACE_PARAMS AmdParamStruct; size_t vol_size;
amd_initcpuio();
- AMD_S3LATE_PARAMS *S3LateParams = create_struct(&AmdParamStruct); + AMD_S3LATE_PARAMS *S3LateParams = amd_create_struct(&AmdParamStruct, + AMD_S3LATE_RESTORE, &_S3LateParams, sizeof(AMD_S3LATE_PARAMS));
get_s3vol_info(&S3LateParams->S3DataBlock.VolatileStorage, &vol_size); S3LateParams->S3DataBlock.VolatileStorageSize = vol_size; @@ -408,7 +409,7 @@ Status = amd_dispatch(S3LateParams); timestamp_add_now(TS_AGESA_S3_LATE_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return Status; } @@ -417,15 +418,11 @@ { AGESA_STATUS Status; AMD_S3FINAL_PARAMS _S3FinalParams; - AMD_INTERFACE_PARAMS AmdParamStruct = { - .AgesaFunctionName = AMD_S3FINAL_RESTORE, - .AllocationMethod = ByHost, - .NewStructSize = sizeof(AMD_S3FINAL_PARAMS), - .NewStructPtr = &_S3FinalParams, - }; + AMD_INTERFACE_PARAMS AmdParamStruct; size_t vol_size;
- AMD_S3FINAL_PARAMS *S3FinalParams = create_struct(&AmdParamStruct); + AMD_S3FINAL_PARAMS *S3FinalParams = amd_create_struct(&AmdParamStruct, + AMD_S3FINAL_RESTORE, &_S3FinalParams, sizeof(AMD_S3FINAL_PARAMS));
get_s3vol_info(&S3FinalParams->S3DataBlock.VolatileStorage, &vol_size); S3FinalParams->S3DataBlock.VolatileStorageSize = vol_size; @@ -434,7 +431,7 @@ Status = amd_dispatch(S3FinalParams); timestamp_add_now(TS_AGESA_S3_FINAL_DONE);
- AmdReleaseStruct(&AmdParamStruct); + amd_release_struct(&AmdParamStruct);
return Status; } diff --git a/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c b/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c index e17cedc..da89ce8 100644 --- a/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c +++ b/src/vendorcode/amd/pi/00670F00/binaryPI/AGESA.c @@ -45,30 +45,3 @@ CONST UINT32 ModuleSignature = MODULE_SIGNATURE; CONST CHAR8 ModuleIdentifier[8] = AGESA_ID;
-/********************************************************************** - * Interface call: AmdCreateStruct - **********************************************************************/ -AGESA_STATUS -AmdCreateStruct ( - IN OUT AMD_INTERFACE_PARAMS *InterfaceParams - ) -{ - MODULE_ENTRY Dispatcher = agesa_get_dispatcher(); - InterfaceParams->StdHeader.Func = AMD_CREATE_STRUCT; - if (!Dispatcher) return AGESA_UNSUPPORTED; - return Dispatcher(InterfaceParams); -} - -/********************************************************************** - * Interface call: AmdReleaseStruct - **********************************************************************/ -AGESA_STATUS -AmdReleaseStruct ( - IN OUT AMD_INTERFACE_PARAMS *InterfaceParams - ) -{ - MODULE_ENTRY Dispatcher = agesa_get_dispatcher(); - InterfaceParams->StdHeader.Func = AMD_RELEASE_STRUCT; - if (!Dispatcher) return AGESA_UNSUPPORTED; - return Dispatcher(InterfaceParams); -}
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 68: aip
Unfortunately that's not true. […]
The file I referred under src/drivers is the top (or bottom, however you look at it) API common to all AGESA v8.
Yes, I know the headers and structs may differ across different PI's (MullinsPI, KaveriPI, CarrizoPI and StoneyPI). That's why there are PI dependent files (like fam15tn/state_machine.c) with open-source AGESA v8 already at place.
I encourage you to have a peek on state of the agesawrapper.c files at the time they were originally merged in [1]. And there was one copy per motherboard!
[1]: https://review.coreboot.org/c/coreboot/+/1158/10/src/mainboard/amd/parmer/ag...
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 111: return module_dispatch(AMD_RELEASE_STRUCT, &aip->StdHeader);
The return of is internal to AGESA. That said, look agesa. […]
I don't see how AGESA.c line 58 is related to any AGESA_BOUNDS_CHK.
Please be clear when you place arguments on changes that are not in upstream coreboot tree. I believe Marshall had previously identified src/soc/amd/common did not work for quad-core (dual-compute-unit) SKUs due to cache coherency issue CB:26115.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31486 )
Change subject: soc/amd/common: Refactor AmdCreateStruct() use ......................................................................
Patch Set 6:
(1 comment)
Sorry I did not had time yesterday to continue reviewing your other patches. I do have about 2 hours now, so I'll start it now.
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31486/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 111: return module_dispatch(AMD_RELEASE_STRUCT, &aip->StdHeader);
I don't see how AGESA.c line 58 is related to any AGESA_BOUNDS_CHK. […]
Yes, I'm aware of Marshall's (and Marc's) finding about 4 core. Also, when I started working, stoney AGESA was private due to several modifications required by Google. I am/was not aware that it has gone public, thus I don't talk about it's internals in coreboot (though I do have access to the source).
-
build bot (Jenkins) (Code Review) -
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Richard Spiegel (Code Review)