On 07/13/16 22:03, Ard Biesheuvel wrote:
> On 13 July 2016 at 18:44, Laszlo Ersek <[email protected]> wrote:
>> (This patch ports OvmfPkg commit 2eb358986052 to ArmVirtPkg. That
>> functionality was not added to QemuBootOrderLib, because it was (and is)
>> independent from QEMU and fw_cfg.)
>>
>> Remove any boot options that point to binaries built into the firmware and
>> have become stale due to any of the following:
>> - FvMain's base address or size changed (historical -- see commit
>> e191a3114f4c),
>> - FvMain's FvNameGuid changed,
>> - the FILE_GUID of the pointed-to binary changed,
>> - the referenced binary is no longer built into the firmware.
>>
>> For example, multiple such "EFI Internal Shell" boot options can coexist.
>> They technically differ from each other, but may not describe any built-in
>> shell binary exactly. Such options can accumulate in a varstore over time,
>> and while they remain generally bootable (thanks to the efforts of
>> BmGetFileBufferByFvFilePath()), they look bad.
>>
>> Filter out any stale options.
>>
>> Cc: Ard Biesheuvel <[email protected]>
>> Fixes: https://github.com/tianocore/edk2/issues/107
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <[email protected]>
>> ---
>> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
>> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 132
>> ++++++++++++++++++++
>> 2 files changed, 133 insertions(+)
>>
>> diff --git
>> a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> index 9c95b69cc974..bec7fabb479c 100644
>> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> @@ -78,6 +78,7 @@ [Guids]
>>
>> [Protocols]
>> gEfiDevicePathProtocolGuid
>> + gEfiFirmwareVolume2ProtocolGuid
>> gEfiGraphicsOutputProtocolGuid
>> gEfiLoadedImageProtocolGuid
>> gEfiPciRootBridgeIoProtocolGuid
>> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> index eaafe7ff57ea..c11196a8a59c 100644
>> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -22,6 +22,7 @@
>> #include <Library/QemuBootOrderLib.h>
>> #include <Library/UefiBootManagerLib.h>
>> #include <Protocol/DevicePath.h>
>> +#include <Protocol/FirmwareVolume2.h>
>> #include <Protocol/GraphicsOutput.h>
>> #include <Protocol/LoadedImage.h>
>> #include <Protocol/PciIo.h>
>> @@ -387,6 +388,136 @@ PlatformRegisterFvBootOption (
>> }
>>
>>
>> +/**
>> + Remove all MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) boot
>> options
>> + whose device paths do not resolve exactly to an FvFile in the system.
>> +
>> + This removes any boot options that point to binaries built into the
>> firmware
>> + and have become stale due to any of the following:
>> + - FvMain's base address or size changed (historical),
>> + - FvMain's FvNameGuid changed,
>> + - the FILE_GUID of the pointed-to binary changed,
>> + - the referenced binary is no longer built into the firmware.
>> +
>> + EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() only
>> + avoids exact duplicates.
>> +**/
>> +STATIC
>> +VOID
>> +RemoveStaleFvFileOptions (
>> + VOID
>> + )
>> +{
>> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
>> + UINTN BootOptionCount;
>> + UINTN Index;
>> +
>> + BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount,
>> + LoadOptionTypeBoot);
>> +
>> + for (Index = 0; Index < BootOptionCount; ++Index) {
>> + EFI_DEVICE_PATH_PROTOCOL *Node1, *Node2, *SearchNode;
>> + EFI_STATUS Status;
>> + EFI_HANDLE FvHandle;
>> +
>> + //
>> + // If the device path starts with neither MemoryMapped(...) nor Fv(...),
>> + // then keep the boot option.
>> + //
>> + Node1 = BootOptions[Index].FilePath;
>> + if (!(DevicePathType (Node1) == HARDWARE_DEVICE_PATH &&
>> + DevicePathSubType (Node1) == HW_MEMMAP_DP) &&
>> + !(DevicePathType (Node1) == MEDIA_DEVICE_PATH &&
>> + DevicePathSubType (Node1) == MEDIA_PIWG_FW_VOL_DP)) {
>> + continue;
>> + }
>> +
>> + //
>> + // If the second device path node is not FvFile(...), then keep the boot
>> + // option.
>> + //
>> + Node2 = NextDevicePathNode (Node1);
>> + if (DevicePathType (Node2) != MEDIA_DEVICE_PATH ||
>> + DevicePathSubType (Node2) != MEDIA_PIWG_FW_FILE_DP) {
>> + continue;
>> + }
>> +
>> + //
>> + // Locate the Firmware Volume2 protocol instance that is denoted by the
>> + // boot option. If this lookup fails (i.e., the boot option references a
>> + // firmware volume that doesn't exist), then we'll proceed to delete the
>> + // boot option.
>> + //
>> + SearchNode = Node1;
>> + Status = gBS->LocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid,
>> + &SearchNode, &FvHandle);
>> +
>> + if (!EFI_ERROR (Status)) {
>> + //
>> + // The firmware volume was found; now let's see if it contains the
>> FvFile
>> + // identified by GUID.
>> + //
>> + EFI_FIRMWARE_VOLUME2_PROTOCOL *FvProtocol;
>> + MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *FvFileNode;
>> + UINTN BufferSize;
>> + EFI_FV_FILETYPE FoundType;
>> + EFI_FV_FILE_ATTRIBUTES FileAttributes;
>> + UINT32 AuthenticationStatus;
>> +
>> + Status = gBS->HandleProtocol (FvHandle,
>> &gEfiFirmwareVolume2ProtocolGuid,
>> + (VOID **)&FvProtocol);
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + FvFileNode = (MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)Node2;
>> + //
>> + // Buffer==NULL means we request metadata only: BufferSize, FoundType,
>> + // FileAttributes.
>> + //
>> + Status = FvProtocol->ReadFile (
>> + FvProtocol,
>> + &FvFileNode->FvFileName, // NameGuid
>> + NULL, // Buffer
>> + &BufferSize,
>> + &FoundType,
>> + &FileAttributes,
>> + &AuthenticationStatus
>> + );
>> + if (!EFI_ERROR (Status)) {
>> + //
>> + // The FvFile was found. Keep the boot option.
>> + //
>> + continue;
>> + }
>> + }
>> +
>> + //
>> + // Delete the boot option.
>> + //
>> + Status = EfiBootManagerDeleteLoadOptionVariable (
>> + BootOptions[Index].OptionNumber, LoadOptionTypeBoot);
>
> So I suppose deleting by index does not reshuffle the list?
That's right. The OptionNumber field is the "Boot####" variable's
number, and EfiBootManagerDeleteLoadOptionVariable() works on the
"BootOrder" and "Boot####" variables themselves.
The BootOptions array that we have in memory at this time is
independent. The "BootOrder" variable is shrunken, but the next call
will again supply a "Boot####" variable number, which is looked up from
scratch in "BootOrder".
>
>> + DEBUG_CODE (
>> + CHAR16 *DevicePathString;
>> +
>> + DevicePathString =
>> ConvertDevicePathToText(BootOptions[Index].FilePath,
>> + FALSE, FALSE);
>> + DEBUG ((
>> + EFI_ERROR (Status) ? EFI_D_WARN : EFI_D_VERBOSE,
>> + "%a: removing stale Boot#%04x %s: %r\n",
>> + __FUNCTION__,
>> + (UINT32)BootOptions[Index].OptionNumber,
>> + DevicePathString == NULL ? L"<unavailable>" : DevicePathString,
>> + Status
>> + ));
>> + if (DevicePathString != NULL) {
>> + FreePool (DevicePathString);
>> + }
>> + );
>
> Indentation?
This was intentional on my part; the canonical edk2 style requires
BlahBlah (
Foo,
Bar,
Quux
);
Are you suggesting we shouldn't follow it in this case, with DEBUG_CODE?
If so I hope you'd be okay with me fixing up that one trailing ")"
before pushing the patch :)
Thanks!
Laszlo
>
>> + }
>> +
>> + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
>> +}
>> +
>> +
>> STATIC
>> VOID
>> PlatformRegisterOptionsAndKeys (
>> @@ -560,6 +691,7 @@ PlatformBootManagerAfterConsole (
>> PcdGetPtr (PcdShellFile), L"EFI Internal Shell", LOAD_OPTION_ACTIVE
>> );
>>
>> + RemoveStaleFvFileOptions ();
>> SetBootOrderFromQemu ();
>> }
>>
>> --
>> 1.8.3.1
>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
