From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by groups.io with SMTP; Thu, 11 Jul 2019 10:53:56 -0700
Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id 224B6308424C;
	Thu, 11 Jul 2019 17:53:56 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-231.ams2.redhat.com [10.36.117.231])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 450D9600CD;
	Thu, 11 Jul 2019 17:53:55 +0000 (UTC)
Subject: Re: [edk2-devel] [Patch v5 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP Protocol
To: devel@edk2.groups.io, eric.dong@intel.com
Cc: Ray Ni <ray.ni@intel.com>
References: <20190710075624.16420-1-eric.dong@intel.com>
 <20190710075624.16420-3-eric.dong@intel.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <bddebd39-0ce1-30eb-1168-ba8cba92a3a2@redhat.com>
Date: Thu, 11 Jul 2019 19:53:54 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20190710075624.16420-3-eric.dong@intel.com>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Thu, 11 Jul 2019 17:53:56 +0000 (UTC)
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

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