Taylor:
Thanks for your detail information. I understand more in the detail. The
changes is good to me. Reviewed-by: Liming Gao <gaolim...@byosoft.com.cn>
Thanks
Liming
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Taylor Beebe
发送时间: 2023年10月9日 3:21
收件人: devel@edk2.groups.io; gaolim...@byosoft.com.cn; 'Ard Biesheuvel'
<a...@kernel.org>
抄送: 'Andrew Fish' <af...@apple.com>; 'Ard Biesheuvel'
<ardb+tianoc...@kernel.org>; 'Dandan Bi' <dandan...@intel.com>; 'Eric Dong'
<eric.d...@intel.com>; 'Gerd Hoffmann' <kra...@redhat.com>; 'Guo Dong'
<guo.d...@intel.com>; 'Gua Guo' <gua....@intel.com>; 'James Lu'
<james...@intel.com>; 'Jian J Wang' <jian.j.w...@intel.com>; 'Jiewen Yao'
<jiewen....@intel.com>; 'Jordan Justen' <jordan.l.jus...@intel.com>; 'Leif
Lindholm' <quic_llind...@quicinc.com>; 'Rahul Kumar' <rahul1.ku...@intel.com>;
'Ray Ni' <ray...@intel.com>; 'Sami Mujawar' <sami.muja...@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 (#109511): https://edk2.groups.io/g/devel/message/109511
Mute This Topic: https://groups.io/mt/101889424/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
