Michael, Let me check whether our internal code has added the S3 check before calling PeiInstallSmmAccessPpi(). Once that is done, I will merge this patch.
Thanks, Ray > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki > Sent: Thursday, September 9, 2021 10:10 PM > To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Yao, Jiewen > <jiewen....@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, > > 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 > >>>>>> not > >>>>> installed 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 > >>>>>> Michael > >>>>> Kubacki > >>>>>> Sent: 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 requirement > >>>>>> > >>>>>> Installation is a platform decision. The buried dependency on boot > >>>>>> mode in this > >>>>> particular 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 to > >>>>> actually install gEfiPeiMmAccessPpiGuid. > >>>>>>>> > >>>>>>>> This change removes this requirement in the function > >>>>>>>> implementation for > >>>>> two 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 (#80441): https://edk2.groups.io/g/devel/message/80441 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] -=-=-=-=-=-=-=-=-=-=-=-
