> -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Friday, June 21, 2019 12:45 AM > To: devel@edk2.groups.io; Dong, Eric <eric.d...@intel.com> > Cc: Ni, Ray <ray...@intel.com> > Subject: Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: > Enable MM MP Protocol. > > 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.)
Hi Laszlo, Can you help to explain more about how to use dedicated wrapper function for the callback? What I can image is using type cast here to fix this issue. Why you think type cast is not enough here? > > > --*-- > > > 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.) Agree, will add error handling here and add the return status code to the function header comments. Thanks, Eric > > Thanks > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42773): https://edk2.groups.io/g/devel/message/42773 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] -=-=-=-=-=-=-=-=-=-=-=-
