Re: [edk2-devel] [Patch v5 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP Protocol

["Laszlo Ersek" <lersek@redhat.com>]

Hello Eric,

On 07/10/19 09:56, Dong, Eric wrote:
> V5 changes:
> 1. some small enhancement.
>
> v4 changes:
> 1. Use link list to save the token info.
>
> v3 changes:
> 1. Fix Token clean up too early caused CheckProcedure return error.
>
> v2 changes:
> 1. Remove some duplicated global variables.
> 2. Enhance token design to support multiple task trig for different APs at the same time.
>
> V1 changes:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1937
>
> Add MM Mp Protocol in PiSmmCpuDxeSmm driver.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 570 ++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   |  18 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   | 193 ++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   3 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmMp.c            | 344 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmMp.h            | 286 ++++++++++
>  6 files changed, 1391 insertions(+), 23 deletions(-)
>  create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMp.c
>  create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/SmmMp.h
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 64fb4d6344..f09e2738c3 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -140,7 +140,7 @@ ReleaseAllAPs (
>
>    BspIndex = mSmmMpSyncData->BspIndex;
>    for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> -    if (Index != BspIndex && *(mSmmMpSyncData->CpuData[Index].Present)) {
> +    if (IsPresentAp (Index)) {
>        ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);
>      }
>    }

version 5 again fails to build for me, with the following error message:

> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c: In function 'ReleaseAllAPs':
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c:139:37: error: variable 'BspIndex' set but not used [-Werror=unused-but-set-variable]

With the following incremental patch:

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index f09e2738c30d..ef16997547b8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -136,9 +136,7 @@ ReleaseAllAPs (
>    )
>  {
>    UINTN                             Index;
> -  UINTN                             BspIndex;
>
> -  BspIndex = mSmmMpSyncData->BspIndex;
>    for (Index = mMaxNumberOfCpus; Index-- > 0;) {
>      if (IsPresentAp (Index)) {
>        ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run);

the build completes fine (using GCC48).

If you change nothing on the series other than squashing the above fix,
you can add my:

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

to both patches in the series.

-*-

Important: please do not push the series until the 5-level paging
commits are reverted, and reapplied (with Mike's R-b on the MdePkg
patch).

Thanks!
Laszlo