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.web10.8008.1631196572636308748 for ; Thu, 09 Sep 2021 07:09:32 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=gMfeIWTk; 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 A052920B6C51; Thu, 9 Sep 2021 07:09:31 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A052920B6C51 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1631196572; bh=Ll/HTSOYtPzPqPZrd2SwTg5Jszi0Sx6VtIgW9XU/Pnw=; h=Subject:To:References:From:Date:In-Reply-To:From; b=gMfeIWTkIN65fa48yBxsDy3V3gKJmhRq7B3jsnKt9vJ0sGeGZA2h2tTOfAuu/29si W95ehKUruRQ2Ch6njDNkA7cnQivjqEWUt4wdMZ27GtfnSx6WxCk2Qu2MXHCiZkWHtF FNxWykoVVrJHErewk8KcLAr4ah4AW0AnhHz+Mj+o= Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/PeiSmmAccessLib: Remove S3 requirement To: devel@edk2.groups.io, ray.ni@intel.com, "Yao, Jiewen" , "Chaganty, Rangasai V" References: <20210809133938.2430-1-mikuback@linux.microsoft.com> <9a5c7c6b-70c5-c1c0-6405-51149013c295@linux.microsoft.com> <153f6512-b336-b328-e711-7444ea0a9bb9@linux.microsoft.com> From: "Michael Kubacki" Message-ID: Date: Thu, 9 Sep 2021 10:09:30 -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, 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 Jiew= en's concern about inconsistent SMRAM state doesn't exist. >=20 > Reviewed-by: Ray Ni >=20 > I agree with you that converting this lib to a PEIM is better. >=20 > Thanks, > Ray >=20 >> -----Original Message----- >> From: Michael Kubacki >> Sent: Friday, August 20, 2021 3:45 AM >> To: devel@edk2.groups.io; Yao, Jiewen ; Ni, Ray ; Chaganty, Rangasai V >> >> 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=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 >> 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= such info. >>> >>> Thank you >>> Yao Jiewen >>> >>>> -----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 pur= pose >>>> 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.i= o; >>>> 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= 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. O= therwise, >>>> there is inconsistency issue in PEI/DXE, when DXE consumes the SmramHo= b. >>>> This is NOT required in S3, because we don=E2=80=99t run DXE in S3 pat= h. >>>> >>>> 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 chang= e? >>>>>> >>>>>> 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 = boot. >>>>>>> >>>>>>> + 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 th= an >>>>>>>> the boot mode being set to BOOT_ON_S3_RESUME. >>>>>>>> >>>>>>>> 2. It is poor API design to implicitly bury this requirement withi= n >>>>>>>> a function whose responsibility is to install the PPI. The = caller >>>>>>>> can easily place arbitrary constraints around whether to ca= ll >>>>>>>> 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/PeiSmmAccessL= i >>>>> 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