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 (#79636): https://edk2.groups.io/g/devel/message/79636 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] -=-=-=-=-=-=-=-=-=-=-=-
