[edk2-devel] [Patch V3 2/6] UefiCpuPkg: Duplicate AsmRelocateApLoopAmd.

[Yuanhao Xie]

Hi Gerd,

-Now you are adding back the AmdSev version.
-It should be the generic version though.
Duplication is as I want to build up the desired functionality in small steps, generic version is updated in patch3 and ready in patch 6.

Call AsmRelocateApLoopStartAmdSev brings more confusion.

Thanks
Regards,
Yuanhao
-----Original Message-----
From: Gerd Hoffmann <kraxel at redhat.com> 
Sent: Friday, February 24, 2023 3:38 PM
To: devel at edk2.groups.io; Xie, Yuanhao <yuanhao.xie at intel.com>
Cc: Dong, Guo <guo.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Rhodes, Sean <sean at starlabs.systems>; Lu, James <james.lu at intel.com>; Guo, Gua <gua.guo at intel.com>
Subject: Re: [edk2-devel] [Patch V3 2/6] UefiCpuPkg: Duplicate AsmRelocateApLoopAmd.

On Fri, Feb 24, 2023 at 02:05:31AM +0800, Yuanhao Xie wrote:
> Duplicate AsmRelocateApLoopAmd for non-SEV-ES enabled processors.
> 
> Cc: Guo Dong <guo.dong at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Sean Rhodes <sean at starlabs.systems>
> Cc: James Lu <james.lu at intel.com>
> Cc: Gua Guo <gua.guo at intel.com>
> Signed-off-by: Yuanhao Xie <yuanhao.xie at intel.com>
> Test-by: Yuanhao Xie <yuanhao.xie at intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  68 ++++++++++++++++++++++++++++++++++++++++++++------------------------
>  UefiCpuPkg/Library/MpInitLib/MpEqu.inc        |  22 ++++++++++++----------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |  31 +++++++++++++++++++++++++++++--
>  UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm  |  33 
> +++++++++++++++++----------------  
> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 171 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++
>  5 files changed, 273 insertions(+), 52 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a84e9e33ba..dd935a79d3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    MP initialize support functions for DXE phase.
>  
> -  Copyright (c) 2016 - 2020, Intel Corporation. All rights 
> reserved.<BR>
> +  Copyright (c) 2016 - 2023, Intel Corporation. All rights 
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -378,32 +378,44 @@ RelocateApLoop (
>    IN OUT VOID  *Buffer
>    )
>  {
> -  CPU_MP_DATA           *CpuMpData;
> -  BOOLEAN               MwaitSupport;
> -  ASM_RELOCATE_AP_LOOP  AsmRelocateApLoopFunc;
> -  UINTN                 ProcessorNumber;
> -  UINTN                 StackStart;
> +  CPU_MP_DATA                  *CpuMpData;
> +  BOOLEAN                      MwaitSupport;
> +  ASM_RELOCATE_AP_LOOP         AsmRelocateApLoopFunc;
> +  ASM_RELOCATE_AP_LOOP_AMDSEV  AsmRelocateApLoopFuncAmdSev;
> +  UINTN                        ProcessorNumber;
> +  UINTN                        StackStart;
>  
>    MpInitLibWhoAmI (&ProcessorNumber);
>    CpuMpData    = GetCpuMpData ();
>    MwaitSupport = IsMwaitSupport ();
>    if (CpuMpData->UseSevEsAPMethod) {
> -    StackStart = CpuMpData->SevEsAPResetStackStart;
> +    StackStart                  = CpuMpData->SevEsAPResetStackStart;
> +    AsmRelocateApLoopFuncAmdSev = 
> + (ASM_RELOCATE_AP_LOOP)(UINTN)mReservedApLoopFunc;

mReservedApLoopFuncAmdSev ?

> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm 
> b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> index c1e8a045a4..6b48913306 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> @@ -347,12 +347,13 @@ PM16Mode:
>  
>  SwitchToRealProcEnd:
>  
> ;---------------------------------------------------------------------
> ---------------- -;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, 
> PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, 
> SevEsAPJumpTable, WakeupBuffer);
> +;  AsmRelocateApLoopAmdSev (MwaitSupport, ApTargetCState, 
> +PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, 
> +SevEsAPJumpTable, WakeupBuffer);
>  
> ;---------------------------------------------------------------------
> ----------------
> -AsmRelocateApLoopStart:
> +
> +AsmRelocateApLoopStartAmdSev:

I'd suggest to do the rename in patch #1 too.

> +;--------------------------------------------------------------------
> +----------------- ;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, 
> +PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, 
> +SevEsAPJumpTable, WakeupBuffer);
> +;--------------------------------------------------------------------
> +-----------------
> +AsmRelocateApLoopStart:
> +BITS 64
> +    cmp        qword [rsp + 56], 0  ; SevEsAPJumpTable
> +    je         NoSevEs

Now you are adding back the AmdSev version.
It should be the generic version though.

If you want add the generic version later in the in the patch series (when changing the function prototype to drop sev support and add paging
support) you can temporary call AsmRelocateApLoopStartAmdSev in the generic code path too.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100490): https://edk2.groups.io/g/devel/message/100490
Mute This Topic: https://groups.io/mt/97188907/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-