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

["Laszlo Ersek" <lersek@redhat.com>]

On 06/25/19 04:15, Dong, Eric wrote:
>> -----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:

>>> 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? 

[1] ISO C99 6.5.2.2 "Function calls", paragraph 9:

    If the function is defined with a type that is not compatible with
    the type (of the expression) pointed to by the expression that
    denotes the called function, the behavior is undefined.

After you pass "Procedure" to InternalSmmStartupThisAp(), then something
inside InternalSmmStartupThisAp() will call "Procedure" through the
following prototype:

  VOID
  EFIAPI
  Procedure (
    IN OUT VOID  *Buffer
    );

However, "Procedure" has the following prototype in reality:

  EFI_STATUS
  EFIAPI
  Procedure (
    IN VOID  *ProcedureArgument
    );

Therefore -- the standard says --, if these two prototypes are not
compatible with each other, then the behavior is undefined, in the code
above.

In simpler terms, you must not call a function as another function type,
unless those function types are compatible.

So let's see if the two function types in question are compatible:

[2] ISO C99 6.7.5.3 "Function declarators (including prototypes)",
paragraph 15:

  For two function types to be compatible, both shall specify compatible
  return types. [...]

In our case, one return type is VOID, and the other return type is
EFI_STATUS.

Those types are not compatible. Therefore the function types are not
compatible. Therefore you must not call EFI_AP_PROCEDURE *as* an
EFI_AP_PROCEDURE2, or vice versa -- otherwise the behavior is undefined.


Regarding the wrapper function and structure, this is what I propose:


typedef struct {
  EFI_AP_PROCEDURE2 Procedure2;
  VOID              *ProcedureArgument;
} PROCEDURE2_WRAPPER;


STATIC
VOID
EFIAPI
Procedure2Wrapper (
  IN OUT VOID *Buffer
  )
{
  PROCEDURE2_WRAPPER *Wrapper;
  EFI_STATUS         Status;

  Wrapper = Buffer;
  Status = Wrapper->Procedure2 (Wrapper->ProcedureArgument);
  //
  // Ignore status deliberately.
  //
}


And then in SmmMpDispatchProcedure():

  PROCEDURE2_WRAPPER *Wrapper;

  Wrapper = AllocatePool (sizeof *Wrapper);
  if (Wrapper = NULL) {
    return EFI_OUT_OF_RESOURCES;
  }
  Wrapper->Procedure2 = Procedure;
  Wrapper->ProcedureArgument = ProcedureArguments;
  InternalSmmStartupThisAp (... Procedure2Wrapper ... Wrapper ...);


The point is that InternalSmmStartupThisAp() receives a pointer to
Procedure2Wrapper(), and that argument *really* has the correct type
(EFI_AP_PROCEDURE).

Of course, "Wrapper" should be released as well:

- For synchronous calls -- i.e., when Token is NULL --, the
SmmMpDispatchProcedure() function should call

  FreePool (Wrapper);

right after InternalSmmStartupThisAp() returns.

- For the async case, when Token is *not* NULL, the SMM_COMPLETION data
type has to be extended with a pointer to the PROCEDURE2_WRAPPER oject.
And then, when the completion is *collected*, Wrapper should be freed.

(Of course, AllocatePool() and FreePool() operate on SMRAM.)

This entire idea is about inserting another level of indirection so that
we can avoid undefined function calls in C code.

Thanks
Laszlo