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, 03 Oct 2019 04:10:56 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BC36D8980FB; Thu, 3 Oct 2019 11:10:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-154.rdu2.redhat.com [10.10.120.154]) by smtp.corp.redhat.com (Postfix) with ESMTP id 94B481001B07; Thu, 3 Oct 2019 11:10:54 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 33/35] StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader tracking From: "Laszlo Ersek" To: Achin Gupta , Jiewen Yao , Supreeth Venkatesh Cc: edk2-devel-groups-io Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20190917194935.24322-1-lersek@redhat.com> <20190917194935.24322-34-lersek@redhat.com> Message-ID: <86cfd468-7fd4-e7b0-dcd5-cc183abd076d@redhat.com> Date: Thu, 3 Oct 2019 13:10:53 +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.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.67]); Thu, 03 Oct 2019 11:10:55 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Pinging StandaloneMmPkg maintainers again, for reviewing this patch. Thanks Laszlo On 09/26/19 14:48, Laszlo Ersek wrote: > Achin, Jiewen, Supreeth, > > can one of you guys please review this patch? > > Thanks > Laszlo > > On 09/17/19 21:49, Laszlo Ersek wrote: >> The FvHasBeenProcessed() and FvIsBeingProcesssed() functions make sure >> that every firmware volume is processed only once (every driver in every >> firmware volume should be discovered only once). For this, the functions >> use a linked list. >> >> In MdeModulePkg's DXE Core and SMM Core, the key used for identifying >> those firmware volumes that have been processed is the EFI_HANDLE on which >> the DXE or SMM firmware volume protocol is installed. In the >> StandaloneMmPkg core however, the key is the address of the firmware >> volume header; that is, it has type (EFI_FIRMWARE_VOLUME_HEADER*). >> >> (EFI_FIRMWARE_VOLUME_HEADER*) has nothing to do with EFI_HANDLE. >> EFI_HANDLE just happens to be specified as (VOID*), and therefore the >> conversion between (EFI_FIRMWARE_VOLUME_HEADER*) and EFI_HANDLE is silent. >> >> (The FvHasBeenProcessed() and FvIsBeingProcesssed() functions were likely >> copied verbatim from MdeModulePkg's DXE Core and/or the SMM Core, and not >> flagged by the compiler in StandaloneMmPkg due to UEFI regrettably >> specifying EFI_HANDLE as (VOID*), thereby enabling the above implicit >> conversion.) >> >> We should not exploit this circumstance. Represent the key type faithfully >> instead. >> >> This is a semantic fix; there is no change in operation. >> >> Cc: Achin Gupta >> Cc: Jiewen Yao >> Cc: Supreeth Venkatesh >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> build-tested only >> >> StandaloneMmPkg/Core/StandaloneMmCore.h | 2 +- >> StandaloneMmPkg/Core/Dispatcher.c | 80 +++++++++++--------- >> StandaloneMmPkg/Core/FwVol.c | 16 ++-- >> 3 files changed, 52 insertions(+), 46 deletions(-) >> >> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h b/StandaloneMmPkg/Core/StandaloneMmCore.h >> index dcf91bc5e916..23ddbe169faf 100644 >> --- a/StandaloneMmPkg/Core/StandaloneMmCore.h >> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h >> @@ -67,7 +67,7 @@ typedef struct { >> >> LIST_ENTRY ScheduledLink; // mScheduledQueue >> >> - EFI_HANDLE FvHandle; >> + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; >> EFI_GUID FileName; >> VOID *Pe32Data; >> UINTN Pe32DataSize; >> diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dispatcher.c >> index 3788389f95ed..9853445a64a1 100644 >> --- a/StandaloneMmPkg/Core/Dispatcher.c >> +++ b/StandaloneMmPkg/Core/Dispatcher.c >> @@ -5,7 +5,7 @@ >> is added to the mDiscoveredList. The Before, and After Depex are >> pre-processed as drivers are added to the mDiscoveredList. If an Apriori >> file exists in the FV those drivers are addeded to the >> - mScheduledQueue. The mFvHandleList is used to make sure a >> + mScheduledQueue. The mFwVolList is used to make sure a >> FV is only processed once. >> >> Step #2 - Dispatch. Remove driver from the mScheduledQueue and load and >> @@ -40,13 +40,13 @@ >> // >> // MM Dispatcher Data structures >> // >> -#define KNOWN_HANDLE_SIGNATURE SIGNATURE_32('k','n','o','w') >> +#define KNOWN_FWVOL_SIGNATURE SIGNATURE_32('k','n','o','w') >> >> typedef struct { >> - UINTN Signature; >> - LIST_ENTRY Link; // mFvHandleList >> - EFI_HANDLE Handle; >> -} KNOWN_HANDLE; >> + UINTN Signature; >> + LIST_ENTRY Link; // mFwVolList >> + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; >> +} KNOWN_FWVOL; >> >> // >> // Function Prototypes >> @@ -86,9 +86,10 @@ LIST_ENTRY mDiscoveredList = INITIALIZE_LIST_HEAD_VARIABLE (mDiscoveredList); >> LIST_ENTRY mScheduledQueue = INITIALIZE_LIST_HEAD_VARIABLE (mScheduledQueue); >> >> // >> -// List of handles who's Fv's have been parsed and added to the mFwDriverList. >> +// List of firmware volume headers whose containing firmware volumes have been >> +// parsed and added to the mFwDriverList. >> // >> -LIST_ENTRY mFvHandleList = INITIALIZE_LIST_HEAD_VARIABLE (mFvHandleList); >> +LIST_ENTRY mFwVolList = INITIALIZE_LIST_HEAD_VARIABLE (mFwVolList); >> >> // >> // Flag for the MM Dispacher. TRUE if dispatcher is execuing. >> @@ -769,26 +770,30 @@ MmInsertOnScheduledQueueWhileProcessingBeforeAndAfter ( >> } >> >> /** >> - Return TRUE if the Fv has been processed, FALSE if not. >> + Return TRUE if the firmware volume has been processed, FALSE if not. >> >> - @param FvHandle The handle of a FV that's being tested >> + @param FwVolHeader The header of the firmware volume that's being >> + tested. >> >> - @retval TRUE Fv protocol on FvHandle has been processed >> - @retval FALSE Fv protocol on FvHandle has not yet been >> - processed >> + @retval TRUE The firmware volume denoted by FwVolHeader has >> + been processed >> + @retval FALSE The firmware volume denoted by FwVolHeader has >> + not yet been processed >> >> **/ >> BOOLEAN >> FvHasBeenProcessed ( >> - IN EFI_HANDLE FvHandle >> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader >> ) >> { >> LIST_ENTRY *Link; >> - KNOWN_HANDLE *KnownHandle; >> + KNOWN_FWVOL *KnownFwVol; >> >> - for (Link = mFvHandleList.ForwardLink; Link != &mFvHandleList; Link = Link->ForwardLink) { >> - KnownHandle = CR (Link, KNOWN_HANDLE, Link, KNOWN_HANDLE_SIGNATURE); >> - if (KnownHandle->Handle == FvHandle) { >> + for (Link = mFwVolList.ForwardLink; >> + Link != &mFwVolList; >> + Link = Link->ForwardLink) { >> + KnownFwVol = CR (Link, KNOWN_FWVOL, Link, KNOWN_FWVOL_SIGNATURE); >> + if (KnownFwVol->FwVolHeader == FwVolHeader) { >> return TRUE; >> } >> } >> @@ -796,28 +801,29 @@ FvHasBeenProcessed ( >> } >> >> /** >> - Remember that Fv protocol on FvHandle has had it's drivers placed on the >> - mDiscoveredList. This fucntion adds entries on the mFvHandleList. Items are >> - never removed/freed from the mFvHandleList. >> + Remember that the firmware volume denoted by FwVolHeader has had its drivers >> + placed on mDiscoveredList. This function adds entries to mFwVolList. Items >> + are never removed/freed from mFwVolList. >> >> - @param FvHandle The handle of a FV that has been processed >> + @param FwVolHeader The header of the firmware volume that's being >> + processed. >> >> **/ >> VOID >> FvIsBeingProcesssed ( >> - IN EFI_HANDLE FvHandle >> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader >> ) >> { >> - KNOWN_HANDLE *KnownHandle; >> + KNOWN_FWVOL *KnownFwVol; >> >> - DEBUG ((DEBUG_INFO, "FvIsBeingProcesssed - 0x%08x\n", FvHandle)); >> + DEBUG ((DEBUG_INFO, "FvIsBeingProcesssed - 0x%08x\n", KnownFwVol)); >> >> - KnownHandle = AllocatePool (sizeof (KNOWN_HANDLE)); >> - ASSERT (KnownHandle != NULL); >> + KnownFwVol = AllocatePool (sizeof (KNOWN_FWVOL)); >> + ASSERT (KnownFwVol != NULL); >> >> - KnownHandle->Signature = KNOWN_HANDLE_SIGNATURE; >> - KnownHandle->Handle = FvHandle; >> - InsertTailList (&mFvHandleList, &KnownHandle->Link); >> + KnownFwVol->Signature = KNOWN_FWVOL_SIGNATURE; >> + KnownFwVol->FwVolHeader = FwVolHeader; >> + InsertTailList (&mFwVolList, &KnownFwVol->Link); >> } >> >> /** >> @@ -842,12 +848,12 @@ FvIsBeingProcesssed ( >> **/ >> EFI_STATUS >> MmAddToDriverList ( >> - IN EFI_HANDLE FvHandle, >> - IN VOID *Pe32Data, >> - IN UINTN Pe32DataSize, >> - IN VOID *Depex, >> - IN UINTN DepexSize, >> - IN EFI_GUID *DriverName >> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader, >> + IN VOID *Pe32Data, >> + IN UINTN Pe32DataSize, >> + IN VOID *Depex, >> + IN UINTN DepexSize, >> + IN EFI_GUID *DriverName >> ) >> { >> EFI_MM_DRIVER_ENTRY *DriverEntry; >> @@ -863,7 +869,7 @@ MmAddToDriverList ( >> >> DriverEntry->Signature = EFI_MM_DRIVER_ENTRY_SIGNATURE; >> CopyGuid (&DriverEntry->FileName, DriverName); >> - DriverEntry->FvHandle = FvHandle; >> + DriverEntry->FwVolHeader = FwVolHeader; >> DriverEntry->Pe32Data = Pe32Data; >> DriverEntry->Pe32DataSize = Pe32DataSize; >> DriverEntry->Depex = Depex; >> diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c >> index 9fe0c257a43a..99ecf4af4714 100644 >> --- a/StandaloneMmPkg/Core/FwVol.c >> +++ b/StandaloneMmPkg/Core/FwVol.c >> @@ -24,22 +24,22 @@ EFI_FV_FILETYPE mMmFileTypes[] = { >> >> EFI_STATUS >> MmAddToDriverList ( >> - IN EFI_HANDLE FvHandle, >> - IN VOID *Pe32Data, >> - IN UINTN Pe32DataSize, >> - IN VOID *Depex, >> - IN UINTN DepexSize, >> - IN EFI_GUID *DriverName >> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader, >> + IN VOID *Pe32Data, >> + IN UINTN Pe32DataSize, >> + IN VOID *Depex, >> + IN UINTN DepexSize, >> + IN EFI_GUID *DriverName >> ); >> >> BOOLEAN >> FvHasBeenProcessed ( >> - IN EFI_HANDLE FvHandle >> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader >> ); >> >> VOID >> FvIsBeingProcesssed ( >> - IN EFI_HANDLE FvHandle >> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader >> ); >> >> EFI_STATUS >> > > > >