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:48:15 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A5309C049E1A; Thu, 26 Sep 2019 12:48:14 +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 593B25E1B0; Thu, 26 Sep 2019 12:48:13 +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: Date: Thu, 26 Sep 2019 14:48:12 +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: <20190917194935.24322-34-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 26 Sep 2019 12:48:14 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >