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; Tue, 25 Jun 2019 03:25:00 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3BBF0307D980; Tue, 25 Jun 2019 10:24:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-121.ams2.redhat.com [10.36.116.121]) by smtp.corp.redhat.com (Postfix) with ESMTP id B8774196E3; Tue, 25 Jun 2019 10:24:53 +0000 (UTC) Subject: Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP Protocol. To: "Dong, Eric" , "devel@edk2.groups.io" Cc: "Ni, Ray" References: <20190619055114.12744-1-eric.dong@intel.com> <20190619055114.12744-3-eric.dong@intel.com> <68f92355-3cd1-1557-67c4-57ecb52eaa98@redhat.com> From: "Laszlo Ersek" Message-ID: <56546a91-cb4f-b5e4-6464-41dcd8c0b714@redhat.com> Date: Tue, 25 Jun 2019 12:24:52 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Tue, 25 Jun 2019 10:24:56 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Ni, Ray >> 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