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

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

I've taken an interest in this utility, because I think it should be
utilized in CI, if possible, for testing the firmware's multiprocessing
capabilities. (+Oliver +Gerd.) Some superficial comments below (I'm not
requesting v5, because I'm joining this review very-very late; feel free
to update it if you wish later on).

On 1/4/23 16:37, 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;

While you need to allocate Buffer dynamically, I think you need not
allocate the string pointed-to by Buffer[N] dynamically; can you just do:

  CHAR16 (*Buffer)[APFUNC_BUFFER_LEN];

?

It is indeed one of those rare cases when we don't want an array of
pointers, but a pointer to an array. (Effectively an array of arrays.)

It's quite funny that I explained precisely this syntax to my son a few
hours ago. :)

> +
> +/** 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);

- "%d" is not a proper conversion specifier for UINTN. The proper way to
format UINTN in decimal is to cast the UINTN to UINT64, and then use
"%Lu" (or "%lu", equivalently).

- you need to make sure that the application is built with the
MdePkg/Library/BasePrintLib instance, and not with the
MdeModulePkg/Library/DxePrintLibPrint2Protocol instance. Protocol usage
from multiple processors is generally not permitted (there are exceptions).

> +  }
> +}
> +
> +/**
> +  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;
> +
> +  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);

Same formatting comment about %d.

> +  }
> +
> +  for (Index = 0; Index < NumCpu; Index++) {

If getting NumCpu above failed, this is undefined behavior (NumCpu will
have indeterminate value).

> +    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"

Same comment -- Index is UINTN.

> +        L"\tID: %016lx\n"
> +        L"\tStatus: %s | ",
> +        Index,
> +        CpuInfo.ProcessorId,
> +        (CpuInfo.StatusFlag & PROCESSOR_AS_BSP_BIT) ? L"BSP" : L"AP"

I *think* the edk2 coding style requries an explicit comparison against
0 here, such as in (a & b) != 0; but if Uncrustify is fine with this,
who am I to complain? :)

> +        );
> +
> +      if ((CpuInfo.StatusFlag & PROCESSOR_AS_BSP_BIT) && (BspIndex != NULL)) {

Same comment here.

> +        *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

All UINT32's, please use %u for them

BTW was this built for all of ARM / AARCH64 / IA32 / X64?

> +        );
> +    }
> +  }
> +
> +  return NumCpu;

If getting NumCpu at the top failed, this returns an indeterminate value
-- undefined behavior.

I think this function can actually return VOID. See near the call site,
later, why.


> +}
> +
> +/** 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 *));

The way to write this with the least thinking is always

  Ptr = Allocate[Zero]Pool (NumberOfElements * sizeof *Ptr);

by which I want to highlight the "sizeof *Ptr" part. Will always have
the correct size, even if you change the element (or otherwise
pointed-to) type, and survives renaming the type, too.

Will translate transparently to my suggestion at the top, i.e. if you
change the element type from pointer to array.

Just a suggestion.

> +  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;
> +  }

Then this separate allocation can be dropped.

> +
> +  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;
> +
> +  ApArg->Mp = Mp;
> +
> +  ApArg->Buffer = AllocateZeroPool (NumCpus * sizeof (VOID *));

Same recommendation here.

> +  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++) {
> +        FreePool (ApArg->Buffer[Index]);
> +      }
> +
> +      FreePool (ApArg->Buffer);
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +  }

And then you can remove the second set of allocations (one fewer
indirections, again).

... Which is useful for a different reason too: your rollback
(releasing) logic is wrong. Note that "Index" has type UINT32, and
because of that, your loop controlling expression

  Index >= 0

is a tautology; it can never evaluate to false.

Plus, I don't really understand the "Index++" as the third expression in
there. Probably a typo and you meant "Index--".


Anyway, I do love this pattern; however, I like to write it like this:

{
  for (Index = 0; Index < NumCpus; Index++) {
    VOID *Buf;

    Buf = AllocateZeroPool (APFUNC_BUFFER_LEN);
    if (Buf == NULL) {
      Status = EFI_OUT_OF_RESOURCES;
      goto Release;
    }
    ApArg->Buffer[Index] = Buf;
  }

  return EFI_SUCCESS;

Release:
  while (Index > 0) {
    --Index;
    FreePool (ApArg->Buffer[Index]);
    ApArg->Buffer[Index] = NULL;
  }
  return Status;
}

(I've omitted the freeing of ApArg->Buffer itself.)

Worth noting:

- due to the goto and the separate Status variable, it composes well
with further rollback steps if you need to extend the set of allocated
resources later

- the release loop counts down exactly as many times as it needs to.
This pattern is *required* for *precisely reversing* the original "for"
loop that did the allocations.

Some people counter-propose this:

  for (; Index-- > 0; ) {
    FreePool (ApArg->Buffer[Index]);
    ApArg->Buffer[Index] = NULL;
  }

where they jokingly call the "-- >" tokens (operators), read together as
"-->", the "downto" operator.

But this is not identical: when this latter loop completes, it will set
Index to (UINT32)-1, rather than 0, and so Index will no longer reflect
the number of elements present in the array. The problem is that, even
when Index is already zero and so we don't enter the loop body, the
post-decrement still happens.

The while loop I propose does set Index to the number of elements left
in the array: it ends with Index==0.

Anyway, end of detour.


One more note: if you change the element type like I propose, then both
functions coalesce to just one. You can no longer allocate the buffer
just for the Nth processor, you simply can't avoid allocating the
buffers for all the previous processors too.

But that's a plus!

> +
> +  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;
> +  }

This looks like a copy-paste error to me. It leaks memory, I believe.

> +
> +  Print (
> +    L"StartupThisAP on Processor %d with %d%s timeout...",
> +    ProcessorIndex,
> +    Timeout,

Both are UINTNs, so %d is not a good match for printing them.

> +    (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;

you leak the buffers here, suggest reworking with gotos

> +  } else {

else-after-return is a wart, I suggest dropping the "else" and unnesting
its branch body.

> +    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,

%u is not a good fit for UINTN

> +    (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;

you leak the buffers here, suggest reworking with goto

> +  } else {
> +    Print (L"done.\n");
> +
> +    for (Index = 0; Index < NumCpus; Index++) {
> +      Print (ApArg.Buffer[Index]);

Hm, I think this deserves a code comment. The array includes an entry
for the BSP too, and that entry will never be populated. But that's OK,
because it is NUL-initialized, so this Print will not output any
characters for that entry.

> +    }
> +  }
> +
> +  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
> +  )
> +{
> +  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;
> +  }

%d vs. UINTN

> +
> +  Status = Mp->EnableDisableAP (Mp, ProcessorIndex, TRUE, &HealthFlag);
> +  if (EFI_ERROR (Status)) {
> +    Print (L"failed: %r\n", Status);
> +    return Status;
> +  } else {

else-after-return

> +    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
> +  )
> +{
> +  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;
> +  }

%d vs UINTN

> +
> +  Status = Mp->EnableDisableAP (Mp, ProcessorIndex, FALSE, &HealthFlag);
> +  if (EFI_ERROR (Status)) {
> +    Print (L"failed: %r\n", Status);
> +    return Status;
> +  } else {

else-after-return

> +    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;
> +  }

I suggest reordering these two steps (ParseArguments and
GetProcessorInformation), and then passing NumCpus to ParseArguments.
I'll explain below why.

> +
> +  if (Options.PrintBspProcessorIndex) {
> +    Status = Mp->WhoAmI (Mp, &CpuIndex);
> +    if (EFI_ERROR (Status)) {
> +      Print (L"WhoAmI failed: %r\n", Status);
> +      return Status;
> +    } else {

else-after-return

> +      Print (L"BSP: %016lx\n", CpuIndex);

%lx is not good for UINTN on 32-bit (ARM, IA32).

> +    }
> +  }
> +
> +  if (Options.PrintProcessorInformation) {
> +    NumCpus = PrintProcessorInformation (Mp, &BspIndex);
> +    if (NumCpus < 2) {
> +      Print (L"Error: Uniprocessor system found.\n");
> +      return EFI_INVALID_PARAMETER;
> +    }

I don't understand the logic here.

First, why it's important that we have at least one AP.

Second, if it *is* important, then why only in case the user wants to
print the information?

Also, you are overwriting "NumCpus", which you fetched above with
GetProcessorInformation().

I think it's not needed for PrintProcessorInformation() to return a value.

Same for BspIndex -- you are overwriting it.

But, most surprisingly, "BspIndex" is not used for anything later on. So
I think neither GetProcessorInformation() nor
PrintProcessorInformation() need to output it.

Hm, maybe you wanted to remove "CpuIndex" instead (with the accompanying
WhoAmI() call), and rely on the output of GetProcessorInformation() instead?

> +  }
> +
> +  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;
> +  CONST CHAR16  *Argument;
> +  BOOLEAN       NeedsValue;
> +  UINTN         *Value;

I propose removing "NeedsValue". The same can be expressed by

  Value != NULL

> +
> +  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;

i.e., Value = NULL

> +
> +    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));

This ZeroMem() looks pointless.

> +      return EFI_SUCCESS;
> +    }
> +
> +    if (NeedsValue) {
> +      if ((ArgIndex + 1) < Argc) {
> +        Status = StrDecimalToUintnS (Argv[ArgIndex + 1], NULL, Value);

Side comment: StrDecimalToUintnS() is not a flawless function; see
<https://bugzilla.tianocore.org/show_bug.cgi?id=2693>. But that's not
too important here.

> +        if (EFI_ERROR (Status)) {
> +          Print (L"Error: option value must be a positive integer.\n");
> +          PrintUsage ();
> +          return EFI_INVALID_PARAMETER;
> +        }

I've got a suggestion for this location. I suggest introducing MaxVal
(UINTN) and setting them appropriately whenever you set the Value
pointer to non-NULL.

Then, after parsing the option-argument here, I suggest performing a
<=MaxVal range check (i.e. inclusive).

Here's why: Value is effectively "untrusted" input. But consider how you
use each of the two possible UINTN option-arguments:

- Timeout is multiplied by 1000 in the functions StartupThisAP and
StartupAllAPs. You don't want an integer overflow there, so best set
MaxVal=MAX_UINTN/1000.

- ProcessorIndex is passed to EnableAP, DisableAP, and StartupThisAP. In
all three functions, the underlying MP protocol members perform range
checking, so that's fine. However, in StartupThisAP, we also use
ProcessorIndex -- indirectly -- for memory allocation. In that
calculation, we use addition and then multiplication -- this can lead to
integer overflow. So best set MaxVal = NumCpus-1. (I don't expect we'll
ever see more than a few thousand CPUs, and with such a count, the
buffer size calculation will not overflow.)

(I probably won't have time or energy to review the next version or the
incremental updates; I just figured I'd raise these.)

Laszlo


> +
> +        ArgIndex++;
> +      } else {
> +        PrintUsage ();
> +        return EFI_INVALID_PARAMETER;
> +      }
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}