Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31174
Change subject: usbdebug: Fix reserve in CAR ......................................................................
usbdebug: Fix reserve in CAR
We need sizeof(struct ehci_dbg_info) of 88 but only reserved 64 bytes. If usbdebug_hw_init() was called late in romstage, for some builds it would corrupt CAR_GLOBALs like console_inited variable and stop logging anything.
Also change pointer initialisation such that glob_dbg_info will hit garbage collection for PRE_RAM stages.
Change-Id: Ib49fca781e55619179aa8888e2d859560e050876 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/drivers/usb/ehci_debug.c 2 files changed, 9 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31174/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 7b10f43..3f33702 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -67,7 +67,7 @@ _car_drivers_storage_end = .; #endif _car_ehci_dbg_info_start = .; - . += 64; + . += 0x60; _car_ehci_dbg_info_end = .; /* _car_global_start and _car_global_end provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index 18d0491..04272e6 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -66,13 +66,14 @@
static inline struct ehci_debug_info *dbgp_ehci_info(void) { - if (IS_ENABLED(CONFIG_USBDEBUG_IN_PRE_RAM) - && (ENV_ROMSTAGE || ENV_BOOTBLOCK || ENV_VERSTAGE)) - glob_dbg_info_p = - (struct ehci_debug_info *)_car_ehci_dbg_info_start; - if (car_get_var(glob_dbg_info_p) == NULL) - car_set_var(glob_dbg_info_p, &glob_dbg_info); - + if (car_get_var(glob_dbg_info_p) == NULL) { + struct ehci_debug_info *info; + if (ENV_BOOTBLOCK || ENV_VERSTAGE || ENV_ROMSTAGE) + info = (void *)_car_ehci_dbg_info_start; + else + info = &glob_dbg_info; + car_set_var(glob_dbg_info_p, info); + } return car_get_var(glob_dbg_info_p); }
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld@70 PS1, Line 70: . += 0x60; add comment sizeof(struct ehci_debug_info) ?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld@70 PS1, Line 70: . += 0x60;
add comment sizeof(struct ehci_debug_info) ?
this is still larger than sizeof(struct ehci_debug_info). Would checking the size during runtime be a good idea?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld@70 PS1, Line 70: . += 0x60;
this is still larger than sizeof(struct ehci_debug_info). […]
yes
Hello Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31174
to look at the new patch set (#2).
Change subject: usbdebug: Fix reserve in CAR ......................................................................
usbdebug: Fix reserve in CAR
We need sizeof(struct ehci_dbg_info) of 88 but only reserved 64 bytes. If usbdebug_hw_init() was called late in romstage, for some builds it would corrupt CAR_GLOBALs like console_inited variable and stop logging anything.
Also change pointer initialisation such that glob_dbg_info will hit garbage collection for PRE_RAM stages.
Change-Id: Ib49fca781e55619179aa8888e2d859560e050876 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/drivers/usb/ehci_debug.c 3 files changed, 16 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31174/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31174/2/src/drivers/usb/ehci_debug.c File src/drivers/usb/ehci_debug.c:
https://review.coreboot.org/#/c/31174/2/src/drivers/usb/ehci_debug.c@74 PS2, Line 74: die("BUG: Increase ehci_dbg_info reserve in CAR"); line over 80 characters
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 2:
(1 comment)
Aaron, Julius; there is a bit of dilemma with car.ld.
1) We need consistent layout across PRE_RAM stages 2) We want RO bootblock, unaware of (future) RW romstage requirements 3) I don't like the semi-random size reserve, like done here for usbdebug
Any ideas how to from improve this? Looking at CONFIG_COMMONLIB_STORAGE and CONFIG_PAGING_IN_CACHE_AS_RAM, I am not sure if the fixed locations are always maintained properly. There is some strong assumption at least, that bootblock and romstage are built with same set of Kconfig options set.
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/#/c/31174/1/src/arch/x86/car.ld@70 PS1, Line 70: . += 0x60;
yes
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 2:
Aaron, Julius; there is a bit of dilemma with car.ld.
- We need consistent layout across PRE_RAM stages
- We want RO bootblock, unaware of (future) RW romstage requirements
- I don't like the semi-random size reserve, like done here for usbdebug
Any ideas how to from improve this? Looking at CONFIG_COMMONLIB_STORAGE and CONFIG_PAGING_IN_CACHE_AS_RAM, I am not sure if the fixed locations are always maintained properly. There is some strong assumption at least, that bootblock and romstage are built with same set of Kconfig options set.
I don't think you're gonna be able to build a system where old RO versions always work with a new RW no matter what you change. We have the same problem with memlayout on Arm devices and we generally don't pay attention to it when reviewing CLs for master.
For Chrome OS, our solution is that we cut a branch whenever we release an RO, and then all further RWs released for that are built from the same branch. Whenever you cherry-pick into the branch (we generally try to only do small fixes, not big refactorings) you need to manually pay attention to RO/RW interface issues like this. I think that's the only reasonable way to solve that and I'd suggest everyone else who wants to use vboot to do the same.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31174/2/src/drivers/usb/ehci_debug.c File src/drivers/usb/ehci_debug.c:
https://review.coreboot.org/#/c/31174/2/src/drivers/usb/ehci_debug.c@77 PS2, Line 77: info = &glob_dbg_info; Nit, we actually have a rule that all branches should be put in braces if one needs them.
Hello Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31174
to look at the new patch set (#3).
Change subject: usbdebug: Fix reserve in CAR ......................................................................
usbdebug: Fix reserve in CAR
We need sizeof(struct ehci_dbg_info) of 88 but only reserved 64 bytes. If usbdebug_hw_init() was called late in romstage, for some builds it would corrupt CAR_GLOBALs like console_inited variable and stop logging anything.
Also change pointer initialisation such that glob_dbg_info will hit garbage collection for PRE_RAM stages.
Change-Id: Ib49fca781e55619179aa8888e2d859560e050876 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/drivers/usb/ehci_debug.c 3 files changed, 17 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31174/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31174/3/src/drivers/usb/ehci_debug.c File src/drivers/usb/ehci_debug.c:
https://review.coreboot.org/#/c/31174/3/src/drivers/usb/ehci_debug.c@74 PS3, Line 74: die("BUG: Increase ehci_dbg_info reserve in CAR"); line over 80 characters
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 3:
Patch Set 2:
Aaron, Julius; there is a bit of dilemma with car.ld.
- We need consistent layout across PRE_RAM stages
- We want RO bootblock, unaware of (future) RW romstage requirements
- I don't like the semi-random size reserve, like done here for usbdebug
Any ideas how to from improve this? Looking at CONFIG_COMMONLIB_STORAGE and CONFIG_PAGING_IN_CACHE_AS_RAM, I am not sure if the fixed locations are always maintained properly. There is some strong assumption at least, that bootblock and romstage are built with same set of Kconfig options set.
I don't think you're gonna be able to build a system where old RO versions always work with a new RW no matter what you change. We have the same problem with memlayout on Arm devices and we generally don't pay attention to it when reviewing CLs for master.
I take this as a challenge, maybe you just shoot the idea down of extending use of CBMEM IDs to pre-ram? You only need an index at fixed CAR location?
We have about 100 bytes available in _car_relocatable before sandy/ivy MRC blob starts to interfere. It would not be the first case when static allocations are just fine for a long time, but eventually it starts to be inconvenient or fall apart. (Like !DYNAMIC_CBMEM).
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
usbdebug: Fix reserve in CAR
We need sizeof(struct ehci_dbg_info) of 88 but only reserved 64 bytes. If usbdebug_hw_init() was called late in romstage, for some builds it would corrupt CAR_GLOBALs like console_inited variable and stop logging anything.
Also change pointer initialisation such that glob_dbg_info will hit garbage collection for PRE_RAM stages.
Change-Id: Ib49fca781e55619179aa8888e2d859560e050876 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/31174 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Nico Huber nico.h@gmx.de --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/drivers/usb/ehci_debug.c 3 files changed, 17 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 7b10f43..1b0d996 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -67,7 +67,8 @@ _car_drivers_storage_end = .; #endif _car_ehci_dbg_info_start = .; - . += 64; + /* Reserve sizeof(struct ehci_dbg_info). */ + . += 88; _car_ehci_dbg_info_end = .; /* _car_global_start and _car_global_end provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram diff --git a/src/arch/x86/include/arch/symbols.h b/src/arch/x86/include/arch/symbols.h index 9ef6a3b..97a07c0 100644 --- a/src/arch/x86/include/arch/symbols.h +++ b/src/arch/x86/include/arch/symbols.h @@ -35,6 +35,9 @@ #define _car_stack_size (_car_stack_end - _car_stack_start)
extern char _car_ehci_dbg_info_start[]; +extern char _car_ehci_dbg_info_end[]; +#define _car_ehci_dbg_info_size \ + (_car_ehci_dbg_info_end - _car_ehci_dbg_info_start)
/* * The _car_relocatable_data_[start|end] symbols cover CAR data which is diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index 18d0491..c5fb984 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -66,13 +66,18 @@
static inline struct ehci_debug_info *dbgp_ehci_info(void) { - if (IS_ENABLED(CONFIG_USBDEBUG_IN_PRE_RAM) - && (ENV_ROMSTAGE || ENV_BOOTBLOCK || ENV_VERSTAGE)) - glob_dbg_info_p = - (struct ehci_debug_info *)_car_ehci_dbg_info_start; - if (car_get_var(glob_dbg_info_p) == NULL) - car_set_var(glob_dbg_info_p, &glob_dbg_info); - + if (car_get_var(glob_dbg_info_p) == NULL) { + struct ehci_debug_info *info; + if (ENV_BOOTBLOCK || ENV_VERSTAGE || ENV_ROMSTAGE) { + /* The message likely does not show if we hit this. */ + if (sizeof(*info) > _car_ehci_dbg_info_size) + die("BUG: Increase ehci_dbg_info reserve in CAR"); + info = (void *)_car_ehci_dbg_info_start; + } else { + info = &glob_dbg_info; + } + car_set_var(glob_dbg_info_p, info); + } return car_get_var(glob_dbg_info_p); }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31174 )
Change subject: usbdebug: Fix reserve in CAR ......................................................................
Patch Set 4:
I take this as a challenge, maybe you just shoot the idea down of extending use of CBMEM IDs to pre-ram? You only need an index at fixed CAR location?
We have about 100 bytes available in _car_relocatable before sandy/ivy MRC blob starts to interfere. It would not be the first case when static allocations are just fine for a long time, but eventually it starts to be inconvenient or fall apart. (Like !DYNAMIC_CBMEM).
I wouldn't really say that anything is "falling apart" right now. Do you have a use case where this is really a problem? For Chrome OS' use of vboot it never really has been (we'd do that branching anyway for overall product stability, not just for this), and I'd be surprised if other people who pick it up wouldn't come to the same conclusion. Trying to make two very diverged versions of coreboot come together as RO and RW would probably be a bad idea for more reasons than this (and I don't think we'd want to introduce a general mandate that all changes will have to be backwards compatible to old verstages in the future, or at least make a big deal out of it every time).
I'm generally wary of introducing complexity without good cause (especially in early stages), which this sounds like it might do. One of the strengths of memlayout is that it is dead simple to reason about, dynamic allocation frameworks maybe not so much. You don't want firmware that just occasionally runs out of memory in the bootblock because the allocator acts slightly different in certain edge cases. (Also, remember that if you'd want to unify this across all architectures, there are some Arm boards so close to bursting that we already have to count pretty much every byte in SRAM.)
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)