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

["Ni, Ray" <ray.ni@intel.com>]

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.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@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.ni@intel.com>
> >
> > I agree with you that converting this lib to a PEIM is better.
> >
> > Thanks,
> > Ray
> >
> >> -----Original Message-----
> >> From: Michael Kubacki <mikuback@linux.microsoft.com>
> >> Sent: Friday, August 20, 2021 3:45 AM
> >> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> >> <rangasai.v.chaganty@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.ni@intel.com>
> >>>> Sent: Thursday, August 19, 2021 6:02 PM
> >>>> To: Yao, Jiewen <jiewen.yao@intel.com>; Michael Kubacki
> >>>> <michael.kubacki@outlook.com>; devel@edk2.groups.io;
> >>>> mikuback@linux.microsoft.com; Chaganty, Rangasai V
> >>>> <rangasai.v.chaganty@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@intel.com>
> >>>> Sent: Thursday, August 19, 2021 10:02 AM
> >>>> To: Michael Kubacki <michael.kubacki@outlook.com>; devel@edk2.groups.io;
> >>>> Ni, Ray <ray.ni@intel.com>; mikuback@linux.microsoft.com; Chaganty,
> >>>> Rangasai V <rangasai.v.chaganty@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@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/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
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >
> >
> >
> >
> >
> 
> 
> 
>