From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web12.75141.1629402308241334438 for ; Thu, 19 Aug 2021 12:45:08 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=UzxKF32r; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.0.0.19] (c-73-27-179-174.hsd1.fl.comcast.net [73.27.179.174]) by linux.microsoft.com (Postfix) with ESMTPSA id 3B36A20C33AA; Thu, 19 Aug 2021 12:45:07 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3B36A20C33AA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1629402307; bh=noUaIJyGGBQjwmdLio9rPiHpgRQNtZuSltTZQdS2c8s=; h=Subject:To:References:From:Date:In-Reply-To:From; b=UzxKF32r8dmWOxEQFvnXAT9yTGxQzKWSMbQ5a7lflL3Gpg9R9UzwXfc1hW7QDSy7d VMZrurKAQCfJR/GVgOGl+GhWouWOKF4cLbdefBf5Izu9opqczbxUyLMDvtNLdYICI2 g2YUFkBiNPt1vJ5OGXoJh2E5akbtAIipVeDquqrw= Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement To: devel@edk2.groups.io, jiewen.yao@intel.com, "Ni, Ray" , "Chaganty, Rangasai V" References: <20210809133938.2430-1-mikuback@linux.microsoft.com> <9a5c7c6b-70c5-c1c0-6405-51149013c295@linux.microsoft.com> From: "Michael Kubacki" Message-ID: <153f6512-b336-b328-e711-7444ea0a9bb9@linux.microsoft.com> Date: Thu, 19 Aug 2021 15:45:06 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Ray, In the end, PI modules are allowed to depend on non-deprecated PI PPIs.=20 The scope of this change is about the API that installs the PPI. So I do=20 not want to diverge the conversation about changing other code to avoid=20 the PPI. I've already been waiting since April to get other IntelSiliconPkg=20 changes merged (https://edk2.groups.io/g/devel/message/78600) and I=20 would like to keep this conversation focused on the patch. This conversation has taken several tangents as I tried to clear up in=20 this message: https://edk2.groups.io/g/devel/message/79544. I did not=20 see a response, please read that if you have not. The maintainability issues of the code have become evident. The code is=20 coupling unnecessary assumptions and dependencies and that is increasing=20 the difficulty of understanding and modifying the code. The goal of this change was for the PeiInstallSmmAccessPpi() function to=20 simply install the PPI without the boot mode being BOOT_ON_S3_RESUME=20 which it already documents should be the case. Because the existing code attempted to couple platform design=20 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=20 the implementation - General dependencies for Standalone MM - Potential synchronization issues between the PI Spec defined=20 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=20 resolved. Note that this issue is not specific to non-S3 flows. PEIMs on=20 S3 flows could easily read the HOB. BZ for 1: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3575 BZ for 2: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3576 It also seems the library would be better as a PEIM but that's another=20 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=E2=80=99t need PPI. Hob should be enough to provide s= uch info. >=20 > Thank you > Yao Jiewen >=20 >> -----Original Message----- >> From: Ni, Ray >> Sent: Thursday, August 19, 2021 6:02 PM >> To: Yao, Jiewen ; Michael Kubacki >> ; devel@edk2.groups.io; >> mikuback@linux.microsoft.com; Chaganty, Rangasai V >> >> 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 purpo= se >> 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 >> Sent: Thursday, August 19, 2021 10:02 AM >> To: Michael Kubacki ; devel@edk2.groups.io; >> Ni, Ray ; mikuback@linux.microsoft.com; Chaganty, >> Rangasai V >> Subject: RE: [edk2-devel] [edk2-platforms][PATCH v1 1/1] >> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement >> >> The history was that we didn=E2=80=99t 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 p= ath. >> 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. Oth= erwise, >> there is inconsistency issue in PEI/DXE, when DXE consumes the SmramHob. >> This is NOT required in S3, because we don=E2=80=99t run DXE in S3 path. >> >> Thank you >> Yao Jiewen >> >>> -----Original Message----- >>> From: Michael Kubacki >>> Sent: Thursday, August 19, 2021 2:47 AM >>> To: devel@edk2.groups.io; Ni, Ray ; >>> mikuback@linux.microsoft.com; Chaganty, Rangasai V >>> ; Yao, Jiewen >>> 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 On Behalf Of >>>> Michael >>> Kubacki >>>> Sent: Tuesday, August 10, 2021 11:36 PM >>>> To: devel@edk2.groups.io; Ni, Ray ; Chaganty, >>>> Rangasai V >>> >>>> Cc: Yao, Jiewen >>>> 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 bo= ot. >>>>> >>>>> + Jiewen for potential security concern. >>>>> >>>>> Thanks, >>>>> Ray >>>>> >>>>>> -----Original Message----- >>>>>> From: Chaganty, Rangasai V >>>>>> Sent: Tuesday, August 10, 2021 6:46 AM >>>>>> To: mikuback@linux.microsoft.com; devel@edk2.groups.io >>>>>> Cc: Ni, Ray >>>>>> Subject: RE: [edk2-platforms][PATCH v1 1/1] >>>>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement >>>>>> >>>>>> Reviewed-by: Sai Chaganty >>>>>> >>>>>> -----Original Message----- >>>>>> From: mikuback@linux.microsoft.com >>>>>> Sent: Monday, August 09, 2021 6:40 AM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Ni, Ray ; Chaganty, Rangasai V >>>>>> >>>>>> Subject: [edk2-platforms][PATCH v1 1/1] >>>>>> IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement >>>>>> >>>>>> From: Michael Kubacki >>>>>> >>>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D3539 >>>>>> >>>>>> 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 cal= ler >>>>>> 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 >>>>>> Cc: Rangasai V Chaganty >>>>>> Signed-off-by: Michael Kubacki >>>>>> --- >>>>>> >>> 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 =3D PeiServicesGetBootMode (&BootMode); >>>>>> - if (EFI_ERROR (Status)) { >>>>>> - // >>>>>> - // If not in S3 boot path. do nothing >>>>>> - // >>>>>> - return EFI_SUCCESS; >>>>>> - } >>>>>> - >>>>>> - if (BootMode !=3D BOOT_ON_S3_RESUME) { >>>>>> - return EFI_SUCCESS; >>>>>> - } >>>>>> // >>>>>> // Initialize private data >>>>>> // >>>>>> -- >>>>>> 2.28.0.windows.1 >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >=20 >=20 >=20 >=20 >=20