Attention is currently required from: Arthur Heymans, Christian Walter, David Milosevic, Lean Sheng Tan, Martin L Roth, Maximilian Brune, Nico Huber.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74798?usp=email )
Change subject: arch/arm64: Add EL1/EL2/EL3 support for arm64 ......................................................................
Patch Set 6:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74798/comment/bf6f793e_d2523a03 : PS5, Line 11: one boots into TF-A first and drops into EL2 for coreboot afterwards.
Just to make sure we're on the same page: We actually think it's wrong. We do it […]
I mean... I'll be honest, I think this is the wrong way, I'm just giving in here because I got the impression that everyone wants this and there is no other way and I don't want to be the guy standing in the way of supporting more platforms. If there are more objections from a wider group of people then maybe we really need to have a more fundamental discussion of whether we want to allow using coreboot in this way (ramstage-only on top of other platform firmware) or not.
Should we maybe open a thread on the mailing list for this, or put it on the agenda for the next leadership meeting?
File src/arch/arm64/Kconfig:
https://review.coreboot.org/c/coreboot/+/74798/comment/560a9fd7_64ae130a : PS6, Line 30: 1 (EL1), 2 (EL2) and 3 (EL3) These are standard Arm architectural terms (like "CPL0" on x86), I don't think we need to add more explanation here since the people that need this should know how that works anyway. I agree it would be good to add a sort of
By default, coreboot is the first firmware that runs on the system and should thus always run at EL3. This option is only provided for edge-case platforms that require running a different firmware before coreboot which drops to a lower exception level.
though.
https://review.coreboot.org/c/coreboot/+/74798/comment/33bdd812_0052994c : PS6, Line 41: depends on ARCH_RAMSTAGE_ARM64 May want to add a `depends on ARM64_CURRENT_EL == 3` here, since this wouldn't work for other levels.
File src/arch/arm64/armv8/cpu.S:
https://review.coreboot.org/c/coreboot/+/74798/comment/b3ec4c8a_0e1f4e14 : PS6, Line 87: #endif Doing this everywhere looks pretty ugly. Can we add a macro like ``` #if CONFIG_ARM64_CURRENT_EL == 1 #define CURRENT_EL(reg) reg##_el1 #elif CONFIG_ARM64_CURRENT_EL == 2 #define CURRENT_EL(reg) reg##_el2 #elif CONFIG_ARM64_CURRENT_EL == 3 #define CURRENT_EL(reg) reg##_el3 #else #error "Invalid setting for CONFIG_ARM64_CURRENT_EL!" #endif ``` to <arch/asm.h> and then use that everywhere instead?
-
Julius Werner (Code Review)