From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 20 Jun 2019 09:45:46 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AC1C3308FED5; Thu, 20 Jun 2019 16:45:28 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-226.ams2.redhat.com [10.36.117.226]) by smtp.corp.redhat.com (Postfix) with ESMTP id B05D85D9E5; Thu, 20 Jun 2019 16:45:27 +0000 (UTC) Subject: Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP Protocol. To: devel@edk2.groups.io, eric.dong@intel.com Cc: Ray Ni References: <20190619055114.12744-1-eric.dong@intel.com> <20190619055114.12744-3-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <68f92355-3cd1-1557-67c4-57ecb52eaa98@redhat.com> Date: Thu, 20 Jun 2019 18:45:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190619055114.12744-3-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Thu, 20 Jun 2019 16:45:33 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/19/19 07:51, Dong, Eric wrote: > Add MM Mp Protocol in PiSmmCpuDxeSmm driver. > > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Eric Dong > --- > 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.
> + > +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