Change in coreboot[master]: lib/romstage_handoff: Add methods to pass on romstage information
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_handoff: Add methods to pass on romstage information ......................................................................
lib/romstage_handoff: Add methods to pass on romstage information
Add 2 generic function that can be used to pass on information from romstage to ramstage. The implementation is such that the saving can be done even if cbmem is not set up initially.
TODO: Does it need guarding because it needlessly increases .bss usage?
Change-Id: I71bbe1b776cbc74468a6218b315894602b6152ba Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/romstage_handoff.h M src/lib/romstage_handoff.c 3 files changed, 79 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/36291/1
diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index 30bf88a..89d9478 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -77,6 +77,7 @@ #define CBMEM_ID_ROM2 0x524f4d32 #define CBMEM_ID_ROM3 0x524f4d33 #define CBMEM_ID_FMAP 0x464d4150 +#define CBMEM_ID_ROMSTAGE_SAVE 0xa7d1f2f3
#define CBMEM_ID_TO_NAME_TABLE \ { CBMEM_ID_ACPI, "ACPI " }, \ diff --git a/src/include/romstage_handoff.h b/src/include/romstage_handoff.h index db998ec..bbb0869 100644 --- a/src/include/romstage_handoff.h +++ b/src/include/romstage_handoff.h @@ -21,4 +21,11 @@ /* Return 1 if resuming or 0 if not. */ int romstage_handoff_is_resume(void);
+/* Returns 0 on success, 1 if the info was copied to a buffer to be migrated + to cbmem later on, < 0 on failure. */ +int save_romstage_info(void *info, size_t info_size); + +/* Return 0 on success, else < 0 on failure */ +int get_romstage_info(void *info, size_t info_size); + #endif /* ROMSTAGE_HANDOFF_H */ diff --git a/src/lib/romstage_handoff.c b/src/lib/romstage_handoff.c index 04ead0a..2f26194 100644 --- a/src/lib/romstage_handoff.c +++ b/src/lib/romstage_handoff.c @@ -77,3 +77,74 @@
return handoff->s3_resume; } + +static u8 save_buffer[256]; +static size_t used_buffer_size; +static int is_migrated; + +static void migrate_save_buffer(int is_recovery) +{ + if (used_buffer_size) + printk(BIOS_DEBUG, "Migrating the romstage save buffer to cbmem\n"); + is_migrated = 1; + save_romstage_info(save_buffer, used_buffer_size); +} + +ROMSTAGE_CBMEM_INIT_HOOK(migrate_save_buffer); + +int save_romstage_info(void *info, size_t save_size) +{ + const struct cbmem_entry *e; + + printk(BIOS_SPEW, "Romstage save: saving %p, size %zu\n", + info, save_size); + + e = cbmem_entry_find(CBMEM_ID_ROMSTAGE_SAVE); + if (e != NULL) { + if (cbmem_entry_size(e) > save_size) { + memcpy(cbmem_entry_start(e), info, save_size); + return 0; + } + cbmem_entry_remove(e); + } + + e = cbmem_entry_add(CBMEM_ID_ROMSTAGE_SAVE, save_size); + if (e == NULL && is_migrated) { + printk(BIOS_ERR, "Can't copy romstage save buffer to cbmem!\n"); + return -1; + } + + if (e != NULL) { + memcpy(cbmem_entry_start(e), info, save_size); + return 0; + } + + /* Cbmem is not yet up, save it for later */ + printk(BIOS_SPEW, "Romstage save: cbmem not yet up, saving info buffer for later\n"); + if (sizeof(save_buffer) < save_size) { + printk(BIOS_ERR, "Romstage save: buffer too small!\n"); + return -1; + } + memcpy(save_buffer, info, save_size); + used_buffer_size = save_size; + return 1; +} + +int get_romstage_info(void *info, size_t save_size) +{ + const struct cbmem_entry *e; + e = cbmem_entry_find(CBMEM_ID_ROMSTAGE_SAVE); + + if (e == NULL) { + printk(BIOS_ERR, "Can't find romstage save\n"); + return -1; + } + + if (save_size > cbmem_entry_size(e)) { + printk(BIOS_ERR, "Romstage save: requested size too large!\n"); + return -1; + } + + memcpy(info, cbmem_entry_start(e), save_size); + return 0; +}
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36291
to look at the new patch set (#2).
Change subject: lib/romstage_handoff: Add methods to pass on romstage information ......................................................................
lib/romstage_handoff: Add methods to pass on romstage information
Add 2 generic function that can be used to pass on information from romstage to ramstage. The implementation is such that the saving can be done even if cbmem is not set up initially.
TODO: Does it need guarding because it needlessly increases .bss usage?
Change-Id: I71bbe1b776cbc74468a6218b315894602b6152ba Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/romstage_handoff.h M src/lib/romstage_handoff.c 3 files changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/36291/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_handoff: Add methods to pass on romstage information ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
Needs more documentation/comments on what it can do.
https://review.coreboot.org/c/coreboot/+/36291/2/src/lib/romstage_handoff.c File src/lib/romstage_handoff.c:
https://review.coreboot.org/c/coreboot/+/36291/2/src/lib/romstage_handoff.c@... PS2, Line 81: 256 Make this a Kconfig variable.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36291
to look at the new patch set (#6).
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
lib/romstage_save: Add methods to pass on romstage information
Add 2 generic function that can be used to pass on information from romstage to ramstage. The implementation is such that the saving can be done even if cbmem is not set up initially.
Change-Id: I71bbe1b776cbc74468a6218b315894602b6152ba Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/commonlib/include/commonlib/cbmem_id.h A src/include/romstage_save.h M src/lib/Kconfig M src/lib/Makefile.inc A src/lib/romstage_save.c 5 files changed, 134 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/36291/6
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36291
to look at the new patch set (#8).
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
lib/romstage_save: Add methods to pass on romstage information
Add 2 generic function that can be used to pass on information from romstage to ramstage. The implementation is such that the saving can be done even if cbmem is not set up initially.
Change-Id: I71bbe1b776cbc74468a6218b315894602b6152ba Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/commonlib/include/commonlib/cbmem_id.h A src/include/romstage_save.h M src/lib/Kconfig M src/lib/Makefile.inc A src/lib/romstage_save.c 5 files changed, 134 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/36291/8
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Do you have other cases than AMD UMA that needs to stash romstage information into BSS prior to initial CBMEM setup? I think this is a bit overkill for that purpose.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Patch Set 13:
Do you have other cases than AMD UMA that needs to stash romstage information into BSS prior to initial CBMEM setup? I think this is a bit overkill for that purpose.
Initially there was a use case for skylake and newer but that went away.
Thinking of it: migrating a structure to cbmem, updating pointers during romstage to migrated data in cbmem and initializing the datastructures in ramstage is quite common and a little error-prone. Would it make sense to abstract that away with something like: persistent_storage_init(void *pointer_to_data, sizeofdata, CBMEM_ID)?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
Do you have other cases than AMD UMA that needs to stash romstage information into BSS prior to initial CBMEM setup? I think this is a bit overkill for that purpose.
Initially there was a use case for skylake and newer but that went away.
Thinking of it: migrating a structure to cbmem, updating pointers during romstage to migrated data in cbmem and initializing the datastructures in ramstage is quite common and a little error-prone. Would it make sense to abstract that away with something like: persistent_storage_init(void *pointer_to_data, sizeofdata, CBMEM_ID)?
Sounds good to me. I might want to take that even further, noting that some (timestamps, console, usbdebug) start their lives already in bootblock and have to be explicitly defined in car.ld currently.
It would require some linker script wrapping to achieve it, but I think it would be nice to be able to tag such structures with, let's call it CAR_PERSIST, and they would magically get migrated all the way to CBMEM. I think Julius insisted such symbols must resolve at build-time, so we would need to find a way to lock the symbol locations inside CAR, across the stages, while other sections (.bss) can grow and shrink.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
Do you have other cases than AMD UMA that needs to stash romstage information into BSS prior to initial CBMEM setup? I think this is a bit overkill for that purpose.
Initially there was a use case for skylake and newer but that went away.
Thinking of it: migrating a structure to cbmem, updating pointers during romstage to migrated data in cbmem and initializing the datastructures in ramstage is quite common and a little error-prone. Would it make sense to abstract that away with something like: persistent_storage_init(void *pointer_to_data, sizeofdata, CBMEM_ID)?
Sounds good to me. I might want to take that even further, noting that some (timestamps, console, usbdebug) start their lives already in bootblock and have to be explicitly defined in car.ld currently.
It would require some linker script wrapping to achieve it, but I think it would be nice to be able to tag such structures with, let's call it CAR_PERSIST, and they would magically get migrated all the way to CBMEM. I think Julius insisted such symbols must resolve at build-time, so we would need to find a way to lock the symbol locations inside CAR, across the stages, while other sections (.bss) can grow and shrink.
That sounds nice, but how to make sure that the symbols in (I assume you have a new region for it in which CAR_PERSISTS goes?) the new region are the same across pre-ram stages? That would somehow require to use some of the bootblock symbols in verstage and romstage. Sounds a bit hacky to do 'nm' and parse output into something to be included by memlayout.ld of romstage & ramstage?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Patch Set 13:
It would require some linker script wrapping to achieve it, but I think it would be nice to be able to tag such structures with, let's call it CAR_PERSIST, and they would magically get migrated all the way to CBMEM. I think Julius insisted such symbols must resolve at build-time, so we would need to find a way to lock the symbol locations inside CAR, across the stages, while other sections (.bss) can grow and shrink.
That sounds nice, but how to make sure that the symbols in (I assume you have a new region for it in which CAR_PERSISTS goes?) the new region are the same across pre-ram stages? That would somehow require to use some of the bootblock symbols in verstage and romstage. Sounds a bit hacky to do 'nm' and parse output into something to be included by memlayout.ld of romstage & ramstage?
That's the question to solve. Ramstage and postcar would not be involved. Maybe one could use objcopy to mirror a complete section from romstage.elf into bootblock.elf, and linker scripts would not need to change at all. Just throwing out ideas, I have no clue :)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Do you really think all that infrastructure you're envisioning there (if it's really possible to build that at all, which I'm not sure about) is really worth it? Seems a bit overkill for a not *that* common problem to me. For many of copy-into-CBMEM use cases we have, you gotta copy it into CBMEM but you also have to do some extra stuff (e.g. timestamps and console need to merge in the early BSS buffer in ramstage, vboot workbuffer needs to be updated with the new size, etc.) so even if you built some common solution for that, I think most cases would still need a custom callback for that. I feel what we have right now is probably good enough?
Same goes for what this patch does, isn't that a bit overkill for what really just needs to be a global variable and a CBMEM_INIT_HOOK?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Same goes for what this patch does, isn't that a bit overkill for what really just needs to be a global variable and a CBMEM_INIT_HOOK?
It only uses a CBMEM_INIT_HOOK if the save function is called before cbmem_init. So it potentially covers more use cases, but it indeed looks like it is overkill atm.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
(1 comment)
As written, this is a single caller API and there's nothing preventing misuse. I'm not understanding the need.
https://review.coreboot.org/c/coreboot/+/36291/13/src/lib/romstage_save.c File src/lib/romstage_save.c:
https://review.coreboot.org/c/coreboot/+/36291/13/src/lib/romstage_save.c@47 PS13, Line 47: cbmem_entry_remove(e); This only works if CBMEM_ID_ROMSTAGE_SAVE is the last entry added. And it also doesn't work if the cbmem is locked (recovered from on resume or through other stages).
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: lib/romstage_save: Add methods to pass on romstage information ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
It would require some linker script wrapping to achieve it, but I think it would be nice to be able to tag such structures with, let's call it CAR_PERSIST, and they would magically get migrated all the way to CBMEM. I think Julius insisted such symbols must resolve at build-time, so we would need to find a way to lock the symbol locations inside CAR, across the stages, while other sections (.bss) can grow and shrink.
That sounds nice, but how to make sure that the symbols in (I assume you have a new region for it in which CAR_PERSISTS goes?) the new region are the same across pre-ram stages? That would somehow require to use some of the bootblock symbols in verstage and romstage. Sounds a bit hacky to do 'nm' and parse output into something to be included by memlayout.ld of romstage & ramstage?
That's the question to solve. Ramstage and postcar would not be involved. Maybe one could use objcopy to mirror a complete section from romstage.elf into bootblock.elf, and linker scripts would not need to change at all. Just throwing out ideas, I have no clue :)
We need a proper data section before we can migrate things automatically. Currently, we support .bss (0 values) but not initialized objects that are writable. Once you have that it is certainly possible to relocate the objects. But even then it can be complicated in that you need everyone to utilize pointers to objects where the pointers live in the correct location. Otherwise you'll have address references in your instruction stream. That will break for xip programs since you can't modify the instructions as they are read only.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36291
to look at the new patch set (#21).
Change subject: [WIP]lib/romstage_save: Add methods to pass on romstage information ......................................................................
[WIP]lib/romstage_save: Add methods to pass on romstage information
Add 2 generic function that can be used to pass on information from romstage to ramstage. The implementation is such that the saving can be done even if cbmem is not set up initially.
TODO: Too complicated?
Change-Id: I71bbe1b776cbc74468a6218b315894602b6152ba Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/commonlib/include/commonlib/cbmem_id.h A src/include/romstage_save.h M src/lib/Kconfig M src/lib/Makefile.inc A src/lib/romstage_save.c 5 files changed, 134 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/36291/21
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36291
to look at the new patch set (#32).
Change subject: [WIP]lib/romstage_save: Add methods to pass on romstage information ......................................................................
[WIP]lib/romstage_save: Add methods to pass on romstage information
Add 2 generic function that can be used to pass on information from romstage to ramstage. The implementation is such that the saving can be done even if cbmem is not set up initially.
TODO: Too complicated?
Change-Id: I71bbe1b776cbc74468a6218b315894602b6152ba Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/commonlib/include/commonlib/cbmem_id.h A src/include/romstage_save.h M src/lib/Kconfig M src/lib/Makefile.inc A src/lib/romstage_save.c 5 files changed, 134 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/36291/32
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36291 )
Change subject: [WIP]lib/romstage_save: Add methods to pass on romstage information ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)