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

["Michael Kubacki" <michael.kubacki@outlook.com>]

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/PeiSmmAccessLib.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
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
> 
> 
> 
>