Ray, When do you think this patch will get merged?
Thanks, Michael On 8/20/2021 4:11 AM, Ni, Ray wrote:
Michael, I am ok with the change that removes S3 check. I also confirmed that Jiewen's concern about inconsistent SMRAM state doesn't exist. Reviewed-by: Ray Ni <ray...@intel.com> I agree with you that converting this lib to a PEIM is better. Thanks, Ray-----Original Message----- From: Michael Kubacki <mikub...@linux.microsoft.com> Sent: Friday, August 20, 2021 3:45 AM To: devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>; Ni, Ray <ray...@intel.com>; Chaganty, Rangasai V <rangasai.v.chaga...@intel.com> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement Ray, In the end, PI modules are allowed to depend on non-deprecated PI PPIs. The scope of this change is about the API that installs the PPI. So I do not want to diverge the conversation about changing other code to avoid the PPI. I've already been waiting since April to get other IntelSiliconPkg changes merged (https://edk2.groups.io/g/devel/message/78600) and I would like to keep this conversation focused on the patch. This conversation has taken several tangents as I tried to clear up in this message: https://edk2.groups.io/g/devel/message/79544. I did not see a response, please read that if you have not. The maintainability issues of the code have become evident. The code is coupling unnecessary assumptions and dependencies and that is increasing the difficulty of understanding and modifying the code. The goal of this change was for the PeiInstallSmmAccessPpi() function to simply install the PPI without the boot mode being BOOT_ON_S3_RESUME which it already documents should be the case. Because the existing code attempted to couple platform design assumptions into a generic silicon package, we had to talk about: - Historical platform S3 requirements - Interface documentation that is inaccurate - Clarification around security ambiguity and implications to modifying the implementation - General dependencies for Standalone MM - Potential synchronization issues between the PI Spec defined interfaces EFI_SMARAM_HOB_DESCRIPTOR_BLOCK and EFI_PEI_MM_ACCESS_PPI - The role of MM Access PPI in other architecture boot flows In the process, some of these topics did yield issues for follow up: 1. The documentation interface should accurately reflect actual behavior. 2. The potential synchronization issue should be better understood and resolved. Note that this issue is not specific to non-S3 flows. PEIMs on S3 flows could easily read the HOB. BZ for 1: https://bugzilla.tianocore.org/show_bug.cgi?id=3575 BZ for 2: https://bugzilla.tianocore.org/show_bug.cgi?id=3576 It also seems the library would be better as a PEIM but that's another topic. Thanks, Michael On 8/19/2021 10:27 AM, Yao, Jiewen wrote:Do you want to open/close/lock SMRAM? If yes, then you need PPI. If no, then you don’t need PPI. Hob should be enough to provide such info. Thank you Yao Jiewen-----Original Message----- From: Ni, Ray <ray...@intel.com> Sent: Thursday, August 19, 2021 6:02 PM To: Yao, Jiewen <jiewen....@intel.com>; Michael Kubacki <michael.kuba...@outlook.com>; devel@edk2.groups.io; mikub...@linux.microsoft.com; Chaganty, Rangasai V <rangasai.v.chaga...@intel.com> Subject: RE: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement Jiewen, We have MmAccess PPI and gEfiMmPeiSmramMemoryReserveGuid HOB. If Standalone IPL can get the SMRAM range from the HOB, what's the purpose of MmAccess PPI? X86 silicon doesn't rely on MmAccess.Close/Lock to close SMRAM anymore. Does ARM need? Michael, Have you considered to remove the dep on MmAccess PPI from standalone MM? Thanks, Ray -----Original Message----- From: Yao, Jiewen <jiewen....@intel.com> Sent: Thursday, August 19, 2021 10:02 AM To: Michael Kubacki <michael.kuba...@outlook.com>; devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; mikub...@linux.microsoft.com; Chaganty, Rangasai V <rangasai.v.chaga...@intel.com> Subject: RE: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement The history was that we didn’t need MmAccessPei without S3. MmAccessPei was added for S3 resume purpose only. Today, if there is real use case to rely on MmAccessPei in normal boot path. Then we can add it. I could see the potential impact is: If MmAccessPei changes the SMRAM attribute in normal boot path, it MUST be reflected in the SmramHob. Otherwise, there is inconsistency issue in PEI/DXE, when DXE consumes the SmramHob. This is NOT required in S3, because we don’t run DXE in S3 path. Thank you Yao Jiewen-----Original Message----- From: Michael Kubacki <michael.kuba...@outlook.com> Sent: Thursday, August 19, 2021 2:47 AM To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; mikub...@linux.microsoft.com; Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Yao, Jiewen <jiewen....@intel.com> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement Jiewen/Sai, are you thinking about this? Thanks, Michael On 8/12/2021 1:20 AM, Ni, Ray wrote:Michael, I need Jiewen's input on why MmAccess and MmCommunication PPIs were notinstalled in normal boot path. Without understanding the reason, I don't have confidence to approve the change.Sai, Do you see other impacts to Intel platforms with this behavior change? Thanks, Ray -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of MichaelKubackiSent: Tuesday, August 10, 2021 11:36 PM To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Chaganty, Rangasai V<rangasai.v.chaga...@intel.com>Cc: Yao, Jiewen <jiewen....@intel.com> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirementInstallation is a platform decision. The buried dependency on boot mode in thisparticular function is just a roadblock platforms have to work around. The role of this API is to install the PPI.Thanks, Michael On 8/9/2021 9:47 PM, Ni, Ray wrote:Michael, Allowing the gPeiSmmAccessPpiGuid PPI installation in normal boot will further allow gEfiPeiSmmCommunicationPpiGuid installation in normal path,while without your change neither of the PPIs is installed in normal boot.+ Jiewen for potential security concern. Thanks, Ray-----Original Message----- From: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com> Sent: Tuesday, August 10, 2021 6:46 AM To: mikub...@linux.microsoft.com; devel@edk2.groups.io Cc: Ni, Ray <ray...@intel.com> Subject: RE: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement Reviewed-by: Sai Chaganty <rangasai.v.chaga...@intel.com> -----Original Message----- From: mikub...@linux.microsoft.com <mikub...@linux.microsoft.com> Sent: Monday, August 09, 2021 6:40 AM To: devel@edk2.groups.io Cc: Ni, Ray <ray...@intel.com>; Chaganty, Rangasai V <rangasai.v.chaga...@intel.com> Subject: [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement From: Michael Kubacki <michael.kuba...@microsoft.com> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3539 PeiInstallSmmAccessPpi() currently requires the boot mode be set to S3 toactually install gEfiPeiMmAccessPpiGuid.This change removes this requirement in the function implementation fortwo reasons:1. Practical use cases exist to require this PPI in cases other than the boot mode being set to BOOT_ON_S3_RESUME. 2. It is poor API design to implicitly bury this requirement within a function whose responsibility is to install the PPI. The caller can easily place arbitrary constraints around whether to call based on conditions such as the boot mode being BOOT_ON_S3_RESUME. Cc: Ray Ni <ray...@intel.com> Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com> Signed-off-by: Michael Kubacki <michael.kuba...@microsoft.com> ---Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLi b/PeiS mmAccessLib.c | 12 ------------1 file changed, 12 deletions(-) diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAc ces sLib/PeiSmmAccessLib.c b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAc ces sLib/PeiSmmAccessLib.c index d9bf4fba983e..4df0d695fdaf 100644 --- a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAc ces sLib/PeiSmmAccessLib.c +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiS +++ mmA +++ cce +++ ssLib/PeiSmmAccessLib.c @@ -252,19 +252,7 @@ PeiInstallSmmAccessPpi ( EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock; SMM_ACCESS_PRIVATE_DATA *SmmAccessPrivate; VOID *HobList; - EFI_BOOT_MODE BootMode; - Status = PeiServicesGetBootMode (&BootMode); - if (EFI_ERROR (Status)) { - // - // If not in S3 boot path. do nothing - // - return EFI_SUCCESS; - } - - if (BootMode != BOOT_ON_S3_RESUME) { - return EFI_SUCCESS; - } // // Initialize private data // -- 2.28.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80438): https://edk2.groups.io/g/devel/message/80438 Mute This Topic: https://groups.io/mt/84768258/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
