[PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump

[PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump

[Leif Lindholm]

Jan's submission earlier today unfortunately made me go have a look at
the ARM/AARCH64 implementations, and there were plenty of other things to
improve:
- Fix comments (drop Itanium mention, correct spelling)
- Make code match existing comments
- Don't try to optimise ARM for executing on the 8088
- Use the correct register sizes on AArch64
- Actually follow the API on ARM

The changes to .asm files have been neither build nor runtime tested.

Cc: Jan Bobek <jbobek@nvidia.com>

Leif Lindholm (5):
  MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations
  MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
  MdePkg/BaseLib: use normal register init in ARM SetJump
    implementations
  MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump
  MdePkg/BaseLib: ensure ARM LongJump never returns 0

 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 14 ++++++++------
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 14 ++++++++------
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       | 10 +++++++---
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     | 10 +++++++---
 4 files changed, 30 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations

[Leif Lindholm]

Drop redundant comment about IPF (clearly copied across from now deleted
code).

Also change
"Instead is resumes execution" ->
"Instead it resumes execution"

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 3 +--
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 3 +--
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       | 3 +--
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     | 3 +--
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
index 72cea259e913..989736cee74c 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -33,7 +33,6 @@ GCC_ASM_EXPORT(InternalLongJump)
 #  value to be returned by SetJump().
 #
 #  If JumpBuffer is NULL, then ASSERT().
-#  For IPF CPUs, if JumpBuffer is not aligned on a 16-byte boundary, then ASSERT().
 #
 #  @param  JumpBuffer    A pointer to CPU context buffer.
 #
@@ -61,7 +60,7 @@ ASM_PFX(SetJump):
 #
 #  Restores the CPU context from the buffer specified by JumpBuffer.
 #  This function never returns to the caller.
-#  Instead is resumes execution based on the state of JumpBuffer.
+#  Instead it resumes execution based on the state of JumpBuffer.
 #
 #  @param  JumpBuffer    A pointer to CPU context buffer.
 #  @param  Value         The value to return when the SetJump() context is restored.
diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
index 20dd0f1b850f..8922128e8c62 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -32,7 +32,6 @@
 ;  value to be returned by SetJump().
 ;
 ;  If JumpBuffer is NULL, then ASSERT().
-;  For IPF CPUs, if JumpBuffer is not aligned on a 16-byte boundary, then ASSERT().
 ;
 ;  @param  JumpBuffer    A pointer to CPU context buffer.
 ;
@@ -60,7 +59,7 @@ SetJump
 ;
 ;  Restores the CPU context from the buffer specified by JumpBuffer.
 ;  This function never returns to the caller.
-;  Instead is resumes execution based on the state of JumpBuffer.
+;  Instead it resumes execution based on the state of JumpBuffer.
 ;
 ;  @param  JumpBuffer    A pointer to CPU context buffer.
 ;  @param  Value         The value to return when the SetJump() context is restored.
diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
index 82d94faf61e9..e4c1946a28ff 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
@@ -19,7 +19,6 @@ GCC_ASM_EXPORT(InternalLongJump)
 #  value to be returned by SetJump().
 #
 #  If JumpBuffer is NULL, then ASSERT().
-#  For IPF CPUs, if JumpBuffer is not aligned on a 16-byte boundary, then ASSERT().
 #
 #  @param  JumpBuffer    A pointer to CPU context buffer.
 #
@@ -42,7 +41,7 @@ ASM_PFX(SetJump):
 #
 #  Restores the CPU context from the buffer specified by JumpBuffer.
 #  This function never returns to the caller.
-#  Instead is resumes execution based on the state of JumpBuffer.
+#  Instead it resumes execution based on the state of JumpBuffer.
 #
 #  @param  JumpBuffer    A pointer to CPU context buffer.
 #  @param  Value         The value to return when the SetJump() context is restored.
diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
index 936f722be60f..e1eff758f7ab 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
@@ -19,7 +19,6 @@
 ;  value to be returned by SetJump().
 ;
 ;  If JumpBuffer is NULL, then ASSERT().
-;  For IPF CPUs, if JumpBuffer is not aligned on a 16-byte boundary, then ASSERT().
 ;
 ;  @param  JumpBuffer    A pointer to CPU context buffer.
 ;
@@ -42,7 +41,7 @@ SetJump
 ;
 ;  Restores the CPU context from the buffer specified by JumpBuffer.
 ;  This function never returns to the caller.
-;  Instead is resumes execution based on the state of JumpBuffer.
+;  Instead it resumes execution based on the state of JumpBuffer.
 ;
 ;  @param  JumpBuffer    A pointer to CPU context buffer.
 ;  @param  Value         The value to return when the SetJump() context is restored.
-- 
2.20.1

[PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations

[Leif Lindholm]

The SetJump comment header states that:
  If JumpBuffer is NULL, then ASSERT().

However, this was not currently done.
Add a call to InternalAssertJumpBuffer.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 3 +++
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 3 +++
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       | 3 +++
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     | 3 +++
 4 files changed, 12 insertions(+)

diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
index 989736cee74c..34765a676430 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -45,6 +45,9 @@ GCC_ASM_EXPORT(InternalLongJump)
 #  );
 #
 ASM_PFX(SetJump):
+        stp     x30, x0, [sp, #-16]!
+        bl      InternalAssertJumpBuffer
+        ldp     x30, x0, [sp], #16
         mov     x16, sp // use IP0 so save SP
 #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
 #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
index 8922128e8c62..f2729a8bb03e 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -44,6 +44,9 @@
 ;  );
 ;
 SetJump
+        stp     x30, x0, [sp, #-16]!
+        bl      InternalAssertJumpBuffer
+        ldp     x30, x0, [sp], #16
         mov     x16, sp // use IP0 so save SP
 #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
 #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
index e4c1946a28ff..54b11ad2197c 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
@@ -31,6 +31,9 @@ GCC_ASM_EXPORT(InternalLongJump)
 #  );
 #
 ASM_PFX(SetJump):
+  push  {r0, lr}
+  bl    InternalAssertJumpBuffer
+  pop   {r0, lr}
   mov   r3, r13
   stmia r0, {r3-r12,r14}
   eor   r0, r0, r0
diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
index e1eff758f7ab..6d47033975f2 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
@@ -31,6 +31,9 @@
 ;  )
 ;
 SetJump
+  PUSH {R0, LR}
+  BL   InternalAssertJumpBuffer
+  POP  {R0, LR}
   MOV  R3, R13
   STM  R0, {R3-R12,R14}
   EOR  R0, R0
-- 
2.20.1

[PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations

[Leif Lindholm]

There may be architectures on which there are benefits to
  eor r0, r0(, r0)
but ARM was never one of them. Change to more readable
  mov r0, #0
instead.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S   | 2 +-
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
index 54b11ad2197c..407df5f41ac5 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
@@ -36,7 +36,7 @@ ASM_PFX(SetJump):
   pop   {r0, lr}
   mov   r3, r13
   stmia r0, {r3-r12,r14}
-  eor   r0, r0, r0
+  mov   r0, #0
   bx    lr
 
 #/**
diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
index 6d47033975f2..3a45f045460a 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
@@ -36,7 +36,7 @@ SetJump
   POP  {R0, LR}
   MOV  R3, R13
   STM  R0, {R3-R12,R14}
-  EOR  R0, R0
+  MOV  RO, #0
   BX   LR
 
 ;/**
-- 
2.20.1

[PATCH 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump

[Leif Lindholm]

Both in SetJump and in InternalLongJump, 32-bit w register views were
used for the UINTN return value. In SetJump, this did not cause errors;
it was only counterintuitive. But in InternalLongJump, it meant the top
32 bits of Value were stripped off.

Change all of these to use the 64-bit x register views.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++----
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
index 34765a676430..b3d37b216542 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -55,7 +55,7 @@ ASM_PFX(SetJump):
         FPR_LAYOUT
 #undef REG_PAIR
 #undef REG_ONE
-        mov     w0, #0
+        mov     x0, #0
         ret
 
 #/**
@@ -84,9 +84,9 @@ ASM_PFX(InternalLongJump):
 #undef REG_PAIR
 #undef REG_ONE
         mov     sp, x16
-        cmp     w1, #0
-        mov     w0, #1
-        csel    w0, w1, w0, ne
+        cmp     x1, #0
+        mov     x0, #1
+        csel    x0, x1, x0, ne
         // use br not ret, as ret is guaranteed to mispredict
         br      x30
 
diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
index f2729a8bb03e..ba4d2389c0cb 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -54,7 +54,7 @@ SetJump
         FPR_LAYOUT
 #undef REG_PAIR
 #undef REG_ONE
-        mov     w0, #0
+        mov     x0, #0
         ret
 
 ;/**
@@ -83,10 +83,10 @@ InternalLongJump
 #undef REG_PAIR
 #undef REG_ONE
         mov     sp, x16
-        cmp     w1, #0
-        mov     w0, #1
+        cmp     x1, #0
+        mov     x0, #1
         beq     exit
-        mov     w0, w1
+        mov     x0, x1
 exit
         // use br not ret, as ret is guaranteed to mispredict
         br      x30
-- 
2.20.1

[PATCH 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0

[Leif Lindholm]

The ARM implementation of of InternalLongJump always returned the value
Value - but it is not supposed to ever return 0. Add the test to prevent
that, and return 1 if Value is 0 - as is already present in AArch64.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S   | 2 ++
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
index 407df5f41ac5..d1a31b69cf26 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
@@ -60,6 +60,8 @@ ASM_PFX(SetJump):
 ASM_PFX(InternalLongJump):
   ldmia  r0, {r3-r12,r14}
   mov    r13, r3
+  cmp    r1, #0
+  moveq  r1, #1
   mov    r0, r1
   bx     lr
 
diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
index 3a45f045460a..603143c27050 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
@@ -60,6 +60,8 @@ SetJump
 InternalLongJump
   LDM   R0, {R3-R12,R14}
   MOV   R13, R3
+  CMP   R1, #0
+  MOVEQ R1, #1
   MOV   R0, R1
   BX    LR
 
-- 
2.20.1

Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations

[Ard Biesheuvel]

On 10/1/20 8:37 PM, Leif Lindholm wrote:
> The SetJump comment header states that:
>    If JumpBuffer is NULL, then ASSERT().
> 
> However, this was not currently done.
> Add a call to InternalAssertJumpBuffer.
> 
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 3 +++
>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 3 +++
>   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       | 3 +++
>   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     | 3 +++
>   4 files changed, 12 insertions(+)
> 
> diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> index 989736cee74c..34765a676430 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> @@ -45,6 +45,9 @@ GCC_ASM_EXPORT(InternalLongJump)
>   #  );
>   #
>   ASM_PFX(SetJump):
> +        stp     x30, x0, [sp, #-16]!
> +        bl      InternalAssertJumpBuffer
> +        ldp     x30, x0, [sp], #16

I think we should make this more idiomatic for AArch64 function calls, i.e.

         stp     x29, x30, [sp, #-32]!
         mov     x29, sp
         str     x0, [sp, #16]
         bl      InternalAssertJumpBuffer
         ldr     x0, [sp, #16]
         ldp     x29, x30, [sp], #32

That way, we'll have a well formed call stack with all the frame records 
linked together, allowing the debugger to show you where SetJump() was 
called from to begin with if you are stuck in the deadloop or hit the 
breakpoint (depending on how the PCD was configured to begin with)

I wouldn't mind putting the whole thing inside #ifndef MDEPKG_NDEBUG 
/#endif btw

>           mov     x16, sp // use IP0 so save SP
>   #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
>   #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
> diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> index 8922128e8c62..f2729a8bb03e 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> @@ -44,6 +44,9 @@
>   ;  );
>   ;
>   SetJump
> +        stp     x30, x0, [sp, #-16]!
> +        bl      InternalAssertJumpBuffer
> +        ldp     x30, x0, [sp], #16

Same here

>           mov     x16, sp // use IP0 so save SP
>   #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
>   #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
> diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> index e4c1946a28ff..54b11ad2197c 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> @@ -31,6 +31,9 @@ GCC_ASM_EXPORT(InternalLongJump)
>   #  );
>   #
>   ASM_PFX(SetJump):
> +  push  {r0, lr}
> +  bl    InternalAssertJumpBuffer
> +  pop   {r0, lr}

...  but not here (but the #ifndef would still be an improvement imo)


>     mov   r3, r13
>     stmia r0, {r3-r12,r14}
>     eor   r0, r0, r0
> diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> index e1eff758f7ab..6d47033975f2 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> @@ -31,6 +31,9 @@
>   ;  )
>   ;
>   SetJump
> +  PUSH {R0, LR}
> +  BL   InternalAssertJumpBuffer
> +  POP  {R0, LR}
>     MOV  R3, R13
>     STM  R0, {R3-R12,R14}
>     EOR  R0, R0
> 

Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations

[Leif Lindholm]

On Thu, Oct 01, 2020 at 22:49:05 +0200, Ard Biesheuvel wrote:
> On 10/1/20 8:37 PM, Leif Lindholm wrote:
> > The SetJump comment header states that:
> >    If JumpBuffer is NULL, then ASSERT().
> > 
> > However, this was not currently done.
> > Add a call to InternalAssertJumpBuffer.
> > 
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 3 +++
> >   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 3 +++
> >   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       | 3 +++
> >   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     | 3 +++
> >   4 files changed, 12 insertions(+)
> > 
> > diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > index 989736cee74c..34765a676430 100644
> > --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > @@ -45,6 +45,9 @@ GCC_ASM_EXPORT(InternalLongJump)
> >   #  );
> >   #
> >   ASM_PFX(SetJump):
> > +        stp     x30, x0, [sp, #-16]!
> > +        bl      InternalAssertJumpBuffer
> > +        ldp     x30, x0, [sp], #16
> 
> I think we should make this more idiomatic for AArch64 function calls, i.e.
> 
>         stp     x29, x30, [sp, #-32]!
>         mov     x29, sp
>         str     x0, [sp, #16]
>         bl      InternalAssertJumpBuffer
>         ldr     x0, [sp, #16]
>         ldp     x29, x30, [sp], #32
> 
> That way, we'll have a well formed call stack with all the frame records
> linked together, allowing the debugger to show you where SetJump() was
> called from to begin with if you are stuck in the deadloop or hit the
> breakpoint (depending on how the PCD was configured to begin with)

Mmm, you have a point.

> I wouldn't mind putting the whole thing inside #ifndef MDEPKG_NDEBUG /#endif
> btw

Well...
At that point we might as well go and un-bonkers-ify the interfaces and
make the C function the wrapper that does the assert check before
calling this function renamed to InternalSetJump - making it symmetric
with the LongJump side of the equation.
Which I was hoping to avoid, since that messes with all architectures.

Urgh, that is actually the sensible thing to do here, isn't it?

/
    Leif

> >           mov     x16, sp // use IP0 so save SP
> >   #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
> >   #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
> > diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > index 8922128e8c62..f2729a8bb03e 100644
> > --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > @@ -44,6 +44,9 @@
> >   ;  );
> >   ;
> >   SetJump
> > +        stp     x30, x0, [sp, #-16]!
> > +        bl      InternalAssertJumpBuffer
> > +        ldp     x30, x0, [sp], #16
> 
> Same here
> 
> >           mov     x16, sp // use IP0 so save SP
> >   #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
> >   #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
> > diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> > index e4c1946a28ff..54b11ad2197c 100644
> > --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> > +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> > @@ -31,6 +31,9 @@ GCC_ASM_EXPORT(InternalLongJump)
> >   #  );
> >   #
> >   ASM_PFX(SetJump):
> > +  push  {r0, lr}
> > +  bl    InternalAssertJumpBuffer
> > +  pop   {r0, lr}
> 
> ...  but not here (but the #ifndef would still be an improvement imo)
> 
> 
> >     mov   r3, r13
> >     stmia r0, {r3-r12,r14}
> >     eor   r0, r0, r0
> > diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > index e1eff758f7ab..6d47033975f2 100644
> > --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > @@ -31,6 +31,9 @@
> >   ;  )
> >   ;
> >   SetJump
> > +  PUSH {R0, LR}
> > +  BL   InternalAssertJumpBuffer
> > +  POP  {R0, LR}
> >     MOV  R3, R13
> >     STM  R0, {R3-R12,R14}
> >     EOR  R0, R0
> > 
> 

Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations

[Ard Biesheuvel]

On 10/1/20 10:55 PM, Leif Lindholm wrote:
> On Thu, Oct 01, 2020 at 22:49:05 +0200, Ard Biesheuvel wrote:
>> On 10/1/20 8:37 PM, Leif Lindholm wrote:
>>> The SetJump comment header states that:
>>>     If JumpBuffer is NULL, then ASSERT().
>>>
>>> However, this was not currently done.
>>> Add a call to InternalAssertJumpBuffer.
>>>
>>> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
>>> ---
>>>    MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 3 +++
>>>    MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 3 +++
>>>    MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       | 3 +++
>>>    MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     | 3 +++
>>>    4 files changed, 12 insertions(+)
>>>
>>> diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
>>> index 989736cee74c..34765a676430 100644
>>> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
>>> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
>>> @@ -45,6 +45,9 @@ GCC_ASM_EXPORT(InternalLongJump)
>>>    #  );
>>>    #
>>>    ASM_PFX(SetJump):
>>> +        stp     x30, x0, [sp, #-16]!
>>> +        bl      InternalAssertJumpBuffer
>>> +        ldp     x30, x0, [sp], #16
>>
>> I think we should make this more idiomatic for AArch64 function calls, i.e.
>>
>>          stp     x29, x30, [sp, #-32]!
>>          mov     x29, sp
>>          str     x0, [sp, #16]
>>          bl      InternalAssertJumpBuffer
>>          ldr     x0, [sp, #16]
>>          ldp     x29, x30, [sp], #32
>>
>> That way, we'll have a well formed call stack with all the frame records
>> linked together, allowing the debugger to show you where SetJump() was
>> called from to begin with if you are stuck in the deadloop or hit the
>> breakpoint (depending on how the PCD was configured to begin with)
> 
> Mmm, you have a point.
> 
>> I wouldn't mind putting the whole thing inside #ifndef MDEPKG_NDEBUG /#endif
>> btw
> 
> Well...
> At that point we might as well go and un-bonkers-ify the interfaces and
> make the C function the wrapper that does the assert check before
> calling this function renamed to InternalSetJump - making it symmetric
> with the LongJump side of the equation.
> Which I was hoping to avoid, since that messes with all architectures.
> 
> Urgh, that is actually the sensible thing to do here, isn't it?
> 

Do rhetorical questions expect an answer?

Re: [edk2-devel] [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations

[Philippe Mathieu-Daudé]

On 10/1/20 8:37 PM, Leif Lindholm wrote:
> There may be architectures on which there are benefits to
>   eor r0, r0(, r0)
> but ARM was never one of them.

=)

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> Change to more readable
>   mov r0, #0
> instead.
> 
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>  MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S   | 2 +-
>  MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Re: [edk2-devel] [PATCH 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump

[Philippe Mathieu-Daudé]

On 10/1/20 8:37 PM, Leif Lindholm wrote:
> Both in SetJump and in InternalLongJump, 32-bit w register views were
> used for the UINTN return value. In SetJump, this did not cause errors;
> it was only counterintuitive. But in InternalLongJump, it meant the top

Typo "counter-intuitive".

> 32 bits of Value were stripped off.
> 
> Change all of these to use the 64-bit x register views.
> 
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++----
>  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations

[Ard Biesheuvel]

On 10/1/20 8:37 PM, Leif Lindholm wrote:
> There may be architectures on which there are benefits to
>    eor r0, r0(, r0)
> but ARM was never one of them. Change to more readable
>    mov r0, #0
> instead.
> 
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S   | 2 +-
>   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> index 54b11ad2197c..407df5f41ac5 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> @@ -36,7 +36,7 @@ ASM_PFX(SetJump):
>     pop   {r0, lr}
>     mov   r3, r13
>     stmia r0, {r3-r12,r14}
> -  eor   r0, r0, r0
> +  mov   r0, #0

Actually, 'movs r0, #0' produces a smaller opcode here when building in 
Thumb2 mode :-)


>     bx    lr
>   
>   #/**
> diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> index 6d47033975f2..3a45f045460a 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> @@ -36,7 +36,7 @@ SetJump
>     POP  {R0, LR}
>     MOV  R3, R13
>     STM  R0, {R3-R12,R14}
> -  EOR  R0, R0
> +  MOV  RO, #0
>     BX   LR
>   
>   ;/**
>