| Message ID | 1468266651-16178-1-git-send-email-lersek@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On 07/12/16 04:36, Ni, Ruiyu wrote: > Laszlo, > OVMF uses old style of FV device path node: MemoryMapped(...). > If you check Nt32Pkg/Nt32Pkg.fdf, there is FvNameGuid field under[FV] section. > ----- > [FV.FvRecovery] > ...... > FvNameGuid = 6D99E806-3D38-42c2-A095-5F4300BFD7DC > ---- > > If you add FvNameGuid field under [FV] section for OVMF, the firmware will > produce Fv(...) device path node for FV. > Which is also the recommended way to identify a FV because it won't cause > the FV boot option file path changing across boot. > > You patch looks good but it assumes a FV boot option should always starts > with MemoryMapped(...) node, which is not always TRUE. > > I recommend you change FDF to add FvNameGuid field to [FV] section > and then just use the following algorithm to remove stale options: > > For each option: > Node = option.FilePath > Status = gBS->LocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid, > &Node, &FvHandle); > If (EFI_ERROR (Status)) { > // Current one is not a FV boot option > Continue; > } > //... Use Fv->ReadFile as what you did in below patch to check. > > With the above code, you don't need to check whether the first node is > a MemoryMapped(...) node and second node is a FvFile(...) node. This sounds great, but my concern is that then the code would not cover UEFI shell boot options that have accumulated in user-side variable stores thus far. The idea I had for this patch was to eliminate all future proliferation, but also kill off any stale options that might already exist. This bug was reported to me by our QE department, and it would be nice if I could just give them a package update without asking them to delete boot options / varstores manually. I guess I could tell them to do that "one last time" before it becomes automatic... Hm. :) The most robust solution would be to implement FvNameGuid (and the dependent code) *and* the currently proposed code as well. I think I should be able to do that; it boils down to setting the SearchNode variable flexibly enough. Also, thinking about your suggestion above, for maximum robustness I'd then like to prepare the code for the case when we change FvNameGuid itself. I don't see any reason why we would do that, but it would be best to write this code once and then forget about it. I'll have to experiment with the code to get more concrete thoughts. :) Thanks! Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Tuesday, July 12, 2016 3:51 AM >> To: edk2-devel-01 <edk2-devel@ml01.01.org> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gary Lin <glin@suse.com>; >> Justen, Jordan L <jordan.l.justen@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> >> Subject: [PATCH] OvmfPkg/PlatformBootManagerLib: remove stale FvFile >> boot options >> >> Changing the base address or the size of DXEFV, changing PcdShellFile, or >> omitting the UEFI Shell from the OVMF binary, all lead to "EFI Internal Shell" >> boot options that are technically different from each other and do 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. >> >> This functionality is not added to QemuBootOrderLib, because it is >> independent from QEMU and fw_cfg. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Gary Lin <glin@suse.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> Notes: >> Once this patch converges, I'll port it to ArmVirtPkg. >> >> OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | >> 1 + >> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 115 >> ++++++++++++++++++++ >> 2 files changed, 116 insertions(+) >> >> diff --git >> a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> index ffa1288e4d1a..f3303b9f2cf3 100644 >> --- >> a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> +++ >> b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> @@ -72,6 +72,7 @@ [Protocols] >> gEfiS3SaveStateProtocolGuid # PROTOCOL >> SOMETIMES_CONSUMED >> gEfiDxeSmmReadyToLockProtocolGuid # PROTOCOL >> SOMETIMES_PRODUCED >> gEfiLoadedImageProtocolGuid # PROTOCOL >> SOMETIMES_PRODUCED >> + gEfiFirmwareVolume2ProtocolGuid # PROTOCOL >> SOMETIMES_CONSUMED >> >> [Guids] >> gEfiXenInfoGuid >> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c >> b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c >> index 912c5ed1ece4..727c8b4792c0 100644 >> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c >> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c >> @@ -15,6 +15,7 @@ >> #include "BdsPlatform.h" >> #include <Guid/XenInfo.h> >> #include <Guid/RootBridgesConnectedEventGroup.h> >> +#include <Protocol/FirmwareVolume2.h> >> >> >> // >> @@ -149,6 +150,119 @@ PlatformRegisterFvBootOption ( >> EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); } >> >> +/** >> + Remove all MemoryMapped(...)/FvFile(...) boot options whose device >> +paths do >> + not resolve exactly to an FvFile in the system. >> + >> + This prevents the proliferation of UEFI Shell boot options if DXEFV's >> +base >> + address or size changes, or a different FILE_GUID binary is used as Shell. >> + EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() >> +only >> + avoids exact duplicates. >> +**/ >> +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; >> + >> + Node1 = BootOptions[Index].FilePath; >> + if (DevicePathType (Node1) != HARDWARE_DEVICE_PATH || >> + DevicePathSubType (Node1) != HW_MEMMAP_DP) { >> + continue; >> + } >> + >> + 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); >> + 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); >> + } >> + ); >> + } >> + >> + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); } >> + >> VOID >> PlatformRegisterOptionsAndKeys ( >> VOID >> @@ -1343,6 +1457,7 @@ Routine Description: >> PcdGetPtr (PcdShellFile), L"EFI Internal Shell", LOAD_OPTION_ACTIVE >> ); >> >> + RemoveStaleFvFileOptions (); >> SetBootOrderFromQemu (); >> } >> >> -- >> 1.8.3.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf index ffa1288e4d1a..f3303b9f2cf3 100644 --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf @@ -72,6 +72,7 @@ [Protocols] gEfiS3SaveStateProtocolGuid # PROTOCOL SOMETIMES_CONSUMED gEfiDxeSmmReadyToLockProtocolGuid # PROTOCOL SOMETIMES_PRODUCED gEfiLoadedImageProtocolGuid # PROTOCOL SOMETIMES_PRODUCED + gEfiFirmwareVolume2ProtocolGuid # PROTOCOL SOMETIMES_CONSUMED [Guids] gEfiXenInfoGuid diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c index 912c5ed1ece4..727c8b4792c0 100644 --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c @@ -15,6 +15,7 @@ #include "BdsPlatform.h" #include <Guid/XenInfo.h> #include <Guid/RootBridgesConnectedEventGroup.h> +#include <Protocol/FirmwareVolume2.h> // @@ -149,6 +150,119 @@ PlatformRegisterFvBootOption ( EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); } +/** + Remove all MemoryMapped(...)/FvFile(...) boot options whose device paths do + not resolve exactly to an FvFile in the system. + + This prevents the proliferation of UEFI Shell boot options if DXEFV's base + address or size changes, or a different FILE_GUID binary is used as Shell. + EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() only + avoids exact duplicates. +**/ +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; + + Node1 = BootOptions[Index].FilePath; + if (DevicePathType (Node1) != HARDWARE_DEVICE_PATH || + DevicePathSubType (Node1) != HW_MEMMAP_DP) { + continue; + } + + 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); + 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); + } + ); + } + + EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); +} + VOID PlatformRegisterOptionsAndKeys ( VOID @@ -1343,6 +1457,7 @@ Routine Description: PcdGetPtr (PcdShellFile), L"EFI Internal Shell", LOAD_OPTION_ACTIVE ); + RemoveStaleFvFileOptions (); SetBootOrderFromQemu (); }
Changing the base address or the size of DXEFV, changing PcdShellFile, or omitting the UEFI Shell from the OVMF binary, all lead to "EFI Internal Shell" boot options that are technically different from each other and do 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. This functionality is not added to QemuBootOrderLib, because it is independent from QEMU and fw_cfg. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Gary Lin <glin@suse.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Once this patch converges, I'll port it to ArmVirtPkg. OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 + OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 115 ++++++++++++++++++++ 2 files changed, 116 insertions(+) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel