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 <achin.gu...@arm.com>
>> Cc: Jiewen Yao <jiewen....@intel.com>
>> Cc: Supreeth Venkatesh <supreeth.venkat...@arm.com>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> 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
>>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48433): https://edk2.groups.io/g/devel/message/48433
Mute This Topic: https://groups.io/mt/34180238/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
