[edk2,1/7] ArmPkg/AsmMacroIoLibV8: remove undocumented assumption from ELx macros

Message ID 1458220815-6944-2-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel March 17, 2016, 1:20 p.m.
The macros EL1_OR_EL2() and EL1_OR_EL2_OR_EL3() allow conditional execution
of assembly sequences based on the current exception level, by jumping to
caller supplied labels 1f, 2f or 3f. However, the jump to 1f is actually
a fallthrough, which means the EL1 code needs to follow right after the
macro invocation, and the 1f label is ignored.

So let's fix this by making all jumps explicit.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPkg/Include/AsmMacroIoLibV8.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel March 22, 2016, 1:55 p.m. | #1
On 21 March 2016 at 17:53, Cohen, Eugene <eugene@hp.com> wrote:
> Alexei and Ard,

>

>> The suggested code for EL1_OR_EL2 macro:

>>

>> mrs    SAFE_XREG, CurrentEL ;\

>> cmp    SAFE_XREG, #0x8      ;\

>> b.eq   2f                   ;\

>> tbnz   SAFE_XREG, #2, 1f    ;\

>> b      .                    ;// We should never get here

>>

>> will successfully branch to 1f label in case of EL3 with SAFE_XREG = 0xC

>> because bit #2 will be set.

>

> Agreed.  If you assume that EL1_OR_EL2 can never be invoked at EL3 then obviously this wouldn't be a concern.  But since there's already a case for handling neither EL1 nor EL2 (b .) then it would make sense to correct this so if it was accidentally used in EL3 it would hit the default case which is better for debugability.  The last tbnz needs to be reverted to the previous cmp/b.ne construct or equivalent.

>


Thanks for spotting that. I committed it as follows:

// CurrentEL : 0xC = EL3; 8 = EL2; 4 = EL1
// This only selects between EL1 and EL2, else we die.
// Provide the Macro with a safe temp xreg to use.
#define EL1_OR_EL2(SAFE_XREG)        \
        mrs    SAFE_XREG, CurrentEL ;\
        cmp    SAFE_XREG, #0x8      ;\
        b.gt   .                    ;\
        b.eq   2f                   ;\
        cbnz   SAFE_XREG, 1f        ;\
        b      .                    ;// We should never get here


// CurrentEL : 0xC = EL3; 8 = EL2; 4 = EL1
// This only selects between EL1 and EL2 and EL3, else we die.
// Provide the Macro with a safe temp xreg to use.
#define EL1_OR_EL2_OR_EL3(SAFE_XREG) \
        mrs    SAFE_XREG, CurrentEL ;\
        cmp    SAFE_XREG, #0x8      ;\
        b.gt   3f                   ;\
        b.eq   2f                   ;\
        cbnz   SAFE_XREG, 1f        ;\
        b      .                    ;// We should never get here
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h b/ArmPkg/Include/AsmMacroIoLibV8.h
index a9f8491bc922..efc47d3bbbc7 100644
--- a/ArmPkg/Include/AsmMacroIoLibV8.h
+++ b/ArmPkg/Include/AsmMacroIoLibV8.h
@@ -25,9 +25,9 @@ 
         mrs    SAFE_XREG, CurrentEL ;\
         cmp    SAFE_XREG, #0x8      ;\
         b.eq   2f                   ;\
-        cmp    SAFE_XREG, #0x4      ;\
-        b.ne   .                    ;// We should never get here
-// EL1 code starts here
+        tbnz   SAFE_XREG, #2, 1f    ;\
+        b      .                    ;// We should never get here
+
 
 // CurrentEL : 0xC = EL3; 8 = EL2; 4 = EL1
 // This only selects between EL1 and EL2 and EL3, else we die.
@@ -36,11 +36,10 @@ 
         mrs    SAFE_XREG, CurrentEL ;\
         cmp    SAFE_XREG, #0xC      ;\
         b.eq   3f                   ;\
-        cmp    SAFE_XREG, #0x8      ;\
-        b.eq   2f                   ;\
-        cmp    SAFE_XREG, #0x4      ;\
-        b.ne   .                    ;// We should never get here
-// EL1 code starts here
+        tbnz   SAFE_XREG, #3, 2f    ;\
+        tbnz   SAFE_XREG, #2, 1f    ;\
+        b      .                    ;// We should never get here
+
 #if defined(__clang__)
 
 // load x0 with _Data