Looks reasonable to me.
Reviewed-by: Andrei Warkentin <andrei.warken...@intel.com>
> -----Original Message-----
> From: Yang Wang <wangy...@bosc.ac.cn>
> Sent: Wednesday, December 27, 2023 8:57 PM
> To: Warkentin, Andrei <andrei.warken...@intel.com>; devel@edk2.groups.io
> Cc: Yang Wang <wangy...@bosc.ac.cn>; Ran Wang <wang...@bosc.ac.cn>;
> Bamvor Jian ZHANG <zhangj...@bosc.ac.cn>; Gao, Liming
> <gaolim...@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Sunil V L <suni...@ventanamicro.com>; Liu,
> Zhiguang <zhiguang....@intel.com>
> 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 <wangy...@bosc.ac.cn>
> Reviewed-by: Ran Wang <wang...@bosc.ac.cn>
> Cc: Bamvor Jian ZHANG <zhangj...@bosc.ac.cn>
> Cc: Andrei Warkentin <andrei.warken...@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Sunil V L <suni...@ventanamicro.com>
> Cc: Zhiguang Liu <zhiguang....@intel.com>
> ---
> 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 *Context1 OPTIONAL,
>
> + IN VOID *Context2 OPTIONAL,
>
> + 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 0000000000..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.<BR>
>
> +//
>
> +// 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 (#113297): https://edk2.groups.io/g/devel/message/113297
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]
-=-=-=-=-=-=-=-=-=-=-=-
