On 13 July 2016 at 22:17, Laszlo Ersek <[email protected]> wrote:
> 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".
>
OK, thanks for clarifying.
>>
>>> + 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 :)
>
I don't care too deeply, tbh, it is just something that stands out
when looking at the patch.
So leave it, or change it, as you prefer.
Reviewed-by: Ard Biesheuvel <[email protected]>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
