[edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
Ni, Ray
ray.ni at intel.com
Fri Aug 20 08:11:02 UTC 2021
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 at intel.com>
I agree with you that converting this lib to a PEIM is better.
Thanks,
Ray
> -----Original Message-----
> From: Michael Kubacki <mikuback at linux.microsoft.com>
> Sent: Friday, August 20, 2021 3:45 AM
> To: devel at edk2.groups.io; Yao, Jiewen <jiewen.yao at intel.com>; Ni, Ray <ray.ni at intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty at 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 at intel.com>
> >> Sent: Thursday, August 19, 2021 6:02 PM
> >> To: Yao, Jiewen <jiewen.yao at intel.com>; Michael Kubacki
> >> <michael.kubacki at outlook.com>; devel at edk2.groups.io;
> >> mikuback at linux.microsoft.com; Chaganty, Rangasai V
> >> <rangasai.v.chaganty at 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 at intel.com>
> >> Sent: Thursday, August 19, 2021 10:02 AM
> >> To: Michael Kubacki <michael.kubacki at outlook.com>; devel at edk2.groups.io;
> >> Ni, Ray <ray.ni at intel.com>; mikuback at linux.microsoft.com; Chaganty,
> >> Rangasai V <rangasai.v.chaganty at 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 at outlook.com>
> >>> Sent: Thursday, August 19, 2021 2:47 AM
> >>> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>;
> >>> mikuback at linux.microsoft.com; Chaganty, Rangasai V
> >>> <rangasai.v.chaganty at intel.com>; Yao, Jiewen <jiewen.yao at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> >>>> Michael
> >>> Kubacki
> >>>> Sent: Tuesday, August 10, 2021 11:36 PM
> >>>> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Chaganty,
> >>>> Rangasai V
> >>> <rangasai.v.chaganty at intel.com>
> >>>> Cc: Yao, Jiewen <jiewen.yao at 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 at intel.com>
> >>>>>> Sent: Tuesday, August 10, 2021 6:46 AM
> >>>>>> To: mikuback at linux.microsoft.com; devel at edk2.groups.io
> >>>>>> Cc: Ni, Ray <ray.ni at intel.com>
> >>>>>> Subject: RE: [edk2-platforms][PATCH v1 1/1]
> >>>>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
> >>>>>>
> >>>>>> Reviewed-by: Sai Chaganty <rangasai.v.chaganty at intel.com>
> >>>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: mikuback at linux.microsoft.com <mikuback at linux.microsoft.com>
> >>>>>> Sent: Monday, August 09, 2021 6:40 AM
> >>>>>> To: devel at edk2.groups.io
> >>>>>> Cc: Ni, Ray <ray.ni at intel.com>; Chaganty, Rangasai V
> >>>>>> <rangasai.v.chaganty at intel.com>
> >>>>>> Subject: [edk2-platforms][PATCH v1 1/1]
> >>>>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement
> >>>>>>
> >>>>>> From: Michael Kubacki <michael.kubacki at 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 at intel.com>
> >>>>>> Cc: Rangasai V Chaganty <rangasai.v.chaganty at intel.com>
> >>>>>> Signed-off-by: Michael Kubacki <michael.kubacki at 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
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79636): https://edk2.groups.io/g/devel/message/79636
Mute This Topic: https://groups.io/mt/84768258/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list