* [edk2-devel] [PATCH v2 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations
2023-09-26 17:15 [edk2-devel] [PATCH v2 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
@ 2023-09-26 17:15 ` Leif Lindholm
2023-09-29 15:06 ` Sami Mujawar
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations Leif Lindholm
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Leif Lindholm @ 2023-09-26 17:15 UTC (permalink / raw)
To: devel; +Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations Leif Lindholm
@ 2023-09-29 15:06 ` Sami Mujawar
0 siblings, 0 replies; 17+ messages in thread
From: Sami Mujawar @ 2023-09-29 15:06 UTC (permalink / raw)
To: Leif Lindholm, devel
Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, nd@arm.com
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-devel] [PATCH v2 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
2023-09-26 17:15 [edk2-devel] [PATCH v2 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations Leif Lindholm
@ 2023-09-26 17:15 ` Leif Lindholm
2023-09-29 15:06 ` Sami Mujawar
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Leif Lindholm @ 2023-09-26 17:15 UTC (permalink / raw)
To: devel; +Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations Leif Lindholm
@ 2023-09-29 15:06 ` Sami Mujawar
2023-10-02 15:52 ` Leif Lindholm
0 siblings, 1 reply; 17+ messages in thread
From: Sami Mujawar @ 2023-09-29 15:06 UTC (permalink / raw)
To: Leif Lindholm, devel
Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, nd@arm.com
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
2023-09-29 15:06 ` Sami Mujawar
@ 2023-10-02 15:52 ` Leif Lindholm
0 siblings, 0 replies; 17+ messages in thread
From: Leif Lindholm @ 2023-10-02 15:52 UTC (permalink / raw)
To: Sami Mujawar
Cc: devel, Philippe Mathieu-Daudé, Ard Biesheuvel, nd@arm.com
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations
2023-09-26 17:15 [edk2-devel] [PATCH v2 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 1/5] MdePkg/BaseLib: fix comments in ARM* SetJump/LongJump implementations Leif Lindholm
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations Leif Lindholm
@ 2023-09-26 17:15 ` Leif Lindholm
2023-09-29 15:07 ` Sami Mujawar
2023-10-02 18:00 ` Philippe Mathieu-Daudé
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0 Leif Lindholm
4 siblings, 2 replies; 17+ messages in thread
From: Leif Lindholm @ 2023-09-26 17:15 UTC (permalink / raw)
To: devel; +Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
@ 2023-09-29 15:07 ` Sami Mujawar
2023-10-02 18:00 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 17+ messages in thread
From: Sami Mujawar @ 2023-09-29 15:07 UTC (permalink / raw)
To: Leif Lindholm, devel
Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, nd@arm.com
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
2023-09-29 15:07 ` Sami Mujawar
@ 2023-10-02 18:00 ` Philippe Mathieu-Daudé
2023-10-03 10:43 ` Leif Lindholm
1 sibling, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-02 18:00 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM SetJump implementations
2023-10-02 18:00 ` Philippe Mathieu-Daudé
@ 2023-10-03 10:43 ` Leif Lindholm
0 siblings, 0 replies; 17+ messages in thread
From: Leif Lindholm @ 2023-10-03 10:43 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: devel, Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump
2023-09-26 17:15 [edk2-devel] [PATCH v2 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
` (2 preceding siblings ...)
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 3/5] MdePkg/BaseLib: use normal register init in ARM " Leif Lindholm
@ 2023-09-26 17:15 ` Leif Lindholm
2023-09-29 15:07 ` Sami Mujawar
` (2 more replies)
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0 Leif Lindholm
4 siblings, 3 replies; 17+ messages in thread
From: Leif Lindholm @ 2023-09-26 17:15 UTC (permalink / raw)
To: devel
Cc: Philippe Mathieu-Daudé, Andrei Warkentin, Ard Biesheuvel,
Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
@ 2023-09-29 15:07 ` Sami Mujawar
2023-09-29 22:00 ` Andrei Warkentin
2023-10-02 17:56 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 17+ messages in thread
From: Sami Mujawar @ 2023-09-29 15:07 UTC (permalink / raw)
To: Leif Lindholm, devel
Cc: Philippe Mathieu-Daudé, Andrei Warkentin, Ard Biesheuvel,
nd@arm.com
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
2023-09-29 15:07 ` Sami Mujawar
@ 2023-09-29 22:00 ` Andrei Warkentin
2023-10-02 17:56 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 17+ messages in thread
From: Andrei Warkentin @ 2023-09-29 22:00 UTC (permalink / raw)
To: Leif Lindholm, devel@edk2.groups.io
Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, Sami Mujawar
[-- 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 --]
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
2023-09-29 15:07 ` Sami Mujawar
2023-09-29 22:00 ` Andrei Warkentin
@ 2023-10-02 17:56 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-02 17:56 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Andrei Warkentin, Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0
2023-09-26 17:15 [edk2-devel] [PATCH v2 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump Leif Lindholm
` (3 preceding siblings ...)
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 4/5] MdePkg/BaseLib: correct register sizes in AArch64 SetJump/LongJump Leif Lindholm
@ 2023-09-26 17:15 ` Leif Lindholm
2023-09-29 15:07 ` Sami Mujawar
2023-10-02 17:58 ` Philippe Mathieu-Daudé
4 siblings, 2 replies; 17+ messages in thread
From: Leif Lindholm @ 2023-09-26 17:15 UTC (permalink / raw)
To: devel; +Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0 Leif Lindholm
@ 2023-09-29 15:07 ` Sami Mujawar
2023-10-02 17:58 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 17+ messages in thread
From: Sami Mujawar @ 2023-09-29 15:07 UTC (permalink / raw)
To: Leif Lindholm, devel
Cc: Philippe Mathieu-Daudé, Ard Biesheuvel, nd@arm.com
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0
2023-09-26 17:15 ` [edk2-devel] [PATCH v2 5/5] MdePkg/BaseLib: ensure ARM LongJump never returns 0 Leif Lindholm
2023-09-29 15:07 ` Sami Mujawar
@ 2023-10-02 17:58 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-02 17:58 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Ard Biesheuvel, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 17+ messages in thread