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

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

Hi Eric,

I cannot apply this patch for testing, because it is corrupt. Git
complains about it (and it's justified):

On 07/02/19 09:37, Dong, Eric wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1937
> 
> 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:
> 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        | 555 ++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   |  11 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   | 160 +++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   3 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmMp.c            | 372 +++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmMp.h            | 286 ++++++++++
>  6 files changed, 1370 insertions(+), 17 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..76bcee7133 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -22,6 +22,7 @@ UINTN                                       mSemaphoreSize;
>  SPIN_LOCK                                   *mPFLock = NULL;
>  SMM_CPU_SYNC_MODE                           mCpuSmmSyncMode;
>  BOOLEAN                                     mMachineCheckSupported = FALSE;
> +SPIN_LOCK                                   **mApTokenLock;
>  

Please note the empty line above. In the patch, the line consists of a
single space character (before the line terminator), expressing that the
line is preseved from the context. So this is a *valid* patch line.

Now compare:

>  /**
>    Performs an atomic compare exchange operation to get semaphore.
> @@ -146,6 +147,45 @@ ReleaseAllAPs (
>    }
>  }
>  
> +/**
> +  Wheck whether task has been finished by all APs.
> +
> +  @param       BlockMode   Whether did it in block mode or non-block mode.
> +
> +  @retval      TRUE        Task has been finished by all APs.
> +  @retval      FALSE       Task not has been finished by all APs.
> +
> +**/
> +BOOLEAN
> +IsTaskFinishInAllAPs (
> +  IN BOOLEAN                        BlockMode
> +  )
> +{
> +  UINTN                             Index;
> +
> +  for (Index = mMaxNumberOfCpus; Index-- > 0;) {
> +    //
> +    // Ignore BSP and APs which not call in SMM.
> +    //
> +    if ((Index == gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu) || (!*(mSmmMpSyncData->CpuData[Index].Present))) {
> +      continue;
> +    }
> +
> +    if (BlockMode) {
> +      AcquireSpinLock(mSmmMpSyncData->CpuData[Index].Busy);
> 

The above line is malformed. In the patch, the line has *zero*
characters before the line terminator. That's invalid: the first
character should be a space (preserved context), plus sign (new line) or
minus sign (line being removed).

I don't know how this patch was generated, but it doesn't conform to the
expected format. Please both repost the series and push it to a topic
branch in your repo.

I had no problems applying the first patch in the series; what's more,
in patch #2, the changes for the other files apply just fine.

Thanks
Laszlo