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 ECD667803CD for ; Fri, 13 Oct 2023 22:43:07 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=KgHKeeDL3nT7DQYwwL2MYF6MXXYGXxcmzt9khra8nao=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To: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=1697236986; v=1; b=eXrh6gC8l8odbmGELjyhCtcRFG7Z9io77mG2HjTAyezESoe9pMgDMhhPR1MtLL+yeA5gaDRh Aac20tQvubTxt9Me3bDV9ESAs+sV7pHxzVuFb1Hw9ZEbGN9/LwMrs1Zdj6NZmBwfbqjWIhGVAhW R0+vKS6dwtBYiySnYlGh78ic= X-Received: by 127.0.0.2 with SMTP id KYupYY7687511xklRtpHH07L; Fri, 13 Oct 2023 15:43:06 -0700 X-Received: from mail-pf1-f173.google.com (mail-pf1-f173.google.com [209.85.210.173]) by mx.groups.io with SMTP id smtpd.web11.52110.1697236986043395749 for ; Fri, 13 Oct 2023 15:43:06 -0700 X-Received: by mail-pf1-f173.google.com with SMTP id d2e1a72fcca58-6b89ab5ddb7so290150b3a.0 for ; Fri, 13 Oct 2023 15:43:05 -0700 (PDT) X-Gm-Message-State: RCegtyanvAeIQk7Xmg0I5LFZx7686176AA= X-Google-Smtp-Source: AGHT+IHzaW3Cf2T/DHWpxklcUWvNmLCAj3Z+S2u4u32KX5klQRBkC661jzFNA7xN/KGAO8Kmg7HKLQ== X-Received: by 2002:a05:6a00:2190:b0:6ba:2ba7:b9cb with SMTP id h16-20020a056a00219000b006ba2ba7b9cbmr260695pfi.12.1697236985232; Fri, 13 Oct 2023 15:43:05 -0700 (PDT) X-Received: from [192.168.0.125] ([50.46.253.1]) by smtp.gmail.com with ESMTPSA id 15-20020aa7914f000000b00692c5b1a731sm13910902pfi.186.2023.10.13.15.43.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 13 Oct 2023 15:43:03 -0700 (PDT) Message-ID: <9a75aa43-4e02-4789-aca7-e0409283bdde@gmail.com> Date: Fri, 13 Oct 2023 15:43:02 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: =?UTF-8?B?UmU6IOWbnuWkjTog5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIHY0IDAwLzE0XSBBZGQgSW1hZ2VQcm9wZXJ0aWVzUmVjb3JkTGliIGFuZCBGaXggTUFUIEJ1Z3M=?= To: 'Ard Biesheuvel' , 'Ray Ni' , 'Leif Lindholm' , 'Jiewen Yao' , 'Sami Mujawar' , 'Gerd Hoffmann' , 'Andrew Fish' , 'Jordan Justen' , 'Rahul Kumar' , 'Eric Dong' , "devel@edk2.groups.io" Cc: Michael Kinney 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> <002c01d9fbe8$aca3f130$05ebd390$@byosoft.com.cn> From: "Taylor Beebe" In-Reply-To: <002c01d9fbe8$aca3f130$05ebd390$@byosoft.com.cn> 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,taylor.d.beebe@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="------------KhsrqmPUEOCY2pOGfec5BgEc" Content-Language: en-US X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=eXrh6gC8; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=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 --------------KhsrqmPUEOCY2pOGfec5BgEc Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Thank you Liming :) Patches 2, 3, 4, 5, and 11 still need reviews -- 4 of them are just adding the new library to the package DSC files. This bug has cost me a significant amount of hours to debug, fix, and unit-test. I have been specific in the cases which cause issues and demonstrated the problem via testing yet this patch has now been in flight for over 2 months. In fact, there have been no significant functional changes to the series since it was originally submitted almost 3 months ago. I complied with Ray's request to reshuffle the commit structure to ease the review burden but am still waiting for feedback or even acknowledgement that it's being looked at. Everyone in the "To" line of this email has the authority to push this toward the finish line. Can I please get some help? Reference: Add ImagePropertiesRecordLib and Fix MAT Bugs by TaylorBeebe · Pull Request #4900 · tianocore/edk2 (github.com) Bugzilla: 4492 – MAT Logic Incorrectly Reports Runtime Images (tianocore.org) -Taylor On 10/10/2023 7:14 PM, gaoliming via groups.io wrote: > > Taylor: > >  Thanks for your detail information. I understand more in the detail. > The changes is good to me. Reviewed-by: Liming Gao > > > Thanks > > Liming > > *发件人:*devel@edk2.groups.io *代表 *Taylor Beebe > *发送时间:*2023年10月9日3:21 > *收件人:*devel@edk2.groups.io; gaoliming@byosoft.com.cn; 'Ard Biesheuvel' > > *抄送:*'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' > *主题:*Re: 回复: [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: > >   I agree to add new ImagePropertiesRecordLib library for DxeCore > and SmmCore. The impact is that platform needs to update their DSC > with new library. > >   Frankly, I have not understood MAT code in detail. So, I have no > comments on this part. > >   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 library. The remaining functional 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 instead of referencing directly the global variables. > >                 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 manipulation logic to ImagePropertiesRecordLib -- still no > functional changes. > > ------------ FUNCTIONAL CHANGES START ------------ > > Patch 8: Add ImagePropertiesRecordLibHostBasedUnitTest which tests the > logic now in ImagePropertiesRecordLib and 3/4 of the tests fail to > show that the logic is incorrect. > >                The test calls into the SplitTable() routine which is > the most complex and invokes almost every other function in > ImagePropertiesRecordLib. > > Patch 9: Fixes the issues in the logic resulting in > ImagePropertiesRecordLibHostBasedUnitTest 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/Misc/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 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 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 change the memory type of loaded runtime image data sections to > EfiRuntimeServicesData so it could apply XP and RO based > >                   on each entry EFI type. This process is unecessary, > though, because the SplitTable() routine in > PiSmmCore/MemoryAttributesTable.c already sets the XP and RO attributes > >                   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 > >                   is still output-wise the same as it was before. > > Patch 12: Update the SMM MAT logic to use ImagePropertiesRecordLib. If > a direct comparison was done between the original DXE and SMM MAT > logic, you would see many differences. Some bugs > >                  on the 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 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 ProtectUefiImage() 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 and SMM debug output and will help us find > >                  MAT issues easier in the future. > > Hope this helps :) > > -Taylor > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109611): https://edk2.groups.io/g/devel/message/109611 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] -=-=-=-=-=-=-=-=-=-=-=- --------------KhsrqmPUEOCY2pOGfec5BgEc Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Thank you Liming :)

Patches 2, 3, 4, 5, and 11 still need reviews -- 4 of them are just adding the new library to the package DSC files.


This bug has cost me a significant amount of hours to debug, fix, and unit-test. I have been

specific in the cases which cause issues and demonstrated the problem via testing yet this

patch has now been in flight for over 2 months. In fact, there have been no significant functional

changes to the series since it was originally submitted almost 3 months ago. I complied with

Ray's request to reshuffle the commit structure to ease the review burden but am still

waiting for feedback or even acknowledgement that it's being looked at.


Everyone in the "To" line of this email has the authority to push this toward the finish line.

Can I please get some help?

Reference: Add ImagePropertiesRecordLib and Fix MAT Bugs by TaylorBeebe · Pull Request #4900 · tianocore/edk2 (github.com)

Bugzilla: 4492 – MAT Logic Incorrectly Reports Runtime Images (tianocore.org)

-Taylor

On 10/10/2023 7:14 PM, gaoliming via groups.io wrote:

Taylor:

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

 

Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Taylor Beebe
发送时间: 2023年10月9日 3:21
收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn; 'Ard Biesheuvel' <ardb@kernel.org>
抄送: 'Andrew Fish' <afish@apple.com>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; 'Dandan Bi' <dandan.bi@intel.com>; 'Eric Dong' <eric.dong@intel.com>; 'Gerd Hoffmann' <kraxel@redhat.com>; 'Guo Dong' <guo.dong@intel.com>; 'Gua Guo' <gua.guo@intel.com>; '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@quicinc.com>; 'Rahul Kumar' <rahul1.kumar@intel.com>; 'Ray Ni' <ray.ni@intel.com>; 'Sami Mujawar' <sami.mujawar@arm.com>; 'Sean Rhodes' <sean@starlabs.systems>
主题: Re: 回复: [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:
  I agree to add new ImagePropertiesRecordLib library for DxeCore and SmmCore. The impact is that platform needs to update their DSC with new library. 
 
  Frankly, I have not understood MAT code in detail. So, I have no comments on this part. 
 
  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 library. The remaining functional 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 instead of referencing directly the global variables.

                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 manipulation logic to ImagePropertiesRecordLib -- still no functional changes.

------------ FUNCTIONAL CHANGES START ------------

Patch 8: Add ImagePropertiesRecordLibHostBasedUnitTest which tests the logic now in ImagePropertiesRecordLib and 3/4 of the tests fail to show that the logic is incorrect.

               The test calls into the SplitTable() routine which is the most complex and invokes almost every other function in ImagePropertiesRecordLib.

Patch 9: Fixes the issues in the logic resulting in ImagePropertiesRecordLibHostBasedUnitTest 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/Misc/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 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 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 change the memory type of loaded runtime image data sections to EfiRuntimeServicesData so it could apply XP and RO based

                  on each entry EFI type. This process is unecessary, though, because the SplitTable() routine in PiSmmCore/MemoryAttributesTable.c already sets the XP and RO attributes

                  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

                  is still output-wise the same as it was before.

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

                 on the 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 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 ProtectUefiImage() 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 and SMM debug output and will help us find

                 MAT issues easier in the future.

 

Hope this helps :)

-Taylor

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

You receive all messages sent to this group.

View/Reply Online (#109611) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------KhsrqmPUEOCY2pOGfec5BgEc--