[edk2-devel] [PATCH v4 3/3] MdeModulePkg: Add new Application/MpServicesTest application
Laszlo Ersek
lersek at redhat.com
Sat Jan 7 22:19:40 UTC 2023
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 at 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;
> +}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98150): https://edk2.groups.io/g/devel/message/98150
Mute This Topic: https://groups.io/mt/96091002/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list