Hi All,
Do you think it worth or not to also update MpFuncs.nasm in
Edk2\UefiCpuPkg\PiSmmCpuDxeSmm?
Thanks,
Star
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, February 4, 2021 10:59 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <ler...@redhat.com>; Dong, Eric <eric.d...@intel.com>;
> Kumar, Rahul1 <rahul1.ku...@intel.com>
> Subject: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Use XADD to avoid
> lock acquire/release
>
> When AP firstly wakes up, MpFuncs.nasm contains below logic to assign an
> unique ApIndex to each AP according to who comes first:
> ---ASM---
> TestLock:
> xchg [edi], eax
> cmp eax, NotVacantFlag
> jz TestLock
>
> mov ecx, esi
> add ecx, ApIndexLocation
> inc dword [ecx]
> mov ebx, [ecx]
>
> Releaselock:
> mov eax, VacantFlag
> xchg [edi], eax
> ---ASM END---
>
> "lock inc" cannot be used to increase ApIndex because not only the global
> ApIndex should be increased, but also the result should be stored to a local
> general purpose register EBX.
>
> This patch learns from the NASM implementation of
> InternalSyncIncrement() to use "XADD" instruction which can increase the
> global ApIndex and store the original ApIndex to EBX in one instruction.
>
> With this patch, OVMF when running in a 255 threads QEMU spends about
> one second to wakeup all APs. Original implementation needs more than
> 10 seconds.
>
> Signed-off-by: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Rahul1 Kumar <rahul1.ku...@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 4 ----
> .../Library/MpInitLib/Ia32/MpFuncs.nasm | 21 +++++--------------
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 +--
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +--
> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 4 ----
> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++------------
> 6 files changed, 11 insertions(+), 42 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> index 244c1e72b7..5f1f0351d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> @@ -12,16 +12,12 @@
> ;
> ;-------------------------------------------------------------------------------
> -
> VacantFlag equ 00h-NotVacantFlag
> equ 0ffh-
> CPU_SWITCH_STATE_IDLE equ 0 CPU_SWITCH_STATE_STORED
> equ 1 CPU_SWITCH_STATE_LOADED equ 2
> MP_CPU_EXCHANGE_INFO_OFFSET equ (SwitchToRealProcEnd -
> RendezvousFunnelProcStart) struc MP_CPU_EXCHANGE_INFO- .Lock:
> resd 1 .StackStart: resd 1 .StackSize: resd 1
> .CFunction:
> resd 1diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 908c2eb447..b7267393db 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -122,23 +122,12 @@ SkipEnableExecuteDisable:
> ; AP init mov edi, esi- add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET- mov eax, NotVacantFlag--
> TestLock:- xchg [edi], eax- cmp eax, NotVacantFlag- jz
> TestLock-- mov ecx, esi- add ecx,
> MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.ApIndex-
> inc dword [ecx]- mov ebx, [ecx]--Releaselock:- mov
> eax,
> VacantFlag- xchg [edi], eax+ add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.ApIndex+ mov ebx, 1+ lock xadd [edi],
> ebx ; EBX = ApIndex+++ inc ebx
> ; EBX is
> CpuNumber + ; program stack mov edi, esi add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.StackSize
> mov eax, [edi]diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 8b1f7f84ba..32a3951742 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1,7 +1,7 @@
> /** @file CPU MP Initialize Library common functions. - Copyright (c)
> 2016 -
> 2020, Intel Corporation. All rights reserved.<BR>+ Copyright (c) 2016 - 2021,
> Intel Corporation. All rights reserved.<BR> Copyright (c) 2020, AMD Inc. All
> rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent@@ -
> 1012,7 +1012,6 @@ FillExchangeInfoData (
> IA32_CR4 Cr4; ExchangeInfo =
> CpuMpData-
> >MpCpuExchangeInfo;- ExchangeInfo->Lock = 0; ExchangeInfo-
> >StackStart = CpuMpData->Buffer; ExchangeInfo->StackSize =
> CpuMpData->CpuApStackSize; ExchangeInfo->BufferStart = CpuMpData-
> >WakeupBuffer;diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 02652eaae1..0bd60388b1 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -1,7 +1,7 @@
> /** @file Common header file for MP Initialize Library. - Copyright (c)
> 2016
> - 2020, Intel Corporation. All rights reserved.<BR>+ Copyright (c) 2016 -
> 2021,
> Intel Corporation. All rights reserved.<BR> Copyright (c) 2020, AMD Inc. All
> rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent@@ -
> 190,7 +190,6 @@ typedef struct _CPU_MP_DATA CPU_MP_DATA;
> // into this structure are used in assembly code in this module // typedef
> struct {- UINTN Lock; UINTN StackStart;
> UINTN
> StackSize; UINTN CFunction;diff --git
> a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> index 3974330991..32e9a262bc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> @@ -12,16 +12,12 @@
> ;
> ;-------------------------------------------------------------------------------
> -
> VacantFlag equ 00h-NotVacantFlag
> equ 0ffh-
> CPU_SWITCH_STATE_IDLE equ 0 CPU_SWITCH_STATE_STORED
> equ 1 CPU_SWITCH_STATE_LOADED equ 2
> MP_CPU_EXCHANGE_INFO_OFFSET equ (SwitchToRealProcEnd -
> RendezvousFunnelProcStart) struc MP_CPU_EXCHANGE_INFO- .Lock:
> resq 1 .StackStart: resq 1 .StackSize: resq 1
> .CFunction:
> resq 1diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 423beb2cca..383b4974f8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -158,21 +158,11 @@ LongModeStart:
> ; AP init mov edi, esi- add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.Lock-
> mov rax, NotVacantFlag+ add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.ApIndex+ mov ebx, 1+ lock xadd [edi],
> ebx ; EBX = ApIndex+++ inc ebx
> ; EBX is
> CpuNumber -TestLock:- xchg qword [edi], rax- cmp rax,
> NotVacantFlag- jz TestLock-- lea ecx, [esi +
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.ApIndex]- inc dword [ecx]- mov ebx,
> [ecx]--Releaselock:- mov rax, VacantFlag- xchg qword
> [edi], rax ;
> program stack mov edi, esi add edi,
> MP_CPU_EXCHANGE_INFO_OFFSET +
> MP_CPU_EXCHANGE_INFO.StackSize--
> 2.27.0.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#71132): https://edk2.groups.io/g/devel/message/71132
> Mute This Topic: https://groups.io/mt/80372087/1779220
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [star.z...@intel.com] -
> =-=-=-=-=-=
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71241): https://edk2.groups.io/g/devel/message/71241
Mute This Topic: https://groups.io/mt/80372087/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
