Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: Use SmmWaitForAllProcessor() in VariableSmm driver.
Thank you for your comment. I will move the SmmCpuRendezvousLib.h file and NullLib implementation to MdePkg. Thanks Zhihao -Original Message- From: Michael Kubacki Sent: Thursday, April 21, 2022 2:42 AM To: devel@edk2.groups.io; Li, Zhihao Cc: Wang, Jian J ; Gao, Liming ; Fu, Siyuan ; Ni, Ray ; Sami Mujawar ; Ilias Apalodimas ; Ard Biesheuvel ; Leif Lindholm Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: Use SmmWaitForAllProcessor() in VariableSmm driver. If I understand this patch correctly, it is exactly duplicating the SmmCpuRendezvousLib library class/interface in ModeModulePkg because code there cannot depend on the library class/interface definition currently in UefiCpuPkg: https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Include/Library/SmmCpuRendezvousLib.h If that's the case, this is creating maintenance debt by requiring the two interfaces always be kept in sync and developer confusion. It is okay to have an interface defined in a more broadly scoped package (e.g. MdePkg) with instances implemented in other packages. For example, the HobLib interface is defined in MdePkg: https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/HobLib.h But, instances are described in many other packages including a NULL instance in MdeModulePkg: https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/BaseHobLibNull/BaseHobLibNull.inf And a Standalone MM instance in StandaloneMmPkg: https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf If this interface is actually consumed by MdeModulePkg, the interface should be defined in a single package that is allowed to be a dependency for MdeModulePkg. The NULL library instance referenced in the MdeModulePkg build should also be implemented in an allowed package. The library interface should be removed from other packages (UefiCpuPkg). Other library instances can then be implemented elsewhere using the library class interface from the singly defined location. Regards, Michael On 4/20/2022 1:32 PM, Li, Zhihao wrote: > REF： https://bugzilla.tianocore.org/show_bug.cgi?id=3854 > > In UefiCpuPkg, there are a new Protocol with the new service > SmmWaitForAllProcessor(), which can be used by SMI handler to > optionally wait for other APs to complete SMM rendezvous in relaxed AP > mode. > > This patch use the new service to let VariableSmm driver work normally > in relaxed AP mode. > > Due to MdeModulePkg can not depend on UefiCpuPkg, use null version > implementation in MdeModulePkg.dsc. > > Cc: Jian J Wang > Cc: Liming Gao > Cc: Siyuan Fu > Cc: Ni Ray > Cc: Sami Mujawar > Cc: Ilias Apalodimas > Cc: Ard Biesheuvel > Cc: Leif Lindholm > > Signed-off-by: Zhihao Li > --- > MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c | > 29 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | > 10 ++- > MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h | > 27 ++ > MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.inf | > 27 ++ > MdeModulePkg/MdeModulePkg.dec| > 5 +++- > MdeModulePkg/MdeModulePkg.dsc| > 5 +++- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | > 3 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | > 3 +- > 8 files changed, 104 insertions(+), 5 deletions(-) > > diff --git > a/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull > .c > b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull > .c > new file mode 100644 > index ..474195bbb374 > --- /dev/null > +++ b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLib > +++ Null.c > @@ -0,0 +1,29 @@ > +/** @file > > + SMM CPU Rendezvous sevice implement. > > + > > + Copyright (c) 2022, Intel Corporation. All rights reserved. > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#include > > +#include > > + > > +/** > > + This routine wait for all AP processors to arrive in SMM. > > + > > + @param[in] BlockingMode Blocking mode or non-blocking mode. > > + > > + @retval EFI_SUCCESS All avaiable APs arrived. > > + @retval EFI_TIMEOUT Wait for all APs until timeout. > > + @retval OTHERFail to register SMM CPU Rendezvous service Protocol. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +SmmWaitForAllProcessor ( > > + IN BOOLEAN BlockingMode > &
Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: Use SmmWaitForAllProcessor() in VariableSmm driver.
If I understand this patch correctly, it is exactly duplicating the
SmmCpuRendezvousLib library class/interface in ModeModulePkg because
code there cannot depend on the library class/interface definition
currently in UefiCpuPkg:
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Include/Library/SmmCpuRendezvousLib.h
If that's the case, this is creating maintenance debt by requiring the
two interfaces always be kept in sync and developer confusion.
It is okay to have an interface defined in a more broadly scoped package
(e.g. MdePkg) with instances implemented in other packages.
For example, the HobLib interface is defined in MdePkg:
https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/HobLib.h
But, instances are described in many other packages including a NULL
instance in MdeModulePkg:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/BaseHobLibNull/BaseHobLibNull.inf
And a Standalone MM instance in StandaloneMmPkg:
https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
If this interface is actually consumed by MdeModulePkg, the interface
should be defined in a single package that is allowed to be a dependency
for MdeModulePkg. The NULL library instance referenced in the
MdeModulePkg build should also be implemented in an allowed package.
The library interface should be removed from other packages
(UefiCpuPkg). Other library instances can then be implemented elsewhere
using the library class interface from the singly defined location.
Regards,
Michael
On 4/20/2022 1:32 PM, Li, Zhihao wrote:
REF： https://bugzilla.tianocore.org/show_bug.cgi?id=3854
In UefiCpuPkg, there are a new Protocol with the new service
SmmWaitForAllProcessor(), which can be used by SMI handler
to optionally wait for other APs to complete SMM rendezvous in
relaxed AP mode.
This patch use the new service to let VariableSmm driver work
normally in relaxed AP mode.
Due to MdeModulePkg can not depend on UefiCpuPkg, use null version
implementation in MdeModulePkg.dsc.
Cc: Jian J Wang
Cc: Liming Gao
Cc: Siyuan Fu
Cc: Ni Ray
Cc: Sami Mujawar
Cc: Ilias Apalodimas
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Signed-off-by: Zhihao Li
---
MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c | 29
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 10
++-
MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h | 27
++
MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.inf | 27
++
MdeModulePkg/MdeModulePkg.dec| 5
+++-
MdeModulePkg/MdeModulePkg.dsc| 5
+++-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3
+-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | 3
+-
8 files changed, 104 insertions(+), 5 deletions(-)
diff --git
a/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c
b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c
new file mode 100644
index ..474195bbb374
--- /dev/null
+++ b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c
@@ -0,0 +1,29 @@
+/** @file
+ SMM CPU Rendezvous sevice implement.
+
+ Copyright (c) 2022, Intel Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include
+#include
+
+/**
+ This routine wait for all AP processors to arrive in SMM.
+
+ @param[in] BlockingMode Blocking mode or non-blocking mode.
+
+ @retval EFI_SUCCESS All avaiable APs arrived.
+ @retval EFI_TIMEOUT Wait for all APs until timeout.
+ @retval OTHERFail to register SMM CPU Rendezvous service Protocol.
+**/
+EFI_STATUS
+EFIAPI
+SmmWaitForAllProcessor (
+ IN BOOLEAN BlockingMode
+ )
+{
+ ASSERT (FALSE);
+ return EFI_SUCCESS;
+}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index 517cae7b00f8..52a9b0e6b202 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -14,7 +14,7 @@
VariableServiceSetVariable(), VariableServiceQueryVariableInfo(),
ReclaimForOS(),
SmmVariableGetStatistics() should also do validation based on its own
knowledge.
-Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2022, Intel Corporation. All rights reserved.
Copyright (c) 2018, Linaro, Ltd. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -28,6 +28,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include
#include
+#include
#include
#include "Variable.h"
@@ -656,6 +657,13 @@ SmmVariableHandler (
goto
[edk2-devel] [PATCH v2 1/1] MdeModulePkg: Use SmmWaitForAllProcessor() in VariableSmm driver.
REF： https://bugzilla.tianocore.org/show_bug.cgi?id=3854
In UefiCpuPkg, there are a new Protocol with the new service
SmmWaitForAllProcessor(), which can be used by SMI handler
to optionally wait for other APs to complete SMM rendezvous in
relaxed AP mode.
This patch use the new service to let VariableSmm driver work
normally in relaxed AP mode.
Due to MdeModulePkg can not depend on UefiCpuPkg, use null version
implementation in MdeModulePkg.dsc.
Cc: Jian J Wang
Cc: Liming Gao
Cc: Siyuan Fu
Cc: Ni Ray
Cc: Sami Mujawar
Cc: Ilias Apalodimas
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Signed-off-by: Zhihao Li
---
MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c | 29
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 10
++-
MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h | 27
++
MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.inf | 27
++
MdeModulePkg/MdeModulePkg.dec| 5
+++-
MdeModulePkg/MdeModulePkg.dsc| 5
+++-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3
+-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | 3
+-
8 files changed, 104 insertions(+), 5 deletions(-)
diff --git
a/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c
b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c
new file mode 100644
index ..474195bbb374
--- /dev/null
+++ b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.c
@@ -0,0 +1,29 @@
+/** @file
+ SMM CPU Rendezvous sevice implement.
+
+ Copyright (c) 2022, Intel Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include
+#include
+
+/**
+ This routine wait for all AP processors to arrive in SMM.
+
+ @param[in] BlockingMode Blocking mode or non-blocking mode.
+
+ @retval EFI_SUCCESS All avaiable APs arrived.
+ @retval EFI_TIMEOUT Wait for all APs until timeout.
+ @retval OTHERFail to register SMM CPU Rendezvous service Protocol.
+**/
+EFI_STATUS
+EFIAPI
+SmmWaitForAllProcessor (
+ IN BOOLEAN BlockingMode
+ )
+{
+ ASSERT (FALSE);
+ return EFI_SUCCESS;
+}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index 517cae7b00f8..52a9b0e6b202 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -14,7 +14,7 @@
VariableServiceSetVariable(), VariableServiceQueryVariableInfo(),
ReclaimForOS(),
SmmVariableGetStatistics() should also do validation based on its own
knowledge.
-Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2022, Intel Corporation. All rights reserved.
Copyright (c) 2018, Linaro, Ltd. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -28,6 +28,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include
#include
+#include
#include
#include "Variable.h"
@@ -656,6 +657,13 @@ SmmVariableHandler (
goto EXIT;
}
+ if ((SmmVariableHeader->Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
+if (EFI_ERROR (SmmWaitForAllProcessor (TRUE))) {
+ DEBUG ((DEBUG_ERROR, "SetVariable: fail to wait for all AP check in
SMM!\n"));
+ goto EXIT;
+}
+ }
+
Status = VariableServiceSetVariable (
SmmVariableHeader->Name,
>Guid,
diff --git a/MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h
b/MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h
new file mode 100644
index ..82e459e9106e
--- /dev/null
+++ b/MdeModulePkg/Include/Library/SmmCpuRendezvousLib.h
@@ -0,0 +1,27 @@
+/** @file
+ SMM CPU Rendezvous library header file.
+
+ Copyright (c) 2022, Intel Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef SMM_CPU_RENDEZVOUS_H_
+#define SMM_CPU_RENDEZVOUS_H_
+
+/**
+ This routine wait for all AP processors to arrive in SMM.
+
+ @param[in] BlockingMode Blocking mode or non-blocking mode.
+
+ @retval EFI_SUCCESS All processors checked in to SMM.
+ @retval EFI_TIMEOUT Wait for all APs until timeout.
+
+**/
+EFI_STATUS
+EFIAPI
+SmmWaitForAllProcessor (
+ IN BOOLEAN BlockingMode
+ );
+
+#endif
diff --git
a/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.inf
b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.inf
new file mode 100644
index ..0bd4f39e7277
--- /dev/null
+++ b/MdeModulePkg/Library/SmmCpuRendezvousLibNull/SmmCpuRendezvousLibNull.inf
@@ -0,0 +1,27 @@
+## @file
+# SMM CPU Rendezvous service
