[edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[Aaron Young]

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.gao@intel.com
Cc: Ray Ni ray.ni@intel.com
Signed-off-by: Aaron Young <aaron.young@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[Laszlo Ersek]

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.gao@intel.com
> Cc: Ray Ni ray.ni@intel.com
> Signed-off-by: Aaron Young <aaron.young@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[Aaron Young via groups.io]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

Thanks for the comments, Laszlo.

I could look into optimizing SetBootOrderFromQemu()->Match() by using
EfiBootManagerGetNextLoadOptionDevicePath() instead of EfiBootManagerGetLoadOptionBuffer().
Makes sense to me and at first glance seems like it would work. My main concern is some
unforeseen change in behavior that manifests in a regression somehow. Would require lots
of testing beyond what I am capable of doing.
However, I'd prefer to do this as a separate task from this PR as it's not really
related, right? i.e. EfiBootManagerGetNextLoadOptionDevicePath() still ends up
calling ConnectAll. just making sure I understand.

Also, I can look into amending the PR commit to add the call chain for ConnectDevicesFromQemu
that you menioned above.

thanks again!

-Aaron


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109526): https://edk2.groups.io/g/devel/message/109526
Mute This Topic: https://groups.io/mt/101876973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 1845 bytes --]

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[Laszlo Ersek]

On 10/11/23 19:36, Aaron Young wrote:
> 
>  Thanks for the comments, Laszlo.
> 
>  I could look into optimizing SetBootOrderFromQemu()->Match() by using
>  EfiBootManagerGetNextLoadOptionDevicePath() instead of
> EfiBootManagerGetLoadOptionBuffer().
>  Makes sense to me and at first glance seems like it would work. My main
> concern is some
>  unforeseen change in behavior that manifests in a regression somehow.
> Would require lots
>  of testing beyond what I am capable of doing.

Yes, this is exactly my concern as well.
EfiBootManagerGetLoadOptionBuffer() contains an internal loop around
EfiBootManagerGetNextLoadOptionDevicePath(), and some short form -->
long form expansions that succeed right now may depend on that loop. If
we just replace the current call to EfiBootManagerGetLoadOptionBuffer()
without repeating the same loop, we could regress things.

>  However, I'd prefer to do this as a separate task from this PR as it's
> not really
>  related, right? i.e. EfiBootManagerGetNextLoadOptionDevicePath() still
> ends up
>  calling ConnectAll. just making sure I understand.

Sure, this is totally unrelated. And, it's not a functional issue, just
an opportunity for some slight optimization (at the risk of a functional
regression, of course). So definitely don't get into it without proper
time allocation.

That's why I'm not doing it myself either! I just thought that you might
be able to consider it, given that you're looking at
BmExpandPartitionDevicePath() already. But, certainly feel free to
ignore it.

Basically I wish UefiBootManagerLib offered an interface that did *all*
of what EfiBootManagerGetLoadOptionBuffer() does, except loading the
file contents. But extending UefiBootManagerLib with new interfaces is
difficult.

> 
>  Also, I can look into amending the PR commit to add the call chain for
> ConnectDevicesFromQemu
>  that you menioned above.

That would be welcome.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109575): https://edk2.groups.io/g/devel/message/109575
Mute This Topic: https://groups.io/mt/101876973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[Gao, Zhichao]

The patch looks good to me.
Reviewed-by: Zhichao Gao <zhichao.gao@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.gao@intel.com
> Cc: Ray Ni ray.ni@intel.com
> Signed-off-by: Aaron Young <aaron.young@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+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[Laszlo Ersek]

On 10/24/23 04:18, Gao, Zhichao wrote:
> The patch looks good to me.
> Reviewed-by: Zhichao Gao <zhichao.gao@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.

I've submitted <https://github.com/tianocore/edk2/pull/4948> for merging
this.

I worked the OVMF call tree into the commit message.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109999): https://edk2.groups.io/g/devel/message/109999
Mute This Topic: https://groups.io/mt/101876973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Optimize BmExpandPartitionDevicePath

[Laszlo Ersek]

On 10/24/23 14:29, Laszlo Ersek wrote:
> On 10/24/23 04:18, Gao, Zhichao wrote:
>> The patch looks good to me.
>> Reviewed-by: Zhichao Gao <zhichao.gao@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.
> 
> I've submitted <https://github.com/tianocore/edk2/pull/4948> for merging
> this.
> 
> I worked the OVMF call tree into the commit message.

Commit a6648418c160.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110006): https://edk2.groups.io/g/devel/message/110006
Mute This Topic: https://groups.io/mt/101876973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-