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, 26 Sep 2019 05:10:33 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8FC388BA02; Thu, 26 Sep 2019 12:10:32 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3F3F26012C; Thu, 26 Sep 2019 12:10:28 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 09/35] MdeModulePkg: stop abusing EFI_EVENT for protocol notify registration To: devel@edk2.groups.io, philmd@redhat.com Cc: Hao A Wu , Jian J Wang , Liming Gao , Ray Ni , Zhichao Gao References: <20190917194935.24322-1-lersek@redhat.com> <20190917194935.24322-10-lersek@redhat.com> <44c7d155-647f-ce11-d006-8071b97d6e8a@redhat.com> From: "Laszlo Ersek" Message-ID: <05fdc8cc-334f-e93e-be1d-c42900065204@redhat.com> Date: Thu, 26 Sep 2019 14:10:27 +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: <44c7d155-647f-ce11-d006-8071b97d6e8a@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 26 Sep 2019 12:10:32 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 09/25/19 18:02, Philippe Mathieu-Daud=C3=A9 wrote: > On 9/17/19 9:49 PM, Laszlo Ersek wrote: >> EfiCreateProtocolNotifyEvent() takes a (VOID**) for "Registration", >> similarly to gBS->RegisterProtocolNotify(). We should pass the address= of >> an actual pointer-to-VOID, and not the address of an EFI_EVENT. EFI_EV= ENT >> just happens to be specified as (VOID*), and has nothing to do with th= e >> registration. >> >> The same applies to gMmst->MmRegisterProtocolNotify(). >> >> "mFtwRegistration", "mFvRegistration", and "mFvbRegistration" are used= for >> nothing else. >> >> This change is a no-op in practice; it's a semantic improvement. >> >> Cc: Hao A Wu >> Cc: Jian J Wang >> Cc: Liming Gao >> Cc: Ray Ni >> Cc: Zhichao Gao >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> lightly tested, as these modules (except LoadFileOnFv2) are part o= f the >> ArmVirt and/or OVMF platforms >> >> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.c = | 2 +- >> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c = | 2 +- >> MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c = | 2 +- >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c = | 2 +- >> 4 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultToleran= tWriteDxe.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantW= riteDxe.c >> index ae8f117905cd..de38ea028af1 100644 >> --- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteD= xe.c >> +++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteD= xe.c >> @@ -47,7 +47,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >> =20 >> #include >> #include "FaultTolerantWrite.h" >> -EFI_EVENT mFvbRegistration =3D NULL; >> +VOID *mFvbRegistration =3D NULL; >> =20 >> =20 >> /** >> diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultToleran= tWriteSmm.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantW= riteSmm.c >> index e8e935a85b5b..9612b394865b 100644 >> --- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteS= mm.c >> +++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteS= mm.c >> @@ -56,7 +56,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >> #include "FaultTolerantWriteSmmCommon.h" >> #include >> =20 >> -EFI_EVENT mFvbRegistration =3D NULL; >> +VOID *mFvbRegistration =3D NULL; >> EFI_FTW_DEVICE *mFtwDevice =3D NULL; >> =20 >> /// >> diff --git a/MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c b/Md= eModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c >> index 4af2da05e145..43fa6ce12875 100644 >> --- a/MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c >> +++ b/MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c >> @@ -36,7 +36,7 @@ typedef struct { >> #define LOAD_FILE_ON_FV2_PRIVATE_DATA_FROM_THIS(a) CR (a, LOAD_FILE_O= N_FV2_PRIVATE_DATA, LoadFile, LOAD_FILE_ON_FV2_PRIVATE_DATA_SIGNATURE) >> #define LOAD_FILE_ON_FV2_PRIVATE_DATA_FROM_LINK(a) CR (a, LOAD_FILE_O= N_FV2_PRIVATE_DATA, Link, LOAD_FILE_ON_FV2_PRIVATE_DATA_SIGNATURE) >> =20 >> -EFI_EVENT mFvRegistration; >> +VOID *mFvRegistration; >> LIST_ENTRY mPrivateDataList; >> =20 >> /** >> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c = b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> index 3d232bb36cb4..7d2b6c8e1fad 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> @@ -13,7 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >> =20 >> EFI_HANDLE mHandle =3D NU= LL; >> EFI_EVENT mVirtualAddressChangeEvent =3D NU= LL; >> -EFI_EVENT mFtwRegistration =3D NU= LL; >> +VOID *mFtwRegistration =3D NU= LL; >> VOID ***mVarCheckAddressPointer =3D NU= LL; >> UINTN mVarCheckAddressPointerCount =3D = 0; >> EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock =3D { = VariableLockRequestToLock }; >=20 > Surprisingly the one declared in > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c is correct :) Indeed! >=20 > Reviewed-by: Philippe Mathieu-Daude Thanks! Laszlo