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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Can you please elaborate on the synchronization issue between the HOB 
and the PPI a bit more? There's no further work needed there?

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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>>>
>>> 
>>>
>>>