On 02/05/21 08:58, Ni, Ray wrote:
> When AP firstly wakes up, MpFuncs.nasm contains below logic to assign
> an unique ApIndex to each AP according to who comes first:
> ---NASM---
> mov edi, esi
> add edi, MP_CPU_EXCHANGE_INFO_FIELD (Lock)
> mov eax, NotVacantFlag
>
> TestLock:
> xchg [edi], eax
> cmp eax, NotVacantFlag
> jz TestLock
>
> mov ecx, esi
> add ecx, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
> inc dword [ecx]
> mov ebx, [ecx]
>
> Releaselock:
> mov eax, VacantFlag
> xchg [edi], eax
> ---NASM END---
>
> "LOCK INC" cannot be used to increase MP_CPU_EXCHANGE_INFO.ApIndex
> because not only the MP_CPU_EXCHANGE_INFO.ApIndex should be
> increased, but also the result should be stored to a thread 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>
> ---
> .../Library/MpInitLib/Ia32/MpFuncs.nasm | 20 ++++---------------
> UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 4 ----
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 1 -
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +--
> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++-------------
> 5 files changed, 9 insertions(+), 37 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 2f1b102717..7bd2415670 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -122,22 +122,10 @@ SkipEnableExecuteDisable:
>
> ; AP init
> mov edi, esi
> - add edi, MP_CPU_EXCHANGE_INFO_FIELD (Lock)
> - mov eax, NotVacantFlag
> -
> -TestLock:
> - xchg [edi], eax
> - cmp eax, NotVacantFlag
> - jz TestLock
> -
> - mov ecx, esi
> - add ecx, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
> - inc dword [ecx]
> - mov ebx, [ecx]
> -
> -Releaselock:
> - mov eax, VacantFlag
> - xchg [edi], eax
> + add edi, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
> + mov ebx, 1
> + lock xadd dword [edi], ebx ; EBX = ApIndex++
> + inc ebx ; EBX is CpuNumber
>
> mov edi, esi
> add edi, MP_CPU_EXCHANGE_INFO_FIELD (StackSize)
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> index 46c2b5c116..2e9368a374 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> @@ -13,9 +13,6 @@
>
> ;-------------------------------------------------------------------------------
> %include "Nasm.inc"
>
> -VacantFlag equ 00h
> -NotVacantFlag equ 0ffh
> -
> CPU_SWITCH_STATE_IDLE equ 0
> CPU_SWITCH_STATE_STORED equ 1
> CPU_SWITCH_STATE_LOADED equ 2
> @@ -72,7 +69,6 @@ endstruc
> ; Equivalent NASM structure of MP_CPU_EXCHANGE_INFO
> ;
> struc MP_CPU_EXCHANGE_INFO
> - .Lock: CTYPE_UINTN 1
> .StackStart: CTYPE_UINTN 1
> .StackSize: CTYPE_UINTN 1
> .CFunction: CTYPE_UINTN 1
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 2568986d8c..5040053dad 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1006,7 +1006,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/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index bf7faaf60b..50df802d1f 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_FIELD (Lock)
> - mov rax, NotVacantFlag
> + add edi, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)
> + mov ebx, 1
> + lock xadd dword [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_FIELD (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_FIELD (StackSize)
>
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71365): https://edk2.groups.io/g/devel/message/71365
Mute This Topic: https://groups.io/mt/80401294/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
