Re: [edk2-devel] [PATCH v3 1/4] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release

["Michael D Kinney" <michael.d.kinney@intel.com>]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, February 9, 2021 6:17 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: [edk2-devel] [PATCH v3 1/4] 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.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  .../Library/MpInitLib/Ia32/MpFuncs.nasm       | 20 ++++++-------------
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++++-----------
>  2 files changed, 12 insertions(+), 26 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 7e81d24aa6..2eaddc93bc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -1,5 +1,5 @@
>  ;------------------------------------------------------------------------------ ;
> 
> -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
> 
> +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  ;
> 
>  ; Module Name:
> 
> @@ -125,19 +125,11 @@ SkipEnableExecuteDisable:
>      add        edi, LockLocation
> 
>      mov        eax, NotVacantFlag
> 
> 
> 
> -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
> 
> +    mov        edi, esi
> 
> +    add        edi, ApIndexLocation
> 
> +    mov        ebx, 1
> 
> +    lock xadd  dword [edi], ebx                 ; EBX = ApIndex++
> 
> +    inc        ebx                              ; EBX is CpuNumber
> 
> 
> 
>      mov        edi, esi
> 
>      add        edi, StackSizeLocation
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index aecfd07bc0..5b588f2dcb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -1,5 +1,5 @@
>  ;------------------------------------------------------------------------------ ;
> 
> -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
> 
> +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  ;
> 
>  ; Module Name:
> 
> @@ -161,18 +161,12 @@ LongModeStart:
>      add        edi, LockLocation
> 
>      mov        rax, NotVacantFlag
> 
> 
> 
> -TestLock:
> 
> -    xchg       qword [edi], rax
> 
> -    cmp        rax, NotVacantFlag
> 
> -    jz         TestLock
> 
> -
> 
> -    lea        ecx, [esi + ApIndexLocation]
> 
> -    inc        dword [ecx]
> 
> -    mov        ebx, [ecx]
> 
> +    mov        edi, esi
> 
> +    add        edi, ApIndexLocation
> 
> +    mov        ebx, 1
> 
> +    lock xadd  dword [edi], ebx                 ; EBX = ApIndex++
> 
> +    inc        ebx                              ; EBX is CpuNumber
> 
> 
> 
> -Releaselock:
> 
> -    mov        rax, VacantFlag
> 
> -    xchg       qword [edi], rax
> 
>      ; program stack
> 
>      mov        edi, esi
> 
>      add        edi, StackSizeLocation
> 
> --
> 2.27.0.windows.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#71517): https://edk2.groups.io/g/devel/message/71517
> Mute This Topic: https://groups.io/mt/80504936/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>