Re: 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs

["Taylor Beebe" <taylor.d.beebe@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4659 bytes --]

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 (#109402): https://edk2.groups.io/g/devel/message/109402
Mute This Topic: https://groups.io/mt/101812772/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6017 bytes --]