Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement

["Yao, Jiewen" <jiewen.yao@intel.com>]

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@outlook.com>
> Sent: Thursday, August 19, 2021 2:47 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>;
> mikuback@linux.microsoft.com; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Yao, Jiewen <jiewen.yao@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.ni@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>
> > Cc: Yao, Jiewen <jiewen.yao@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@intel.com>
> >>> Sent: Tuesday, August 10, 2021 6:46 AM
> >>> To: mikuback@linux.microsoft.com; devel@edk2.groups.io
> >>> Cc: Ni, Ray <ray.ni@intel.com>
> >>> Subject: RE: [edk2-platforms][PATCH v1 1/1]
> >>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
> >>>
> >>> Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>
> >>>
> >>> -----Original Message-----
> >>> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> >>> Sent: Monday, August 09, 2021 6:40 AM
> >>> To: devel@edk2.groups.io
> >>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> >>> <rangasai.v.chaganty@intel.com>
> >>> Subject: [edk2-platforms][PATCH v1 1/1]
> >>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
> >>>
> >>> From: Michael Kubacki <michael.kubacki@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@intel.com>
> >>> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> >>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >>> ---
> >>>
> Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiS
> mmAccessLib.c | 12 ------------
> >>>    1 file changed, 12 deletions(-)
> >>>
> >>> diff --git
> >>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcces
> >>> sLib/PeiSmmAccessLib.c
> >>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcces
> >>> sLib/PeiSmmAccessLib.c index d9bf4fba983e..4df0d695fdaf 100644
> >>> ---
> >>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcces
> >>> sLib/PeiSmmAccessLib.c
> >>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmA
> >>> +++ 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
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
> >
> > 
> >
> >