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

2024-01-23 Thread Michael D Kinney
Hi Zhi,

Thanks for the update.  One minor comment on 'STATIC' should be 'static'.

We no longer depend on macros to redefine 'STATIC' and it is better to
just used the standard C 'static' form.

With that change:

Reviewed-by: Michael D Kinney 


Mike

> -Original Message-
> From: Jin, Zhi 
> Sent: Monday, January 22, 2024 1:53 AM
> To: devel@edk2.groups.io
> Cc: Jin, Zhi ; Liming Gao ;
> Ni, Ray ; Kinney, Michael D
> 
> Subject: [PATCH v2 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.
> To ensure the assumption that the caller of CoreGetProtocolInterface()
> must pass in a valid UserHandle that is checked with
> CoreValidateHandle(),
> add the parameter check in CoreOpenProtocolInformation(), and declare
> CoreGetProtocolInterface() as static.
> 
> v1 -> v2:
>   1. Update the description of UserHandle to state that the caller
>  must pass in a valid UserHandle that is checked with
>  CoreValidateHandle().
>   2. Declare CoreGetProtocolInterface() as static.
> 
> Cc: Liming Gao 
> Cc: Ray Ni 
> Cc: Michael D Kinney 
> Signed-off-by: Zhi Jin 
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> index 51e5b5d3b3..24e4fbf5f3 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> @@ -918,28 +918,25 @@ CoreUninstallMultipleProtocolInterfaces (
>Locate a certain GUID protocol interface in a Handle's protocols.
> 
>@param  UserHandle The handle to obtain the protocol
> interface on
> + The caller must pass in a valid
> UserHandle that
> + is checked with CoreValidateHandle().
>@param  Protocol   The GUID of the protocol
> 
>@return The requested protocol interface for the handle
> 
>  **/
> +STATIC
>  PROTOCOL_INTERFACE  *
>  CoreGetProtocolInterface (
>IN  EFI_HANDLE  UserHandle,
>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 +1389,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 (#114234): https://edk2.groups.io/g/devel/message/114234
Mute This Topic: https://groups.io/mt/103883257/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




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

2024-01-22 Thread 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.
To ensure the assumption that the caller of CoreGetProtocolInterface()
must pass in a valid UserHandle that is checked with CoreValidateHandle(),
add the parameter check in CoreOpenProtocolInformation(), and declare
CoreGetProtocolInterface() as static.

v1 -> v2:
  1. Update the description of UserHandle to state that the caller
 must pass in a valid UserHandle that is checked with
 CoreValidateHandle().
  2. Declare CoreGetProtocolInterface() as static.

Cc: Liming Gao 
Cc: Ray Ni 
Cc: Michael D Kinney 
Signed-off-by: Zhi Jin 
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c 
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 51e5b5d3b3..24e4fbf5f3 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -918,28 +918,25 @@ CoreUninstallMultipleProtocolInterfaces (
   Locate a certain GUID protocol interface in a Handle's protocols.
 
   @param  UserHandle The handle to obtain the protocol interface on
+ The caller must pass in a valid UserHandle 
that
+ is checked with CoreValidateHandle().
   @param  Protocol   The GUID of the protocol
 
   @return The requested protocol interface for the handle
 
 **/
+STATIC
 PROTOCOL_INTERFACE  *
 CoreGetProtocolInterface (
   IN  EFI_HANDLE  UserHandle,
   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 +1389,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 (#114137): https://edk2.groups.io/g/devel/message/114137
Mute This Topic: https://groups.io/mt/103883257/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-