Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37032 )
Change subject: lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support ......................................................................
lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support
Change-Id: Id409f9abf33c851b6d08903bc111a6b8ec6bf8cf Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/lib/imd_cbmem.c 1 file changed, 2 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/37032/1
diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 6eb3e60..d58ba80 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -24,27 +24,6 @@ #include <stdlib.h> #include <arch/early_variables.h>
-/* - * We need special handling on x86 where CAR global migration is employed. One - * cannot use true globals in that circumstance because CAR is where the globals - * are backed -- creating a circular dependency. For non CAR platforms globals - * are free to be used as well as any stages that are purely executing out of - * RAM. For CAR platforms that don't migrate globals the as-linked globals can - * be used, but they need special decoration using CAR_GLOBAL. That ensures - * proper object placement in conjunction with the linker. - * - * For the CAR global migration platforms we have to always try to partially - * recover CBMEM from cbmem_top() whenever we try to access it. In other - * environments we're not so constrained and just keep the backing imd struct - * in a global. This also means that we can easily tell whether CBMEM has - * explicitly been initialized or recovered yet on those platforms, and don't - * need to put the burden on board or chipset code to tell us by returning - * NULL from cbmem_top() before that point. - */ -#define CAN_USE_GLOBALS \ - (!CONFIG(ARCH_X86) || ENV_RAMSTAGE || ENV_POSTCAR || \ - !CONFIG(CAR_GLOBAL_MIGRATION)) - /* The program loader passes on cbmem_top and the program entry point has to fill in the _cbmem_top_ptr symbol based on the calling arguments. */ uintptr_t _cbmem_top_ptr; @@ -67,11 +46,8 @@
static inline struct imd *cbmem_get_imd(void) { - if (CAN_USE_GLOBALS) { - static struct imd imd_cbmem CAR_GLOBAL; - return &imd_cbmem; - } - return NULL; + static struct imd imd_cbmem; + return &imd_cbmem; }
static inline const struct cbmem_entry *imd_to_cbmem(const struct imd_entry *e) @@ -115,12 +91,6 @@ struct imd *imd;
imd = imd_init_backing(backing); - if (!CAN_USE_GLOBALS) { - /* Always partially recover if we can't keep track of whether - * we have already initialized CBMEM in this stage. */ - imd_handle_init(imd, cbmem_top()); - imd_handle_init_partial_recovery(imd); - }
return imd; }
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37032
to look at the new patch set (#2).
Change subject: lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support ......................................................................
lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support
Change-Id: Id409f9abf33c851b6d08903bc111a6b8ec6bf8cf Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/lib/imd_cbmem.c 1 file changed, 2 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/37032/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37032 )
Change subject: lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37032/8/src/lib/imd_cbmem.c File src/lib/imd_cbmem.c:
https://review.coreboot.org/c/coreboot/+/37032/8/src/lib/imd_cbmem.c@81 PS8, Line 81: return imd; Followup, cbmem_get_imd() never returns NULL now?
https://review.coreboot.org/c/coreboot/+/37032/8/src/lib/imd_cbmem.c@291 PS8, Line 291: imd_region_used(cbmem_get_imd(), baseptr, size); I guess this was restricted to be used only with CAR_GLOBAL and non-NULL parameter?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37032 )
Change subject: lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37032 )
Change subject: lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support ......................................................................
Patch Set 8:
(2 comments)
will push a follow-up CL in a few minutes to deal with the comments.
https://review.coreboot.org/c/coreboot/+/37032/8/src/lib/imd_cbmem.c File src/lib/imd_cbmem.c:
https://review.coreboot.org/c/coreboot/+/37032/8/src/lib/imd_cbmem.c@81 PS8, Line 81: return imd;
Followup, cbmem_get_imd() never returns NULL now?
Done
https://review.coreboot.org/c/coreboot/+/37032/8/src/lib/imd_cbmem.c@291 PS8, Line 291: imd_region_used(cbmem_get_imd(), baseptr, size);
I guess this was restricted to be used only with CAR_GLOBAL and non-NULL parameter?
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37032 )
Change subject: lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support ......................................................................
lib/imd_cbmem.c: Drop CAR_GLOBAL_MIGRATION support
Change-Id: Id409f9abf33c851b6d08903bc111a6b8ec6bf8cf Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/37032 Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/imd_cbmem.c 1 file changed, 2 insertions(+), 33 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 6eb3e60..6fd48d5 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -22,28 +22,6 @@ #include <imd.h> #include <lib.h> #include <stdlib.h> -#include <arch/early_variables.h> - -/* - * We need special handling on x86 where CAR global migration is employed. One - * cannot use true globals in that circumstance because CAR is where the globals - * are backed -- creating a circular dependency. For non CAR platforms globals - * are free to be used as well as any stages that are purely executing out of - * RAM. For CAR platforms that don't migrate globals the as-linked globals can - * be used, but they need special decoration using CAR_GLOBAL. That ensures - * proper object placement in conjunction with the linker. - * - * For the CAR global migration platforms we have to always try to partially - * recover CBMEM from cbmem_top() whenever we try to access it. In other - * environments we're not so constrained and just keep the backing imd struct - * in a global. This also means that we can easily tell whether CBMEM has - * explicitly been initialized or recovered yet on those platforms, and don't - * need to put the burden on board or chipset code to tell us by returning - * NULL from cbmem_top() before that point. - */ -#define CAN_USE_GLOBALS \ - (!CONFIG(ARCH_X86) || ENV_RAMSTAGE || ENV_POSTCAR || \ - !CONFIG(CAR_GLOBAL_MIGRATION))
/* The program loader passes on cbmem_top and the program entry point has to fill in the _cbmem_top_ptr symbol based on the calling arguments. */ @@ -67,11 +45,8 @@
static inline struct imd *cbmem_get_imd(void) { - if (CAN_USE_GLOBALS) { - static struct imd imd_cbmem CAR_GLOBAL; - return &imd_cbmem; - } - return NULL; + static struct imd imd_cbmem; + return &imd_cbmem; }
static inline const struct cbmem_entry *imd_to_cbmem(const struct imd_entry *e) @@ -115,12 +90,6 @@ struct imd *imd;
imd = imd_init_backing(backing); - if (!CAN_USE_GLOBALS) { - /* Always partially recover if we can't keep track of whether - * we have already initialized CBMEM in this stage. */ - imd_handle_init(imd, cbmem_top()); - imd_handle_init_partial_recovery(imd); - }
return imd; }
-
Arthur Heymans (Code Review) -
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)