Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP Protocol.

["Dong, Eric" <eric.dong@intel.com>]

> -----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.dong@intel.com>
> Cc: Ni, Ray <ray.ni@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.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Eric Dong <eric.dong@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
> 
>