[edk2-devel] [PATCH v2 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump

[edk2-devel] [PATCH v2 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump

[Leif Lindholm]

Back in the mists of time (January 2020) I submitted a set to clean up
and fix a few bugs in ARM/AArch64 BaseLib SetJump/LongJump implementations:
https://edk2.groups.io/g/devel/message/65812

Then hubris struck and I meant to refactor the code for all architectures,
and since this was in my first week in a new job, that meant everything
got completely dropped on the floor.

Then Andrei's fix of similar issues for RiscV64 made me remember this set.
And I figured, let's dial back the ambition and get the actual fixes
merged.

The overall scope remains:
- 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

And like the original submission, the changes to .asm files have been
neither build nor runtime tested.

Any acks/reviewed-bys have been dropped, it having been over 3.5 years.

Changes since v1:
- Rebased to current edk2 main.
- Incorporate Ard's feedback on maintaining a clean call stack for
  InternalAssertJumpBuffer, *and* conditionalising it on !MDEPKG_NDEBUG.
- Changed authorship to my current identity (the company I worked at
  during v1 having been acquired by my current employer, this feels
  like a reasonable course of action).

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.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   | 19 +++++++++++++------
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 19 +++++++++++++------
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       |  7 ++++---
 MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     |  7 ++++---
 4 files changed, 34 insertions(+), 18 deletions(-)

-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109074): https://edk2.groups.io/g/devel/message/109074
Mute This Topic: https://groups.io/mt/101600799/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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 <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.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 0d902d94d31c..78db9b3d1e78 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.
 #
@@ -62,7 +61,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 df70f298998e..d8b267addc1a 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.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109075): https://edk2.groups.io/g/devel/message/109075
Mute This Topic: https://groups.io/mt/101600801/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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 <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
---
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++++++
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
index 78db9b3d1e78..de79ad3a0a3e 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -46,6 +46,14 @@ GCC_ASM_EXPORT(InternalLongJump)
 #
 ASM_PFX(SetJump):
         AARCH64_BTI(c)
+#ifndef MDEPKG_NDEBUG
+        stp     x29, x30, [sp, #-32]!
+        mov     x29, sp
+        str     x0, [sp, #16]
+        bl      InternalAssertJumpBuffer
+        ldr     x0, [sp, #16]
+        ldp     x29, x30, [sp], #32
+#endif
         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 d8b267addc1a..c2774eece311 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -44,6 +44,14 @@
 ;  );
 ;
 SetJump
+#ifndef MDEPKG_NDEBUG
+        stp     x29, x30, [sp, #-32]!
+        mov     x29, sp
+        str     x0, [sp, #16]
+        bl      InternalAssertJumpBuffer
+        ldr     x0, [sp, #16]
+        ldp     x29, x30, [sp], #32
+#endif
         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]
-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109076): https://edk2.groups.io/g/devel/message/109076
Mute This Topic: https://groups.io/mt/101600803/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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 <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.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 e4c1946a28ff..e91320252255 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
@@ -33,7 +33,7 @@ GCC_ASM_EXPORT(InternalLongJump)
 ASM_PFX(SetJump):
   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 e1eff758f7ab..ef02d85e0e66 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
@@ -33,7 +33,7 @@
 SetJump
   MOV  R3, R13
   STM  R0, {R3-R12,R14}
-  EOR  R0, R0
+  MOV  RO, #0
   BX   LR
 
 ;/**
-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109078): https://edk2.groups.io/g/devel/message/109078
Mute This Topic: https://groups.io/mt/101600809/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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 <quic_llindhol@quicinc.com>
Reanimated-by: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.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 de79ad3a0a3e..3e58119b25d2 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -61,7 +61,7 @@ ASM_PFX(SetJump):
         FPR_LAYOUT
 #undef REG_PAIR
 #undef REG_ONE
-        mov     w0, #0
+        mov     x0, #0
         ret
 
 #/**
@@ -91,9 +91,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
         ret
 
 ASM_FUNCTION_REMOVE_IF_UNREFERENCED
diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
index c2774eece311..6ec8f35f2c9f 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -59,7 +59,7 @@ SetJump
         FPR_LAYOUT
 #undef REG_PAIR
 #undef REG_ONE
-        mov     w0, #0
+        mov     x0, #0
         ret
 
 ;/**
@@ -88,10 +88,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.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109077): https://edk2.groups.io/g/devel/message/109077
Mute This Topic: https://groups.io/mt/101600808/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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 <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.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 e91320252255..14006c6123e3 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
@@ -57,6 +57,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 ef02d85e0e66..15eb3dc28fb7 100644
--- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
@@ -57,6 +57,8 @@ SetJump
 InternalLongJump
   LDM   R0, {R3-R12,R14}
   MOV   R13, R3
+  CMP   R1, #0
+  MOVEQ R1, #1
   MOV   R0, R1
   BX    LR
 
-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109079): https://edk2.groups.io/g/devel/message/109079
Mute This Topic: https://groups.io/mt/101600811/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations

[Sami Mujawar]

Hi Leif,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 26/09/2023 06:15 pm, Leif Lindholm wrote:
> 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 <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.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 0d902d94d31c..78db9b3d1e78 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.
>   #
> @@ -62,7 +61,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 df70f298998e..d8b267addc1a 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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109200): https://edk2.groups.io/g/devel/message/109200
Mute This Topic: https://groups.io/mt/101600801/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Leif,

Thank you for this patch.

This patch looks good to me.

Just a question, should we also do the same for the AArch32 builds?

In either case,

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 26/09/2023 06:15 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 <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++++++
>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++++++
>   2 files changed, 16 insertions(+)
>
> diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> index 78db9b3d1e78..de79ad3a0a3e 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> @@ -46,6 +46,14 @@ GCC_ASM_EXPORT(InternalLongJump)
>   #
>   ASM_PFX(SetJump):
>           AARCH64_BTI(c)
> +#ifndef MDEPKG_NDEBUG
> +        stp     x29, x30, [sp, #-32]!
> +        mov     x29, sp
> +        str     x0, [sp, #16]
> +        bl      InternalAssertJumpBuffer
> +        ldr     x0, [sp, #16]
> +        ldp     x29, x30, [sp], #32
> +#endif
>           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 d8b267addc1a..c2774eece311 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> @@ -44,6 +44,14 @@
>   ;  );
>   ;
>   SetJump
> +#ifndef MDEPKG_NDEBUG
> +        stp     x29, x30, [sp, #-32]!
> +        mov     x29, sp
> +        str     x0, [sp, #16]
> +        bl      InternalAssertJumpBuffer
> +        ldr     x0, [sp, #16]
> +        ldp     x29, x30, [sp], #32
> +#endif
>           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]


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109201): https://edk2.groups.io/g/devel/message/109201
Mute This Topic: https://groups.io/mt/101600803/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Leif,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 26/09/2023 06:15 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 <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.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 e4c1946a28ff..e91320252255 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> @@ -33,7 +33,7 @@ GCC_ASM_EXPORT(InternalLongJump)
>   ASM_PFX(SetJump):
>     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 e1eff758f7ab..ef02d85e0e66 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> @@ -33,7 +33,7 @@
>   SetJump
>     MOV  R3, R13
>     STM  R0, {R3-R12,R14}
> -  EOR  R0, R0
> +  MOV  RO, #0
>     BX   LR
>   
>   ;/**


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109202): https://edk2.groups.io/g/devel/message/109202
Mute This Topic: https://groups.io/mt/101600809/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Leif,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 26/09/2023 06:15 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
> 32 bits of Value were stripped off.
>
> Change all of these to use the 64-bit x register views.
>
> Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> Reanimated-by: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.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 de79ad3a0a3e..3e58119b25d2 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> @@ -61,7 +61,7 @@ ASM_PFX(SetJump):
>           FPR_LAYOUT
>   #undef REG_PAIR
>   #undef REG_ONE
> -        mov     w0, #0
> +        mov     x0, #0
>           ret
>   
>   #/**
> @@ -91,9 +91,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
>           ret
>   
>   ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> index c2774eece311..6ec8f35f2c9f 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> @@ -59,7 +59,7 @@ SetJump
>           FPR_LAYOUT
>   #undef REG_PAIR
>   #undef REG_ONE
> -        mov     w0, #0
> +        mov     x0, #0
>           ret
>   
>   ;/**
> @@ -88,10 +88,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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109203): https://edk2.groups.io/g/devel/message/109203
Mute This Topic: https://groups.io/mt/101600808/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0

[Sami Mujawar]

Hi Leif,

Thank you for this patch.

There is a minor typo in the commit message, other than that these 
changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 26/09/2023 06:15 pm, Leif Lindholm wrote:
> The ARM implementation of of InternalLongJump always returned the value
[SAMI] Minor typo in that 'of' is repeated twice.
> 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 <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.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 e91320252255..14006c6123e3 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> @@ -57,6 +57,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 ef02d85e0e66..15eb3dc28fb7 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> @@ -57,6 +57,8 @@ SetJump
>   InternalLongJump
>     LDM   R0, {R3-R12,R14}
>     MOV   R13, R3
> +  CMP   R1, #0
> +  MOVEQ R1, #1
>     MOV   R0, R1
>     BX    LR
>   


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109204): https://edk2.groups.io/g/devel/message/109204
Mute This Topic: https://groups.io/mt/101600811/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Andrei Warkentin]

[-- Attachment #1: Type: text/plain, Size: 3194 bytes --]

Reviewed-by: Andrei Warkentin <andrei.warkentin@intel.com>
________________________________
От: Leif Lindholm <quic_llindhol@quicinc.com>
Отправлено: вторник, сентября 26, 2023 12:16 PM
Кому: devel@edk2.groups.io <devel@edk2.groups.io>
Копия: Philippe Mathieu-Daudé <philmd@linaro.org>; Warkentin, Andrei <andrei.warkentin@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>
Тема: [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump

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 <quic_llindhol@quicinc.com>
Reanimated-by: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.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 de79ad3a0a3e..3e58119b25d2 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -61,7 +61,7 @@ ASM_PFX(SetJump):
         FPR_LAYOUT
 #undef REG_PAIR
 #undef REG_ONE
-        mov     w0, #0
+        mov     x0, #0
         ret

 #/**
@@ -91,9 +91,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
         ret

 ASM_FUNCTION_REMOVE_IF_UNREFERENCED
diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
index c2774eece311..6ec8f35f2c9f 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -59,7 +59,7 @@ SetJump
         FPR_LAYOUT
 #undef REG_PAIR
 #undef REG_ONE
-        mov     w0, #0
+        mov     x0, #0
         ret

 ;/**
@@ -88,10 +88,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.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109226): https://edk2.groups.io/g/devel/message/109226
Mute This Topic: https://groups.io/mt/101600808/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6144 bytes --]

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

[Leif Lindholm]

On Fri, Sep 29, 2023 at 16:06:54 +0100, Sami Mujawar wrote:
> Hi Leif,
> 
> Thank you for this patch.
> 
> This patch looks good to me.
> 
> Just a question, should we also do the same for the AArch32 builds?

Hmm, fair point.
But I just managed to get rid of this set after nearly 4 years :):)

> In either case,
> 
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Thanks!
Commit message typo in 5/5 fixed and series merged as
f6a314e5b5dc..1a66bd51ca21.

Regards,

Leif

> Regards,
> 
> Sami Mujawar
> 
> On 26/09/2023 06:15 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 <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > ---
> >   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++++++
> >   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++++++
> >   2 files changed, 16 insertions(+)
> > 
> > diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > index 78db9b3d1e78..de79ad3a0a3e 100644
> > --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > @@ -46,6 +46,14 @@ GCC_ASM_EXPORT(InternalLongJump)
> >   #
> >   ASM_PFX(SetJump):
> >           AARCH64_BTI(c)
> > +#ifndef MDEPKG_NDEBUG
> > +        stp     x29, x30, [sp, #-32]!
> > +        mov     x29, sp
> > +        str     x0, [sp, #16]
> > +        bl      InternalAssertJumpBuffer
> > +        ldr     x0, [sp, #16]
> > +        ldp     x29, x30, [sp], #32
> > +#endif
> >           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 d8b267addc1a..c2774eece311 100644
> > --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > @@ -44,6 +44,14 @@
> >   ;  );
> >   ;
> >   SetJump
> > +#ifndef MDEPKG_NDEBUG
> > +        stp     x29, x30, [sp, #-32]!
> > +        mov     x29, sp
> > +        str     x0, [sp, #16]
> > +        bl      InternalAssertJumpBuffer
> > +        ldr     x0, [sp, #16]
> > +        ldp     x29, x30, [sp], #32
> > +#endif
> >           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]


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109256): https://edk2.groups.io/g/devel/message/109256
Mute This Topic: https://groups.io/mt/101600803/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Philippe Mathieu-Daudé]

On 26/9/23 19:15, 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
> 32 bits of Value were stripped off.
> 
> Change all of these to use the 64-bit x register views.
> 
> Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> Reanimated-by: Andrei Warkentin <andrei.warkentin@intel.com>

=)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++----
>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++----
>   2 files changed, 8 insertions(+), 8 deletions(-)



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109288): https://edk2.groups.io/g/devel/message/109288
Mute This Topic: https://groups.io/mt/101600808/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0

[Philippe Mathieu-Daudé]

On 26/9/23 19:15, Leif Lindholm wrote:
> 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 <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> ---
>   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S   | 2 ++
>   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm | 2 ++

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109289): https://edk2.groups.io/g/devel/message/109289
Mute This Topic: https://groups.io/mt/101600811/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Philippe Mathieu-Daudé]

Hi Leif,

On 26/9/23 19:15, 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 <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.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.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> index e1eff758f7ab..ef02d85e0e66 100644
> --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> @@ -33,7 +33,7 @@
>   SetJump
>     MOV  R3, R13
>     STM  R0, {R3-R12,R14}
> -  EOR  R0, R0
> +  MOV  RO, #0

Hmm. s/RO/R0/ ?

>     BX   LR
>   
>   ;/**



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109290): https://edk2.groups.io/g/devel/message/109290
Mute This Topic: https://groups.io/mt/101600809/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Leif Lindholm]

On Mon, Oct 02, 2023 at 20:00:21 +0200, Philippe Mathieu-Daudé wrote:
> Hi Leif,
> 
> On 26/9/23 19:15, 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 <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Sami Mujawar <sami.mujawar@arm.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.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > index e1eff758f7ab..ef02d85e0e66 100644
> > --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > @@ -33,7 +33,7 @@
> >   SetJump
> >     MOV  R3, R13
> >     STM  R0, {R3-R12,R14}
> > -  EOR  R0, R0
> > +  MOV  RO, #0
> 
> Hmm. s/RO/R0/ ?

Hmm, what?
I know for a fact I spotted that while importing the v1, and fixed it.
No idea how I managed to unfix it.

Too late, already got merged. Sent a fix out.

Hmm ... I think this says something about non-gcc/clang support for
(32-bit) Arm. Are we getting to the point where we might want to
retire it?

Thanks!

/
    Leif

> >     BX   LR
> >   ;/**
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109277): https://edk2.groups.io/g/devel/message/109277
Mute This Topic: https://groups.io/mt/101600809/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-