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

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

[Michael Kubacki]

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/PeiSmmAccessLib/PeiSmmAccessLib.c b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
index d9bf4fba983e..4df0d695fdaf 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/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

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

[Chaganty, Rangasai V]

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/PeiSmmAccessLib/PeiSmmAccessLib.c b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
index d9bf4fba983e..4df0d695fdaf 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
+++ 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

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

[Ni, Ray]

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/PeiSmmAccessLib/PeiSmmAccessLib.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
> index d9bf4fba983e..4df0d695fdaf 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
> +++ 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

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

[Michael Kubacki]

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/PeiSmmAccessLib/PeiSmmAccessLib.c
>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
>> index d9bf4fba983e..4df0d695fdaf 100644
>> --- a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.c
>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
>> +++ 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
> 
> 
> 
> 
> 

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

[Ni, Ray]

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

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

[Michael Kubacki]

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

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

[Chaganty, Rangasai V]

I've looked into Intel Platforms and we have atleast one platform that could potentially get impacted. However, it can be addressed by adding BootMode checks by the caller.
The more important question, as Ray pointed out is, are there security implications in installing these PPIs in normal boot, that justifies PeiSmmAccessLib to absorb the bootmode checks.
If there are then it would be interesting to see how to support rationale #1 below -  "Practical use cases exist to require this PPI in cases other than   the boot mode being set to BOOT_ON_S3_RESUME".

Regards,
Sai

-----Original Message-----
From: Michael Kubacki <michael.kubacki@outlook.com> 
Sent: Wednesday, August 18, 2021 11: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/PeiSmmAccessLib.c | 12 ------------
>>>    1 file changed, 12 deletions(-)
>>>
>>> diff --git
>>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
>>> s
>>> sLib/PeiSmmAccessLib.c
>>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
>>> s sLib/PeiSmmAccessLib.c index d9bf4fba983e..4df0d695fdaf 100644
>>> ---
>>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
>>> s
>>> sLib/PeiSmmAccessLib.c
>>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmm
>>> +++ A
>>> +++ 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
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

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

[Michael Kubacki]

 From a design perspective, I disagree this function is the proper place 
to try to enforce this.

The single responsibility of this function is to install the MM Access 
PPI. That is it.

---

 From a security perspective, the boot mode is a weak way to enforce this.

Platform code often overrides/updates the boot mode based on arbitrary 
conditions several times in the boot. A bug in that messy process should 
not compromise the system.

---

It is not clear what the problem is.

1. What security guarantees is this function trying to make? Why?

2. Is there a security problem or not?
   2.a. If so, why is security dependent on a PI Specification PPI not 
being installed?

---

As-is the function interface is broken and the boot mode dependency 
makes it worse:

1. It does not say boot mode must be BOOT_ON_S3_RESUME to install the 
PPI though it must.
2. It claims that a return value of EFI_SUCCESS indicates the PPI was 
installed. That is incorrect conditional on boot mode.
3. The EFI_NOT_FOUND return value is documented incorrectly.
4. The function returns EFI_SUCCESS if PeiServicesInstallPpi () fails.

My point is that a simple and accurate function interface will help 
platforms achieve their integration and security goals better than one 
that implicitly attempts to implement ambiguous requirements.

Thanks,
Michael

On 8/18/2021 5:15 PM, Chaganty, Rangasai V wrote:
> I've looked into Intel Platforms and we have atleast one platform that could potentially get impacted. However, it can be addressed by adding BootMode checks by the caller.
> The more important question, as Ray pointed out is, are there security implications in installing these PPIs in normal boot, that justifies PeiSmmAccessLib to absorb the bootmode checks.
> If there are then it would be interesting to see how to support rationale #1 below -  "Practical use cases exist to require this PPI in cases other than   the boot mode being set to BOOT_ON_S3_RESUME".
> 
> Regards,
> Sai
> 
> -----Original Message-----
> From: Michael Kubacki <michael.kubacki@outlook.com>
> Sent: Wednesday, August 18, 2021 11: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/PeiSmmAccessLib.c | 12 ------------
>>>>     1 file changed, 12 deletions(-)
>>>>
>>>> diff --git
>>>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
>>>> s
>>>> sLib/PeiSmmAccessLib.c
>>>> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
>>>> s sLib/PeiSmmAccessLib.c index d9bf4fba983e..4df0d695fdaf 100644
>>>> ---
>>>> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
>>>> s
>>>> sLib/PeiSmmAccessLib.c
>>>> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmm
>>>> +++ A
>>>> +++ 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
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
>>
>> 
>>
>>

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

[Yao, Jiewen]

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

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

[Ni, Ray]

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

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

[Yao, Jiewen]

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

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

[Michael Kubacki]

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

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

[Ni, Ray]

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

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

[Michael Kubacki]

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

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

[Michael Kubacki]

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

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

[Ni, Ray]

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