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

[edk2-devel] [PATCH v2 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.
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 <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Zhi Jin <zhi.jin@intel.com>
---
 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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>
Reviewed-by: Michael D Kinney <michael.d.kinney@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 (#114138): https://edk2.groups.io/g/devel/message/114138
Mute This Topic: https://groups.io/mt/103883258/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[gaoliming via groups.io]

Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Zhi Jin
> 发送时间: 2024年1月22日 17:53
> 收件人: devel@edk2.groups.io
> 抄送: Zhi Jin <zhi.jin@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Ray Ni <ray.ni@intel.com>; Michael D Kinney <michael.d.kinney@intel.com>
> 主题: [edk2-devel] [PATCH v2 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>
> Reviewed-by: Michael D Kinney <michael.d.kinney@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 (#114840): https://edk2.groups.io/g/devel/message/114840
Mute This Topic: https://groups.io/mt/104067208/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[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 <michael.d.kinney@intel.com>


Mike

> -----Original Message-----
> From: Jin, Zhi <zhi.jin@intel.com>
> Sent: Monday, January 22, 2024 1:53 AM
> To: devel@edk2.groups.io
> Cc: Jin, Zhi <zhi.jin@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> 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 <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> ---
>  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/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 v2 1/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface

[Zhi Jin]

Hi Mike,

	After changing 'STATIC' to 'static', I hit an error from the EccCheck Test as below:
		ERROR - EFI coding style error
		ERROR - *Error code: 5001
		ERROR - *Return type of a function should exist and in the first line
		ERROR - *file: /.../Build/.pytool/Plugin/EccCheck/MdeModulePkg/Core/Dxe/Hand/Handle.c
		ERROR - *Line number: 928
		ERROR - *[CoreGetProtocolInterface] Return Type should appear at the start of line

	Looks like 'STATIC' is treated as a modifier by EccCheck, while 'static' is not.
	In BaseTools/Source/Python/Ecc/config.ini:
		ModifierList = IN, OUT, OPTIONAL, UNALIGNED, EFI_RUNTIMESERVICE, EFI_BOOTSERVICE, EFIAPI, TPMINTERNALAPI, STATIC

	Should we add 'static' to ModifierList to fix this?

BRs
Zhi Jin

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Wednesday, January 24, 2024 6:22 AM
To: Jin, Zhi <zhi.jin@intel.com>; devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH v2 1/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface

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 <michael.d.kinney@intel.com>


Mike

> -----Original Message-----
> From: Jin, Zhi <zhi.jin@intel.com>
> Sent: Monday, January 22, 2024 1:53 AM
> To: devel@edk2.groups.io
> Cc: Jin, Zhi <zhi.jin@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> 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 <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> ---
>  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 (#114766): https://edk2.groups.io/g/devel/message/114766
Mute This Topic: https://groups.io/mt/103883257/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael D Kinney]

Hi Zhi,

Thanks for pointing this out.

For your patch, use 'STATIC'.

I do see that 'STATIC' is use consistently today.  If we want to move to consistent
use of 'static', that should be its own task to make that update across all the files
and update ECC to support both 'static' and 'STATIC' temporarily and later only
support 'static'.

Mike

> -----Original Message-----
> From: Jin, Zhi <zhi.jin@intel.com>
> Sent: Tuesday, January 30, 2024 1:14 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH v2 1/2] MdeModulePkg: Remove the handle validation
> check in CoreGetProtocolInterface
> 
> Hi Mike,
> 
> 	After changing 'STATIC' to 'static', I hit an error from the
> EccCheck Test as below:
> 		ERROR - EFI coding style error
> 		ERROR - *Error code: 5001
> 		ERROR - *Return type of a function should exist and in the
> first line
> 		ERROR - *file:
> /.../Build/.pytool/Plugin/EccCheck/MdeModulePkg/Core/Dxe/Hand/Handle.c
> 		ERROR - *Line number: 928
> 		ERROR - *[CoreGetProtocolInterface] Return Type should appear
> at the start of line
> 
> 	Looks like 'STATIC' is treated as a modifier by EccCheck, while
> 'static' is not.
> 	In BaseTools/Source/Python/Ecc/config.ini:
> 		ModifierList = IN, OUT, OPTIONAL, UNALIGNED,
> EFI_RUNTIMESERVICE, EFI_BOOTSERVICE, EFIAPI, TPMINTERNALAPI, STATIC
> 
> 	Should we add 'static' to ModifierList to fix this?
> 
> BRs
> Zhi Jin
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, January 24, 2024 6:22 AM
> To: Jin, Zhi <zhi.jin@intel.com>; devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v2 1/2] MdeModulePkg: Remove the handle validation
> check in CoreGetProtocolInterface
> 
> 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 <michael.d.kinney@intel.com>
> 
> 
> Mike
> 
> > -----Original Message-----
> > From: Jin, Zhi <zhi.jin@intel.com>
> > Sent: Monday, January 22, 2024 1:53 AM
> > To: devel@edk2.groups.io
> > Cc: Jin, Zhi <zhi.jin@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>;
> > Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > 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 <gaoliming@byosoft.com.cn>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> > ---
> >  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 (#114810): https://edk2.groups.io/g/devel/message/114810
Mute This Topic: https://groups.io/mt/103883257/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 v2 1/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface

[Zhi Jin]

Thanks, Mike.

PR (https://github.com/tianocore/edk2/pull/5321) has passed CI.

BRs
Zhi Jin

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Wednesday, January 31, 2024 1:56 AM
To: Jin, Zhi <zhi.jin@intel.com>; devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH v2 1/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface

Hi Zhi,

Thanks for pointing this out.

For your patch, use 'STATIC'.

I do see that 'STATIC' is use consistently today.  If we want to move to consistent
use of 'static', that should be its own task to make that update across all the files
and update ECC to support both 'static' and 'STATIC' temporarily and later only
support 'static'.

Mike

> -----Original Message-----
> From: Jin, Zhi <zhi.jin@intel.com>
> Sent: Tuesday, January 30, 2024 1:14 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH v2 1/2] MdeModulePkg: Remove the handle validation
> check in CoreGetProtocolInterface
> 
> Hi Mike,
> 
> 	After changing 'STATIC' to 'static', I hit an error from the
> EccCheck Test as below:
> 		ERROR - EFI coding style error
> 		ERROR - *Error code: 5001
> 		ERROR - *Return type of a function should exist and in the
> first line
> 		ERROR - *file:
> /.../Build/.pytool/Plugin/EccCheck/MdeModulePkg/Core/Dxe/Hand/Handle.c
> 		ERROR - *Line number: 928
> 		ERROR - *[CoreGetProtocolInterface] Return Type should appear
> at the start of line
> 
> 	Looks like 'STATIC' is treated as a modifier by EccCheck, while
> 'static' is not.
> 	In BaseTools/Source/Python/Ecc/config.ini:
> 		ModifierList = IN, OUT, OPTIONAL, UNALIGNED,
> EFI_RUNTIMESERVICE, EFI_BOOTSERVICE, EFIAPI, TPMINTERNALAPI, STATIC
> 
> 	Should we add 'static' to ModifierList to fix this?
> 
> BRs
> Zhi Jin
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, January 24, 2024 6:22 AM
> To: Jin, Zhi <zhi.jin@intel.com>; devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v2 1/2] MdeModulePkg: Remove the handle validation
> check in CoreGetProtocolInterface
> 
> 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 <michael.d.kinney@intel.com>
> 
> 
> Mike
> 
> > -----Original Message-----
> > From: Jin, Zhi <zhi.jin@intel.com>
> > Sent: Monday, January 22, 2024 1:53 AM
> > To: devel@edk2.groups.io
> > Cc: Jin, Zhi <zhi.jin@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>;
> > Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > 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 <gaoliming@byosoft.com.cn>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> > ---
> >  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 (#114845): https://edk2.groups.io/g/devel/message/114845
Mute This Topic: https://groups.io/mt/103883257/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[gaoliming via groups.io]

Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Zhi Jin
> 发送时间: 2024年1月22日 17:53
> 收件人: devel@edk2.groups.io
> 抄送: Zhi Jin <zhi.jin@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Ray Ni <ray.ni@intel.com>; Michael D Kinney <michael.d.kinney@intel.com>
> 主题: [edk2-devel] [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 <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> ---
>  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 (#114857): https://edk2.groups.io/g/devel/message/114857
Mute This Topic: https://groups.io/mt/104069905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: Wednesday, January 31, 2024 1:06 PM
> To: devel@edk2.groups.io; Jin, Zhi <zhi.jin@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: 回复: [edk2-devel] [PATCH v2 1/2] MdeModulePkg: Remove the
> handle validation check in CoreGetProtocolInterface
> 
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> 
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Zhi Jin
> > 发送时间: 2024年1月22日 17:53
> > 收件人: devel@edk2.groups.io
> > 抄送: Zhi Jin <zhi.jin@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> > Ray Ni <ray.ni@intel.com>; Michael D Kinney <michael.d.kinney@intel.com>
> > 主题: [edk2-devel] [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 <gaoliming@byosoft.com.cn>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Zhi Jin <zhi.jin@intel.com>
> > ---
> >  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 (#115018): https://edk2.groups.io/g/devel/message/115018
Mute This Topic: https://groups.io/mt/104114721/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-