Attention is currently required from: David Milosevic, Julius Werner, Lean Sheng Tan.
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78284?usp=email )
Change subject: arch/arm64: Support calling a trusted monitor ......................................................................
Patch Set 5:
(6 comments)
File src/arch/arm64/include/armv8/arch/monitor_services.h:
PS3:
nit: I'd maybe just call these smc.h / smc. […]
As in, the smc instruction, not the SMCCC spec? That's fine, done.
https://review.coreboot.org/c/coreboot/+/78284/comment/779b5d46_f9e00098 : PS3, Line 3: #ifndef ARM_ARM64_SMCCC_H
Should match path and filename
Done
https://review.coreboot.org/c/coreboot/+/78284/comment/b5b21d6b_21f27da9 : PS3, Line 23: SMC_INVALID_PARAMETER = -3,
Where are these from? Are these official in some way or did you make them up? I'm only aware of SMC_ […]
These are from the calling convention spec, see https://developer.arm.com/documentation/den0028. As of revision F, return codes are section 7.1. I'll add this as a comment.
File src/arch/arm64/monitor_services.c:
https://review.coreboot.org/c/coreboot/+/78284/comment/f2c48416_ceadbbe9 : PS3, Line 11: struct smc_args
Returning a big struct by value is ugly (especially if you pass it up through multiple frames). […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/78284/comment/c6933e26_9618e20a : PS3, Line 18: if (current_el() == EL3) {
In the EL-patch (CB:74798) we now have this decided in Kconfig anyway so you can just write `assert( […]
Okay, makes sense to me. The boot path for a platform would be known at compile-time.
https://review.coreboot.org/c/coreboot/+/78284/comment/0900761f_97d876e3 : PS3, Line 30: register uint64_t r7 __asm__("x7") = arg6;
This is pretty ugly and I'm not sure it's entirely safe. […]
Acknowledged. I'm unfamiliar with Arm assembly, and besides, `assert` is a C macro, so I'll make the actual smc assembly a 'private' function (only smc.c has the prototype).
-
Benjamin Doron (Code Review)