From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id E9D3C7803CF for ; Wed, 11 Oct 2023 02:15:38 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=MhdPAFvSeb5Z9jHsQ42XnhsDO4AuGgF7l88Ig5LwOEg=; c=relaxed/simple; d=groups.io; h=From:To:Cc:References:In-Reply-To:Subject:Date:Message-ID:MIME-Version:Thread-Index:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Language; s=20140610; t=1696990537; v=1; b=Uz7EViAnA4W5rkbO7qhs5WAfdLw7xiXdnB7CPzSIIbFHvKE9mqhNkBk76FeZ+Om/4p8+bdFk 3CEw+ywmpKnhTGGmacH/GsoxtcStLcqLFfuP1bFwNO2I9Mp/7bsP7cvziQRGbaV2WRHe4xF5TL7 3DBEM6ArjIPBRJX5Q8XVeDKw= X-Received: by 127.0.0.2 with SMTP id qZrHYY7687511xzWd3zPcox3; Tue, 10 Oct 2023 19:15:37 -0700 X-Received: from cxsh.intel-email.com (cxsh.intel-email.com [121.46.250.151]) by mx.groups.io with SMTP id smtpd.web11.8211.1696990533715269317 for ; Tue, 10 Oct 2023 19:15:36 -0700 X-Received: from cxsh.intel-email.com (localhost [127.0.0.1]) by cxsh.intel-email.com (Postfix) with ESMTP id C3065DDA859 for ; Wed, 11 Oct 2023 10:15:26 +0800 (CST) X-Received: from localhost (localhost [127.0.0.1]) by cxsh.intel-email.com (Postfix) with ESMTP id B6566DDA840 for ; Wed, 11 Oct 2023 10:15:26 +0800 (CST) X-Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by cxsh.intel-email.com (Postfix) with SMTP id B5759DDA849 for ; Wed, 11 Oct 2023 10:15:23 +0800 (CST) X-Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP(SSL) for ; Wed, 11 Oct 2023 10:14:31 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming via groups.io" To: , , "'Ard Biesheuvel'" Cc: "'Andrew Fish'" , "'Ard Biesheuvel'" , "'Dandan Bi'" , "'Eric Dong'" , "'Gerd Hoffmann'" , "'Guo Dong'" , "'Gua Guo'" , "'James Lu'" , "'Jian J Wang'" , "'Jiewen Yao'" , "'Jordan Justen'" , "'Leif Lindholm'" , "'Rahul Kumar'" , "'Ray Ni'" , "'Sami Mujawar'" , "'Sean Rhodes'" References: <177845D072580598.19347@groups.io> <177BEFB7EF58B50B.8497@groups.io> <17887E55BF3885BC.16687@groups.io> <83c03b0c-c8e8-44cf-8b45-761f31577153@gmail.com> <081601d9f8e3$2ac79d90$8056d8b0$@byosoft.com.cn> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IFtlZGsyLWRldmVsXSBbUEFUQ0ggdjQgMDAvMTRdIEFkZCBJbWFnZVByb3BlcnRpZXNSZWNvcmRMaWIgYW5kIEZpeCBNQVQgQnVncw==?= Date: Wed, 11 Oct 2023 10:14:31 +0800 Message-ID: <002c01d9fbe8$aca3f130$05ebd390$@byosoft.com.cn> MIME-Version: 1.0 Thread-Index: AQFdgWnAcGHDUewmrqk8JTotDZU7EQLmu+JAArORhA4CK9SolwGS8ZGdAg2BCgUCsuxnPAFDO9B6AlFKck8BnRZZnrCidRow Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,gaoliming@byosoft.com.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: vaMKlvaA5X0ubtiQez3YAEmNx7686176AA= Content-Type: multipart/alternative; boundary="----=_NextPart_000_002D_01D9FC2B.BAC73130" Content-Language: zh-cn X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Uz7EViAn; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io ------=_NextPart_000_002D_01D9FC2B.BAC73130 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Taylor: Thanks for your detail information. I understand more in the detail. The c= hanges is good to me. Reviewed-by: Liming Gao =20 Thanks Liming =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io = =E4=BB=A3=E8=A1=A8 Taylor Beebe =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B410=E6=9C=889=E6=97=A5 3:= 21 =E6=94=B6=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io; gaoliming@byosoft.com.cn= ; 'Ard Biesheuvel' =E6=8A=84=E9=80=81: 'Andrew Fish' ; 'Ard Biesheuvel' ; 'Dandan Bi' ; 'Eric Dong' ; 'Gerd Hoffmann' ; 'Guo Dong' ; 'Gua Guo' ; 'James Lu' ; 'Jian J Wang' ; 'Jiewen Yao' ; 'Jordan Justen' ; 'Leif Lindholm' ; 'Rahul Kumar' ; 'Ray Ni' ; 'Sami Mujawar' ; 'Sean Rhodes' =E4=B8=BB=E9=A2=98: Re: =E5=9B=9E=E5=A4=8D: [edk2-devel] [PATCH v4 00/14] A= dd ImagePropertiesRecordLib and Fix MAT Bugs =20 On 10/6/2023 10:57 PM, gaoliming via groups.io wrote: Taylor: I agree to add new ImagePropertiesRecordLib library for DxeCore and SmmCo= re. The impact is that platform needs to update their DSC with new library.= =20 =20 Frankly, I have not understood MAT code in detail. So, I have no comments= on this part.=20 =20 Last, what test have been done to verify the current functionality? TLDR: Patch 8 adds the unit test which invokes the lions share of the new l= ibrary. The remaining functional changes were tested by output comparison. =20 To provide context on how best to review this series, where the functional = changes are, and how they were validated, here's a breakdown of each patch: Patch 1: Add the ImagePropertiesRecordLib definition and "blank" implementa= tion. Patches 2-5: Add instances of the library to the platform DSC files. Patch 6: Updates the logic in Dxe/Misc/MemoryAttributesTable.c to use param= eters passed in instead of referencing directly the global variables. This functionally changes nothing but allows the logic to b= e moved to a library. Patch 7: Move the Dxe/Misc/MemoryAttributesTable.c Image Properties Record = manipulation logic to ImagePropertiesRecordLib -- still no functional chang= es. ------------ FUNCTIONAL CHANGES START ------------ Patch 8: Add ImagePropertiesRecordLibHostBasedUnitTest which tests the logi= c now in ImagePropertiesRecordLib and 3/4 of the tests fail to show that th= e logic is incorrect. The test calls into the SplitTable() routine which is the mo= st complex and invokes almost every other function in ImagePropertiesRecord= Lib. Patch 9: Fixes the issues in the logic resulting in ImagePropertiesRecordLi= bHostBasedUnitTest passing fully. The fixes change some logic in SpitTable(= ) and SplitRecord() (which are tested by the unit test) And increases the buffer size for the split table in Dxe/Mis= c/MemoryAttributesTable.c to fix another edge case. The rest of the exposed= functions in ImagePropertiesRecordLib.h are unchanged through this patch. Patch 10: Simply updates function headers, adds return status values to som= e functions, and adds NULL checks to sanity check the caller input. Patch 11: Makes a minor change to the SMM memory attribute logic to use the= attributes present in the memory map created by the SplitTable() routine. = This needs to be done because the original SMM MAT logic would manipulate the split memory map to ch= ange the memory type of loaded runtime image data sections to EfiRuntimeSer= vicesData so it could apply XP and RO based on each entry EFI type. This process is unecessary, thoug= h, because the SplitTable() routine in PiSmmCore/MemoryAttributesTable.c al= ready sets the XP and RO attributes appropriately on PE images, and EnforceMemoryMapAttribute= () in PiSmmCore/MemoryAttributesTable.c sets the XP and RO attributes on th= e non PE runtime regions. All that to say, this is still output-wise the same as it was before. Patch 12: Update the SMM MAT logic to use ImagePropertiesRecordLib. If a di= rect comparison was done between the original DXE and SMM MAT logic, you wo= uld see many differences. Some bugs on the DXE side were actually fixed on the SMM side. For t= he rest, as best I can tell, there was no reason for the remaining differen= ces. I also checked the SMM MAT table pre and post this patch series on OvmfPkg and found output the same. Patch 13: This patch consolidates the DXE and SMM logic for creating Image = Properties Records into the library which is extremely close to the Protect= UefiImage() logic in MemoryProtection.c. If you're familiar to that logic, this should be easy to review. Patch 14: Just updates the DumpImageRecord() routine to be more helpful and= print the loaded image .efi name. This info will be dumped for both DXE an= d SMM debug output and will help us find MAT issues easier in the future. =20 Hope this helps :) -Taylor=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109511): https://edk2.groups.io/g/devel/message/109511 Mute This Topic: https://groups.io/mt/101889424/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- ------=_NextPart_000_002D_01D9FC2B.BAC73130 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Taylor:

=C2=A0Thanks for your detail information. I understand more in = the detail. The changes is good to me. Reviewed-by: Liming Gao <gaolimin= g@byosoft.com.cn>

&nb= sp;

Thanks

Liming

= =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@e= dk2.groups.io <devel@edk2.groups.io> =E4=BB=A3=E8=A1=A8 T= aylor Beebe
=E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B410=E6=9C=889=E6=97=A5 3:21
=E6=94=B6= =E4=BB=B6=E4=BA=BA: devel@= edk2.groups.io; gaoliming@byosoft.com.cn; 'Ard Biesheuvel' <ardb@kernel.= org>
=E6=8A=84=E9=80=81: 'Andrew Fish' <afish@apple.com>; 'Ard Biesheuvel' <= ardb+tianocore@kernel.org>; 'Dandan Bi' <dandan.bi@intel.com>; 'Er= ic Dong' <eric.dong@intel.com>; 'Gerd Hoffmann' <kraxel@redhat.com= >; 'Guo Dong' <guo.dong@intel.com>; 'Gua Guo' <gua.guo@intel.co= m>; 'James Lu' <james.lu@intel.com>; 'Jian J Wang' <jian.j.wang= @intel.com>; 'Jiewen Yao' <jiewen.yao@intel.com>; 'Jordan Justen' = <jordan.l.justen@intel.com>; 'Leif Lindholm' <quic_llindhol@quicin= c.com>; 'Rahul Kumar' <rahul1.kumar@intel.com>; 'Ray Ni' <ray.n= i@intel.com>; 'Sami Mujawar' <sami.mujawar@arm.com>; 'Sean Rhodes'= <sean@starlabs.systems>
=E4=B8=BB=E9=A2=98: Re: =E5=9B=9E=E5=A4=8D: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and= Fix MAT Bugs

=  

On 10/= 6/2023 10:57 PM, gaoliming via groups.io wrote:

Taylor:
=C2=A0 I agree to =
add new ImagePropertiesRecordLib library for DxeCore and SmmCore. The impac=
t is that platform needs to update their DSC with new library. <=
/span>
 
=C2=A0 Frankly, I have not understood MAT code in detail. So=
, I have no comments on this part. 
 
=C2=A0 Last,=
 what test have been done to verify the current functionality?

TLDR: Patch 8 adds the unit t= est which invokes the lions share of the new library. The remaining functio= nal changes were tested by output comparison.

 

To provide = context on how best to review this series, where the functional changes are= , and how they were validated, here's a breakdown of each patch:=

Patch 1: Add the ImagePropertiesRecordLib = definition and "blank" implementation.

Patches 2-5: Add instances of the library to the platform = DSC files.

Patch 6: Updates the = logic in Dxe/Misc/MemoryAttributesTable.c to use parameters passed in inste= ad of referencing directly the global variables.

            &= nbsp;   This functionally changes nothing but allows the logic to= be moved to a library.

Patch 7:= Move the Dxe/Misc/MemoryAttributesTable.c Image Properties Record manipula= tion logic to ImagePropertiesRecordLib -- still no functional changes.=

------------ FUNCTIONAL CHANGES STAR= T ------------

Patch 8: Add Imag= ePropertiesRecordLibHostBasedUnitTest which tests the logic now in ImagePro= pertiesRecordLib and 3/4 of the tests fail to show that the logic is incorr= ect.

     &n= bsp;         The test calls into the SplitTab= le() routine which is the most complex and invokes almost every other funct= ion in ImagePropertiesRecordLib.

Patch 9: Fixes the issues in the logic resulting in ImagePropertiesRecordL= ibHostBasedUnitTest passing fully. The fixes change some logic in SpitTable= () and SplitRecord() (which are tested by the unit test)<= /p>

        &n= bsp;      And increases the buffer size for the sp= lit table in Dxe/Misc/MemoryAttributesTable.c to fix another edge case. The= rest of the exposed functions in ImagePropertiesRecordLib.h

       &nbs= p;       are unchanged through this patch.

Patch 10: Simply updates function = headers, adds return status values to some functions, and adds NULL checks = to sanity check the caller input.

Patch 11: Makes a minor change to the SMM memory attribute logic to use t= he attributes present in the memory map created by the SplitTable() routine= . This needs to be done because the original

          &n= bsp;       SMM MAT logic would manipulate the= split memory map to change the memory type of loaded runtime image data se= ctions to EfiRuntimeServicesData so it could apply XP and RO based

      &nbs= p;           on each entr= y EFI type. This process is unecessary, though, because the SplitTable() ro= utine in PiSmmCore/MemoryAttributesTable.c already sets the XP and RO attri= butes

    &n= bsp;            = ; appropriately on PE images, and EnforceMemoryMapAttribute() in PiSmmCore/= MemoryAttributesTable.c sets the XP and RO attributes on the non PE runtime= regions. All that to say, this

=             &nb= sp;     is still output-wise the same as it was before.=

Patch 12: Update the SMM MAT lo= gic to use ImagePropertiesRecordLib. If a direct comparison was done betwee= n the original DXE and SMM MAT logic, you would see many differences. Some = bugs

    &nb= sp;            on th= e DXE side were actually fixed on the SMM side. For the rest, as best I can= tell, there was no reason for the remaining differences. I also checked th= e SMM MAT table pre and post this

            &= nbsp;    patch series on OvmfPkg and found output the same.<= o:p>

Patch 13: This patch consolidate= s the DXE and SMM logic for creating Image Properties Records into the libr= ary which is extremely close to the ProtectUefiImage() logic in MemoryProte= ction.c. If you're

  &= nbsp;           &nbs= p;   familiar to that logic, this should be easy to review.<= /o:p>

Patch 14: Just updates the DumpImageR= ecord() routine to be more helpful and print the loaded image .efi name. Th= is info will be dumped for both DXE and SMM debug output and will help us f= ind

    &nbs= p;            MAT is= sues easier in the future.

=  

Hope this helps :)<= /span>

-Taylor

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#109511) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
------=_NextPart_000_002D_01D9FC2B.BAC73130--