Attention is currently required from: Jason Nien, Martin Roth, Rob Barnes.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/72455 )
Change subject: mb/google/skyrim: Add EC_HOST_EVENT_PANIC to SCI mask ......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/72455/comment/4ed7a9be_07b234ba PS1, Line 10: If system safe mode is also enabled : on the EC, the kernel will have a short period to extract and save info : about the EC panic. Thanks for the detailed explanations.
So with this change an SCI will interrupt AP/OS but it will be reset almost immediately. I don't expect this results in any new risks that aren't already present.
This is the only part that seems like it could be risky. As part of interrupt handling, the CPU HW should be masking interrupts to prevent being pre-empted by another interrupt. As long as the entire SOC state is reset including any interrupt masks (which I would expect to happen), this should be fine, but it's worth validating. I wouldn't be too surprised if we see some weird timing issue some day though, if we get enough EC panics to trigger it.
https://review.coreboot.org/c/coreboot/+/72455/comment/83334748_5bc782f1 PS1, Line 16: TEST=Observe kernel ec panic handler run when ec panics
Sure. I'll enumerate the scenarios to be tested, let me know if I missed any: […]
LGTM
-
Tim Van Patten (Code Review)