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

["Andrei Warkentin" <andrei.warkentin@intel.com>]

Looks reasonable to me.

Reviewed-by: Andrei Warkentin <andrei.warkentin@intel.com>

> -----Original Message-----
> From: Yang Wang <wangyang@bosc.ac.cn>
> Sent: Wednesday, December 27, 2023 8:57 PM
> To: Warkentin, Andrei <andrei.warkentin@intel.com>; devel@edk2.groups.io
> Cc: Yang Wang <wangyang@bosc.ac.cn>; Ran Wang <wangran@bosc.ac.cn>;
> Bamvor Jian ZHANG <zhangjian@bosc.ac.cn>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sunil V L <sunilvl@ventanamicro.com>; Liu,
> Zhiguang <zhiguang.liu@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 <wangyang@bosc.ac.cn>
> Reviewed-by: Ran Wang <wangran@bosc.ac.cn>
> Cc: Bamvor Jian ZHANG <zhangjian@bosc.ac.cn>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Zhiguang Liu <zhiguang.liu@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-