Re: [Patch V2] UefiCpuPkg/MpInitLib: Remove redundant parameter.

[Laszlo Ersek <lersek@redhat.com>]

Hi Eric,

On 06/29/18 05:20, Eric Dong wrote:
> Parameter StartCount duplicates with RunningCount. After this change,
> RunningCount means the running AP count.
>
> V2 changes: Remove volatile for RunningCount.
>
> Done Test:
> 1.PI SCT Test
> 2.Boot OS / S3
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jeff Fan <vanjeff_919@hotmail.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +--
>  2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 3945771764..52c9679099 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1400,7 +1400,7 @@ CheckAllAPs (
>      // value of state after setting the it to CpuStateFinished, so BSP can safely make use of its value.
>      //
>      if (GetApState(CpuData) != CpuStateBusy) {
> -      CpuMpData->RunningCount ++;
> +      CpuMpData->RunningCount --;
>        CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
>
>        //
> @@ -1425,7 +1425,7 @@ CheckAllAPs (
>    //
>    // If all APs finish, return EFI_SUCCESS.
>    //
> -  if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> +  if (CpuMpData->RunningCount == 0) {
>      return EFI_SUCCESS;
>    }
>
> @@ -1442,7 +1442,7 @@ CheckAllAPs (
>      //
>      if (CpuMpData->FailedCpuList != NULL) {
>        *CpuMpData->FailedCpuList =
> -         AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 1) * sizeof (UINTN));
> +         AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN));
>        ASSERT (*CpuMpData->FailedCpuList != NULL);
>      }
>      ListIndex = 0;
> @@ -2121,7 +2121,7 @@ StartupAllAPsWorker (
>      return EFI_NOT_STARTED;
>    }
>
> -  CpuMpData->StartCount = 0;
> +  CpuMpData->RunningCount = 0;
>    for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; ProcessorNumber++) {
>      CpuData = &CpuMpData->CpuData[ProcessorNumber];
>      CpuData->Waiting = FALSE;
> @@ -2131,7 +2131,7 @@ StartupAllAPsWorker (
>          // Mark this processor as responsible for current calling.
>          //
>          CpuData->Waiting = TRUE;
> -        CpuMpData->StartCount++;
> +        CpuMpData->RunningCount++;
>        }
>      }
>    }
> @@ -2140,7 +2140,6 @@ StartupAllAPsWorker (
>    CpuMpData->ProcArguments = ProcedureArgument;
>    CpuMpData->SingleThread  = SingleThread;
>    CpuMpData->FinishedCount = 0;
> -  CpuMpData->RunningCount  = 0;
>    CpuMpData->FailedCpuList = FailedCpuList;
>    CpuMpData->ExpectedTime  = CalculateTimeout (
>                                 TimeoutInMicroseconds,
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 90c09fb8fb..ad62acf766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -210,9 +210,8 @@ struct _CPU_MP_DATA {
>    UINTN                          BackupBuffer;
>    UINTN                          BackupBufferSize;
>
> -  volatile UINT32                StartCount;
>    volatile UINT32                FinishedCount;
> -  volatile UINT32                RunningCount;
> +  UINT32                         RunningCount;
>    BOOLEAN                        SingleThread;
>    EFI_AP_PROCEDURE               Procedure;
>    VOID                           *ProcArguments;
>

I got confused by the way you sent out this patch.

First I thought that you meant it separately (stand-alone). I tried to
test it, but it didn't apply. Also my intent was to ask you whether you
wanted to send a new version of

  [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP.


However, upon seeing that this patch wouldn't apply, I'm now thinking
that you would like to preserve

  [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP.

without any changes, and your intent is to only update

  [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Remove redundant parameter.

to version 2.


In such cases, please do not post an independent patch. Instead, pick
one of the following:

- Repost the entire series as v2, and mark in the cover letter that
  patch #1 is unchanged from the v1 posting.

- Alternatively, post the patch in response to the *original* v1 thread
  (using --in-reply-to= with git-send-email), and use the subject

  [edk2] [Patch v2 2/2] UefiCpuPkg/MpInitLib: Remove redundant
                        parameter.

Either of these approaches will let reviewers know that you intend the
two patches to go together (and in what order), and that only patch #2
has been updated.


So... Assuming this was indeed your intent, I applied the following two
patches locally, for testing:

  [edk2] [Patch 1_2] UefiCpuPkg_MpInitLib: Not use disabled AP.
  Message-Id: <20180628112920.5296-1-eric.dong@intel.com>
  http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com

  [edk2] [Patch V2] UefiCpuPkg_MpInitLib: Remove redundant parameter.
  Message-Id: <20180629032047.6340-1-eric.dong@intel.com>
  http://mid.mail-archive.com/20180629032047.6340-1-eric.dong@intel.com
  (i.e., this patch)

I tested the following three scenarios with QEMU/KVM, using normal boot
and S3, and 8 virtual CPUs (1 socket, 4 cores, 2 threads -- same
topology as my physical laptop CPU):

(1) i440fx machine type, X64 build, without SMM
(2) q35 machine type, IA32 build, with SMM
(3) q35 machine type, IA32X64 build, with SMM

The guest OS was Fedora in every case.

The series keeps test case (1) functional.

The series breaks test cases (2) and (3) however; normal boot for each
(S3 is not possible to attempt). The symptom is that, for both cases (2)
and (3), the ExitBootServices() handlers are invoked, but the following
message is never logged:

    MpInitChangeApLoopCallback() done!

and the boot process gets stuck.

With more context, a before/after log file diff, for case (2):

>  VirtioScsiExitBoot: Context=0x7DF3F010
>  SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
> -MpInitChangeApLoopCallback() done!

With more context, a before/after log file diff, for case (3):

>  VirtioScsiExitBoot: Context=0x7DBD4398
>  VirtioRngExitBoot: Context=0x7DC47318
>  SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
> -MpInitChangeApLoopCallback() done!

I guess the patch series breaks something in
MpInitChangeApLoopCallback() when the SMM driver stack is included in
the build.

... After repeated testing, the boot succeeds *sometimes* (rarely). In
most cases, the boot fails as described above.

Thanks,
Laszlo