The patch looks good to me. Reviewed-by: Zhichao Gao <zhichao....@intel.com>
The comments of this patch and proposal would be taken care in another patch. Better to have a Bugzilla record for the planned change. Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Aaron > Young > Sent: Tuesday, October 10, 2023 11:07 PM > To: devel@edk2.groups.io > Subject: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize > BmExpandPartitionDevicePath > > 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. > > 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);-- > 2.39.3 > > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#109496): > https://edk2.groups.io/g/devel/message/109496 > Mute This Topic: https://groups.io/mt/101876973/1768756 > Group Owner: devel+ow...@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao....@intel.com] > -=-=-=-=-=-= > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109979): https://edk2.groups.io/g/devel/message/109979 Mute This Topic: https://groups.io/mt/101876973/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
