Re: [edk2-devel] [PATCH v4 3/3] MdeModulePkg: Add new Application/MpServicesTest application

["Ni, Ray" <ray.ni@intel.com>]

Rebecca,
Have you reviewed UefiCpuPkg\Test\UnitTest\EfiMpServicesPpiProtocol? It's based on UnitTestFrameworkPkg for unit testing on MP PPI and MP protocol.
Do you think if the MpServicesTest app and the EfiMpServicesPpiProtocol have some overlapped functionalities?



Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca Cran
> Sent: Sunday, January 8, 2023 12:56 PM
> To: Kun Qin <kuqin12@gmail.com>; devel@edk2.groups.io; Sami Mujawar <sami.mujawar@arm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Leif Lindholm <quic_llindhol@quicinc.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Tiger Liu <TigerLiu@zhaoxin.com>
> Subject: Re: [edk2-devel] [PATCH v4 3/3] MdeModulePkg: Add new Application/MpServicesTest application
> 
> Thanks! I've fixed all the issues you noted and will send out a v5 patch
> in the next couple of days.
> 
> --
> Rebecca Cran
> 
> On 1/6/23 15:33, Kun Qin wrote:
> > Hi Rebecca,
> >
> > Thanks for sharing this patch. I found a few minor issues when running
> > this test app. Please see comments with [KQ] below.
> >
> > Regards,
> > Kun
> >
> > On 1/4/2023 7:37 AM, Rebecca Cran wrote:
> >> The MpServicesTest application exercises the EFI_MP_SERVICES_PROTOCOL.
> >>
> >> usage:
> >>    MpServicesTest -A [-O]
> >>    MpServicesTest -T <Timeout>
> >>    MpServicesTest -S <Processor #>
> >>    MpServicesTest -P
> >>    MpServicesTest -U
> >>    MpServicesTest -W
> >>    MpServicesTest -E <Processor #>
> >>    MpServicesTest -D <Processor #>
> >>    MpServicesTest -h
> >>
> >> Parameter:
> >>    -A:  Run all APs.
> >>    -O:  Run APs sequentially (use with -A).
> >>    -T:  Specify timeout in milliseconds. Default is to wait forever.
> >>    -S:  Specify the single AP to run.
> >>    -P:  Print processor information.
> >>    -U:  Set the specified AP to the Unhealthy status (use with -E/-D).
> >>    -W:  Run WhoAmI and print index of BSP.
> >>    -E:  Enable the specified AP.
> >>    -D:  Disable the specified AP.
> >>    -h:  Print this help page.
> >>
> >> Signed-off-by: Rebecca Cran <rebecca@quicinc.com>
> >> ---
> >>   MdeModulePkg/MdeModulePkg.dsc                              |   2 +
> >>   MdeModulePkg/Application/MpServicesTest/MpServicesTest.inf |  40 ++
> >>   MdeModulePkg/Application/MpServicesTest/Options.h          |  39 ++
> >>   MdeModulePkg/Application/MpServicesTest/MpServicesTest.c   | 560
> >> ++++++++++++++++++++
> >>   MdeModulePkg/Application/MpServicesTest/Options.c          | 164 ++++++
> >>   5 files changed, 805 insertions(+)
> >>
> >> diff --git a/MdeModulePkg/MdeModulePkg.dsc
> >> b/MdeModulePkg/MdeModulePkg.dsc
> >> index 659482ab737f..6992b3ae8db6 100644
> >> --- a/MdeModulePkg/MdeModulePkg.dsc
> >> +++ b/MdeModulePkg/MdeModulePkg.dsc
> >> @@ -166,6 +166,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
> >>
> >> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> >>     DebugLib|MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf
> >>     FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
> >> +  ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> >>   [LibraryClasses.common.MM_STANDALONE]
> >>     HobLib|MdeModulePkg/Library/BaseHobLibNull/BaseHobLibNull.inf
> >> @@ -445,6 +446,7 @@ [Components]
> >>
> >> MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf
> >>   [Components.IA32, Components.X64, Components.AARCH64]
> >> +  MdeModulePkg/Application/MpServicesTest/MpServicesTest.inf
> >>     MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
> >>     MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf
> >>     MdeModulePkg/Universal/EbcDxe/EbcDebuggerConfig.inf
> >> diff --git
> >> a/MdeModulePkg/Application/MpServicesTest/MpServicesTest.inf
> >> b/MdeModulePkg/Application/MpServicesTest/MpServicesTest.inf
> >> new file mode 100644
> >> index 000000000000..07ee4afec845
> >> --- /dev/null
> >> +++ b/MdeModulePkg/Application/MpServicesTest/MpServicesTest.inf
> >> @@ -0,0 +1,40 @@
> >> +## @file
> >> +#  UEFI Application to exercise EFI_MP_SERVICES_PROTOCOL.
> >> +#
> >> +#  Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights
> >> reserved.<BR>
> >> +#
> >> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> >> +#
> >> +##
> >> +
> >> +[Defines]
> >> +  INF_VERSION                    = 1.29
> >> +  BASE_NAME                      = MpServicesTest
> >> +  FILE_GUID                      = 43e9defa-7209-4b0d-b136-cc4ca02cb469
> >> +  MODULE_TYPE                    = UEFI_APPLICATION
> >> +  VERSION_STRING                 = 0.1
> >> +  ENTRY_POINT                    = UefiMain
> >> +
> >> +#
> >> +# The following information is for reference only and not required by
> >> the build tools.
> >> +#
> >> +#  VALID_ARCHITECTURES           = IA32 X64 AARCH64
> >> +#
> >> +
> >> +[Sources]
> >> +  MpServicesTest.c
> >> +  Options.c
> >> +  Options.h
> >> +
> >> +[Packages]
> >> +  MdePkg/MdePkg.dec
> >> +
> >> +[LibraryClasses]
> >> +  BaseLib
> >> +  ShellLib
> >> +  UefiApplicationEntryPoint
> >> +  UefiLib
> >> +
> >> +[Protocols]
> >> +  gEfiMpServiceProtocolGuid    ## CONSUMES
> >> +
> >> diff --git a/MdeModulePkg/Application/MpServicesTest/Options.h
> >> b/MdeModulePkg/Application/MpServicesTest/Options.h
> >> new file mode 100644
> >> index 000000000000..cb28230ab095
> >> --- /dev/null
> >> +++ b/MdeModulePkg/Application/MpServicesTest/Options.h
> >> @@ -0,0 +1,39 @@
> >> +/** @file
> >> +  Options handling code.
> >> +
> >> +  Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights
> >> reserved.<BR>
> >> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> >> +**/
> >> +
> >> +#ifndef MPSERVICESTEST_OPTIONS_H_
> >> +#define MPSERVICESTEST_OPTIONS_H_
> >> +
> >> +#define INFINITE_TIMEOUT  0
> >> +
> >> +typedef struct {
> >> +  UINTN      Timeout;
> >> +  UINTN      ProcessorIndex;
> >> +  BOOLEAN    RunAllAPs;
> >> +  BOOLEAN    RunSingleAP;
> >> +  BOOLEAN    DisableProcessor;
> >> +  BOOLEAN    EnableProcessor;
> >> +  BOOLEAN    SetProcessorHealthy;
> >> +  BOOLEAN    SetProcessorUnhealthy;
> >> +  BOOLEAN    PrintProcessorInformation;
> >> +  BOOLEAN    PrintBspProcessorIndex;
> >> +  BOOLEAN    RunAPsSequentially;
> >> +} MP_SERVICES_TEST_OPTIONS;
> >> +
> >> +/**
> >> +  Parses any arguments provided on the command line.
> >> +
> >> +  @param Options  The arguments structure.
> >> +
> >> +  @return EFI_SUCCESS on success, or an error code.
> >> +**/
> >> +EFI_STATUS
> >> +ParseArguments (
> >> +  MP_SERVICES_TEST_OPTIONS  *Options
> >> +  );
> >> +
> >> +#endif /* MPSERVICESTEST_OPTIONS_H_ */
> >> diff --git a/MdeModulePkg/Application/MpServicesTest/MpServicesTest.c
> >> b/MdeModulePkg/Application/MpServicesTest/MpServicesTest.c
> >> new file mode 100644
> >> index 000000000000..3f3d9752d500
> >> --- /dev/null
> >> +++ b/MdeModulePkg/Application/MpServicesTest/MpServicesTest.c
> >> @@ -0,0 +1,560 @@
> >> +/** @file
> >> +  UEFI Application to exercise EFI_MP_SERVICES_PROTOCOL.
> >> +
> >> +  Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights
> >> reserved.<BR>
> >> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> >> +**/
> >> +
> >> +#include <Uefi.h>
> >> +#include <Library/BaseMemoryLib.h>
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/MemoryAllocationLib.h>
> >> +#include <Library/PrintLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +#include <Library/UefiLib.h>
> >> +#include <Pi/PiMultiPhase.h>
> >> +#include <Protocol/MpService.h>
> >> +
> >> +#include "Options.h"
> >> +
> >> +#define APFUNC_BUFFER_LEN  256
> >> +
> >> +typedef struct {
> >> +  EFI_MP_SERVICES_PROTOCOL    *Mp;
> >> +  CHAR16                      **Buffer;
> >> +} APFUNC_ARG;
> >> +
> >> +/** The procedure to run with the MP Services interface.
> >> +
> >> +  @param Arg The procedure argument.
> >> +
> >> +**/
> >> +STATIC
> >> +VOID
> >> +EFIAPI
> >> +ApFunction (
> >> +  IN OUT VOID  *Arg
> >> +  )
> >> +{
> >> +  APFUNC_ARG  *Param;
> >> +  UINTN       ProcessorId;
> >> +
> >> +  if (Arg != NULL) {
> >> +    Param = Arg;
> >> +
> >> +    Param->Mp->WhoAmI (Param->Mp, &ProcessorId);
> >> +    UnicodeSPrint (Param->Buffer[ProcessorId], APFUNC_BUFFER_LEN,
> >> L"Hello from CPU %ld\n", ProcessorId);
> >> +  }
> >> +}
> >> +
> >> +/**
> >> +  Fetches the number of processors and which processor is the BSP.
> >> +
> >> +  @param Mp  MP Services Protocol.
> >> +  @param NumProcessors Number of processors.
> >> +  @param BspIndex      The index of the BSP.
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +GetProcessorInformation (
> >> +  IN  EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  OUT UINTN                     *NumProcessors,
> >> +  OUT UINTN                     *BspIndex
> >> +  )
> >> +{
> >> +  EFI_STATUS  Status;
> >> +  UINTN       NumEnabledProcessors;
> >> +
> >> +  Status = Mp->GetNumberOfProcessors (Mp, NumProcessors,
> >> &NumEnabledProcessors);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  Status = Mp->WhoAmI (Mp, BspIndex);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/** Displays information returned from MP Services Protocol.
> >> +
> >> +  @param Mp       The MP Services Protocol
> >> +  @param BspIndex On return, contains the index of the BSP.
> >> +
> >> +  @return The number of CPUs in the system.
> >> +
> >> +**/
> >> +STATIC
> >> +UINTN
> >> +PrintProcessorInformation (
> >> +  IN   EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  OUT  UINTN                     *BspIndex
> >> +  )
> >> +{
> >> +  EFI_STATUS                 Status;
> >> +  EFI_PROCESSOR_INFORMATION  CpuInfo;
> >> +  UINTN                      Index;
> >> +  UINTN                      NumCpu;
> >> +  UINTN                      NumEnabledCpu;
> > [KQ] The NumCpu and NumEnabledCpu probably should be initialized to 0s?
> > Otherwise if the
> > GetNumberOfProcessors function somehow fails, the rest of the call will
> > essentially be no-op,
> > instead of running into undefined number of CPUs.
> >> +
> >> +  Status = Mp->GetNumberOfProcessors (Mp, &NumCpu, &NumEnabledCpu);
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"GetNumberOfProcessors failed: %r\n", Status);
> >> +  } else {
> >> +    Print (L"Number of CPUs: %ld, Enabled: %d\n", NumCpu,
> >> NumEnabledCpu);
> >> +  }
> >> +
> >> +  for (Index = 0; Index < NumCpu; Index++) {
> >> +    Status = Mp->GetProcessorInfo (Mp, CPU_V2_EXTENDED_TOPOLOGY |
> >> Index, &CpuInfo);
> >> +    if (EFI_ERROR (Status)) {
> >> +      Print (L"GetProcessorInfo for Processor %d failed: %r\n",
> >> Index, Status);
> >> +    } else {
> >> +      Print (
> >> +        L"Processor %d:\n"
> >> +        L"\tID: %016lx\n"
> >> +        L"\tStatus: %s | ",
> >> +        Index,
> >> +        CpuInfo.ProcessorId,
> >> +        (CpuInfo.StatusFlag & PROCESSOR_AS_BSP_BIT) ? L"BSP" : L"AP"
> >> +        );
> >> +
> >> +      if ((CpuInfo.StatusFlag & PROCESSOR_AS_BSP_BIT) && (BspIndex !=
> >> NULL)) {
> >> +        *BspIndex = Index;
> >> +      }
> >> +
> >> +      Print (L"%s | ", (CpuInfo.StatusFlag & PROCESSOR_ENABLED_BIT) ?
> >> L"Enabled" : L"Disabled");
> >> +      Print (L"%s\n", (CpuInfo.StatusFlag &
> >> PROCESSOR_HEALTH_STATUS_BIT) ? L"Healthy" : L"Faulted");
> >> +
> >> +      Print (
> >> +        L"\tLocation: Package %d, Core %d, Thread %d\n"
> >> +        L"\tExtended Information: Package %d, Module %d, Tile %d, Die
> >> %d, Core %d, Thread %d\n\n",
> >> +        CpuInfo.Location.Package,
> >> +        CpuInfo.Location.Core,
> >> +        CpuInfo.Location.Thread,
> >> +        CpuInfo.ExtendedInformation.Location2.Package,
> >> +        CpuInfo.ExtendedInformation.Location2.Module,
> >> +        CpuInfo.ExtendedInformation.Location2.Tile,
> >> +        CpuInfo.ExtendedInformation.Location2.Die,
> >> +        CpuInfo.ExtendedInformation.Location2.Core,
> >> +        CpuInfo.ExtendedInformation.Location2.Thread
> >> +        );
> >> +    }
> >> +  }
> >> +
> >> +  return NumCpu;
> >> +}
> >> +
> >> +/** Allocates memory in ApArg for the single AP specified.
> >> +
> >> +  @param ApArg          Pointer to the AP argument structure.
> >> +  @param Mp             The MP Services Protocol.
> >> +  @param ProcessorIndex The index of the AP.
> >> +
> >> +  @retval EFI_SUCCESS          Memory was successfully allocated.
> >> +  @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +AllocateApFuncBufferSingleAP (
> >> +  IN APFUNC_ARG                *ApArg,
> >> +  IN EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  IN UINTN                     ProcessorIndex
> >> +  )
> >> +{
> >> +  ApArg->Mp = Mp;
> >> +
> >> +  ApArg->Buffer = AllocateZeroPool ((ProcessorIndex + 1) * sizeof
> >> (VOID *));
> >> +  if (ApArg->Buffer == NULL) {
> >> +    Print (L"Failed to allocate buffer for AP buffer\n");
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  ApArg->Buffer[ProcessorIndex] = AllocateZeroPool (APFUNC_BUFFER_LEN);
> >> +  if (ApArg->Buffer[ProcessorIndex] == NULL) {
> >> +    Print (L"Failed to allocate buffer for AP buffer\n");
> >> +    FreePool (ApArg->Buffer);
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/** Allocates memory in ApArg for all APs.
> >> +
> >> +  @param ApArg   Pointer to the AP argument structure.
> >> +  @param Mp      The MP Services Protocol.
> >> +  @param NumCpus The number of CPUs.
> >> +
> >> +  @retval EFI_SUCCESS          Memory was successfully allocated.
> >> +  @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +AllocateApFuncBufferAllAPs (
> >> +  IN APFUNC_ARG                *ApArg,
> >> +  IN EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  IN UINTN                     NumCpus
> >> +  )
> >> +{
> >> +  UINT32  Index;
> > [KQ] The Index of UINT32 compared to NumCpus of UINTN could make some
> > compilers unhappy.
> >> +
> >> +  ApArg->Mp = Mp;
> >> +
> >> +  ApArg->Buffer = AllocateZeroPool (NumCpus * sizeof (VOID *));
> >> +  if (ApArg->Buffer == NULL) {
> >> +    Print (L"Failed to allocate buffer for AP message\n");
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  for (Index = 0; Index < NumCpus; Index++) {
> >> +    ApArg->Buffer[Index] = AllocateZeroPool (APFUNC_BUFFER_LEN);
> >> +    if (ApArg->Buffer[Index] == NULL) {
> >> +      Print (L"Failed to allocate buffer for AP message\n");
> >> +      for (--Index; Index >= 0; Index++) {
> > [KQ] This Index increment could cause the loop not ending as expected.
> >> +        FreePool (ApArg->Buffer[Index]);
> >> +      }
> >> +
> >> +      FreePool (ApArg->Buffer);
> >> +      return EFI_OUT_OF_RESOURCES;
> >> +    }
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/** Frees memory in ApArg for all APs.
> >> +
> >> +  @param ApArg   Pointer to the AP argument structure.
> >> +  @param NumCpus The number of CPUs.
> >> +
> >> +**/
> >> +STATIC
> >> +VOID
> >> +FreeApFuncBuffer (
> >> +  APFUNC_ARG  *ApArg,
> >> +  UINTN       NumCpus
> >> +  )
> >> +{
> >> +  UINTN  Index;
> >> +
> >> +  for (Index = 0; Index < NumCpus; Index++) {
> >> +    if (ApArg->Buffer[Index] != NULL) {
> >> +      FreePool (ApArg->Buffer[Index]);
> >> +    }
> >> +  }
> >> +
> >> +  FreePool (ApArg->Buffer);
> >> +}
> >> +
> >> +/** Runs a specified AP.
> >> +
> >> +  @param Mp             The MP Services Protocol.
> >> +  @param ProcessorIndex The processor index.
> >> +  @param Timeout        Timeout in milliseconds.
> >> +
> >> +  @return EFI_SUCCESS on success, or an error code.
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +StartupThisAP (
> >> +  IN EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  IN UINTN                     ProcessorIndex,
> >> +  IN UINTN                     Timeout
> >> +  )
> >> +{
> >> +  EFI_STATUS  Status;
> >> +  APFUNC_ARG  ApArg;
> >> +
> >> +  Status = AllocateApFuncBufferSingleAP (&ApArg, Mp, ProcessorIndex);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  Status = AllocateApFuncBufferSingleAP (&ApArg, Mp, ProcessorIndex);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> > [KQ] I guess the above double calls are not intended?
> >> +  Print (
> >> +    L"StartupThisAP on Processor %d with %d%s timeout...",
> >> +    ProcessorIndex,
> >> +    Timeout,
> >> +    (Timeout == INFINITE_TIMEOUT) ? L" (infinite)" : L"ms"
> >> +    );
> >> +  Status = Mp->StartupThisAP (
> >> +                 Mp,
> >> +                 ApFunction,
> >> +                 ProcessorIndex,
> >> +                 NULL,
> >> +                 Timeout * 1000,
> >> +                 &ApArg,
> >> +                 NULL
> >> +                 );
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"failed: %r\n", Status);
> >> +    return Status;
> >> +  } else {
> >> +    Print (L"done.\n");
> >> +    Print (ApArg.Buffer[ProcessorIndex]);
> >> +  }
> >> +
> >> +  FreeApFuncBuffer (&ApArg, ProcessorIndex + 1);
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/** Runs all APs.
> >> +
> >> +  @param Mp                 The MP Services Protocol.
> >> +  @param NumCpus            The number of CPUs in the system.
> >> +  @param Timeout            Timeout in milliseconds.
> >> +  @param RunAPsSequentially Run APs sequentially (FALSE: run
> >> simultaneously)
> >> +
> >> +  @return EFI_SUCCESS on success, or an error code.
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +StartupAllAPs (
> >> +  IN EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  IN UINTN                     NumCpus,
> >> +  IN UINTN                     Timeout,
> >> +  IN BOOLEAN                   RunAPsSequentially
> >> +  )
> >> +{
> >> +  EFI_STATUS  Status;
> >> +  UINTN       Index;
> >> +  APFUNC_ARG  ApArg;
> >> +
> >> +  Status = AllocateApFuncBufferAllAPs (&ApArg, Mp, NumCpus);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  Print (
> >> +    L"Running with SingleThread %s, %u%s timeout...",
> >> +    (RunAPsSequentially) ? L"TRUE" : L"FALSE",
> >> +    Timeout,
> >> +    (Timeout == INFINITE_TIMEOUT) ? L" (infinite)" : L"ms"
> >> +    );
> >> +
> >> +  Status = Mp->StartupAllAPs (
> >> +                 Mp,
> >> +                 ApFunction,
> >> +                 RunAPsSequentially,
> >> +                 NULL,
> >> +                 Timeout * 1000,
> >> +                 &ApArg,
> >> +                 NULL
> >> +                 );
> >> +
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"failed: %r\n", Status);
> >> +
> >> +    return Status;
> >> +  } else {
> >> +    Print (L"done.\n");
> >> +
> >> +    for (Index = 0; Index < NumCpus; Index++) {
> >> +      Print (ApArg.Buffer[Index]);
> >> +    }
> >> +  }
> >> +
> >> +  FreeApFuncBuffer (&ApArg, NumCpus);
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/**
> >> +  Enables the specified AP.
> >> +
> >> +  @param Mp               The MP Services Protocol.
> >> +  @param ProcessorIndex   The processor to enable.
> >> +  @param ProcessorHealthy The health status of the processor.
> >> +
> >> +  @return EFI_SUCCESS on success, or an error code.
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +EnableAP (
> >> +  IN EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  UINTN                        ProcessorIndex,
> >> +  BOOLEAN                      ProcessorHealthy
> > [KQ] These parameters should have the "IN" attributes?
> >> +  )
> >> +{
> >> +  EFI_STATUS  Status;
> >> +  UINT32      HealthFlag;
> >> +
> >> +  if (ProcessorHealthy) {
> >> +    Print (L"Enabling Processor %d with HealthFlag healthy...",
> >> ProcessorIndex);
> >> +    HealthFlag = PROCESSOR_HEALTH_STATUS_BIT;
> >> +  } else {
> >> +    Print (L"Enabling Processor %d with HealthFlag faulted...",
> >> ProcessorIndex);
> >> +    HealthFlag = 0;
> >> +  }
> >> +
> >> +  Status = Mp->EnableDisableAP (Mp, ProcessorIndex, TRUE, &HealthFlag);
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"failed: %r\n", Status);
> >> +    return Status;
> >> +  } else {
> >> +    Print (L"done.\n");
> >> +  }
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +/**
> >> +  Disables the specified AP.
> >> +
> >> +  @param Mp               The MP Services Protocol.
> >> +  @param ProcessorIndex   The processor to disable.
> >> +  @param ProcessorHealthy The health status of the processor.
> >> +
> >> +  @return EFI_SUCCESS on success, or an error code.
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +DisableAP (
> >> +  IN EFI_MP_SERVICES_PROTOCOL  *Mp,
> >> +  UINTN                        ProcessorIndex,
> >> +  BOOLEAN                      ProcessorHealthy
> > [KQ] These parameters should have the "IN" attributes?
> >> +  )
> >> +{
> >> +  EFI_STATUS  Status;
> >> +  UINT32      HealthFlag;
> >> +
> >> +  if (ProcessorHealthy) {
> >> +    Print (L"Disabling Processor %d with HealthFlag healthy...",
> >> ProcessorIndex);
> >> +    HealthFlag = PROCESSOR_HEALTH_STATUS_BIT;
> >> +  } else {
> >> +    Print (L"Disabling Processor %d with HealthFlag faulted...",
> >> ProcessorIndex);
> >> +    HealthFlag = 0;
> >> +  }
> >> +
> >> +  Status = Mp->EnableDisableAP (Mp, ProcessorIndex, FALSE, &HealthFlag);
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"failed: %r\n", Status);
> >> +    return Status;
> >> +  } else {
> >> +    Print (L"done.\n");
> >> +  }
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +/**
> >> +  The user Entry Point for Application. The user code starts with
> >> this function
> >> +  as the real entry point for the application.
> >> +
> >> +  @param[in] ImageHandle    The firmware allocated handle for the EFI
> >> image.
> >> +  @param[in] SystemTable    A pointer to the EFI System Table.
> >> +
> >> +  @retval EFI_SUCCESS       The entry point is executed successfully.
> >> +  @retval other             Some error occurs when executing this
> >> entry point.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +UefiMain (
> >> +  IN EFI_HANDLE        ImageHandle,
> >> +  IN EFI_SYSTEM_TABLE  *SystemTable
> >> +  )
> >> +{
> >> +  EFI_STATUS                Status;
> >> +  EFI_MP_SERVICES_PROTOCOL  *Mp;
> >> +  UINTN                     BspIndex;
> >> +  UINTN                     CpuIndex;
> >> +  UINTN                     NumCpus;
> >> +  BOOLEAN                   ProcessorHealthy;
> >> +  MP_SERVICES_TEST_OPTIONS  Options;
> >> +
> >> +  BspIndex = 0;
> >> +
> >> +  Status = gBS->LocateProtocol (
> >> +                  &gEfiMpServiceProtocolGuid,
> >> +                  NULL,
> >> +                  (VOID **)&Mp
> >> +                  );
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"Failed to locate EFI_MP_SERVICES_PROTOCOL (%r). Not
> >> installed on platform?\n", Status);
> >> +    return EFI_NOT_FOUND;
> >> +  }
> >> +
> >> +  Status = ParseArguments (&Options);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  Status = GetProcessorInformation (Mp, &NumCpus, &BspIndex);
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"Error: Failed to fetch processor information.\n");
> >> +    return Status;
> >> +  }
> >> +
> >> +  if (Options.PrintBspProcessorIndex) {
> >> +    Status = Mp->WhoAmI (Mp, &CpuIndex);
> >> +    if (EFI_ERROR (Status)) {
> >> +      Print (L"WhoAmI failed: %r\n", Status);
> >> +      return Status;
> >> +    } else {
> >> +      Print (L"BSP: %016lx\n", CpuIndex);
> >> +    }
> >> +  }
> >> +
> >> +  if (Options.PrintProcessorInformation) {
> >> +    NumCpus = PrintProcessorInformation (Mp, &BspIndex);
> >> +    if (NumCpus < 2) {
> >> +      Print (L"Error: Uniprocessor system found.\n");
> >> +      return EFI_INVALID_PARAMETER;
> >> +    }
> >> +  }
> >> +
> >> +  if (Options.RunSingleAP) {
> >> +    Status = StartupThisAP (
> >> +               Mp,
> >> +               Options.ProcessorIndex,
> >> +               Options.Timeout
> >> +               );
> >> +    if (EFI_ERROR (Status)) {
> >> +      return Status;
> >> +    }
> >> +  }
> >> +
> >> +  if (Options.RunAllAPs) {
> >> +    Status = StartupAllAPs (Mp, NumCpus, Options.Timeout,
> >> Options.RunAPsSequentially);
> >> +    if (EFI_ERROR (Status)) {
> >> +      return Status;
> >> +    }
> >> +  }
> >> +
> >> +  if (Options.EnableProcessor) {
> >> +    ProcessorHealthy = TRUE;
> >> +    if (Options.SetProcessorUnhealthy) {
> >> +      ProcessorHealthy = FALSE;
> >> +    }
> >> +
> >> +    Status = EnableAP (Mp, Options.ProcessorIndex, ProcessorHealthy);
> >> +    if (EFI_ERROR (Status)) {
> >> +      return Status;
> >> +    }
> >> +  }
> >> +
> >> +  if (Options.DisableProcessor) {
> >> +    ProcessorHealthy = TRUE;
> >> +    if (Options.SetProcessorUnhealthy) {
> >> +      ProcessorHealthy = FALSE;
> >> +    }
> >> +
> >> +    Status = DisableAP (Mp, Options.ProcessorIndex, ProcessorHealthy);
> >> +    if (EFI_ERROR (Status)) {
> >> +      return Status;
> >> +    }
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> diff --git a/MdeModulePkg/Application/MpServicesTest/Options.c
> >> b/MdeModulePkg/Application/MpServicesTest/Options.c
> >> new file mode 100644
> >> index 000000000000..e820c061e1ec
> >> --- /dev/null
> >> +++ b/MdeModulePkg/Application/MpServicesTest/Options.c
> >> @@ -0,0 +1,164 @@
> >> +/** @file
> >> +  Options handling code.
> >> +
> >> +  Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights
> >> reserved.<BR>
> >> +  Copyright (c) 2010-2019  Finnbarr P. Murphy.   All rights
> >> reserved.<BR>
> >> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> >> +**/
> >> +
> >> +#include <Uefi.h>
> >> +#include <Library/BaseMemoryLib.h>
> >> +#include <Protocol/ShellParameters.h>
> >> +#include <Library/BaseLib.h>
> >> +#include <Library/UefiLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +
> >> +#include "Options.h"
> >> +
> >> +STATIC UINTN   Argc;
> >> +STATIC CHAR16  **Argv;
> >> +
> >> +/**
> >> +
> >> +  This function provides argc and argv.
> >> +
> >> +  @return Status
> >> +**/
> >> +EFI_STATUS
> >> +GetArg (
> >> +  VOID
> >> +  )
> >> +{
> >> +  EFI_STATUS                     Status;
> >> +  EFI_SHELL_PARAMETERS_PROTOCOL  *ShellParameters;
> >> +
> >> +  Status = gBS->HandleProtocol (
> >> +                  gImageHandle,
> >> +                  &gEfiShellParametersProtocolGuid,
> >> +                  (VOID **)&ShellParameters
> >> +                  );
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  Argc = ShellParameters->Argc;
> >> +  Argv = ShellParameters->Argv;
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/**
> >> +  Print app usage.
> >> +**/
> >> +STATIC
> >> +VOID
> >> +PrintUsage (
> >> +  VOID
> >> +  )
> >> +{
> >> +  Print (L"MpServicesTest:  usage\n");
> >> +  Print (L"  MpServicesTest -A [-O]\n");
> >> +  Print (L"  MpServicesTest -T <Timeout>\n");
> >> +  Print (L"  MpServicesTest -S <Processor #>\n");
> >> +  Print (L"  MpServicesTest -P\n");
> >> +  Print (L"  MpServicesTest -U\n");
> >> +  Print (L"  MpServicesTest -W\n");
> >> +  Print (L"  MpServicesTest -E <Processor #>\n");
> >> +  Print (L"  MpServicesTest -D <Processor #>\n");
> >> +  Print (L"  MpServicesTest -h\n");
> >> +  Print (L"Parameter:\n");
> >> +  Print (L"  -A:  Run all APs.\n");
> >> +  Print (L"  -O:  Run APs sequentially (use with -A).\n");
> >> +  Print (L"  -T:  Specify timeout in milliseconds. Default is to wait
> >> forever.\n");
> >> +  Print (L"  -S:  Specify the single AP to run.\n");
> >> +  Print (L"  -P:  Print processor information.\n");
> >> +  Print (L"  -U:  Set the specified AP to the Unhealthy status (use
> >> with -E/-D).\n");
> >> +  Print (L"  -W:  Run WhoAmI and print index of BSP.\n");
> >> +  Print (L"  -E:  Enable the specified AP.\n");
> >> +  Print (L"  -D:  Disable the specified AP.\n");
> >> +  Print (L"  -h:  Print this help page.\n");
> >> +}
> >> +
> >> +/**
> >> +  Parses any arguments provided on the command line.
> >> +
> >> +  @param Options  The arguments structure.
> >> +
> >> +  @return EFI_SUCCESS on success, or an error code.
> >> +**/
> >> +EFI_STATUS
> >> +ParseArguments (
> >> +  MP_SERVICES_TEST_OPTIONS  *Options
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status;
> >> +  UINT32        ArgIndex;
> > [KQ] Similar to the other comment, ArgIndex is of UINT32 is compared to
> > Argc of UINTN could make some compilers unhappy.
> >> +  CONST CHAR16  *Argument;
> >> +  BOOLEAN       NeedsValue;
> >> +  UINTN         *Value;
> >> +
> >> +  Status = GetArg ();
> >> +  if (EFI_ERROR (Status)) {
> >> +    Print (L"Please use the UEFI Shell to run this application!\n",
> >> Status);
> >> +    return Status;
> >> +  }
> >> +
> >> +  if (Argc == 1) {
> >> +    PrintUsage ();
> >> +  }
> >> +
> >> +  ZeroMem (Options, sizeof (MP_SERVICES_TEST_OPTIONS));
> >> +
> >> +  for (ArgIndex = 1; ArgIndex < Argc; ArgIndex++) {
> >> +    Argument   = Argv[ArgIndex];
> >> +    NeedsValue = FALSE;
> >> +
> >> +    if (StrCmp (Argument, L"-A") == 0) {
> >> +      Options->RunAllAPs = TRUE;
> >> +    } else if (StrCmp (Argument, L"-O") == 0) {
> >> +      Options->RunAPsSequentially = TRUE;
> >> +    } else if (StrCmp (Argument, L"-T") == 0) {
> >> +      NeedsValue = TRUE;
> >> +      Value      = &Options->Timeout;
> >> +    } else if (StrCmp (Argument, L"-S") == 0) {
> >> +      Options->RunSingleAP = TRUE;
> >> +      NeedsValue           = TRUE;
> >> +      Value                = &Options->ProcessorIndex;
> >> +    } else if (StrCmp (Argument, L"-P") == 0) {
> >> +      Options->PrintProcessorInformation = TRUE;
> >> +    } else if (StrCmp (Argument, L"-U") == 0) {
> >> +      Options->SetProcessorUnhealthy = TRUE;
> >> +    } else if (StrCmp (Argument, L"-W") == 0) {
> >> +      Options->PrintBspProcessorIndex = TRUE;
> >> +    } else if (StrCmp (Argument, L"-E") == 0) {
> >> +      Options->EnableProcessor = TRUE;
> >> +      NeedsValue               = TRUE;
> >> +      Value                    = &Options->ProcessorIndex;
> >> +    } else if (StrCmp (Argument, L"-D") == 0) {
> >> +      Options->DisableProcessor = TRUE;
> >> +      NeedsValue                = TRUE;
> >> +      Value                     = &Options->ProcessorIndex;
> >> +    } else {
> >> +      PrintUsage ();
> >> +      ZeroMem (Options, sizeof (MP_SERVICES_TEST_OPTIONS));
> >> +      return EFI_SUCCESS;
> >> +    }
> >> +
> >> +    if (NeedsValue) {
> >> +      if ((ArgIndex + 1) < Argc) {
> >> +        Status = StrDecimalToUintnS (Argv[ArgIndex + 1], NULL, Value);
> >> +        if (EFI_ERROR (Status)) {
> >> +          Print (L"Error: option value must be a positive integer.\n");
> >> +          PrintUsage ();
> >> +          return EFI_INVALID_PARAMETER;
> >> +        }
> >> +
> >> +        ArgIndex++;
> >> +      } else {
> >> +        PrintUsage ();
> >> +        return EFI_INVALID_PARAMETER;
> >> +      }
> >> +    }
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> 
> 
> 
>