[edk2-devel] [PATCH 1/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface

[edk2-devel] [PATCH 1/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface

[Zhi Jin]

CoreGetProtocolInterface() is called by CoreOpenProtocol(),
CoreCloseProtocol() and CoreOpenProtocolInformation().
Before CoreOpenProtocol() calls CoreGetProtocolInterface(), the input
parameter UserHandle has been already checked for validation. So does
CoreCloseProtocol().
Removing the handle validation check in CoreGetProtocolInterface()
could improve the performance, as CoreOpenProtocol() is called very
frequently.
Meanwhile, need to make it the caller's responsibility to check the
parameters, and add the check in CoreOpenProtocolInformation().

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhi Jin <zhi.jin@intel.com>
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 51e5b5d3b3..a0d2d03267 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -916,6 +916,8 @@ CoreUninstallMultipleProtocolInterfaces (
 
 /**
   Locate a certain GUID protocol interface in a Handle's protocols.
+  Note: This function doesn't do parameters checking, it's caller's responsibility
+  to pass in valid parameters.
 
   @param  UserHandle             The handle to obtain the protocol interface on
   @param  Protocol               The GUID of the protocol
@@ -929,17 +931,11 @@ CoreGetProtocolInterface (
   IN  EFI_GUID    *Protocol
   )
 {
-  EFI_STATUS          Status;
   PROTOCOL_ENTRY      *ProtEntry;
   PROTOCOL_INTERFACE  *Prot;
   IHANDLE             *Handle;
   LIST_ENTRY          *Link;
 
-  Status = CoreValidateHandle (UserHandle);
-  if (EFI_ERROR (Status)) {
-    return NULL;
-  }
-
   Handle = (IHANDLE *)UserHandle;
 
   //
@@ -1392,6 +1388,15 @@ CoreOpenProtocolInformation (
   //
   CoreAcquireProtocolLock ();
 
+  //
+  // Check for invalid UserHandle
+  //
+  Status = CoreValidateHandle (UserHandle);
+  if (EFI_ERROR (Status)) {
+    Status = EFI_NOT_FOUND;
+    goto Done;
+  }
+
   //
   // Look at each protocol interface for a match
   //
-- 
2.39.2



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

[edk2-devel] [PATCH 2/2] MdeModulePkg: Optimize CoreConnectSingleController

[Zhi Jin]

CoreConnectSingleController() searches for the Driver Family Override
Protocol drivers by looping and checking each Driver Binding Handles.
This loop can be skipped by checking if any Driver Family Override
Protocol installed in the platform first, to improve the performance.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhi Jin <zhi.jin@intel.com>
---
 MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index 0b824c62b7..64d7474f15 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -497,7 +497,12 @@ CoreConnectSingleController (
   //
   // Add the Driver Family Override Protocol drivers for ControllerHandle
   //
-  while (TRUE) {
+  Status = CoreLocateProtocol (
+             &gEfiDriverFamilyOverrideProtocolGuid,
+             NULL,
+             (VOID **)&DriverFamilyOverride
+             );
+  while (!EFI_ERROR (Status) && (DriverFamilyOverride != NULL)) {
     HighestIndex   = DriverBindingHandleCount;
     HighestVersion = 0;
     for (Index = 0; Index < DriverBindingHandleCount; Index++) {
-- 
2.39.2



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

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg: Optimize CoreConnectSingleController

[Michael D Kinney]

I agree that this implements the similar check as other 
optional protocols to adjust driver binding order to skip
checks for which where are no instances of the optional
protocol.

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>




> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhi Jin
> Sent: Tuesday, January 16, 2024 10:45 PM
> To: devel@edk2.groups.io
> Cc: Jin, Zhi <zhi.jin@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH 2/2] MdeModulePkg: Optimize
> CoreConnectSingleController
> 
> CoreConnectSingleController() searches for the Driver Family Override
> Protocol drivers by looping and checking each Driver Binding Handles.
> This loop can be skipped by checking if any Driver Family Override
> Protocol installed in the platform first, to improve the performance.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> index 0b824c62b7..64d7474f15 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> +++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> @@ -497,7 +497,12 @@ CoreConnectSingleController (
>    //
>    // Add the Driver Family Override Protocol drivers for
> ControllerHandle
>    //
> -  while (TRUE) {
> +  Status = CoreLocateProtocol (
> +             &gEfiDriverFamilyOverrideProtocolGuid,
> +             NULL,
> +             (VOID **)&DriverFamilyOverride
> +             );
> +  while (!EFI_ERROR (Status) && (DriverFamilyOverride != NULL)) {
>      HighestIndex   = DriverBindingHandleCount;
>      HighestVersion = 0;
>      for (Index = 0; Index < DriverBindingHandleCount; Index++) {
> --
> 2.39.2
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114106): https://edk2.groups.io/g/devel/message/114106
Mute This Topic: https://groups.io/mt/103781274/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/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface

[Michael D Kinney]

Hi Zhi,

Some comments below.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhi Jin
> Sent: Tuesday, January 16, 2024 10:45 PM
> To: devel@edk2.groups.io
> Cc: Jin, Zhi <zhi.jin@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg: Remove the handle
> validation check in CoreGetProtocolInterface
> 
> CoreGetProtocolInterface() is called by CoreOpenProtocol(),
> CoreCloseProtocol() and CoreOpenProtocolInformation().
> Before CoreOpenProtocol() calls CoreGetProtocolInterface(), the input
> parameter UserHandle has been already checked for validation. So does
> CoreCloseProtocol().
> Removing the handle validation check in CoreGetProtocolInterface()
> could improve the performance, as CoreOpenProtocol() is called very
> frequently.
> Meanwhile, need to make it the caller's responsibility to check the
> parameters, and add the check in CoreOpenProtocolInformation().
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> index 51e5b5d3b3..a0d2d03267 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> @@ -916,6 +916,8 @@ CoreUninstallMultipleProtocolInterfaces (
> 
>  /**
>    Locate a certain GUID protocol interface in a Handle's protocols.
> +  Note: This function doesn't do parameters checking, it's caller's
> responsibility
> +  to pass in valid parameters.
> 
>    @param  UserHandle             The handle to obtain the protocol
> interface on

Instead of a Note:, I recommend the description of this parameter
be updated to state that the caller must pass in a valid UserHandle
that is checked with CoreValidateHandle().

Also, this API CoreGetProtocolInterface() is an internal helper 
function.  I also recommend that this function be declared 'static'
so the scope of the assumption that UserHandle is valid is only 
by calls from other functions in this same file.

>    @param  Protocol               The GUID of the protocol
> @@ -929,17 +931,11 @@ CoreGetProtocolInterface (
>    IN  EFI_GUID    *Protocol
>    )
>  {
> -  EFI_STATUS          Status;
>    PROTOCOL_ENTRY      *ProtEntry;
>    PROTOCOL_INTERFACE  *Prot;
>    IHANDLE             *Handle;
>    LIST_ENTRY          *Link;
> 
> -  Status = CoreValidateHandle (UserHandle);
> -  if (EFI_ERROR (Status)) {
> -    return NULL;
> -  }
> -
>    Handle = (IHANDLE *)UserHandle;
> 
>    //
> @@ -1392,6 +1388,15 @@ CoreOpenProtocolInformation (
>    //
>    CoreAcquireProtocolLock ();
> 
> +  //
> +  // Check for invalid UserHandle
> +  //
> +  Status = CoreValidateHandle (UserHandle);
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_NOT_FOUND;
> +    goto Done;
> +  }
> +
>    //
>    // Look at each protocol interface for a match
>    //
> --
> 2.39.2
> 
> 
> 
> 
> 



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