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