Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31489
Change subject: [WIP] soc/amd/common: Use static allocation for params ......................................................................
[WIP] soc/amd/common: Use static allocation for params
Lifetime of the structure is the duration of call to AGESA. There is no need to allocate and release these from AGESA's internal heap for every single call.
Change-Id: Ibef6ca8481f926d4e18e1aef5136e69f5834feb1 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/pi/agesawrapper.c 1 file changed, 22 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/31489/1
diff --git a/src/soc/amd/common/block/pi/agesawrapper.c b/src/soc/amd/common/block/pi/agesawrapper.c index 821535d..e769a45 100644 --- a/src/soc/amd/common/block/pi/agesawrapper.c +++ b/src/soc/amd/common/block/pi/agesawrapper.c @@ -410,6 +410,26 @@
}
+union AMD_MAX_ALLOC_PARAMS { + AMD_INTERFACE_PARAMS p1; + AMD_RESET_PARAMS p2; + AMD_EARLY_PARAMS p3; + AMD_POST_PARAMS p4; + AMD_RESUME_PARAMS p5; + AMD_ENV_PARAMS p6; + AMD_MID_PARAMS p7; + AMD_LATE_PARAMS p8; +#if 1 + AMD_RTB_PARAMS p9; +#else + AMD_S3SAVE_PARAMS p10; +#endif + AMD_S3LATE_PARAMS p11; + AMD_S3FINAL_PARAMS p12; +}; + +static union AMD_MAX_ALLOC_PARAMS sp; + AGESA_STATUS agesa_execute_state(AGESA_STRUCT_NAME func) { AGESA_STATUS status = AGESA_UNSUPPORTED; @@ -417,19 +437,9 @@ AMD_CONFIG_PARAMS *StdHeader = &template; AMD_INTERFACE_PARAMS AmdParamStruct; AMD_INTERFACE_PARAMS *aip = &AmdParamStruct; - union { - AMD_RESET_PARAMS ResetParams; - AMD_S3LATE_PARAMS S3LateParams; - AMD_S3FINAL_PARAMS S3FinalParams; - } sp;
- if ((func == AMD_INIT_RESET) || (func == AMD_S3LATE_RESTORE) || - (func == AMD_S3FINAL_RESTORE)) { - memset(&sp, 0, sizeof(sp)); - amd_create_struct(aip, func, &sp, sizeof(sp)); - } else { - amd_create_struct(aip, func, NULL, 0); - } + memset(&sp, 0, sizeof(sp)); + amd_create_struct(aip, func, &sp, sizeof(sp));
StdHeader = aip->NewStructPtr; StdHeader->Func = func;
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31489 )
Change subject: [WIP] soc/amd/common: Use static allocation for params ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 425: : : : : : : : This section of code is needed, otherwise aip->AllocationMethod will always be ByHost and cause confusion inside AGESA (will assume heap is always ByHost) However, AGESA internals are different depending on ByHost, PreMemHeap and PostMemDram. I believe that it'll not boot without this code.
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 430: static union AMD_MAX_ALLOC_PARAMS sp; I'm ok with making this static, but only p2, p11 and p12 are needed.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31489 )
Change subject: [WIP] soc/amd/common: Use static allocation for params ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 425: : : : : : : :
This section of code is needed, otherwise aip->AllocationMethod will always be ByHost and cause con […]
The point of this entire commit is that the lifetime of the parameter struct is the duration of the execution of the state, we release the struct before we return from agesa_execute_state().
As for agesa internals, ByHost means the artifact will not be allocated fro the heap at all. There is one argument that would justify allocating it from the heap; the size of the struct involved. It would be faster to have these on stack or .bss, I don't know if we can use latter due to SMP aspects.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31489 )
Change subject: [WIP] soc/amd/common: Use static allocation for params ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 425: : : : : : : :
The point of this entire commit is that the lifetime of the parameter struct is the duration of the […]
Do you have a platform to test? I'm not comfortable approving it until it's tested. If you don't have a platform, I can do it, but it'll have to wait an opening on my schedule (2 or 3 days).
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31489 )
Change subject: [WIP] soc/amd/common: Use static allocation for params ......................................................................
Patch Set 5: Code-Review-1
(1 comment)
y
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31489/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 425: : : : : : : :
Do you have a platform to test? I'm not comfortable approving it until it's tested. […]
I finally had a chance to test it, and as I suspected it would, it hanged at amd_init_early. That section of code is needed and I'll block any change that remove it.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31489 )
Change subject: [WIP] soc/amd/common: Use static allocation for params ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31489/5/src/soc/amd/common/block/pi... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/c/coreboot/+/31489/5/src/soc/amd/common/block/pi... PS5, Line 425: : : : : : : :
I finally had a chance to test it, and as I suspected it would, it hanged at amd_init_early. […]
Ok.. I had forgotten about the multiprocessor-init in romstage. APs would overwrite BSP structure, if we place it in bss. I may play around with this a little more, to see if I get relevant performance boost.
The location of AGESA heap for cold boots is a bit of a problem since coreboot is not really aware of that, currently. SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT feature would not work. See comments in CB:33956.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31489 )
Change subject: [WIP] soc/amd/common: Use static allocation for params ......................................................................
Abandoned
-
Kyösti Mälkki (Code Review) -
Richard Spiegel (Code Review)