Re: [edk2-devel] [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform

2024-01-14 Thread WangYang



> -原始邮件-
> 发件人: "Sunil V L" 
> 发送时间: 2024-01-11 23:21:21 (星期四)
> 收件人: devel@edk2.groups.io, andrei.warken...@intel.com
> 抄送: "Yang Wang" , "Ran Wang" , 
> "Bamvor Jian ZHANG" , "Gao, Liming" 
> , "Kinney, Michael D" , 
> "Liu, Zhiguang" 
> 主题: Re: [edk2-devel] [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv 
> platform
> 
> On Fri, Jan 05, 2024 at 03:47:07PM +, Andrei Warkentin wrote:
> > Looks reasonable to me.
> > 
> > Reviewed-by: Andrei Warkentin 
> > 
> > > -Original Message-
> > > From: Yang Wang 
> > > Sent: Wednesday, December 27, 2023 8:57 PM
> > > To: Warkentin, Andrei ; devel@edk2.groups.io
> > > Cc: Yang Wang ; Ran Wang ;
> > > Bamvor Jian ZHANG ; Gao, Liming
> > > ; Kinney, Michael D
> > > ; Sunil V L ; Liu,
> > > Zhiguang 
> > > Subject: [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform
> > > 
> > > For scene of
> > > HandOffToDxeCore()->SwitchStack(DxeCoreEntryPoint)->
> > > InternalSwitchStack()->LongJump(),Variable HobList.Raw
> > > will be passed (from *Context1 to register a0) to
> > > DxeMain() in parameter *HobStart.
> > > 
> > > However, meanwhile the function LongJump() overrides
> > > register a0 with a1 (-1)  due to commit (ea628f28e5 "RISCV: Fix
> > > InternalLongJump to return correct value"), then cause hang.
> > > 
> > > Replacing calling LongJump() with new InternalSwitchStackAsm() to pass
> > > addres data in register s0 to register a0 could fix this issue (just
> > > like the solution in MdePkg/Library/BaseLib/AArch64/SwitchStack.S)
> > > 
> > > Signed-off-by: Yang Wang 
> > > Reviewed-by: Ran Wang 
> > > Cc: Bamvor Jian ZHANG 
> > > Cc: Andrei Warkentin 
> > > Cc: Liming Gao 
> > > Cc: Michael D Kinney 
> > > Cc: Sunil V L 
> > > Cc: Zhiguang Liu 
> > > ---
> Thanks for the patch!. Merged this as #5255 after fixing a minor
> formatting issue.

Thank you.

> 
> Thanks,
> Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113785): https://edk2.groups.io/g/devel/message/113785
Mute This Topic: https://groups.io/mt/103395756/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




Re: [edk2-devel] [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform

2024-01-11 Thread Sunil V L
On Fri, Jan 05, 2024 at 03:47:07PM +, Andrei Warkentin wrote:
> Looks reasonable to me.
> 
> Reviewed-by: Andrei Warkentin 
> 
> > -Original Message-
> > From: Yang Wang 
> > Sent: Wednesday, December 27, 2023 8:57 PM
> > To: Warkentin, Andrei ; devel@edk2.groups.io
> > Cc: Yang Wang ; Ran Wang ;
> > Bamvor Jian ZHANG ; Gao, Liming
> > ; Kinney, Michael D
> > ; Sunil V L ; Liu,
> > Zhiguang 
> > Subject: [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform
> > 
> > For scene of
> > HandOffToDxeCore()->SwitchStack(DxeCoreEntryPoint)->
> > InternalSwitchStack()->LongJump(),Variable HobList.Raw
> > will be passed (from *Context1 to register a0) to
> > DxeMain() in parameter *HobStart.
> > 
> > However, meanwhile the function LongJump() overrides
> > register a0 with a1 (-1)  due to commit (ea628f28e5 "RISCV: Fix
> > InternalLongJump to return correct value"), then cause hang.
> > 
> > Replacing calling LongJump() with new InternalSwitchStackAsm() to pass
> > addres data in register s0 to register a0 could fix this issue (just
> > like the solution in MdePkg/Library/BaseLib/AArch64/SwitchStack.S)
> > 
> > Signed-off-by: Yang Wang 
> > Reviewed-by: Ran Wang 
> > Cc: Bamvor Jian ZHANG 
> > Cc: Andrei Warkentin 
> > Cc: Liming Gao 
> > Cc: Michael D Kinney 
> > Cc: Sunil V L 
> > Cc: Zhiguang Liu 
> > ---
Thanks for the patch!. Merged this as #5255 after fixing a minor
formatting issue.

Thanks,
Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113624): https://edk2.groups.io/g/devel/message/113624
Mute This Topic: https://groups.io/mt/103395756/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




Re: [edk2-devel] [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform

2024-01-05 Thread Andrei Warkentin
Looks reasonable to me.

Reviewed-by: Andrei Warkentin 

> -Original Message-
> From: Yang Wang 
> Sent: Wednesday, December 27, 2023 8:57 PM
> To: Warkentin, Andrei ; devel@edk2.groups.io
> Cc: Yang Wang ; Ran Wang ;
> Bamvor Jian ZHANG ; Gao, Liming
> ; Kinney, Michael D
> ; Sunil V L ; Liu,
> Zhiguang 
> Subject: [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform
> 
> For scene of
> HandOffToDxeCore()->SwitchStack(DxeCoreEntryPoint)->
> InternalSwitchStack()->LongJump(),Variable HobList.Raw
> will be passed (from *Context1 to register a0) to
> DxeMain() in parameter *HobStart.
> 
> However, meanwhile the function LongJump() overrides
> register a0 with a1 (-1)  due to commit (ea628f28e5 "RISCV: Fix
> InternalLongJump to return correct value"), then cause hang.
> 
> Replacing calling LongJump() with new InternalSwitchStackAsm() to pass
> addres data in register s0 to register a0 could fix this issue (just
> like the solution in MdePkg/Library/BaseLib/AArch64/SwitchStack.S)
> 
> Signed-off-by: Yang Wang 
> Reviewed-by: Ran Wang 
> Cc: Bamvor Jian ZHANG 
> Cc: Andrei Warkentin 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Cc: Sunil V L 
> Cc: Zhiguang Liu 
> ---
> Change in v2:
> - Remove JumpBuffer variable parameter
> - Take these in the order of Context1, Context2, EntryPoint, NewStack
> - Fix BaseLib.inf, add Compilation SwitchStack.S
> - Drop REG_S/REG_L
> 
>  MdePkg/Library/BaseLib/BaseLib.inf|  1 +
>  .../BaseLib/RiscV64/InternalSwitchStack.c | 31 
>  MdePkg/Library/BaseLib/RiscV64/SwitchStack.S  | 37
> +++
>  3 files changed, 62 insertions(+), 7 deletions(-)
>  create mode 100644 MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> 
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
> b/MdePkg/Library/BaseLib/BaseLib.inf
> index 5338938944..6b46949be3 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -397,6 +397,7 @@
>RiscV64/CpuPause.c
> 
>RiscV64/MemoryFence.S | GCC
> 
>RiscV64/RiscVSetJumpLongJump.S| GCC
> 
> +  RiscV64/SwitchStack.S | GCC
> 
>RiscV64/RiscVCpuBreakpoint.S  | GCC
> 
>RiscV64/RiscVCpuPause.S   | GCC
> 
>RiscV64/RiscVInterrupt.S  | GCC
> 
> diff --git a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> index b78424c163..3216d241ad 100644
> --- a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> +++ b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> @@ -8,6 +8,29 @@
> 
> 
>  #include "BaseLibInternals.h"
> 
> 
> 
> +/**
> 
> +  Transfers control to a function starting with a new stack.
> 
> +
> 
> +  This internal worker function transfers control to the function
> 
> +  specified by EntryPoint using the new stack specified by NewStack,
> 
> +  and passes in the parameters specified by Context1 and Context2.
> 
> +  Context1 and Context2 are optional and may be NULL.
> 
> +  The function EntryPoint must never return.
> 
> +
> 
> +  @param Context1 The first parameter to pass in.
> 
> +  @param Context2 The second Parameter to pass in
> 
> +  @param EntryPoint   The pointer to the function to enter.
> 
> +  @param NewStack The new Location of the stack
> 
> +
> 
> +**/
> 
> +VOID
> 
> +EFIAPI
> 
> +InternalSwitchStackAsm (
> 
> +  IN  VOID  *Context1OPTIONAL,
> 
> +  IN  VOID  *Context2OPTIONAL,
> 
> +  IN  SWITCH_STACK_ENTRY_POINT  EntryPoint,
> 
> +  IN  VOID  *NewStack
> 
> +  );
> 
>  /**
> 
>Transfers control to a function starting with a new stack.
> 
> 
> 
> @@ -42,12 +65,6 @@ InternalSwitchStack (
>IN  VA_LIST   Marker
> 
>)
> 
>  {
> 
> -  BASE_LIBRARY_JUMP_BUFFER  JumpBuffer;
> 
> -
> 
> -  JumpBuffer.RA = (UINTN)EntryPoint;
> 
> -  JumpBuffer.SP = (UINTN)NewStack - sizeof (VOID *);
> 
> -  JumpBuffer.S0 = (UINT64)(UINTN)Context1;
> 
> -  JumpBuffer.S1 = (UINT64)(UINTN)Context2;
> 
> -  LongJump (&JumpBuffer, (UINTN)-1);
> 
> +  InternalSwitchStackAsm (Context1, Context2, EntryPoint, (VOID
> *)((UINTN)NewStack - sizeof (VOID *)));
> 
>ASSERT (FALSE);
> 
>  }
> 
> diff --git a/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> new file mode 100644
> index 00..db535c1aab
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> @@ -0,0 +1,37 @@
> +//--
> 
> +//
> 
> +// InternalSwitchStackAsm for RISC-V
> 
> +//
> 
> +// Copyright (c) 2023, Bosc Corporation. All rights reserved.
> 
> +//
> 
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> +//
> 
> +//--
> 
> +.align 3
> 
> +
> 
> +#/**
> 
> +#
> 
> +# This allows the caller to switch the stack

[edk2-devel] [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform

2023-12-27 Thread 王洋
For scene of
HandOffToDxeCore()->SwitchStack(DxeCoreEntryPoint)->
InternalSwitchStack()->LongJump(),Variable HobList.Raw
will be passed (from *Context1 to register a0) to
DxeMain() in parameter *HobStart.

However, meanwhile the function LongJump() overrides
register a0 with a1 (-1)  due to commit (ea628f28e5 "RISCV: Fix
InternalLongJump to return correct value"), then cause hang.

Replacing calling LongJump() with new InternalSwitchStackAsm() to pass
addres data in register s0 to register a0 could fix this issue (just
like the solution in MdePkg/Library/BaseLib/AArch64/SwitchStack.S)

Signed-off-by: Yang Wang 
Reviewed-by: Ran Wang 
Cc: Bamvor Jian ZHANG 
Cc: Andrei Warkentin 
Cc: Liming Gao 
Cc: Michael D Kinney 
Cc: Sunil V L 
Cc: Zhiguang Liu 
---
Change in v2:
- Remove JumpBuffer variable parameter
- Take these in the order of Context1, Context2, EntryPoint, NewStack 
- Fix BaseLib.inf, add Compilation SwitchStack.S
- Drop REG_S/REG_L

 MdePkg/Library/BaseLib/BaseLib.inf|  1 +
 .../BaseLib/RiscV64/InternalSwitchStack.c | 31 
 MdePkg/Library/BaseLib/RiscV64/SwitchStack.S  | 37 +++
 3 files changed, 62 insertions(+), 7 deletions(-)
 create mode 100644 MdePkg/Library/BaseLib/RiscV64/SwitchStack.S

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf 
b/MdePkg/Library/BaseLib/BaseLib.inf
index 5338938944..6b46949be3 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -397,6 +397,7 @@
   RiscV64/CpuPause.c
   RiscV64/MemoryFence.S | GCC
   RiscV64/RiscVSetJumpLongJump.S| GCC
+  RiscV64/SwitchStack.S | GCC
   RiscV64/RiscVCpuBreakpoint.S  | GCC
   RiscV64/RiscVCpuPause.S   | GCC
   RiscV64/RiscVInterrupt.S  | GCC
diff --git a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c 
b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
index b78424c163..3216d241ad 100644
--- a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
+++ b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
@@ -8,6 +8,29 @@
 
 #include "BaseLibInternals.h"
 
+/**
+  Transfers control to a function starting with a new stack.
+
+  This internal worker function transfers control to the function
+  specified by EntryPoint using the new stack specified by NewStack,
+  and passes in the parameters specified by Context1 and Context2.
+  Context1 and Context2 are optional and may be NULL.
+  The function EntryPoint must never return.
+
+  @param Context1 The first parameter to pass in.
+  @param Context2 The second Parameter to pass in
+  @param EntryPoint   The pointer to the function to enter.
+  @param NewStack The new Location of the stack
+
+**/
+VOID
+EFIAPI
+InternalSwitchStackAsm (
+  IN  VOID  *Context1OPTIONAL,
+  IN  VOID  *Context2OPTIONAL,
+  IN  SWITCH_STACK_ENTRY_POINT  EntryPoint,
+  IN  VOID  *NewStack
+  );
 /**
   Transfers control to a function starting with a new stack.
 
@@ -42,12 +65,6 @@ InternalSwitchStack (
   IN  VA_LIST   Marker
   )
 {
-  BASE_LIBRARY_JUMP_BUFFER  JumpBuffer;
-
-  JumpBuffer.RA = (UINTN)EntryPoint;
-  JumpBuffer.SP = (UINTN)NewStack - sizeof (VOID *);
-  JumpBuffer.S0 = (UINT64)(UINTN)Context1;
-  JumpBuffer.S1 = (UINT64)(UINTN)Context2;
-  LongJump (&JumpBuffer, (UINTN)-1);
+  InternalSwitchStackAsm (Context1, Context2, EntryPoint, (VOID 
*)((UINTN)NewStack - sizeof (VOID *)));
   ASSERT (FALSE);
 }
diff --git a/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S 
b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
new file mode 100644
index 00..db535c1aab
--- /dev/null
+++ b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
@@ -0,0 +1,37 @@
+//--
+//
+// InternalSwitchStackAsm for RISC-V
+//
+// Copyright (c) 2023, Bosc Corporation. All rights reserved.
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+//--
+.align 3
+
+#/**
+#
+# This allows the caller to switch the stack and goes to the new entry point
+#
+# @param  Context  Parameter to pass in
+# @param  Context2 Parameter2 to pass in
+# @param  EntryPoint   The pointer to the location to enter
+# @param  NewStack New Location of the stack
+#
+# @return Nothing. Goes to the Entry Point passing in the new parameters
+#
+#**/
+#VOID
+#EFIAPI
+#InternalSwitchStackAsm (
+#  VOID  *Context,
+#  VOID  *Context2,
+#  SWITCH_STACK_ENTRY_POINT EntryPoint,
+#  VOID  *NewStack
+#  );
+#
+.globl InternalSwitchStackAsm
+InternalSwitchStackAsm:
+  mv ra, a2
+  mv sp, a3
+  ret
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112940): https://edk2.groups.io/g/devel/message/112940
Mute This Topic: https://groups.io/mt/10339575