[edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

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

Adds a NULL instance of SmmAccessLib.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c   | 33 ++++++++++++++++++++
 Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf | 26 +++++++++++++++
 Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                                     |  1 +
 3 files changed, 60 insertions(+)

diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
new file mode 100644
index 000000000000..f5ad306b380b
--- /dev/null
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
@@ -0,0 +1,33 @@
+/** @file
+  A NULL library instance of SmmAccessLib.
+
+  Copyright (c) 2019 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) Microsoft Corporation.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+#include <Library/DebugLib.h>
+#include <Library/SmmAccessLib.h>
+
+/**
+  This function is to install an SMM Access PPI
+
+  @retval EFI_SUCCESS           - Ppi successfully started and installed.
+  @retval EFI_NOT_FOUND         - Ppi can't be found.
+  @retval EFI_OUT_OF_RESOURCES  - Ppi does not have enough resources to initialize the driver.
+  @retval EFI_UNSUPPORTED       - The PPI was not installed and installation is unsupported in
+                                  this instance of function implementation.
+
+**/
+EFI_STATUS
+EFIAPI
+PeiInstallSmmAccessPpi (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
new file mode 100644
index 000000000000..7fd3b0b89655
--- /dev/null
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
@@ -0,0 +1,26 @@
+## @file
+# A NULL library instance of SmmAccessLib.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) Microsoft Corporation.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+INF_VERSION     = 0x00010017
+BASE_NAME       = BaseSmmAccessLibNull
+FILE_GUID       = C1A14AB6-B757-4046-9B92-9DCE1A2154C6
+VERSION_STRING  = 1.0
+MODULE_TYPE     = BASE
+LIBRARY_CLASS   = SmmAccessLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  IntelSiliconPkg/IntelSiliconPkg.dec
+
+[LibraryClasses]
+  DebugLib
+
+[Sources]
+  BaseSmmAccessLibNull.c
diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
index 1092371d848e..dd0928ec58f3 100644
--- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
+++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
@@ -90,6 +90,7 @@ [Components]
   IntelSiliconPkg/Feature/Capsule/MicrocodeUpdateDxe/MicrocodeUpdateDxe.inf
   IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashAccessLibNull/MicrocodeFlashAccessLibNull.inf
   IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
+  IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
   IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
   IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
   IntelSiliconPkg/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
-- 
2.28.0.windows.1

Re: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Chaganty, Rangasai V]

Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>  

-----Original Message-----
From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com> 
Sent: Monday, August 09, 2021 7:16 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
Subject: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

From: Michael Kubacki <michael.kubacki@microsoft.com>

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

Adds a NULL instance of SmmAccessLib.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c   | 33 ++++++++++++++++++++
 Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf | 26 +++++++++++++++
 Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                                     |  1 +
 3 files changed, 60 insertions(+)

diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
new file mode 100644
index 000000000000..f5ad306b380b
--- /dev/null
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAcc
+++ essLibNull/BaseSmmAccessLibNull.c
@@ -0,0 +1,33 @@
+/** @file
+  A NULL library instance of SmmAccessLib.
+
+  Copyright (c) 2019 - 2020, Intel Corporation. All rights 
+ reserved.<BR>  Copyright (c) Microsoft Corporation.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+#include <Library/DebugLib.h>
+#include <Library/SmmAccessLib.h>
+
+/**
+  This function is to install an SMM Access PPI
+
+  @retval EFI_SUCCESS           - Ppi successfully started and installed.
+  @retval EFI_NOT_FOUND         - Ppi can't be found.
+  @retval EFI_OUT_OF_RESOURCES  - Ppi does not have enough resources to initialize the driver.
+  @retval EFI_UNSUPPORTED       - The PPI was not installed and installation is unsupported in
+                                  this instance of function implementation.
+
+**/
+EFI_STATUS
+EFIAPI
+PeiInstallSmmAccessPpi (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
new file mode 100644
index 000000000000..7fd3b0b89655
--- /dev/null
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAcc
+++ essLibNull/BaseSmmAccessLibNull.inf
@@ -0,0 +1,26 @@
+## @file
+# A NULL library instance of SmmAccessLib.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> # 
+Copyright (c) Microsoft Corporation.<BR> # SPDX-License-Identifier: 
+BSD-2-Clause-Patent # ##
+
+[Defines]
+INF_VERSION     = 0x00010017
+BASE_NAME       = BaseSmmAccessLibNull
+FILE_GUID       = C1A14AB6-B757-4046-9B92-9DCE1A2154C6
+VERSION_STRING  = 1.0
+MODULE_TYPE     = BASE
+LIBRARY_CLASS   = SmmAccessLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  IntelSiliconPkg/IntelSiliconPkg.dec
+
+[LibraryClasses]
+  DebugLib
+
+[Sources]
+  BaseSmmAccessLibNull.c
diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
index 1092371d848e..dd0928ec58f3 100644
--- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
+++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
@@ -90,6 +90,7 @@ [Components]
   IntelSiliconPkg/Feature/Capsule/MicrocodeUpdateDxe/MicrocodeUpdateDxe.inf
   IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashAccessLibNull/MicrocodeFlashAccessLibNull.inf
   IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
+  
+ IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmm
+ AccessLibNull.inf
   IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
   IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
   IntelSiliconPkg/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
--
2.28.0.windows.1

Re: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Ni, Ray]

Michael,
If your platform doesn't need SmmAccessPPI, you don't put the SmmAccess PEIM in the FDF.
Why do you need:
1. Put SmmAccess PEIM in FDF
2. Let SmmAccess PEIM link to a NULL dummy-do-nothing library

I feel the additional abstraction is not necessary.

Thanks,
Ray

> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Monday, August 9, 2021 10:16 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3540
> 
> Adds a NULL instance of SmmAccessLib.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c   | 33
> ++++++++++++++++++++
>  Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf | 26
> +++++++++++++++
>  Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                                     |  1 +
>  3 files changed, 60 insertions(+)
> 
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
> new file mode 100644
> index 000000000000..f5ad306b380b
> --- /dev/null
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
> @@ -0,0 +1,33 @@
> +/** @file
> +  A NULL library instance of SmmAccessLib.
> +
> +  Copyright (c) 2019 - 2020, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) Microsoft Corporation.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <Library/DebugLib.h>
> +#include <Library/SmmAccessLib.h>
> +
> +/**
> +  This function is to install an SMM Access PPI
> +
> +  @retval EFI_SUCCESS           - Ppi successfully started and installed.
> +  @retval EFI_NOT_FOUND         - Ppi can't be found.
> +  @retval EFI_OUT_OF_RESOURCES  - Ppi does not have enough resources to initialize the driver.
> +  @retval EFI_UNSUPPORTED       - The PPI was not installed and installation is unsupported in
> +                                  this instance of function implementation.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +PeiInstallSmmAccessPpi (
> +  VOID
> +  )
> +{
> +  ASSERT (FALSE);
> +  return EFI_UNSUPPORTED;
> +}
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
> new file mode 100644
> index 000000000000..7fd3b0b89655
> --- /dev/null
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
> @@ -0,0 +1,26 @@
> +## @file
> +# A NULL library instance of SmmAccessLib.
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) Microsoft Corporation.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +INF_VERSION     = 0x00010017
> +BASE_NAME       = BaseSmmAccessLibNull
> +FILE_GUID       = C1A14AB6-B757-4046-9B92-9DCE1A2154C6
> +VERSION_STRING  = 1.0
> +MODULE_TYPE     = BASE
> +LIBRARY_CLASS   = SmmAccessLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  IntelSiliconPkg/IntelSiliconPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +
> +[Sources]
> +  BaseSmmAccessLibNull.c
> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> index 1092371d848e..dd0928ec58f3 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> @@ -90,6 +90,7 @@ [Components]
>    IntelSiliconPkg/Feature/Capsule/MicrocodeUpdateDxe/MicrocodeUpdateDxe.inf
>    IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashAccessLibNull/MicrocodeFlashAccessLibNull.inf
>    IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
> +  IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
>    IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
>    IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
>    IntelSiliconPkg/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
> --
> 2.28.0.windows.1

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

There is not a SmmAccess PEIM in IntelSiliconPkg.

There is a SmmAccessDxe driver:
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAccess.inf

And there is a PeiSmmAccessLib:
https://github.com/tianocore/edk2-platforms/tree/master/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess

As long as this is a library class, the NULL library instance is 
necessary for the same reason many NULL instances are necessary.

Scenario #1:

If a user has a library that depends on SmmAccessLib and that library is 
linked to two different modules, the user may wish to actually call the 
install function from one module and not link the functionality (or 
inherit the non-NULL SmmAccessLib dependencies) in the other module.

Scenario #2:

Someone is trying to build common code. That common code has a PEIM. 
That PEIM links SmmAccessLib. A downstream package leveraging that PEIM 
does not use the functionality in the PEIM that invokes SmmAccessLib. 
Therefore, the package would like to reduce the overall PEIM 
dependencies by linking a NULL library instance.

---

This is the result of an invoke-once interface being exposed as a 
generic library class.

---

This patch is not trying to modify interfaces such as converting the 
library class to a driver or anything more involved/impactful. It is 
simply making the design already in place have an alternative, optional 
instance available for platform integration.

Thanks
Michael

On 8/9/2021 10:30 PM, Ni, Ray wrote:
> Michael,
> If your platform doesn't need SmmAccessPPI, you don't put the SmmAccess PEIM in the FDF.
> Why do you need:
> 1. Put SmmAccess PEIM in FDF
> 2. Let SmmAccess PEIM link to a NULL dummy-do-nothing library
> 
> I feel the additional abstraction is not necessary.
> 
> Thanks,
> Ray
> 
>> -----Original Message-----
>> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
>> Sent: Monday, August 9, 2021 10:16 PM
>> To: devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
>> Subject: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3540
>>
>> Adds a NULL instance of SmmAccessLib.
>>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>   Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c   | 33
>> ++++++++++++++++++++
>>   Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf | 26
>> +++++++++++++++
>>   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                                     |  1 +
>>   3 files changed, 60 insertions(+)
>>
>> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
>> new file mode 100644
>> index 000000000000..f5ad306b380b
>> --- /dev/null
>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c
>> @@ -0,0 +1,33 @@
>> +/** @file
>> +  A NULL library instance of SmmAccessLib.
>> +
>> +  Copyright (c) 2019 - 2020, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) Microsoft Corporation.<BR>
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +
>> +#include <Uefi.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/SmmAccessLib.h>
>> +
>> +/**
>> +  This function is to install an SMM Access PPI
>> +
>> +  @retval EFI_SUCCESS           - Ppi successfully started and installed.
>> +  @retval EFI_NOT_FOUND         - Ppi can't be found.
>> +  @retval EFI_OUT_OF_RESOURCES  - Ppi does not have enough resources to initialize the driver.
>> +  @retval EFI_UNSUPPORTED       - The PPI was not installed and installation is unsupported in
>> +                                  this instance of function implementation.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +PeiInstallSmmAccessPpi (
>> +  VOID
>> +  )
>> +{
>> +  ASSERT (FALSE);
>> +  return EFI_UNSUPPORTED;
>> +}
>> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
>> new file mode 100644
>> index 000000000000..7fd3b0b89655
>> --- /dev/null
>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
>> @@ -0,0 +1,26 @@
>> +## @file
>> +# A NULL library instance of SmmAccessLib.
>> +#
>> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>> +# Copyright (c) Microsoft Corporation.<BR>
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
>> +[Defines]
>> +INF_VERSION     = 0x00010017
>> +BASE_NAME       = BaseSmmAccessLibNull
>> +FILE_GUID       = C1A14AB6-B757-4046-9B92-9DCE1A2154C6
>> +VERSION_STRING  = 1.0
>> +MODULE_TYPE     = BASE
>> +LIBRARY_CLASS   = SmmAccessLib
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  IntelSiliconPkg/IntelSiliconPkg.dec
>> +
>> +[LibraryClasses]
>> +  DebugLib
>> +
>> +[Sources]
>> +  BaseSmmAccessLibNull.c
>> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
>> index 1092371d848e..dd0928ec58f3 100644
>> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
>> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
>> @@ -90,6 +90,7 @@ [Components]
>>     IntelSiliconPkg/Feature/Capsule/MicrocodeUpdateDxe/MicrocodeUpdateDxe.inf
>>     IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashAccessLibNull/MicrocodeFlashAccessLibNull.inf
>>     IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
>> +  IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.inf
>>     IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
>>     IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
>>     IntelSiliconPkg/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
>> --
>> 2.28.0.windows.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Ni, Ray]

Michael,
Can you give a concrete example? I feel difficult to understand the two scenarios.

Thanks,
Ray


-----Original Message-----
From: Michael Kubacki <mikuback@linux.microsoft.com> 
Sent: Tuesday, August 10, 2021 11:29 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

There is not a SmmAccess PEIM in IntelSiliconPkg.

There is a SmmAccessDxe driver:
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAccess.inf

And there is a PeiSmmAccessLib:
https://github.com/tianocore/edk2-platforms/tree/master/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess

As long as this is a library class, the NULL library instance is necessary for the same reason many NULL instances are necessary.

Scenario #1:

If a user has a library that depends on SmmAccessLib and that library is linked to two different modules, the user may wish to actually call the install function from one module and not link the functionality (or inherit the non-NULL SmmAccessLib dependencies) in the other module.

Scenario #2:

Someone is trying to build common code. That common code has a PEIM. 
That PEIM links SmmAccessLib. A downstream package leveraging that PEIM does not use the functionality in the PEIM that invokes SmmAccessLib. 
Therefore, the package would like to reduce the overall PEIM dependencies by linking a NULL library instance.

---

This is the result of an invoke-once interface being exposed as a generic library class.

---

This patch is not trying to modify interfaces such as converting the library class to a driver or anything more involved/impactful. It is simply making the design already in place have an alternative, optional instance available for platform integration.

Thanks
Michael

On 8/9/2021 10:30 PM, Ni, Ray wrote:
> Michael,
> If your platform doesn't need SmmAccessPPI, you don't put the SmmAccess PEIM in the FDF.
> Why do you need:
> 1. Put SmmAccess PEIM in FDF
> 2. Let SmmAccess PEIM link to a NULL dummy-do-nothing library
> 
> I feel the additional abstraction is not necessary.
> 
> Thanks,
> Ray
> 
>> -----Original Message-----
>> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
>> Sent: Monday, August 9, 2021 10:16 PM
>> To: devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V 
>> <rangasai.v.chaganty@intel.com>
>> Subject: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add 
>> BaseSmmAccessLibNull
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3540
>>
>> Adds a NULL instance of SmmAccessLib.
>>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>   Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/BaseSmmAccessLibNull.c   | 33
>> ++++++++++++++++++++
>>   
>> Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccess
>> LibNull/BaseSmmAccessLibNull.inf | 26
>> +++++++++++++++
>>   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                                     |  1 +
>>   3 files changed, 60 insertions(+)
>>
>> diff --git 
>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAcce
>> ssLibNull/BaseSmmAccessLibNull.c 
>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAcce
>> ssLibNull/BaseSmmAccessLibNull.c
>> new file mode 100644
>> index 000000000000..f5ad306b380b
>> --- /dev/null
>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmm
>> +++ AccessLibNull/BaseSmmAccessLibNull.c
>> @@ -0,0 +1,33 @@
>> +/** @file
>> +  A NULL library instance of SmmAccessLib.
>> +
>> +  Copyright (c) 2019 - 2020, Intel Corporation. All rights 
>> + reserved.<BR>  Copyright (c) Microsoft Corporation.<BR>
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +**/
>> +
>> +#include <Uefi.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/SmmAccessLib.h>
>> +
>> +/**
>> +  This function is to install an SMM Access PPI
>> +
>> +  @retval EFI_SUCCESS           - Ppi successfully started and installed.
>> +  @retval EFI_NOT_FOUND         - Ppi can't be found.
>> +  @retval EFI_OUT_OF_RESOURCES  - Ppi does not have enough resources to initialize the driver.
>> +  @retval EFI_UNSUPPORTED       - The PPI was not installed and installation is unsupported in
>> +                                  this instance of function implementation.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +PeiInstallSmmAccessPpi (
>> +  VOID
>> +  )
>> +{
>> +  ASSERT (FALSE);
>> +  return EFI_UNSUPPORTED;
>> +}
>> diff --git 
>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAcce
>> ssLibNull/BaseSmmAccessLibNull.inf
>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAcce
>> ssLibNull/BaseSmmAccessLibNull.inf
>> new file mode 100644
>> index 000000000000..7fd3b0b89655
>> --- /dev/null
>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmm
>> +++ AccessLibNull/BaseSmmAccessLibNull.inf
>> @@ -0,0 +1,26 @@
>> +## @file
>> +# A NULL library instance of SmmAccessLib.
>> +#
>> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> # 
>> +Copyright (c) Microsoft Corporation.<BR> # SPDX-License-Identifier: 
>> +BSD-2-Clause-Patent # ##
>> +
>> +[Defines]
>> +INF_VERSION     = 0x00010017
>> +BASE_NAME       = BaseSmmAccessLibNull
>> +FILE_GUID       = C1A14AB6-B757-4046-9B92-9DCE1A2154C6
>> +VERSION_STRING  = 1.0
>> +MODULE_TYPE     = BASE
>> +LIBRARY_CLASS   = SmmAccessLib
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  IntelSiliconPkg/IntelSiliconPkg.dec
>> +
>> +[LibraryClasses]
>> +  DebugLib
>> +
>> +[Sources]
>> +  BaseSmmAccessLibNull.c
>> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc 
>> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
>> index 1092371d848e..dd0928ec58f3 100644
>> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
>> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
>> @@ -90,6 +90,7 @@ [Components]
>>     IntelSiliconPkg/Feature/Capsule/MicrocodeUpdateDxe/MicrocodeUpdateDxe.inf
>>     IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashAccessLibNull/MicrocodeFlashAccessLibNull.inf
>>     IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
>> +  
>> + IntelSiliconPkg/Feature/SmmAccess/Library/BaseSmmAccessLibNull/Base
>> + SmmAccessLibNull.inf
>>     IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
>>     IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
>>     IntelSiliconPkg/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
>> --
>> 2.28.0.windows.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

Sure.

Scenario #1:

MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both link against an instance of BoardInitLib.

Many boards link against a single BoardInitLib instance. See example - https://github.com/tianocore/edk2-platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc#L203

That BoardInitLib instance may link against SmmAccessLib. PlatformInitPreMem may wish to library class override the SmmAccessLib to the NULL instance while keeping it to non-NULL instance in PlatformInitPostMem.

Scenario #2:

A PEIM is built that checks whether the boot mode is S3. If so, it calls PeiInstallSmmAccessPpi(). A particular platform does not support S3, therefore, it links BaseSmmAccessLibNull as its library instance for SmmAccessLib.

[-- Attachment #2: Type: text/html, Size: 953 bytes --]

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

Hi Ray,

It's been a while.

This change is really not that complicated. Can it be merged if there 
are no substantial opens?

Regards,
Michael

On 8/12/2021 10:16 PM, Michael Kubacki wrote:
> Sure.
> 
> Scenario #1:
> 
> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and 
> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both 
> link against an instance of BoardInitLib.
> 
> Many boards link against a single BoardInitLib instance. See example - 
> https://github.com/tianocore/edk2-platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc#L203
> 
> That BoardInitLib instance may link against SmmAccessLib. 
> PlatformInitPreMem may wish to library class override the SmmAccessLib 
> to the NULL instance while keeping it to non-NULL instance in 
> PlatformInitPostMem.
> 
> Scenario #2:
> 
> A PEIM is built that checks whether the boot mode is S3. If so, it calls 
> PeiInstallSmmAccessPpi(). A particular platform does not support S3, 
> therefore, it links BaseSmmAccessLibNull as its library instance for 
> SmmAccessLib.
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

Michael,
I don’t think scenario #1 is a good reason for NULL instance of SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem and post-mem board init.
Below solution can avoid NULL SmmAccessLib:
Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem one doesn’t link to SmmAccessLib.

For scenario #2, if a particular platform doesn’t support S3, why does this platform include the PEIM?

Please understand that I want to avoid introducing more abstraction layers.

Thanks,
Ray

From: Michael Kubacki <mikuback@linux.microsoft.com>
Sent: Friday, August 13, 2021 10:16 AM
To: Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

Sure.

Scenario #1:

MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both link against an instance of BoardInitLib.

Many boards link against a single BoardInitLib instance. See example - https://github.com/tianocore/edk2-platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc#L203

That BoardInitLib instance may link against SmmAccessLib. PlatformInitPreMem may wish to library class override the SmmAccessLib to the NULL instance while keeping it to non-NULL instance in PlatformInitPostMem.

Scenario #2:

A PEIM is built that checks whether the boot mode is S3. If so, it calls PeiInstallSmmAccessPpi(). A particular platform does not support S3, therefore, it links BaseSmmAccessLibNull as its library instance for SmmAccessLib.

[-- Attachment #2: Type: text/html, Size: 5690 bytes --]

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

I don't understand your argument.

The library class (SmmAccessLib) that already exists is the abstraction 
layer. This is not introducing a new layer of abstraction. It is using 
the current layer of abstraction.

Thanks,
Michael

On 8/19/2021 5:49 AM, Ni, Ray wrote:
> Michael,
> 
> I don’t think scenario #1 is a good reason for NULL instance of 
> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem 
> and post-mem board init.
> 
> Below solution can avoid NULL SmmAccessLib:
> 
> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem 
> one doesn’t link to SmmAccessLib.
> 
> For scenario #2, if a particular platform doesn’t support S3, why does 
> this platform include the PEIM?
> 
> Please understand that I want to avoid introducing more abstraction layers.
> 
> Thanks,
> 
> Ray
> 
> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
> *Sent:* Friday, August 13, 2021 10:16 AM
> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] 
> IntelSiliconPkg: Add BaseSmmAccessLibNull
> 
> Sure.
> 
> Scenario #1:
> 
> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and 
> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both 
> link against an instance of BoardInitLib.
> 
> Many boards link against a single BoardInitLib instance. See example - 
> https://github.com/tianocore/edk2-platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc#L203 
> <https://github.com/tianocore/edk2-platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenBoardPkg.dsc#L203>
> 
> That BoardInitLib instance may link against SmmAccessLib. 
> PlatformInitPreMem may wish to library class override the SmmAccessLib 
> to the NULL instance while keeping it to non-NULL instance in 
> PlatformInitPostMem.
> 
> Scenario #2:
> 
> A PEIM is built that checks whether the boot mode is S3. If so, it calls 
> PeiInstallSmmAccessPpi(). A particular platform does not support S3, 
> therefore, it links BaseSmmAccessLibNull as its library instance for 
> SmmAccessLib.
> 
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Ni, Ray]

Null SmmAccessLib is confusing to me. Have you evaluated the option:
Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
one doesn’t link to SmmAccessLib

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Friday, August 20, 2021 12:53 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
> 
> I don't understand your argument.
> 
> The library class (SmmAccessLib) that already exists is the abstraction
> layer. This is not introducing a new layer of abstraction. It is using
> the current layer of abstraction.
> 
> Thanks,
> Michael
> 
> On 8/19/2021 5:49 AM, Ni, Ray wrote:
> > Michael,
> >
> > I don’t think scenario #1 is a good reason for NULL instance of
> > SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
> > and post-mem board init.
> >
> > Below solution can avoid NULL SmmAccessLib:
> >
> > Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
> > one doesn’t link to SmmAccessLib.
> >
> > For scenario #2, if a particular platform doesn’t support S3, why does
> > this platform include the PEIM?
> >
> > Please understand that I want to avoid introducing more abstraction layers.
> >
> > Thanks,
> >
> > Ray
> >
> > *From:* Michael Kubacki <mikuback@linux.microsoft.com>
> > *Sent:* Friday, August 13, 2021 10:16 AM
> > *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
> > IntelSiliconPkg: Add BaseSmmAccessLibNull
> >
> > Sure.
> >
> > Scenario #1:
> >
> > MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
> > MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
> > link against an instance of BoardInitLib.
> >
> > Many boards link against a single BoardInitLib instance. See example -
> > https://github.com/tianocore/edk2-
> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
> oardPkg.dsc#L203
> > <https://github.com/tianocore/edk2-
> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
> oardPkg.dsc#L203>
> >
> > That BoardInitLib instance may link against SmmAccessLib.
> > PlatformInitPreMem may wish to library class override the SmmAccessLib
> > to the NULL instance while keeping it to non-NULL instance in
> > PlatformInitPostMem.
> >
> > Scenario #2:
> >
> > A PEIM is built that checks whether the boot mode is S3. If so, it calls
> > PeiInstallSmmAccessPpi(). A particular platform does not support S3,
> > therefore, it links BaseSmmAccessLibNull as its library instance for
> > SmmAccessLib.
> >
> >
> 
> 
> 
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

Since you asked for an example that was just one that I provided. I 
don't think it detracts from the fact that a NULL instance makes sense 
if the SmmAccessLib library class exists. The fact that a NULL instance 
could not be allowed to exist is also confusing.

I don't want to get too distracted with the example given, but I 
completely agree that a different library instance should be used for 
pre-memory and post-memory. I think the library interface is too broad 
in scope and that contributes to causing this issue so I filed this BZ 
to request the BoardInitLib API be refactored:
https://bugzilla.tianocore.org/show_bug.cgi?id=3578

 From the other email thread about SmmAccessLib, I think we are on the 
same page that the library would be better as a PEIM. Is that something 
that could be done soon? Or could we have this until that is done?

Thanks,
Michael

On 8/20/2021 1:33 AM, Ni, Ray wrote:
> Null SmmAccessLib is confusing to me. Have you evaluated the option:
> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
> one doesn’t link to SmmAccessLib
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Friday, August 20, 2021 12:53 AM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
>>
>> I don't understand your argument.
>>
>> The library class (SmmAccessLib) that already exists is the abstraction
>> layer. This is not introducing a new layer of abstraction. It is using
>> the current layer of abstraction.
>>
>> Thanks,
>> Michael
>>
>> On 8/19/2021 5:49 AM, Ni, Ray wrote:
>>> Michael,
>>>
>>> I don’t think scenario #1 is a good reason for NULL instance of
>>> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
>>> and post-mem board init.
>>>
>>> Below solution can avoid NULL SmmAccessLib:
>>>
>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>> one doesn’t link to SmmAccessLib.
>>>
>>> For scenario #2, if a particular platform doesn’t support S3, why does
>>> this platform include the PEIM?
>>>
>>> Please understand that I want to avoid introducing more abstraction layers.
>>>
>>> Thanks,
>>>
>>> Ray
>>>
>>> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
>>> *Sent:* Friday, August 13, 2021 10:16 AM
>>> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>>> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>
>>> Sure.
>>>
>>> Scenario #1:
>>>
>>> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
>>> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
>>> link against an instance of BoardInitLib.
>>>
>>> Many boards link against a single BoardInitLib instance. See example -
>>> https://github.com/tianocore/edk2-
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>> oardPkg.dsc#L203
>>> <https://github.com/tianocore/edk2-
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>> oardPkg.dsc#L203>
>>>
>>> That BoardInitLib instance may link against SmmAccessLib.
>>> PlatformInitPreMem may wish to library class override the SmmAccessLib
>>> to the NULL instance while keeping it to non-NULL instance in
>>> PlatformInitPostMem.
>>>
>>> Scenario #2:
>>>
>>> A PEIM is built that checks whether the boot mode is S3. If so, it calls
>>> PeiInstallSmmAccessPpi(). A particular platform does not support S3,
>>> therefore, it links BaseSmmAccessLibNull as its library instance for
>>> SmmAccessLib.
>>>
>>>
>>
>>
>> 
>>
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

Ray,

Do you have plans to do something here? Whether take this patch or 
refactor SmmAccessLib to a PEIM?

Thanks,
Michael

On 8/20/2021 3:34 PM, Michael Kubacki wrote:
> Since you asked for an example that was just one that I provided. I 
> don't think it detracts from the fact that a NULL instance makes sense 
> if the SmmAccessLib library class exists. The fact that a NULL instance 
> could not be allowed to exist is also confusing.
> 
> I don't want to get too distracted with the example given, but I 
> completely agree that a different library instance should be used for 
> pre-memory and post-memory. I think the library interface is too broad 
> in scope and that contributes to causing this issue so I filed this BZ 
> to request the BoardInitLib API be refactored:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3578
> 
>  From the other email thread about SmmAccessLib, I think we are on the 
> same page that the library would be better as a PEIM. Is that something 
> that could be done soon? Or could we have this until that is done?
> 
> Thanks,
> Michael
> 
> On 8/20/2021 1:33 AM, Ni, Ray wrote:
>> Null SmmAccessLib is confusing to me. Have you evaluated the option:
>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>> one doesn’t link to SmmAccessLib
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
>>> Michael Kubacki
>>> Sent: Friday, August 20, 2021 12:53 AM
>>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] 
>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>
>>> I don't understand your argument.
>>>
>>> The library class (SmmAccessLib) that already exists is the abstraction
>>> layer. This is not introducing a new layer of abstraction. It is using
>>> the current layer of abstraction.
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 8/19/2021 5:49 AM, Ni, Ray wrote:
>>>> Michael,
>>>>
>>>> I don’t think scenario #1 is a good reason for NULL instance of
>>>> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
>>>> and post-mem board init.
>>>>
>>>> Below solution can avoid NULL SmmAccessLib:
>>>>
>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>>> one doesn’t link to SmmAccessLib.
>>>>
>>>> For scenario #2, if a particular platform doesn’t support S3, why does
>>>> this platform include the PEIM?
>>>>
>>>> Please understand that I want to avoid introducing more abstraction 
>>>> layers.
>>>>
>>>> Thanks,
>>>>
>>>> Ray
>>>>
>>>> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
>>>> *Sent:* Friday, August 13, 2021 10:16 AM
>>>> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>>>> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>
>>>> Sure.
>>>>
>>>> Scenario #1:
>>>>
>>>> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
>>>> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
>>>> link against an instance of BoardInitLib.
>>>>
>>>> Many boards link against a single BoardInitLib instance. See example -
>>>> https://github.com/tianocore/edk2-
>>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB 
>>>
>>> oardPkg.dsc#L203
>>>> <https://github.com/tianocore/edk2-
>>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB 
>>>
>>> oardPkg.dsc#L203>
>>>>
>>>> That BoardInitLib instance may link against SmmAccessLib.
>>>> PlatformInitPreMem may wish to library class override the SmmAccessLib
>>>> to the NULL instance while keeping it to non-NULL instance in
>>>> PlatformInitPostMem.
>>>>
>>>> Scenario #2:
>>>>
>>>> A PEIM is built that checks whether the boot mode is S3. If so, it 
>>>> calls
>>>> PeiInstallSmmAccessPpi(). A particular platform does not support S3,
>>>> therefore, it links BaseSmmAccessLibNull as its library instance for
>>>> SmmAccessLib.
>>>>
>>>>
>>>
>>>
>>> 
>>>
>>

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Ni, Ray]

No, I don't.
I still don't think having a NULL SmmAccessLib is a good idea.

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Thursday, September 9, 2021 10:12 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
> 
> Ray,
> 
> Do you have plans to do something here? Whether take this patch or
> refactor SmmAccessLib to a PEIM?
> 
> Thanks,
> Michael
> 
> On 8/20/2021 3:34 PM, Michael Kubacki wrote:
> > Since you asked for an example that was just one that I provided. I
> > don't think it detracts from the fact that a NULL instance makes sense
> > if the SmmAccessLib library class exists. The fact that a NULL instance
> > could not be allowed to exist is also confusing.
> >
> > I don't want to get too distracted with the example given, but I
> > completely agree that a different library instance should be used for
> > pre-memory and post-memory. I think the library interface is too broad
> > in scope and that contributes to causing this issue so I filed this BZ
> > to request the BoardInitLib API be refactored:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=3578
> >
> >  From the other email thread about SmmAccessLib, I think we are on the
> > same page that the library would be better as a PEIM. Is that something
> > that could be done soon? Or could we have this until that is done?
> >
> > Thanks,
> > Michael
> >
> > On 8/20/2021 1:33 AM, Ni, Ray wrote:
> >> Null SmmAccessLib is confusing to me. Have you evaluated the option:
> >> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
> >> one doesn’t link to SmmAccessLib
> >>
> >>> -----Original Message-----
> >>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>> Michael Kubacki
> >>> Sent: Friday, August 20, 2021 12:53 AM
> >>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> >>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
> >>> IntelSiliconPkg: Add BaseSmmAccessLibNull
> >>>
> >>> I don't understand your argument.
> >>>
> >>> The library class (SmmAccessLib) that already exists is the abstraction
> >>> layer. This is not introducing a new layer of abstraction. It is using
> >>> the current layer of abstraction.
> >>>
> >>> Thanks,
> >>> Michael
> >>>
> >>> On 8/19/2021 5:49 AM, Ni, Ray wrote:
> >>>> Michael,
> >>>>
> >>>> I don’t think scenario #1 is a good reason for NULL instance of
> >>>> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
> >>>> and post-mem board init.
> >>>>
> >>>> Below solution can avoid NULL SmmAccessLib:
> >>>>
> >>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
> >>>> one doesn’t link to SmmAccessLib.
> >>>>
> >>>> For scenario #2, if a particular platform doesn’t support S3, why does
> >>>> this platform include the PEIM?
> >>>>
> >>>> Please understand that I want to avoid introducing more abstraction
> >>>> layers.
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Ray
> >>>>
> >>>> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
> >>>> *Sent:* Friday, August 13, 2021 10:16 AM
> >>>> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> >>>> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
> >>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
> >>>>
> >>>> Sure.
> >>>>
> >>>> Scenario #1:
> >>>>
> >>>> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
> >>>> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
> >>>> link against an instance of BoardInitLib.
> >>>>
> >>>> Many boards link against a single BoardInitLib instance. See example -
> >>>> https://github.com/tianocore/edk2-
> >>>
> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
> >>>
> >>> oardPkg.dsc#L203
> >>>> <https://github.com/tianocore/edk2-
> >>>
> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
> >>>
> >>> oardPkg.dsc#L203>
> >>>>
> >>>> That BoardInitLib instance may link against SmmAccessLib.
> >>>> PlatformInitPreMem may wish to library class override the SmmAccessLib
> >>>> to the NULL instance while keeping it to non-NULL instance in
> >>>> PlatformInitPostMem.
> >>>>
> >>>> Scenario #2:
> >>>>
> >>>> A PEIM is built that checks whether the boot mode is S3. If so, it
> >>>> calls
> >>>> PeiInstallSmmAccessPpi(). A particular platform does not support S3,
> >>>> therefore, it links BaseSmmAccessLibNull as its library instance for
> >>>> SmmAccessLib.
> >>>>
> >>>>
> >>>
> >>>
> >>> 
> >>>
> >>

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

So you would rather leave it as a library class instead of refactoring 
it to a PEIM?

Again, the problem is it is a library class. So I am asking whether you 
want to treat it as a library class or you are going to refactor it to a 
PEIM.

On 9/9/2021 10:49 AM, Ni, Ray wrote:
> No, I don't.
> I still don't think having a NULL SmmAccessLib is a good idea.
> 
>> -----Original Message-----
>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>> Sent: Thursday, September 9, 2021 10:12 PM
>> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
>>
>> Ray,
>>
>> Do you have plans to do something here? Whether take this patch or
>> refactor SmmAccessLib to a PEIM?
>>
>> Thanks,
>> Michael
>>
>> On 8/20/2021 3:34 PM, Michael Kubacki wrote:
>>> Since you asked for an example that was just one that I provided. I
>>> don't think it detracts from the fact that a NULL instance makes sense
>>> if the SmmAccessLib library class exists. The fact that a NULL instance
>>> could not be allowed to exist is also confusing.
>>>
>>> I don't want to get too distracted with the example given, but I
>>> completely agree that a different library instance should be used for
>>> pre-memory and post-memory. I think the library interface is too broad
>>> in scope and that contributes to causing this issue so I filed this BZ
>>> to request the BoardInitLib API be refactored:
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3578
>>>
>>>   From the other email thread about SmmAccessLib, I think we are on the
>>> same page that the library would be better as a PEIM. Is that something
>>> that could be done soon? Or could we have this until that is done?
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 8/20/2021 1:33 AM, Ni, Ray wrote:
>>>> Null SmmAccessLib is confusing to me. Have you evaluated the option:
>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>>> one doesn’t link to SmmAccessLib
>>>>
>>>>> -----Original Message-----
>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>> Michael Kubacki
>>>>> Sent: Friday, August 20, 2021 12:53 AM
>>>>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>>>>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>>
>>>>> I don't understand your argument.
>>>>>
>>>>> The library class (SmmAccessLib) that already exists is the abstraction
>>>>> layer. This is not introducing a new layer of abstraction. It is using
>>>>> the current layer of abstraction.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>>> On 8/19/2021 5:49 AM, Ni, Ray wrote:
>>>>>> Michael,
>>>>>>
>>>>>> I don’t think scenario #1 is a good reason for NULL instance of
>>>>>> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
>>>>>> and post-mem board init.
>>>>>>
>>>>>> Below solution can avoid NULL SmmAccessLib:
>>>>>>
>>>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>>>>> one doesn’t link to SmmAccessLib.
>>>>>>
>>>>>> For scenario #2, if a particular platform doesn’t support S3, why does
>>>>>> this platform include the PEIM?
>>>>>>
>>>>>> Please understand that I want to avoid introducing more abstraction
>>>>>> layers.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Ray
>>>>>>
>>>>>> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
>>>>>> *Sent:* Friday, August 13, 2021 10:16 AM
>>>>>> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>>>>>> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>>>
>>>>>> Sure.
>>>>>>
>>>>>> Scenario #1:
>>>>>>
>>>>>> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
>>>>>> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
>>>>>> link against an instance of BoardInitLib.
>>>>>>
>>>>>> Many boards link against a single BoardInitLib instance. See example -
>>>>>> https://github.com/tianocore/edk2-
>>>>>
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>>>>>
>>>>> oardPkg.dsc#L203
>>>>>> <https://github.com/tianocore/edk2-
>>>>>
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>>>>>
>>>>> oardPkg.dsc#L203>
>>>>>>
>>>>>> That BoardInitLib instance may link against SmmAccessLib.
>>>>>> PlatformInitPreMem may wish to library class override the SmmAccessLib
>>>>>> to the NULL instance while keeping it to non-NULL instance in
>>>>>> PlatformInitPostMem.
>>>>>>
>>>>>> Scenario #2:
>>>>>>
>>>>>> A PEIM is built that checks whether the boot mode is S3. If so, it
>>>>>> calls
>>>>>> PeiInstallSmmAccessPpi(). A particular platform does not support S3,
>>>>>> therefore, it links BaseSmmAccessLibNull as its library instance for
>>>>>> SmmAccessLib.
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
> 
> 
> 
> 
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Ni, Ray]

I think refactoring it to a PEIM is better.
But I am not sure if having below Bugzilla completed can meet your needs without refactoring the lib to PEIM.

> I don't want to get too distracted with the example given, but I 
> completely agree that a different library instance should be used for 
> pre-memory and post-memory. I think the library interface is too broad 
> in scope and that contributes to causing this issue so I filed this BZ 
> to request the BoardInitLib API be refactored:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3578

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Thursday, September 9, 2021 10:54 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
> 
> So you would rather leave it as a library class instead of refactoring
> it to a PEIM?
> 
> Again, the problem is it is a library class. So I am asking whether you
> want to treat it as a library class or you are going to refactor it to a
> PEIM.
> 
> On 9/9/2021 10:49 AM, Ni, Ray wrote:
> > No, I don't.
> > I still don't think having a NULL SmmAccessLib is a good idea.
> >
> >> -----Original Message-----
> >> From: Michael Kubacki <mikuback@linux.microsoft.com>
> >> Sent: Thursday, September 9, 2021 10:12 PM
> >> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> >> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
> >>
> >> Ray,
> >>
> >> Do you have plans to do something here? Whether take this patch or
> >> refactor SmmAccessLib to a PEIM?
> >>
> >> Thanks,
> >> Michael
> >>
> >> On 8/20/2021 3:34 PM, Michael Kubacki wrote:
> >>> Since you asked for an example that was just one that I provided. I
> >>> don't think it detracts from the fact that a NULL instance makes sense
> >>> if the SmmAccessLib library class exists. The fact that a NULL instance
> >>> could not be allowed to exist is also confusing.
> >>>
> >>> I don't want to get too distracted with the example given, but I
> >>> completely agree that a different library instance should be used for
> >>> pre-memory and post-memory. I think the library interface is too broad
> >>> in scope and that contributes to causing this issue so I filed this BZ
> >>> to request the BoardInitLib API be refactored:
> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=3578
> >>>
> >>>   From the other email thread about SmmAccessLib, I think we are on the
> >>> same page that the library would be better as a PEIM. Is that something
> >>> that could be done soon? Or could we have this until that is done?
> >>>
> >>> Thanks,
> >>> Michael
> >>>
> >>> On 8/20/2021 1:33 AM, Ni, Ray wrote:
> >>>> Null SmmAccessLib is confusing to me. Have you evaluated the option:
> >>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
> >>>> one doesn’t link to SmmAccessLib
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>>>> Michael Kubacki
> >>>>> Sent: Friday, August 20, 2021 12:53 AM
> >>>>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> >>>>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
> >>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
> >>>>>
> >>>>> I don't understand your argument.
> >>>>>
> >>>>> The library class (SmmAccessLib) that already exists is the abstraction
> >>>>> layer. This is not introducing a new layer of abstraction. It is using
> >>>>> the current layer of abstraction.
> >>>>>
> >>>>> Thanks,
> >>>>> Michael
> >>>>>
> >>>>> On 8/19/2021 5:49 AM, Ni, Ray wrote:
> >>>>>> Michael,
> >>>>>>
> >>>>>> I don’t think scenario #1 is a good reason for NULL instance of
> >>>>>> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
> >>>>>> and post-mem board init.
> >>>>>>
> >>>>>> Below solution can avoid NULL SmmAccessLib:
> >>>>>>
> >>>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
> >>>>>> one doesn’t link to SmmAccessLib.
> >>>>>>
> >>>>>> For scenario #2, if a particular platform doesn’t support S3, why does
> >>>>>> this platform include the PEIM?
> >>>>>>
> >>>>>> Please understand that I want to avoid introducing more abstraction
> >>>>>> layers.
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>> Ray
> >>>>>>
> >>>>>> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
> >>>>>> *Sent:* Friday, August 13, 2021 10:16 AM
> >>>>>> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> >>>>>> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
> >>>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
> >>>>>>
> >>>>>> Sure.
> >>>>>>
> >>>>>> Scenario #1:
> >>>>>>
> >>>>>> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
> >>>>>> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
> >>>>>> link against an instance of BoardInitLib.
> >>>>>>
> >>>>>> Many boards link against a single BoardInitLib instance. See example -
> >>>>>> https://github.com/tianocore/edk2-
> >>>>>
> >>
> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
> >>>>>
> >>>>> oardPkg.dsc#L203
> >>>>>> <https://github.com/tianocore/edk2-
> >>>>>
> >>
> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
> >>>>>
> >>>>> oardPkg.dsc#L203>
> >>>>>>
> >>>>>> That BoardInitLib instance may link against SmmAccessLib.
> >>>>>> PlatformInitPreMem may wish to library class override the SmmAccessLib
> >>>>>> to the NULL instance while keeping it to non-NULL instance in
> >>>>>> PlatformInitPostMem.
> >>>>>>
> >>>>>> Scenario #2:
> >>>>>>
> >>>>>> A PEIM is built that checks whether the boot mode is S3. If so, it
> >>>>>> calls
> >>>>>> PeiInstallSmmAccessPpi(). A particular platform does not support S3,
> >>>>>> therefore, it links BaseSmmAccessLibNull as its library instance for
> >>>>>> SmmAccessLib.
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >
> >
> >
> >
> >
> 
> 
> 
> 

Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

[Michael Kubacki]

I think it would be nice to have both. That was just an example of where 
SmmAccessLib could be linked that would cause two instances of the 
library API to be invoked by common code that is linked to two different 
modules.

Some platforms might not use BoardInitLib that could benefit from the 
PEIM in IntelSiliconPkg.

On 9/9/2021 10:58 AM, Ni, Ray wrote:
> I think refactoring it to a PEIM is better.
> But I am not sure if having below Bugzilla completed can meet your needs without refactoring the lib to PEIM.
> 
>> I don't want to get too distracted with the example given, but I
>> completely agree that a different library instance should be used for
>> pre-memory and post-memory. I think the library interface is too broad
>> in scope and that contributes to causing this issue so I filed this BZ
>> to request the BoardInitLib API be refactored:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3578
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Thursday, September 9, 2021 10:54 PM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
>>
>> So you would rather leave it as a library class instead of refactoring
>> it to a PEIM?
>>
>> Again, the problem is it is a library class. So I am asking whether you
>> want to treat it as a library class or you are going to refactor it to a
>> PEIM.
>>
>> On 9/9/2021 10:49 AM, Ni, Ray wrote:
>>> No, I don't.
>>> I still don't think having a NULL SmmAccessLib is a good idea.
>>>
>>>> -----Original Message-----
>>>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>>>> Sent: Thursday, September 9, 2021 10:12 PM
>>>> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>>>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>
>>>> Ray,
>>>>
>>>> Do you have plans to do something here? Whether take this patch or
>>>> refactor SmmAccessLib to a PEIM?
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>> On 8/20/2021 3:34 PM, Michael Kubacki wrote:
>>>>> Since you asked for an example that was just one that I provided. I
>>>>> don't think it detracts from the fact that a NULL instance makes sense
>>>>> if the SmmAccessLib library class exists. The fact that a NULL instance
>>>>> could not be allowed to exist is also confusing.
>>>>>
>>>>> I don't want to get too distracted with the example given, but I
>>>>> completely agree that a different library instance should be used for
>>>>> pre-memory and post-memory. I think the library interface is too broad
>>>>> in scope and that contributes to causing this issue so I filed this BZ
>>>>> to request the BoardInitLib API be refactored:
>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3578
>>>>>
>>>>>    From the other email thread about SmmAccessLib, I think we are on the
>>>>> same page that the library would be better as a PEIM. Is that something
>>>>> that could be done soon? Or could we have this until that is done?
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>>> On 8/20/2021 1:33 AM, Ni, Ray wrote:
>>>>>> Null SmmAccessLib is confusing to me. Have you evaluated the option:
>>>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>>>>> one doesn’t link to SmmAccessLib
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>>>> Michael Kubacki
>>>>>>> Sent: Friday, August 20, 2021 12:53 AM
>>>>>>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>>>>>>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>>>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>>>>
>>>>>>> I don't understand your argument.
>>>>>>>
>>>>>>> The library class (SmmAccessLib) that already exists is the abstraction
>>>>>>> layer. This is not introducing a new layer of abstraction. It is using
>>>>>>> the current layer of abstraction.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Michael
>>>>>>>
>>>>>>> On 8/19/2021 5:49 AM, Ni, Ray wrote:
>>>>>>>> Michael,
>>>>>>>>
>>>>>>>> I don’t think scenario #1 is a good reason for NULL instance of
>>>>>>>> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
>>>>>>>> and post-mem board init.
>>>>>>>>
>>>>>>>> Below solution can avoid NULL SmmAccessLib:
>>>>>>>>
>>>>>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>>>>>>> one doesn’t link to SmmAccessLib.
>>>>>>>>
>>>>>>>> For scenario #2, if a particular platform doesn’t support S3, why does
>>>>>>>> this platform include the PEIM?
>>>>>>>>
>>>>>>>> Please understand that I want to avoid introducing more abstraction
>>>>>>>> layers.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Ray
>>>>>>>>
>>>>>>>> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
>>>>>>>> *Sent:* Friday, August 13, 2021 10:16 AM
>>>>>>>> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>>>>>>>> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>>>>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>>>>>
>>>>>>>> Sure.
>>>>>>>>
>>>>>>>> Scenario #1:
>>>>>>>>
>>>>>>>> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
>>>>>>>> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
>>>>>>>> link against an instance of BoardInitLib.
>>>>>>>>
>>>>>>>> Many boards link against a single BoardInitLib instance. See example -
>>>>>>>> https://github.com/tianocore/edk2-
>>>>>>>
>>>>
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>>>>>>>
>>>>>>> oardPkg.dsc#L203
>>>>>>>> <https://github.com/tianocore/edk2-
>>>>>>>
>>>>
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>>>>>>>
>>>>>>> oardPkg.dsc#L203>
>>>>>>>>
>>>>>>>> That BoardInitLib instance may link against SmmAccessLib.
>>>>>>>> PlatformInitPreMem may wish to library class override the SmmAccessLib
>>>>>>>> to the NULL instance while keeping it to non-NULL instance in
>>>>>>>> PlatformInitPostMem.
>>>>>>>>
>>>>>>>> Scenario #2:
>>>>>>>>
>>>>>>>> A PEIM is built that checks whether the boot mode is S3. If so, it
>>>>>>>> calls
>>>>>>>> PeiInstallSmmAccessPpi(). A particular platform does not support S3,
>>>>>>>> therefore, it links BaseSmmAccessLibNull as its library instance for
>>>>>>>> SmmAccessLib.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>> 
>>
>