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

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

I'd go for using "mv" over add with immediate of zero.

Also the REG_S/REG_L business doesn't really help with legibility. Why not just use sd and ld? I get that you're copying from RiscVSetJumpLongJump.S...which seems like another issue.

If the function is called SwitchStack, why are you restoring all the other regs? Following the AArch64 version is probably a better idea. Just don't take a BASE_LIBRARY_JUMP_BUFFER  as a parameter, take EntryPoint, Context, Context2 and NewStack. And you can even take these in the order of Context, Context2, EntryPoint, NewStack to avoid the extra movement of values in registers.

A

> -----Original Message-----
> From: 王洋 <wangyang@bosc.ac.cn>
> Sent: Sunday, December 24, 2023 8:47 PM
> To: devel@edk2.groups.io; sunilvl@ventanamicro.com; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>
> Subject: [Resend PATCH] MdePkg/BaseLib:Fix boot DxeCore hang on riscv
> platform
> 
> From f15d405067860a8087c5eb4080bc3e08ca5e0e21 Mon Sep 17 00:00:00
> 2001
> From: wangyang <wangyang@bosc.ac.cn>
> Date: Wed, 20 Dec 2023 20:27:42 +0800
> Subject: [PATCH] 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/LoongArch64/SwitchStack.S)
> 
> Signed-off-by: Yang Wang <wangyang@bosc.ac.cn>
> Reviewed-by: Ran Wang <wangran@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>
> ---
>  .../BaseLib/RiscV64/InternalSwitchStack.c     |  7 +++-
>  MdePkg/Library/BaseLib/RiscV64/SwitchStack.S  | 40 +++++++++++++++++++
>  2 files changed, 46 insertions(+), 1 deletion(-)  create mode 100644
> MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> 
> diff --git a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> index b78424c163..c60fbdb896 100644
> --- a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> +++ b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> @@ -8,6 +8,11 @@
> 
>  #include "BaseLibInternals.h"
> 
> +UINTN
> +EFIAPI
> +InternalSwitchStackAsm (
> +  IN     BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
> +  );
>  /**
>    Transfers control to a function starting with a new stack.
> 
> @@ -48,6 +53,6 @@ InternalSwitchStack (
>    JumpBuffer.SP = (UINTN)NewStack - sizeof (VOID *);
>    JumpBuffer.S0 = (UINT64)(UINTN)Context1;
>    JumpBuffer.S1 = (UINT64)(UINTN)Context2;
> -  LongJump (&JumpBuffer, (UINTN)-1);
> +  InternalSwitchStackAsm (&JumpBuffer);
>    ASSERT (FALSE);
>  }
> diff --git a/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> new file mode 100644
> index 0000000000..59b8d60e7e
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> @@ -0,0 +1,40 @@
> +//---------------------------------------------------------------------
> +---------
> +//
> +// InternalSwitchStackAsm for RISC-V
> +//
> +// Copyright (c) 2023, Bosc Corporation. All rights reserved.<BR> // //
> +SPDX-License-Identifier: BSD-2-Clause-Patent //
> +//---------------------------------------------------------------------
> +---------
> +# define REG_S  sd
> +# define REG_L  ld
> +# define SZREG  8
> +.align 3
> +
> +/**
> +  This allows the caller to switch the stack and goes to the new entry
> +point
> +
> +  @param  JumpBuffer    A pointer to CPU context buffer.
> +**/
> +
> +    .globl InternalSwitchStackAsm
> +InternalSwitchStackAsm:
> +  REG_L ra,  0*SZREG(a0)
> +  REG_L s0,  1*SZREG(a0)
> +  REG_L s1,  2*SZREG(a0)
> +  REG_L s2,  3*SZREG(a0)
> +  REG_L s3,  4*SZREG(a0)
> +  REG_L s4,  5*SZREG(a0)
> +  REG_L s5,  6*SZREG(a0)
> +  REG_L s6,  7*SZREG(a0)
> +  REG_L s7,  8*SZREG(a0)
> +  REG_L s8,  9*SZREG(a0)
> +  REG_L s9,  10*SZREG(a0)
> +  REG_L s10, 11*SZREG(a0)
> +  REG_L s11, 12*SZREG(a0)
> +  REG_L sp,  13*SZREG(a0)
> +
> +  add   a0, s0, 0
> +  add   a1, s1, 0
> +  ret
> --
> 2.25.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112927): https://edk2.groups.io/g/devel/message/112927
Mute This Topic: https://groups.io/mt/103369618/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-