* [PATCH 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations
2020-10-01 18:37 [PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
@ 2020-10-01 18:37 ` Leif Lindholm
2020-10-01 18:37 ` [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations Leif Lindholm
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Leif Lindholm @ 2020-10-01 18:37 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
2020-10-01 18:37 [PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
2020-10-01 18:37 ` [PATCH 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations Leif Lindholm
@ 2020-10-01 18:37 ` Leif Lindholm
2020-10-01 20:49 ` Ard Biesheuvel
2020-10-01 18:37 ` [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2020-10-01 18:37 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
2020-10-01 18:37 ` [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations Leif Lindholm
@ 2020-10-01 20:49 ` Ard Biesheuvel
2020-10-01 20:55 ` Leif Lindholm
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-10-01 20:49 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Michael D Kinney, Liming Gao, Zhiguang Liu
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
2020-10-01 20:49 ` Ard Biesheuvel
@ 2020-10-01 20:55 ` Leif Lindholm
2020-10-01 20:58 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2020-10-01 20:55 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Michael D Kinney, Liming Gao, Zhiguang Liu
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
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
2020-10-01 20:55 ` Leif Lindholm
@ 2020-10-01 20:58 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-10-01 20:58 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Michael D Kinney, Liming Gao, Zhiguang Liu
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations
2020-10-01 18:37 [PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
2020-10-01 18:37 ` [PATCH 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations Leif Lindholm
2020-10-01 18:37 ` [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations Leif Lindholm
@ 2020-10-01 18:37 ` Leif Lindholm
2020-10-05 8:17 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-10-13 12:16 ` Ard Biesheuvel
2020-10-01 18:37 ` [PATCH 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
2020-10-01 18:37 ` [PATCH 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0 Leif Lindholm
4 siblings, 2 replies; 12+ messages in thread
From: Leif Lindholm @ 2020-10-01 18:37 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations
2020-10-01 18:37 ` [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
@ 2020-10-05 8:17 ` Philippe Mathieu-Daudé
2020-10-13 12:16 ` Ard Biesheuvel
1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-05 8:17 UTC (permalink / raw)
To: devel, leif; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu
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(-)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations
2020-10-01 18:37 ` [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
2020-10-05 8:17 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2020-10-13 12:16 ` Ard Biesheuvel
1 sibling, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-10-13 12:16 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Michael D Kinney, Liming Gao, Zhiguang Liu
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
>
> ;/**
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump
2020-10-01 18:37 [PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
` (2 preceding siblings ...)
2020-10-01 18:37 ` [PATCH 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
@ 2020-10-01 18:37 ` Leif Lindholm
2020-10-05 8:19 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-10-01 18:37 ` [PATCH 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0 Leif Lindholm
4 siblings, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2020-10-01 18:37 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump
2020-10-01 18:37 ` [PATCH 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
@ 2020-10-05 8:19 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-05 8:19 UTC (permalink / raw)
To: devel, leif; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0
2020-10-01 18:37 [PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
` (3 preceding siblings ...)
2020-10-01 18:37 ` [PATCH 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
@ 2020-10-01 18:37 ` Leif Lindholm
4 siblings, 0 replies; 12+ messages in thread
From: Leif Lindholm @ 2020-10-01 18:37 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu
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
^ permalink raw reply related [flat|nested] 12+ messages in thread