[edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
Yao, Jiewen
jiewen.yao at intel.com
Thu Aug 19 14:27:11 UTC 2021
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.ni at intel.com>
> Sent: Thursday, August 19, 2021 6:02 PM
> To: Yao, Jiewen <jiewen.yao at intel.com>; Michael Kubacki
> <michael.kubacki at outlook.com>; devel at edk2.groups.io;
> mikuback at linux.microsoft.com; Chaganty, Rangasai V
> <rangasai.v.chaganty at 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.yao at intel.com>
> Sent: Thursday, August 19, 2021 10:02 AM
> To: Michael Kubacki <michael.kubacki at outlook.com>; devel at edk2.groups.io;
> Ni, Ray <ray.ni at intel.com>; mikuback at linux.microsoft.com; Chaganty,
> Rangasai V <rangasai.v.chaganty at 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.kubacki at outlook.com>
> > Sent: Thursday, August 19, 2021 2:47 AM
> > To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>;
> > mikuback at linux.microsoft.com; Chaganty, Rangasai V
> > <rangasai.v.chaganty at intel.com>; Yao, Jiewen <jiewen.yao at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > Michael
> > Kubacki
> > > Sent: Tuesday, August 10, 2021 11:36 PM
> > > To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Chaganty,
> > > Rangasai V
> > <rangasai.v.chaganty at intel.com>
> > > Cc: Yao, Jiewen <jiewen.yao at 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.chaganty at intel.com>
> > >>> Sent: Tuesday, August 10, 2021 6:46 AM
> > >>> To: mikuback at linux.microsoft.com; devel at edk2.groups.io
> > >>> Cc: Ni, Ray <ray.ni at intel.com>
> > >>> Subject: RE: [edk2-platforms][PATCH v1 1/1]
> > >>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
> > >>>
> > >>> Reviewed-by: Sai Chaganty <rangasai.v.chaganty at intel.com>
> > >>>
> > >>> -----Original Message-----
> > >>> From: mikuback at linux.microsoft.com <mikuback at linux.microsoft.com>
> > >>> Sent: Monday, August 09, 2021 6:40 AM
> > >>> To: devel at edk2.groups.io
> > >>> Cc: Ni, Ray <ray.ni at intel.com>; Chaganty, Rangasai V
> > >>> <rangasai.v.chaganty at intel.com>
> > >>> Subject: [edk2-platforms][PATCH v1 1/1]
> > >>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
> > >>>
> > >>> From: Michael Kubacki <michael.kubacki at 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.ni at intel.com>
> > >>> Cc: Rangasai V Chaganty <rangasai.v.chaganty at intel.com>
> > >>> Signed-off-by: Michael Kubacki <michael.kubacki at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list