from:"Ma, Hua"

Re: [edk2-devel] [PATCH] UefiPayloadPkg: Add macro to enable selection of timer

2024-01-04 Thread Ma, Hua


Add maintainers to help review the patch,

thank you,
Ma Hua

On 1/5/2024 1:38 PM, Ma, Hua wrote:

Add macro to enable selection of timer

- HPET:  UEFI Payload will use HPET timer
- LAPIC: UEFI Payload will use local APIC timer

Signed-off-by: Hua Ma 
---
  UefiPayloadPkg/UefiPayloadPkg.dsc | 16 
  UefiPayloadPkg/UefiPayloadPkg.fdf |  4 
  2 files changed, 20 insertions(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc 
b/UefiPayloadPkg/UefiPayloadPkg.dsc
index b8b13ad201..4f195c1e52 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -139,6 +139,12 @@
# Note: for emulation platform such as QEMU, this may not work and should 
set it as FALSE
DEFINE CPU_TIMER_LIB_ENABLE  = TRUE
  
+  #

+  # HPET:  UEFI Payload will use HPET timer
+  # LAPIC: UEFI Payload will use local APIC timer
+  #
+  DEFINE TIMER_SUPPORT  = HPET
+
DEFINE MULTIPLE_DEBUG_PORT_SUPPORT = FALSE
  
  [BuildOptions]

@@ -676,7 +682,17 @@
MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf
  
  
+!if $(TIMER_SUPPORT) == "HPET"

PcAtChipsetPkg/HpetTimerDxe/HpetTimerDxe.inf
+!elseif $(TIMER_SUPPORT) == "LAPIC"
+  OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf {
+
+  
NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+  }
+!else
+  !error "Invalid TIMER_SUPPORT"
+!endif
+
MdeModulePkg/Universal/Metronome/Metronome.inf
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
diff --git a/UefiPayloadPkg/UefiPayloadPkg.fdf 
b/UefiPayloadPkg/UefiPayloadPkg.fdf
index 835798be1c..7d04a8cffd 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.fdf
+++ b/UefiPayloadPkg/UefiPayloadPkg.fdf
@@ -161,7 +161,11 @@ INF 
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
  !endif
  INF UefiCpuPkg/CpuDxe/CpuDxe.inf
  
+!if $(TIMER_SUPPORT) == "HPET"

  INF PcAtChipsetPkg/HpetTimerDxe/HpetTimerDxe.inf
+!elseif $(TIMER_SUPPORT) == "LAPIC"
+INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+!endif
  INF MdeModulePkg/Universal/Metronome/Metronome.inf
  INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
  INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113220): https://edk2.groups.io/g/devel/message/113220
Mute This Topic: https://groups.io/mt/103538255/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] UefiPayloadPkg: Add macro to enable selection of timer

2024-01-04 Thread Ma, Hua

Add macro to enable selection of timer

- HPET:  UEFI Payload will use HPET timer
- LAPIC: UEFI Payload will use local APIC timer

Signed-off-by: Hua Ma 
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 16 
 UefiPayloadPkg/UefiPayloadPkg.fdf |  4 
 2 files changed, 20 insertions(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc 
b/UefiPayloadPkg/UefiPayloadPkg.dsc
index b8b13ad201..4f195c1e52 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -139,6 +139,12 @@
   # Note: for emulation platform such as QEMU, this may not work and should 
set it as FALSE
   DEFINE CPU_TIMER_LIB_ENABLE  = TRUE
 
+  #
+  # HPET:  UEFI Payload will use HPET timer
+  # LAPIC: UEFI Payload will use local APIC timer
+  #
+  DEFINE TIMER_SUPPORT  = HPET
+
   DEFINE MULTIPLE_DEBUG_PORT_SUPPORT = FALSE
 
 [BuildOptions]
@@ -676,7 +682,17 @@
   MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf
 
 
+!if $(TIMER_SUPPORT) == "HPET"
   PcAtChipsetPkg/HpetTimerDxe/HpetTimerDxe.inf
+!elseif $(TIMER_SUPPORT) == "LAPIC"
+  OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf {
+
+  
NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+  }
+!else
+  !error "Invalid TIMER_SUPPORT"
+!endif
+
   MdeModulePkg/Universal/Metronome/Metronome.inf
   MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
   MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
diff --git a/UefiPayloadPkg/UefiPayloadPkg.fdf 
b/UefiPayloadPkg/UefiPayloadPkg.fdf
index 835798be1c..7d04a8cffd 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.fdf
+++ b/UefiPayloadPkg/UefiPayloadPkg.fdf
@@ -161,7 +161,11 @@ INF 
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
 !endif
 INF UefiCpuPkg/CpuDxe/CpuDxe.inf
 
+!if $(TIMER_SUPPORT) == "HPET"
 INF PcAtChipsetPkg/HpetTimerDxe/HpetTimerDxe.inf
+!elseif $(TIMER_SUPPORT) == "LAPIC"
+INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+!endif
 INF MdeModulePkg/Universal/Metronome/Metronome.inf
 INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
 INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113219): https://edk2.groups.io/g/devel/message/113219
Mute This Topic: https://groups.io/mt/103538255/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2] MdeModulePkg: Add a check for metadata size in NvmExpress Driver

2022-03-02 Thread Ma, Hua

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3856

Currently this NvmeExpress Driver do not support metadata handling.
According to the NVME specs, metadata may be transferred to the host after
the logical block data. It can overrun the input buffer which may only
be the size of logical block data.

Add a check to return not support for the namespaces formatted with
metadata.

v2 changes:
 - Change debug log level from INFO to ERROR
 - Change to if (NamespaceData->LbaFormat[LbaFmtIdx].Ms != 0)

v1: https://edk2.groups.io/g/devel/message/87242

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Hao A Wu 
Cc: Ray Ni 

Signed-off-by: Hua Ma 
---
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c   | 15 +++
 .../Bus/Pci/NvmExpressPei/NvmExpressPei.c | 15 +++
 2 files changed, 30 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c 
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
index 5a1eda8e8d..388583e4d5 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
@@ -139,6 +139,21 @@ EnumerateNvmeDevNamespace (
 
 Flbas   = NamespaceData->Flbas;
 LbaFmtIdx   = Flbas & 0xF;
+
+//
+// Currently this NVME driver only suport Metadata Size == 0
+//
+if (NamespaceData->LbaFormat[LbaFmtIdx].Ms != 0) {
+  DEBUG ((
+DEBUG_ERROR,
+"NVME IDENTIFY NAMESPACE [%d] Ms(%d) is not supported.\n",
+NamespaceId,
+NamespaceData->LbaFormat[LbaFmtIdx].Ms
+));
+  Status = EFI_UNSUPPORTED;
+  goto Exit;
+}
+
 Lbads   = NamespaceData->LbaFormat[LbaFmtIdx].Lbads;
 Device->Media.BlockSize = (UINT32)1 << Lbads;
 
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c 
b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
index f73053fc3f..e8a29f23c7 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
@@ -104,6 +104,21 @@ EnumerateNvmeDevNamespace (
   //
   Flbas = NamespaceData->Flbas;
   LbaFmtIdx = Flbas & 0xF;
+
+  //
+  // Currently this NVME driver only suport Metadata Size == 0
+  //
+  if (NamespaceData->LbaFormat[LbaFmtIdx].Ms != 0) {
+DEBUG ((
+  DEBUG_ERROR,
+  "NVME IDENTIFY NAMESPACE [%d] Ms(%d) is not supported.\n",
+  NamespaceId,
+  NamespaceData->LbaFormat[LbaFmtIdx].Ms
+  ));
+Status = EFI_UNSUPPORTED;
+goto Exit;
+  }
+
   Lbads = NamespaceData->LbaFormat[LbaFmtIdx].Lbads;
 
   NamespaceInfo->Media.InterfaceType  = MSG_NVME_NAMESPACE_DP;
-- 
2.32.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#87249): https://edk2.groups.io/g/devel/message/87249
Mute This Topic: https://groups.io/mt/89519902/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1] MdeModulePkg: Add a check for metadata size in NvmExpress Driver

2022-03-02 Thread Ma, Hua

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3856

Currently this NvmeExpress Driver do not support metadata handling.
According to the NVME specs, metadata may be transferred to the host after
the logical block data. It can overrun the input buffer which may only
be the size of logical block data.

Add a check to return not support for the namespaces formatted with
metadata.

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Hao A Wu 
Cc: Ray Ni 

Signed-off-by: Hua Ma 
---
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c   | 15 +++
 .../Bus/Pci/NvmExpressPei/NvmExpressPei.c | 15 +++
 2 files changed, 30 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c 
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
index 5a1eda8e8d..46b7dcba20 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
@@ -139,6 +139,21 @@ EnumerateNvmeDevNamespace (
 
 Flbas   = NamespaceData->Flbas;
 LbaFmtIdx   = Flbas & 0xF;
+
+//
+// Currently this NVME driver only suport Metadata Size == 0
+//
+if (NamespaceData->LbaFormat[LbaFmtIdx].Ms) {
+  DEBUG ((
+DEBUG_INFO,
+"NVME IDENTIFY NAMESPACE [%d] Ms(%d) is not supported.\n",
+NamespaceId,
+NamespaceData->LbaFormat[LbaFmtIdx].Ms
+));
+  Status = EFI_UNSUPPORTED;
+  goto Exit;
+}
+
 Lbads   = NamespaceData->LbaFormat[LbaFmtIdx].Lbads;
 Device->Media.BlockSize = (UINT32)1 << Lbads;
 
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c 
b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
index f73053fc3f..6e27950648 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
@@ -104,6 +104,21 @@ EnumerateNvmeDevNamespace (
   //
   Flbas = NamespaceData->Flbas;
   LbaFmtIdx = Flbas & 0xF;
+
+  //
+  // Currently this NVME driver only suport Metadata Size == 0
+  //
+  if (NamespaceData->LbaFormat[LbaFmtIdx].Ms) {
+DEBUG ((
+  DEBUG_INFO,
+  "NVME IDENTIFY NAMESPACE [%d] Ms(%d) is not supported.\n",
+  NamespaceId,
+  NamespaceData->LbaFormat[LbaFmtIdx].Ms
+  ));
+Status = EFI_UNSUPPORTED;
+goto Exit;
+  }
+
   Lbads = NamespaceData->LbaFormat[LbaFmtIdx].Lbads;
 
   NamespaceInfo->Media.InterfaceType  = MSG_NVME_NAMESPACE_DP;
-- 
2.32.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#87242): https://edk2.groups.io/g/devel/message/87242
Mute This Topic: https://groups.io/mt/89517497/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

2021-10-13 Thread Ma, Hua

Hi Jian,

Thanks for the comment.
Patch v3 is just sent with the update.
Please help to review.

Thank you,
Ma Hua

> -Original Message-
> From: Wang, Jian J 
> Sent: Wednesday, October 13, 2021 12:26 PM
> To: Ma, Hua ; devel@edk2.groups.io
> Cc: Liming Gao ; Bi, Dandan
> ; Ni, Ray 
> Subject: RE: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when
> iterating gHandleList
> 
> Hi Hua,
> 
> It looks a bit odd to me to add 'IsLocked' parameter and acquire lock inside
> CoreValidateHandle() if it's FALSE. Maybe we can keep the function
> prototype as-is but do something like below:
> 
> a) Just keep ASSERT_LOCKED(&gProtocolDatabaseLock) in
> CoreValidateHandle()
> b) Call CoreAcquireProtocolLock() before any calling of
> CoreValidateHandle()
>  and CoreReleaseProtocolLock() afterwards.
> 
> Actually, CoreAcquireProtocolLock() is always called wherever
> CoreValidateHandle() is called. The problem is that, in many cases,
> CoreAcquireProtocolLock() is called after CoreValidateHandle(). We can
> simply move the calling of CoreAcquireProtocolLock() before
> CoreValidateHandle() to fix this problem.
> 
> For those cases CoreAcquireProtocolLock() is not called at all, just simply 
> add
> it.
> 
> Regards,
> Jian
> 
> > -----Original Message-
> > From: Ma, Hua 
> > Sent: Tuesday, October 12, 2021 4:34 PM
> > To: devel@edk2.groups.io
> > Cc: Ma, Hua ; Wang, Jian J ;
> > Liming Gao ; Bi, Dandan
> > ; Ni, Ray 
> > Subject: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when
> > iterating gHandleList
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680
> >
> > This patch fixes the following issue:
> >
> > The global variable gHandleList is a linked list.
> > This list is locked when a entry is added or removed from the list,
> > but there is no lock when iterating this list in function
> > CoreValidateHandle().
> > It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
> > iterated entry in the list is just removed by other task during iterating.
> > Locking the list when iterating can fix this issue.
> >
> > v2 changes:
> >  - Add lock check and comments in CoreGetProtocolInterface() before
> > calling CoreValidateHandle()
> >  - Update the comments in CoreValidateHandle() header file
> >
> > v1: https://edk2.groups.io/g/devel/topic/86233569
> >
> > Cc: Jian J Wang 
> > Cc: Liming Gao 
> > Cc: Dandan Bi 
> > Cc: Ray Ni 
> > Signed-off-by: Hua Ma 
> > ---
> >  MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++--
> >  MdeModulePkg/Core/Dxe/Hand/Handle.c| 56 +++
> ---
> >  MdeModulePkg/Core/Dxe/Hand/Handle.h|  5 +-
> >  MdeModulePkg/Core/Dxe/Hand/Notify.c|  2 +-
> >  4 files changed, 50 insertions(+), 23 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > index feabf12faf..eb8a765d2c 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > +++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > @@ -68,7 +68,7 @@ CoreConnectController (
> >//
> >// Make sure ControllerHandle is valid
> >//
> > -  Status = CoreValidateHandle (ControllerHandle);
> > +  Status = CoreValidateHandle (ControllerHandle, FALSE);
> >if (EFI_ERROR (Status)) {
> >  return Status;
> >}
> > @@ -154,7 +154,7 @@ CoreConnectController (
> >  //
> >  // Make sure the DriverBindingHandle is valid
> >  //
> > -Status = CoreValidateHandle (ControllerHandle);
> > +Status = CoreValidateHandle (ControllerHandle, TRUE);
> >  if (EFI_ERROR (Status)) {
> >//
> >// Release the protocol lock on the handle database @@ -268,7
> > +268,7 @@ AddSortedDriverBindingProtocol (
> >//
> >// Make sure the DriverBindingHandle is valid
> >//
> > -  Status = CoreValidateHandle (DriverBindingHandle);
> > +  Status = CoreValidateHandle (DriverBindingHandle, FALSE);
> >if (EFI_ERROR (Status)) {
> >  return;
> >}
> > @@ -746,7 +746,7 @@ CoreDisconnectController (
> >//
> >// Make sure ControllerHandle is valid
> >//
> > -  Status = CoreValidateHandle (ControllerHandle);
> > +  Status = CoreValidateHandle (ControllerHandle, FALSE);
> >if (EFI_ERROR (Status)) {
> >  return Status;
> >}
> > @@ -755,7 +755,7 @@ CoreDisconnectController (
> >// Make sure ChildHandle

[edk2-devel] [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

2021-10-13 Thread Ma, Hua

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680

This patch fixes the following issue:

The global variable gHandleList is a linked list.
This list is locked when a entry is added or removed from the list,
but there is no lock when iterating this list in function
CoreValidateHandle().
It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
iterated entry in the list is just removed by other task during iterating.

Currently some caller functions of CoreValidateHandle() have
CoreAcquireProtocolLock(), but some caller functions of
CoreValidateHandle() do not CoreAcquireProtocolLock().
Add CoreAcquireProtocolLock() always when CoreValidateHandle() is called,
Also, A lock check is added in the CoreValidateHandle().

v3 changes:
 - keep ASSERT_LOCKED(&gProtocolDatabaseLock) in CoreValidateHandle()
 - Call CoreAcquireProtocolLock() before any calling of
  CoreValidateHandle() and CoreReleaseProtocolLock() afterwards
 - Update the commit message

v2 changes:
 - Add lock check and comments in CoreGetProtocolInterface() before
calling CoreValidateHandle()
 - Update the comments in CoreValidateHandle() header file

v1: https://edk2.groups.io/g/devel/topic/86233569

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Dandan Bi 
Cc: Ray Ni 
Signed-off-by: Hua Ma 
---
 MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 16 +
 MdeModulePkg/Core/Dxe/Hand/Handle.c| 75 --
 MdeModulePkg/Core/Dxe/Hand/Handle.h|  1 +
 MdeModulePkg/Core/Dxe/Hand/Notify.c| 13 ++--
 4 files changed, 64 insertions(+), 41 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c 
b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..12a202417c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,12 @@ CoreConnectController (
   //
   // Make sure ControllerHandle is valid
   //
+  CoreAcquireProtocolLock ();
+
   Status = CoreValidateHandle (ControllerHandle);
+
+  CoreReleaseProtocolLock ();
+
   if (EFI_ERROR (Status)) {
 return Status;
   }
@@ -268,7 +273,12 @@ AddSortedDriverBindingProtocol (
   //
   // Make sure the DriverBindingHandle is valid
   //
+  CoreAcquireProtocolLock ();
+
   Status = CoreValidateHandle (DriverBindingHandle);
+
+  CoreReleaseProtocolLock ();
+
   if (EFI_ERROR (Status)) {
 return;
   }
@@ -746,8 +756,11 @@ CoreDisconnectController (
   //
   // Make sure ControllerHandle is valid
   //
+  CoreAcquireProtocolLock ();
+
   Status = CoreValidateHandle (ControllerHandle);
   if (EFI_ERROR (Status)) {
+CoreReleaseProtocolLock ();
 return Status;
   }
 
@@ -757,10 +770,13 @@ CoreDisconnectController (
   if (ChildHandle != NULL) {
 Status = CoreValidateHandle (ChildHandle);
 if (EFI_ERROR (Status)) {
+  CoreReleaseProtocolLock ();
   return Status;
 }
   }
 
+  CoreReleaseProtocolLock ();
+
   Handle = ControllerHandle;
 
   //
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c 
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..92979281b7 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -53,6 +53,7 @@ CoreReleaseProtocolLock (
 
 /**
   Check whether a handle is a valid EFI_HANDLE
+  The gProtocolDatabaseLock must be owned
 
   @param  UserHandle The handle to check
 
@@ -72,6 +73,8 @@ CoreValidateHandle (
 return EFI_INVALID_PARAMETER;
   }
 
+  ASSERT_LOCKED(&gProtocolDatabaseLock);
+
   for (Link = gHandleList.BackLink; Link != &gHandleList; Link = 
Link->BackLink) {
 Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
 if (Handle == (IHANDLE *) UserHandle) {
@@ -720,19 +723,19 @@ CoreUninstallProtocolInterface (
 return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // Lock the protocol database
+  //
+  CoreAcquireProtocolLock ();
+
   //
   // Check that UserHandle is a valid handle
   //
   Status = CoreValidateHandle (UserHandle);
   if (EFI_ERROR (Status)) {
-return Status;
+goto Done;
   }
 
-  //
-  // Lock the protocol database
-  //
-  CoreAcquireProtocolLock ();
-
   //
   // Check that Protocol exists on UserHandle, and Interface matches the 
interface in the database
   //
@@ -1010,12 +1013,17 @@ CoreOpenProtocol (
 return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // Lock the protocol database
+  //
+  CoreAcquireProtocolLock ();
+
   //
   // Check for invalid UserHandle
   //
   Status = CoreValidateHandle (UserHandle);
   if (EFI_ERROR (Status)) {
-return Status;
+goto Done;
   }
 
   //
@@ -1025,31 +1033,32 @@ CoreOpenProtocol (
   case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
 Status = CoreValidateHandle (ImageHandle);
 if (EFI_ERROR (Status)) {
-  return Status;
+  goto Done;
 }
 Status = CoreValidateHandle (ControllerHandle);
 if (EFI_ERROR (Status)) {
-  return Status;
+  goto Done;
 }
 if (UserHandle == ControllerHandle) {
-  return EFI_INVALID_PARAMETER;
+  Status = EFI_INVALID_

Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

2021-10-12 Thread Ma, Hua

Hi Dandan,

Thanks for the comment.
Patch v2 is just sent with the update.
Please help to review.

Thank you,
Ma Hua

> -Original Message-
> From: Bi, Dandan 
> Sent: Tuesday, October 12, 2021 12:13 PM
> To: Ma, Hua ; devel@edk2.groups.io
> Cc: Wang, Jian J ; Liming Gao
> ; Ni, Ray 
> Subject: RE: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when
> iterating gHandleList
> 
> Hi Hua,
> 
> One minor comment inline, please check.
> 
> 
> 
> Thanks,
> Dandan
> 
> > -Original Message-
> > From: Ma, Hua 
> > Sent: Monday, October 11, 2021 6:45 PM
> > To: devel@edk2.groups.io
> > Cc: Ma, Hua ; Wang, Jian J ;
> > Liming Gao ; Bi, Dandan
> ;
> > Ni, Ray 
> > Subject: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating
> > gHandleList
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680
> >
> > This patch fixes the following issue:
> >
> > The global variable gHandleList is a linked list.
> > This list is locked when a entry is added or removed from the list, but 
> > there
> is no
> > lock when iterating this list in function CoreValidateHandle().
> > It can lead to "Handle.c (76): CR has Bad Signature" assertion if the 
> > iterated
> > entry in the list is just removed by other task during iterating.
> > Locking the list when iterating can fix this issue.
> >
> > Cc: Jian J Wang 
> > Cc: Liming Gao 
> > Cc: Dandan Bi 
> > Cc: Ray Ni 
> > Signed-off-by: Hua Ma 
> > ---
> >  MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++---
> >  MdeModulePkg/Core/Dxe/Hand/Handle.c| 47 ++--
> --
> >  MdeModulePkg/Core/Dxe/Hand/Handle.h|  3 +-
> >  MdeModulePkg/Core/Dxe/Hand/Notify.c|  2 +-
> >  4 files changed, 39 insertions(+), 23 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > index feabf12faf..eb8a765d2c 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > +++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > @@ -68,7 +68,7 @@ CoreConnectController (
> >//
> >// Make sure ControllerHandle is valid
> >//
> > -  Status = CoreValidateHandle (ControllerHandle);
> > +  Status = CoreValidateHandle (ControllerHandle, FALSE);
> >if (EFI_ERROR (Status)) {
> >  return Status;
> >}
> > @@ -154,7 +154,7 @@ CoreConnectController (
> >  //
> >  // Make sure the DriverBindingHandle is valid
> >  //
> > -Status = CoreValidateHandle (ControllerHandle);
> > +Status = CoreValidateHandle (ControllerHandle, TRUE);
> >  if (EFI_ERROR (Status)) {
> >//
> >// Release the protocol lock on the handle database @@ -268,7 +268,7
> @@
> > AddSortedDriverBindingProtocol (
> >//
> >// Make sure the DriverBindingHandle is valid
> >//
> > -  Status = CoreValidateHandle (DriverBindingHandle);
> > +  Status = CoreValidateHandle (DriverBindingHandle, FALSE);
> >if (EFI_ERROR (Status)) {
> >  return;
> >}
> > @@ -746,7 +746,7 @@ CoreDisconnectController (
> >//
> >// Make sure ControllerHandle is valid
> >//
> > -  Status = CoreValidateHandle (ControllerHandle);
> > +  Status = CoreValidateHandle (ControllerHandle, FALSE);
> >if (EFI_ERROR (Status)) {
> >  return Status;
> >}
> > @@ -755,7 +755,7 @@ CoreDisconnectController (
> >// Make sure ChildHandle is valid if it is not NULL
> >//
> >if (ChildHandle != NULL) {
> > -Status = CoreValidateHandle (ChildHandle);
> > +Status = CoreValidateHandle (ChildHandle, FALSE);
> >  if (EFI_ERROR (Status)) {
> >return Status;
> >  }
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > index 6eccb41ecb..b4f389bb35 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > @@ -55,31 +55,46 @@ CoreReleaseProtocolLock (
> >Check whether a handle is a valid EFI_HANDLE
> >
> >@param  UserHandle The handle to check
> > +  @param  IsLocked   The protocol lock is acquried or not
> >
> >@retval EFI_INVALID_PARAMETER  The handle is NULL or not a valid
> > EFI_HANDLE.
> > +  @retval EFI_NOT_FOUND  The handle is not found in the handle
> > database.
> >@retval EFI_SUCCESSThe handle is valid EFI_HANDLE.
&g

[edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

2021-10-12 Thread Ma, Hua

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680

This patch fixes the following issue:

The global variable gHandleList is a linked list.
This list is locked when a entry is added or removed from the list,
but there is no lock when iterating this list in function
CoreValidateHandle().
It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
iterated entry in the list is just removed by other task during iterating.
Locking the list when iterating can fix this issue.

v2 changes:
 - Add lock check and comments in CoreGetProtocolInterface() before
calling CoreValidateHandle()
 - Update the comments in CoreValidateHandle() header file

v1: https://edk2.groups.io/g/devel/topic/86233569

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Dandan Bi 
Cc: Ray Ni 
Signed-off-by: Hua Ma 
---
 MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++--
 MdeModulePkg/Core/Dxe/Hand/Handle.c| 56 +++---
 MdeModulePkg/Core/Dxe/Hand/Handle.h|  5 +-
 MdeModulePkg/Core/Dxe/Hand/Notify.c|  2 +-
 4 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c 
b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..eb8a765d2c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,7 @@ CoreConnectController (
   //
   // Make sure ControllerHandle is valid
   //
-  Status = CoreValidateHandle (ControllerHandle);
+  Status = CoreValidateHandle (ControllerHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return Status;
   }
@@ -154,7 +154,7 @@ CoreConnectController (
 //
 // Make sure the DriverBindingHandle is valid
 //
-Status = CoreValidateHandle (ControllerHandle);
+Status = CoreValidateHandle (ControllerHandle, TRUE);
 if (EFI_ERROR (Status)) {
   //
   // Release the protocol lock on the handle database
@@ -268,7 +268,7 @@ AddSortedDriverBindingProtocol (
   //
   // Make sure the DriverBindingHandle is valid
   //
-  Status = CoreValidateHandle (DriverBindingHandle);
+  Status = CoreValidateHandle (DriverBindingHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return;
   }
@@ -746,7 +746,7 @@ CoreDisconnectController (
   //
   // Make sure ControllerHandle is valid
   //
-  Status = CoreValidateHandle (ControllerHandle);
+  Status = CoreValidateHandle (ControllerHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return Status;
   }
@@ -755,7 +755,7 @@ CoreDisconnectController (
   // Make sure ChildHandle is valid if it is not NULL
   //
   if (ChildHandle != NULL) {
-Status = CoreValidateHandle (ChildHandle);
+Status = CoreValidateHandle (ChildHandle, FALSE);
 if (EFI_ERROR (Status)) {
   return Status;
 }
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c 
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..46f67d3d6a 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -55,31 +55,46 @@ CoreReleaseProtocolLock (
   Check whether a handle is a valid EFI_HANDLE
 
   @param  UserHandle The handle to check
+  @param  IsLocked   The protocol lock is acquried or not
 
   @retval EFI_INVALID_PARAMETER  The handle is NULL or not a valid EFI_HANDLE.
+  @retval EFI_NOT_FOUND  The handle is not found in the handle 
database.
   @retval EFI_SUCCESSThe handle is valid EFI_HANDLE.
 
 **/
 EFI_STATUS
 CoreValidateHandle (
-  IN  EFI_HANDLEUserHandle
+  IN  EFI_HANDLEUserHandle,
+  IN  BOOLEAN   IsLocked
   )
 {
   IHANDLE *Handle;
   LIST_ENTRY  *Link;
+  EFI_STATUS  Status;
 
   if (UserHandle == NULL) {
 return EFI_INVALID_PARAMETER;
   }
 
+  if (IsLocked) {
+ASSERT_LOCKED(&gProtocolDatabaseLock);
+  } else {
+CoreAcquireProtocolLock ();
+  }
+
+  Status = EFI_NOT_FOUND;
   for (Link = gHandleList.BackLink; Link != &gHandleList; Link = 
Link->BackLink) {
 Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
 if (Handle == (IHANDLE *) UserHandle) {
-  return EFI_SUCCESS;
+  Status = EFI_SUCCESS;
+  break;
 }
   }
 
-  return EFI_INVALID_PARAMETER;
+  if (!IsLocked) {
+CoreReleaseProtocolLock ();
+  }
+  return Status;
 }
 
 
@@ -428,7 +443,7 @@ CoreInstallProtocolInterfaceNotify (
 //
 InsertTailList (&gHandleList, &Handle->AllHandles);
   } else {
-Status = CoreValidateHandle (Handle);
+Status = CoreValidateHandle (Handle, TRUE);
 if (EFI_ERROR (Status)) {
   DEBUG((DEBUG_ERROR, "InstallProtocolInterface: input handle at 0x%x is 
invalid\n", Handle));
   goto Done;
@@ -723,7 +738,7 @@ CoreUninstallProtocolInterface (
   //
   // Check that UserHandle is a valid handle
   //
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return Status;
   }
@@ -899,7 +914,16 @@ CoreGetProtocolInterface (
   IHANDLE *H

Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

2021-10-11 Thread Ma, Hua

> -Original Message-
> From: Michael Brown 
> Sent: Monday, October 11, 2021 7:28 PM
> To: devel@edk2.groups.io; Ma, Hua 
> Cc: Wang, Jian J ; Liming Gao
> ; Bi, Dandan ; Ni, Ray
> 
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock
> when iterating gHandleList
> 
> On 11/10/2021 11:45, Ma, Hua wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680
> >
> > This patch fixes the following issue:
> >
> > The global variable gHandleList is a linked list.
> > This list is locked when a entry is added or removed from the list,
> > but there is no lock when iterating this list in function
> > CoreValidateHandle().
> > It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
> > iterated entry in the list is just removed by other task during iterating.
> > Locking the list when iterating can fix this issue.
> 
> At a first glance, it looks as though if the caller does not already
> hold the lock, then the result from CoreValidateHandle() may be invalid
> by the time that control returns to the caller.
> 
> Under what circumstances is it valid to call CoreValidateHandle() when
> the caller does not _already_ hold the lock (i.e. IsLocked==FALSE)?
> 
> Thanks,
> 
> Michael

Hi Michael,

Thanks for the review,
In current CoreValidateHandle implementation:

  for (Link = gHandleList.BackLink; Link != &gHandleList; Link = 
Link->BackLink) {
Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
if (Handle == (IHANDLE *) UserHandle) {
  return EFI_SUCCESS;
}
  }

Let's say, if the list have 4 entries, (A->B->C->D), and the caller is trying 
to validate if entry C is valid,
When caller task just set local variable Link to B, and before it calls CR() 
macro to validate the signature,
At this point, if other task just deletes other entry, entry B, and reset B's 
signature. 
Then when back to caller task,  There is a signature mismatch assertion for B.
But the result should be still valid for C if call CoreValidateHandle() again.
The patch is trying to fix this kind of problem.

I feel one valid kind of calling CoreValidateHandle() without holding the lock 
is:
when the handle is passed in as parameter, and do a parameter checking,
and later, if need, validated the handle again when use the handle.
for example in current code, in AddSortedDriverBindingProtocol, 
CoreConnectController,
This patch do not change where to add a handle validation.

Thank you,
Ma Hua

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81751): https://edk2.groups.io/g/devel/message/81751
Mute This Topic: https://groups.io/mt/86233569/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

2021-10-11 Thread Ma, Hua

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680

This patch fixes the following issue:

The global variable gHandleList is a linked list.
This list is locked when a entry is added or removed from the list,
but there is no lock when iterating this list in function
CoreValidateHandle().
It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
iterated entry in the list is just removed by other task during iterating.
Locking the list when iterating can fix this issue.

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Dandan Bi 
Cc: Ray Ni 
Signed-off-by: Hua Ma 
---
 MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++---
 MdeModulePkg/Core/Dxe/Hand/Handle.c| 47 ++
 MdeModulePkg/Core/Dxe/Hand/Handle.h|  3 +-
 MdeModulePkg/Core/Dxe/Hand/Notify.c|  2 +-
 4 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c 
b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..eb8a765d2c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,7 @@ CoreConnectController (
   //
   // Make sure ControllerHandle is valid
   //
-  Status = CoreValidateHandle (ControllerHandle);
+  Status = CoreValidateHandle (ControllerHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return Status;
   }
@@ -154,7 +154,7 @@ CoreConnectController (
 //
 // Make sure the DriverBindingHandle is valid
 //
-Status = CoreValidateHandle (ControllerHandle);
+Status = CoreValidateHandle (ControllerHandle, TRUE);
 if (EFI_ERROR (Status)) {
   //
   // Release the protocol lock on the handle database
@@ -268,7 +268,7 @@ AddSortedDriverBindingProtocol (
   //
   // Make sure the DriverBindingHandle is valid
   //
-  Status = CoreValidateHandle (DriverBindingHandle);
+  Status = CoreValidateHandle (DriverBindingHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return;
   }
@@ -746,7 +746,7 @@ CoreDisconnectController (
   //
   // Make sure ControllerHandle is valid
   //
-  Status = CoreValidateHandle (ControllerHandle);
+  Status = CoreValidateHandle (ControllerHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return Status;
   }
@@ -755,7 +755,7 @@ CoreDisconnectController (
   // Make sure ChildHandle is valid if it is not NULL
   //
   if (ChildHandle != NULL) {
-Status = CoreValidateHandle (ChildHandle);
+Status = CoreValidateHandle (ChildHandle, FALSE);
 if (EFI_ERROR (Status)) {
   return Status;
 }
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c 
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..b4f389bb35 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -55,31 +55,46 @@ CoreReleaseProtocolLock (
   Check whether a handle is a valid EFI_HANDLE
 
   @param  UserHandle The handle to check
+  @param  IsLocked   The protocol lock is acquried or not
 
   @retval EFI_INVALID_PARAMETER  The handle is NULL or not a valid EFI_HANDLE.
+  @retval EFI_NOT_FOUND  The handle is not found in the handle 
database.
   @retval EFI_SUCCESSThe handle is valid EFI_HANDLE.
 
 **/
 EFI_STATUS
 CoreValidateHandle (
-  IN  EFI_HANDLEUserHandle
+  IN  EFI_HANDLEUserHandle,
+  IN  BOOLEAN   IsLocked
   )
 {
   IHANDLE *Handle;
   LIST_ENTRY  *Link;
+  EFI_STATUS  Status;
 
   if (UserHandle == NULL) {
 return EFI_INVALID_PARAMETER;
   }
 
+  if (IsLocked) {
+ASSERT_LOCKED(&gProtocolDatabaseLock);
+  } else {
+CoreAcquireProtocolLock ();
+  }
+
+  Status = EFI_NOT_FOUND;
   for (Link = gHandleList.BackLink; Link != &gHandleList; Link = 
Link->BackLink) {
 Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
 if (Handle == (IHANDLE *) UserHandle) {
-  return EFI_SUCCESS;
+  Status = EFI_SUCCESS;
+  break;
 }
   }
 
-  return EFI_INVALID_PARAMETER;
+  if (!IsLocked) {
+CoreReleaseProtocolLock ();
+  }
+  return Status;
 }
 
 
@@ -428,7 +443,7 @@ CoreInstallProtocolInterfaceNotify (
 //
 InsertTailList (&gHandleList, &Handle->AllHandles);
   } else {
-Status = CoreValidateHandle (Handle);
+Status = CoreValidateHandle (Handle, TRUE);
 if (EFI_ERROR (Status)) {
   DEBUG((DEBUG_ERROR, "InstallProtocolInterface: input handle at 0x%x is 
invalid\n", Handle));
   goto Done;
@@ -723,7 +738,7 @@ CoreUninstallProtocolInterface (
   //
   // Check that UserHandle is a valid handle
   //
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
 return Status;
   }
@@ -899,7 +914,7 @@ CoreGetProtocolInterface (
   IHANDLE *Handle;
   LIST_ENTRY  *Link;
 
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, TRUE);
   if (EFI_ERROR (Status)) {
 return NULL;
   }
@@ -1013,7 +1028,7 @@ CoreOpenPro

[edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Add lock protection in CoreLocateHandleBuffer()

2021-09-29 Thread Ma, Hua

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3666

Currently, CoreLocateHandleBuffer() follows three steps:
1) get the size of protocol database firstly
2) allocate the buffer based on the size
3) get the protocol database into the buffer
There is no lock protection for the whole three steps. If a new protocol
added in step 2) by other task, e.g. (event timer handle USB device
hotplug). The size of protocol database may be increased and cannot fit
into the previous buffer in step 3). The protocol database cannot be
returned successfully, EFI_BUFFER_TOO_SMALL error will be returned.

This patch adds the lock to protect the whole three steps.
It can make sure the correct protocol database be returned.

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Dandan Bi 
Signed-off-by: Hua Ma 
---
 MdeModulePkg/Core/Dxe/Hand/Locate.c | 64 +++--
 1 file changed, 51 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Locate.c 
b/MdeModulePkg/Core/Dxe/Hand/Locate.c
index be17f4cbc3..4987c046c6 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Locate.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Locate.c
@@ -86,7 +86,8 @@ CoreGetNextLocateByProtocol (
 
 
 /**
-  Locates the requested handle(s) and returns them in Buffer.
+  Internal function for locating the requested handle(s) and returns them in 
Buffer.
+  The caller should already have acquired the ProtocolLock.
 
   @param  SearchType The type of search to perform to locate the
  handles
@@ -104,8 +105,7 @@ CoreGetNextLocateByProtocol (
 
 **/
 EFI_STATUS
-EFIAPI
-CoreLocateHandle (
+InternalCoreLocateHandle (
   IN EFI_LOCATE_SEARCH_TYPE   SearchType,
   IN EFI_GUID *Protocol   OPTIONAL,
   IN VOID *SearchKey  OPTIONAL,
@@ -143,11 +143,6 @@ CoreLocateHandle (
   ResultBuffer = (IHANDLE **) Buffer;
   Status = EFI_SUCCESS;
 
-  //
-  // Lock the protocol database
-  //
-  CoreAcquireProtocolLock ();
-
   //
   // Get the search function based on type
   //
@@ -190,7 +185,6 @@ CoreLocateHandle (
   }
 
   if (EFI_ERROR(Status)) {
-CoreReleaseProtocolLock ();
 return Status;
   }
 
@@ -247,10 +241,47 @@ CoreLocateHandle (
 }
   }
 
-  CoreReleaseProtocolLock ();
   return Status;
 }
 
+/**
+  Locates the requested handle(s) and returns them in Buffer.
+
+  @param  SearchType The type of search to perform to locate the
+ handles
+  @param  Protocol   The protocol to search for
+  @param  SearchKey  Dependant on SearchType
+  @param  BufferSize On input the size of Buffer.  On output the
+ size of data returned.
+  @param  Buffer The buffer to return the results in
+
+  @retval EFI_BUFFER_TOO_SMALL   Buffer too small, required buffer size is
+ returned in BufferSize.
+  @retval EFI_INVALID_PARAMETER  Invalid parameter
+  @retval EFI_SUCCESSSuccessfully found the requested handle(s) and
+ returns them in Buffer.
+
+**/
+EFI_STATUS
+EFIAPI
+CoreLocateHandle (
+  IN EFI_LOCATE_SEARCH_TYPE   SearchType,
+  IN EFI_GUID *Protocol   OPTIONAL,
+  IN VOID *SearchKey  OPTIONAL,
+  IN OUT UINTN*BufferSize,
+  OUT EFI_HANDLE  *Buffer
+  )
+{
+  EFI_STATUS Status;
+
+  //
+  // Lock the protocol database
+  //
+  CoreAcquireProtocolLock ();
+  Status = InternalCoreLocateHandle(SearchType, Protocol, SearchKey, 
BufferSize, Buffer);
+  CoreReleaseProtocolLock ();
+  return Status;
+}
 
 
 /**
@@ -610,7 +641,6 @@ Done:
   return Status;
 }
 
-
 /**
   Function returns an array of handles that support the requested protocol
   in a buffer allocated from pool. This is a version of CoreLocateHandle()
@@ -657,7 +687,12 @@ CoreLocateHandleBuffer (
   BufferSize = 0;
   *NumberHandles = 0;
   *Buffer = NULL;
-  Status = CoreLocateHandle (
+
+  //
+  // Lock the protocol database
+  //
+  CoreAcquireProtocolLock();
+  Status = InternalCoreLocateHandle (
  SearchType,
  Protocol,
  SearchKey,
@@ -674,15 +709,17 @@ CoreLocateHandleBuffer (
 if (Status != EFI_INVALID_PARAMETER) {
   Status = EFI_NOT_FOUND;
 }
+CoreReleaseProtocolLock ();
 return Status;
   }
 
   *Buffer = AllocatePool (BufferSize);
   if (*Buffer == NULL) {
+CoreReleaseProtocolLock ();
 return EFI_OUT_OF_RESOURCES;
   }
 
-  Status = CoreLocateHandle (
+  Status = InternalCoreLocateHandle (
  SearchType,
  Protocol,
  SearchKey,
@@ -695,6 +732,7 @@ CoreLocateHandleBuffer (
 *NumberHandles = 0;
   }
 
+  CoreReleaseProtocolLock ();
   return Status;
 }
 
-- 
2.32.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81270): https://edk2.groups.io/g/devel/message/81270
Mute This Topic: https

Re: [edk2-devel] [PATCH] UefiPayloadPkg: Add macro to enable selection of timer

[edk2-devel] [PATCH] UefiPayloadPkg: Add macro to enable selection of timer

[edk2-devel] [PATCH v2] MdeModulePkg: Add a check for metadata size in NvmExpress Driver

[edk2-devel] [PATCH v1] MdeModulePkg: Add a check for metadata size in NvmExpress Driver

Re: [edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

[edk2-devel] [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

[edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

[edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

[edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Add lock protection in CoreLocateHandleBuffer()

11 matches

Site Navigation

Mail list logo

Footer information