On 06/19/19 07:51, Dong, Eric wrote:
> Add MM Mp Protocol in PiSmmCpuDxeSmm driver.
>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Eric Dong <eric.d...@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/MpProtocol.c | 375 +++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/MpProtocol.h | 283 +++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 468 ++++++++++++++++++-
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 11 +
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 172 ++++++-
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 3 +
> 6 files changed, 1296 insertions(+), 16 deletions(-)
> create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/MpProtocol.c
> create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/MpProtocol.h
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpProtocol.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpProtocol.c
> new file mode 100644
> index 0000000000..8cf69428c2
> --- /dev/null
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpProtocol.c
> @@ -0,0 +1,375 @@
> +/** @file
> +SMM MP protocol implementation
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "PiSmmCpuDxeSmm.h"
> +#include "MpProtocol.h"
> +
> +///
> +/// SMM MP Protocol instance
> +///
> +EFI_SMM_MP_PROTOCOL mSmmMp = {
> + EFI_SMM_MP_PROTOCOL_REVISION,
> + 0,
> + SmmMpGetNumberOfProcessors,
> + SmmMpDispatchProcedure,
> + SmmMpBroadcastProcedure,
> + SmmMpSetStartupProcedure,
> + SmmMpCheckForProcedure,
> + SmmMpWaitForProcedure
> +};
> +
> +/**
> + Service to retrieves the number of logical processor in the platform.
> +
> + @param[in] This The EFI_MM_MP_PROTOCOL instance.
> + @param[out] NumberOfProcessors Pointer to the total number of logical
> processors in the system,
> + including the BSP and all APs.
> +
> + @retval EFI_SUCCESS The number of processors was retrieved
> successfully
> + @retval EFI_INVALID_PARAMETER NumberOfProcessors is NULL
> +**/
> +
> +EFI_STATUS
> +EFIAPI
> +SmmMpGetNumberOfProcessors (
> + IN CONST EFI_SMM_MP_PROTOCOL *This,
> + OUT UINTN *NumberOfProcessors
> + )
> +{
> + if (NumberOfProcessors == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + *NumberOfProcessors = gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus;
> +
> + return EFI_SUCCESS;
> +}
> +
> +
> +/**
> + This service allows the caller to invoke a procedure one of the
> application processors (AP). This
> + function uses an optional token parameter to support blocking and
> non-blocking modes. If the token
> + is passed into the call, the function will operate in a non-blocking
> fashion and the caller can
> + check for completion with CheckOnProcedure or WaitForProcedure.
> +
> + @param[in] This The EFI_MM_MP_PROTOCOL instance.
> + @param[in] Procedure A pointer to the procedure to be run
> on the designated target
> + AP of the system. Type
> EFI_AP_PROCEDURE2 is defined below in
> + related definitions.
> + @param[in] CpuNumber The zero-based index of the
> processor number of the target
> + AP, on which the code stream is
> supposed to run. If the number
> + points to the calling processor then
> it will not run the
> + supplied code.
> + @param[in] TimeoutInMicroseconds Indicates the time limit in
> microseconds for this AP to
> + finish execution of Procedure,
> either for blocking or
> + non-blocking mode. Zero means
> infinity. If the timeout
> + expires before this AP returns from
> Procedure, then Procedure
> + on the AP is terminated. If the
> timeout expires in blocking
> + mode, the call returns EFI_TIMEOUT.
> If the timeout expires
> + in non-blocking mode, the timeout
> determined can be through
> + CheckOnProcedure or WaitForProcedure.
> + Note that timeout support is
> optional. Whether an
> + implementation supports this
> feature, can be determined via
> + the Attributes data member.
> + @param[in,out] ProcedureArguments Allows the caller to pass a list of
> parameters to the code
> + that is run by the AP. It is an
> optional common mailbox
> + between APs and the caller to share
> information.
> + @param[in,out] Token This is parameter is broken into two
> components:
> + 1.Token->Completion is an optional
> parameter that allows the
> + caller to execute the procedure in a
> blocking or non-blocking
> + fashion. If it is NULL the call is
> blocking, and the call will
> + not return until the AP has
> completed the procedure. If the
> + token is not NULL, the call will
> return immediately. The caller
> + can check whether the procedure has
> completed with
> + CheckOnProcedure or WaitForProcedure.
> + 2.Token->Status The implementation
> updates the address pointed
> + at by this variable with the status
> code returned by Procedure
> + when it completes execution on the
> target AP, or with EFI_TIMEOUT
> + if the Procedure fails to complete
> within the optional timeout.
> + The implementation will update this
> variable with EFI_NOT_READY
> + prior to starting Procedure on the
> target AP
> + @param[in,out] CPUStatus This optional pointer may be used to
> get the status code returned
> + by Procedure when it completes
> execution on the target AP, or with
> + EFI_TIMEOUT if the Procedure fails
> to complete within the optional
> + timeout. The implementation will
> update this variable with
> + EFI_NOT_READY prior to starting
> Procedure on the target AP.
> +
> + @retval EFI_SUCCESS In the blocking case, this indicates
> that Procedure has completed
> + execution on the target AP.
> + In the non-blocking case this
> indicates that the procedure has
> + been successfully scheduled for
> execution on the target AP.
> + @retval EFI_INVALID_PARAMETER The input arguments are out of
> range. Either the target AP is the
> + caller of the function, or the
> Procedure or Token is NULL
> + @retval EFI_NOT_READY If the target AP is busy executing
> another procedure
> + @retval EFI_ALREADY_STARTED Token is already in use for another
> procedure
> + @retval EFI_TIMEOUT In blocking mode, the timeout
> expired before the specified AP
> + has finished
> +**/
> +EFI_STATUS
> +EFIAPI
> +SmmMpDispatchProcedure (
> + IN CONST EFI_SMM_MP_PROTOCOL *This,
> + IN EFI_AP_PROCEDURE2 Procedure,
> + IN UINTN CpuNumber,
> + IN UINTN TimeoutInMicroseconds,
> + IN OUT VOID *ProcedureArguments OPTIONAL,
> + IN OUT SMM_COMPLETION *Token,
> + IN OUT EFI_STATUS *CPUStatus
> + )
> +{
> + if (Token != NULL) {
> + *Token = AllocatePool (sizeof (SPIN_LOCK));
> + ASSERT (*Token != NULL);
> + InitializeSpinLock ((SPIN_LOCK *)(*Token));
> +
> + return InternalSmmStartupThisAp(
> + Procedure,
> + CpuNumber,
> + ProcedureArguments,
> + SmmCpuCallInNewType,
> + (SPIN_LOCK *)(*Token),
> + TimeoutInMicroseconds,
> + CPUStatus
> + );
> + }
> +
> + return InternalSmmStartupThisAp(
> + Procedure,
> + CpuNumber,
> + ProcedureArguments,
> + SmmCpuCallInNewType,
> + NULL,
> + TimeoutInMicroseconds,
> + CPUStatus
> + );
> +}
This function breaks the build for me:
passing argument 1 of 'InternalSmmStartupThisAp' from incompatible
pointer type [-Werror]
InternalSmmStartupThisAp() expects an EFI_AP_PROCEDURE as first parameter:
typedef
VOID
(EFIAPI *EFI_AP_PROCEDURE)(
IN OUT VOID *Buffer
);
but you pass EFI_AP_PROCEDURE2:
typedef
EFI_STATUS
(EFIAPI *EFI_AP_PROCEDURE2)(
IN VOID *ProcedureArgument
);
Assuming the return status propagation is otherwise correctly
implemented through "CpuStatus" -- I didn't check --, please use a
dedicated wrapper function for the callback, so that we can avoid type
punning of function pointers. (It's possible that you'll have to
introduce a separate wrapper structure too, for passing in both the
function pointer and the procedure argument pointer -- and then that
structure should be allocated dynamically.)
--*--
Independently, I think that catching "out of memory" situations in
PiSmmCpuDxeSmm with ASSERTs is bad practice. This is a privileged /
security-relevant driver, and the function in question is exposed as a
protocol member function (i.e. it's not *only* a part of driver
startup). Which I believe might make it possible for the OS, indirectly
(via EFI runtime calls for example), to trigger this function.
So, even though the PI spec doesn't list EFI_OUT_OF_RESOURCES as a valid
return code for this member function, we should return that, if
AllocatePool() fails. (This applies to SmmMpBroadcastProcedure() too.)
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42658): https://edk2.groups.io/g/devel/message/42658
Mute This Topic: https://groups.io/mt/32120263/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
