* [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull @ 2021-08-09 14:15 Michael Kubacki 2021-08-09 22:42 ` Chaganty, Rangasai V 2021-08-10 2:30 ` Ni, Ray 0 siblings, 2 replies; 16+ messages in thread From: Michael Kubacki @ 2021-08-09 14:15 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Rangasai V Chaganty 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-09 14:15 [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull Michael Kubacki @ 2021-08-09 22:42 ` Chaganty, Rangasai V 2021-08-10 2:30 ` Ni, Ray 1 sibling, 0 replies; 16+ messages in thread From: Chaganty, Rangasai V @ 2021-08-09 22:42 UTC (permalink / raw) To: mikuback@linux.microsoft.com, devel@edk2.groups.io; +Cc: Ni, Ray 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-09 14:15 [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull Michael Kubacki 2021-08-09 22:42 ` Chaganty, Rangasai V @ 2021-08-10 2:30 ` Ni, Ray 2021-08-10 15:28 ` [edk2-devel] " Michael Kubacki 1 sibling, 1 reply; 16+ messages in thread From: Ni, Ray @ 2021-08-10 2:30 UTC (permalink / raw) To: mikuback@linux.microsoft.com, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-10 2:30 ` Ni, Ray @ 2021-08-10 15:28 ` Michael Kubacki 2021-08-12 5:32 ` Ni, Ray 0 siblings, 1 reply; 16+ messages in thread From: Michael Kubacki @ 2021-08-10 15:28 UTC (permalink / raw) To: devel, ray.ni; +Cc: Chaganty, Rangasai V 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 > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-10 15:28 ` [edk2-devel] " Michael Kubacki @ 2021-08-12 5:32 ` Ni, Ray 2021-08-13 2:16 ` Michael Kubacki 0 siblings, 1 reply; 16+ messages in thread From: Ni, Ray @ 2021-08-12 5:32 UTC (permalink / raw) To: Michael Kubacki, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V 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 > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-12 5:32 ` Ni, Ray @ 2021-08-13 2:16 ` Michael Kubacki 2021-08-18 18:45 ` Michael Kubacki 2021-08-19 9:49 ` Ni, Ray 0 siblings, 2 replies; 16+ messages in thread From: Michael Kubacki @ 2021-08-13 2:16 UTC (permalink / raw) To: Ni, Ray, devel [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-13 2:16 ` Michael Kubacki @ 2021-08-18 18:45 ` Michael Kubacki 2021-08-19 9:49 ` Ni, Ray 1 sibling, 0 replies; 16+ messages in thread From: Michael Kubacki @ 2021-08-18 18:45 UTC (permalink / raw) To: Ray, devel 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. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-13 2:16 ` Michael Kubacki 2021-08-18 18:45 ` Michael Kubacki @ 2021-08-19 9:49 ` Ni, Ray 2021-08-19 16:53 ` Michael Kubacki 1 sibling, 1 reply; 16+ messages in thread From: Ni, Ray @ 2021-08-19 9:49 UTC (permalink / raw) To: Michael Kubacki, devel@edk2.groups.io [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-19 9:49 ` Ni, Ray @ 2021-08-19 16:53 ` Michael Kubacki 2021-08-20 5:33 ` Ni, Ray 0 siblings, 1 reply; 16+ messages in thread From: Michael Kubacki @ 2021-08-19 16:53 UTC (permalink / raw) To: devel, ray.ni 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. > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-19 16:53 ` Michael Kubacki @ 2021-08-20 5:33 ` Ni, Ray 2021-08-20 19:34 ` Michael Kubacki 0 siblings, 1 reply; 16+ messages in thread From: Ni, Ray @ 2021-08-20 5:33 UTC (permalink / raw) To: devel@edk2.groups.io, mikuback@linux.microsoft.com 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. > > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-20 5:33 ` Ni, Ray @ 2021-08-20 19:34 ` Michael Kubacki 2021-09-09 14:12 ` Michael Kubacki 0 siblings, 1 reply; 16+ messages in thread From: Michael Kubacki @ 2021-08-20 19:34 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io 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. >>> >>> >> >> >> >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-08-20 19:34 ` Michael Kubacki @ 2021-09-09 14:12 ` Michael Kubacki 2021-09-09 14:49 ` Ni, Ray 0 siblings, 1 reply; 16+ messages in thread From: Michael Kubacki @ 2021-09-09 14:12 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io 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. >>>> >>>> >>> >>> >>> >>> >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-09-09 14:12 ` Michael Kubacki @ 2021-09-09 14:49 ` Ni, Ray 2021-09-09 14:54 ` Michael Kubacki 0 siblings, 1 reply; 16+ messages in thread From: Ni, Ray @ 2021-09-09 14:49 UTC (permalink / raw) To: Michael Kubacki, devel@edk2.groups.io 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. > >>>> > >>>> > >>> > >>> > >>> > >>> > >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-09-09 14:49 ` Ni, Ray @ 2021-09-09 14:54 ` Michael Kubacki 2021-09-09 14:58 ` Ni, Ray 0 siblings, 1 reply; 16+ messages in thread From: Michael Kubacki @ 2021-09-09 14:54 UTC (permalink / raw) To: devel, ray.ni 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. >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> >>>> > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-09-09 14:54 ` Michael Kubacki @ 2021-09-09 14:58 ` Ni, Ray 2021-09-09 15:23 ` Michael Kubacki 0 siblings, 1 reply; 16+ messages in thread From: Ni, Ray @ 2021-09-09 14:58 UTC (permalink / raw) To: devel@edk2.groups.io, mikuback@linux.microsoft.com 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. > >>>>>> > >>>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>> > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull 2021-09-09 14:58 ` Ni, Ray @ 2021-09-09 15:23 ` Michael Kubacki 0 siblings, 0 replies; 16+ messages in thread From: Michael Kubacki @ 2021-09-09 15:23 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io 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. >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> >>> >>> >>> >>> >>> >> >> >> >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-09-09 15:23 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-08-09 14:15 [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull Michael Kubacki 2021-08-09 22:42 ` Chaganty, Rangasai V 2021-08-10 2:30 ` Ni, Ray 2021-08-10 15:28 ` [edk2-devel] " Michael Kubacki 2021-08-12 5:32 ` Ni, Ray 2021-08-13 2:16 ` Michael Kubacki 2021-08-18 18:45 ` Michael Kubacki 2021-08-19 9:49 ` Ni, Ray 2021-08-19 16:53 ` Michael Kubacki 2021-08-20 5:33 ` Ni, Ray 2021-08-20 19:34 ` Michael Kubacki 2021-09-09 14:12 ` Michael Kubacki 2021-09-09 14:49 ` Ni, Ray 2021-09-09 14:54 ` Michael Kubacki 2021-09-09 14:58 ` Ni, Ray 2021-09-09 15:23 ` Michael Kubacki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox