On 10/10/23 17:06, Aaron Young wrote:
> Reference: https://github.com/tianocore/edk2/pull/4892
>
> BmExpandPartitionDevicePath is called to expand "short-form" device paths
> which are commonly used with OS boot options. To expand a device path, it
> calls EfiBootManagerConnectAll to connect all the possible BlockIo
> devices in the system to search for a matching partition. However, this
> is sometimes unnecessary on certain platforms (such as OVMF/QEMU) because
> the boot devices are previously explicity connected
> (See: ConnectDevicesFromQemu). EfiBootManagerConnectAll calls are
> extremely costly in terms of boot time and resources and should be avoided
> whenever feasible.
>
> Therefore optimize BmExpandPartitionDevicePath to first search the
> existing BlockIo handles for a match. If a match is not found, then
> fallback to the original code to call EfiBootManagerConnectAll and search
> again. Thus, this optimization should be extremely low-risk given the
> fallback to previous behavior.
I'm not going to look at the code part for now, but this sounds like a
very good idea.
For reference, the call tree is as follows:
PlatformBootManagerAfterConsole()
[OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
PlatformBdsConnectSequence()
[OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
ConnectDevicesFromQemu()
[OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
...
EfiBootManagerRefreshAllBootOption()
[MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
...
SetBootOrderFromQemu()
[OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
Match()
[OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c]
EfiBootManagerGetLoadOptionBuffer()
[MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
BmGetNextLoadOptionBuffer()
[MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c]
BmGetNextLoadOptionDevicePath()
[MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
BmExpandPartitionDevicePath()
[MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
Perhaps capture this in the commit message, if you agree it's useful.
An *additional* pain point for me has been the need to call
EfiBootManagerGetLoadOptionBuffer() from Match() -- because
EfiBootManagerGetLoadOptionBuffer() used to be the *only*
UefiBootManagerLib API that exposed the short-form completion logic.
EfiBootManagerGetLoadOptionBuffer() is suboptimal because it doesn't
only complete short-form device paths, but it also loads the referenced
file -- which is totally unnecessary for Match(), so we release the
lodaded buffer immediately.
I *think* what we're actually after is the
BmGetNextLoadOptionDevicePath() function -- but it is not exposed by the
UefiBootManagerLib class.
However, it seems that in commits b4e1ad87d0ee
("MdeModulePkg/CapsuleApp: Add a function used to get next DevicePath",
2019-01-31) and 68a4e15e1497 ("MdeModulePkg: Rename confusion function
name", 2019-02-25), BmGetNextLoadOptionDevicePath() got effectively
exposed as EfiBootManagerGetNextLoadOptionDevicePath().
And now I'm thinking, perhaps we could get away with calling
AbsDevicePath = EfiBootManagerGetNextLoadOptionDevicePath (
DevicePath,
NULL
);
from Match(), instead of
FileBuffer = EfiBootManagerGetLoadOptionBuffer (
DevicePath,
&AbsDevicePath,
&FileSize
);
? That would stop us from needlessly loading the file contents!
Can you investigate that as well, perhaps? :)
(The second parameter of EfiBootManagerGetNextLoadOptionDevicePath()
only matters if we are interested in multiple expansions of the same
short-form device path. If we pass a non-NULL device path in the second
parameter, then EfiBootManagerGetNextLoadOptionDevicePath() will first
locate *that* absoute device path in the possible resolutions, and
return the *next* one after that. It's pretty ineffective, but it lets
the caller cycle through all potential absolute resolutions for the same
short path. BmGetNextLoadOptionBuffer() contains such a loop / iteration
internally, but I'm not sure if we need that. Hmmm... there's an example
in "MdeModulePkg/Application/CapsuleApp/CapsuleOnDisk.c", but that call
site is also wrapped in a loop! So EfiBootManagerGetLoadOptionBuffer()
seems *safer* after all...)
Thanks!
Laszlo
>
> NOTE: The existing optimization in the code to use a "HDDP" variable to
> save the last matched device paths does not cover the first time a boot
> option is expanded (i.e. before the "HDDP" is created) nor when the device
> configuration has changed (resulting in the boot device moving to a
> different location in the PCI Bus/Dev hierarchy). This new optimization
> covers both of these cases on requisite platforms which explicity connect
> boot devices.
>
> In our testing on OVMF/QEMU VMs with dozens of configured vnic devices,
> these extraneous calls to EfiBootManagerConnectAll from
> BmExpandPartitionDevicePath were found to cause many seconds (or even
> minutes) of additional VM boot time in some cases - due to the vnics
> being unnecessarily connected.
>
> Cc: Zhichao Gao zhichao....@intel.com
> Cc: Ray Ni ray...@intel.com
> Signed-off-by: Aaron Young <aaron.yo...@oracle.com>
> ---
> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 90 ++++++++++++--------
> 1 file changed, 56 insertions(+), 34 deletions(-)
>
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index c3763c4483c7..7a97f7cdcc6b 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -880,6 +880,8 @@ BmExpandPartitionDevicePath (
> BOOLEAN NeedAdjust;
> EFI_DEVICE_PATH_PROTOCOL *Instance;
> UINTN Size;
> + BOOLEAN MatchFound;
> + BOOLEAN ConnectAllAttempted;
>
> //
> // Check if there is prestore 'HDDP' variable.
> @@ -974,49 +976,69 @@ BmExpandPartitionDevicePath (
> // If we get here we fail to find or 'HDDP' not exist, and now we need
> // to search all devices in the system for a matched partition
> //
> - EfiBootManagerConnectAll ();
> - Status = gBS->LocateHandleBuffer (ByProtocol, &gEfiBlockIoProtocolGuid,
> NULL, &BlockIoHandleCount, &BlockIoBuffer);
> - if (EFI_ERROR (Status)) {
> - BlockIoHandleCount = 0;
> - BlockIoBuffer = NULL;
> - }
> -
> - //
> - // Loop through all the device handles that support the BLOCK_IO Protocol
> - //
> - for (Index = 0; Index < BlockIoHandleCount; Index++) {
> - BlockIoDevicePath = DevicePathFromHandle (BlockIoBuffer[Index]);
> - if (BlockIoDevicePath == NULL) {
> - continue;
> + BlockIoBuffer = NULL;
> + MatchFound = FALSE;
> + ConnectAllAttempted = FALSE;
> + do {
> + if (BlockIoBuffer != NULL) {
> + FreePool (BlockIoBuffer);
> }
>
> - if (BmMatchPartitionDevicePathNode (BlockIoDevicePath,
> (HARDDRIVE_DEVICE_PATH *)FilePath)) {
> - //
> - // Find the matched partition device path
> - //
> - TempDevicePath = AppendDevicePath (BlockIoDevicePath,
> NextDevicePathNode (FilePath));
> - FullPath = BmGetNextLoadOptionDevicePath (TempDevicePath, NULL);
> - FreePool (TempDevicePath);
> + Status = gBS->LocateHandleBuffer (ByProtocol, &gEfiBlockIoProtocolGuid,
> NULL, &BlockIoHandleCount, &BlockIoBuffer);
> + if (EFI_ERROR (Status)) {
> + BlockIoHandleCount = 0;
> + BlockIoBuffer = NULL;
> + }
>
> - if (FullPath != NULL) {
> - BmCachePartitionDevicePath (&CachedDevicePath, BlockIoDevicePath);
> + //
> + // Loop through all the device handles that support the BLOCK_IO Protocol
> + //
> + for (Index = 0; Index < BlockIoHandleCount; Index++) {
> + BlockIoDevicePath = DevicePathFromHandle (BlockIoBuffer[Index]);
> + if (BlockIoDevicePath == NULL) {
> + continue;
> + }
>
> + if (BmMatchPartitionDevicePathNode (BlockIoDevicePath,
> (HARDDRIVE_DEVICE_PATH *)FilePath)) {
> //
> - // Save the matching Device Path so we don't need to do a connect
> all next time
> - // Failing to save only impacts performance next time expanding the
> short-form device path
> + // Find the matched partition device path
> //
> - Status = gRT->SetVariable (
> - L"HDDP",
> - &mBmHardDriveBootVariableGuid,
> - EFI_VARIABLE_BOOTSERVICE_ACCESS |
> EFI_VARIABLE_NON_VOLATILE,
> - GetDevicePathSize (CachedDevicePath),
> - CachedDevicePath
> - );
> + TempDevicePath = AppendDevicePath (BlockIoDevicePath,
> NextDevicePathNode (FilePath));
> + FullPath = BmGetNextLoadOptionDevicePath (TempDevicePath,
> NULL);
> + FreePool (TempDevicePath);
> +
> + if (FullPath != NULL) {
> + BmCachePartitionDevicePath (&CachedDevicePath, BlockIoDevicePath);
>
> - break;
> + //
> + // Save the matching Device Path so we don't need to do a connect
> all next time
> + // Failing to save only impacts performance next time expanding
> the short-form device path
> + //
> + Status = gRT->SetVariable (
> + L"HDDP",
> + &mBmHardDriveBootVariableGuid,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + GetDevicePathSize (CachedDevicePath),
> + CachedDevicePath
> + );
> + MatchFound = TRUE;
> + break;
> + }
> }
> }
> - }
> +
> + //
> + // If we found a matching BLOCK_IO handle or we've already
> + // tried a ConnectAll, we are done searching.
> + //
> + if (MatchFound || ConnectAllAttempted) {
> + break;
> + }
> +
> + EfiBootManagerConnectAll ();
> + ConnectAllAttempted = TRUE;
> + } while (1);
>
> if (CachedDevicePath != NULL) {
> FreePool (CachedDevicePath);
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109501): https://edk2.groups.io/g/devel/message/109501
Mute This Topic: https://groups.io/mt/101876973/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
