Re: [edk2-devel] [PATCH v5 00/16] Add ImagePropertiesRecordLib and Fix MAT Bugs​

["Ni, Ray" <ray.ni@intel.com>]

Taylor,
Thanks for spending time on removing the duplicated code in DxeCore and SmmCore.

I am ok with your current patch series. But I do have some suggestions that you or someone else could possibly enhance the lib API in future:

In MdeModulePkg\Core\Dxe\Misc\MemoryAttributesTable.c, InsertImageRecord() is implemented to call the new ImagePropertiesRecordLib as follows:
1. CreateImagePropertiesRecord() to create the IMAGE_PROPERTIES_RECORD instance.
  1.a. The lib parses the PE-COFF header to extract all the sections.
      Should we update PeCoffLib to add the missing capability that extracts the section info?
      I am ok to keep the section extraction code in ImagePropertiesRecordLib as the comments below are more cared by me.

2. if no code section, error
  2.a. Can we update CreateImagePropertiesRecord() to return failure status if no code section is found?
     We only need the lib API to return success for no-code-section case when caller does require to handle it differently.

3. IsImageRecordCodeSectionValid() to check if code sections are valid.
  Should we update the lib implementation to do the check before CreateImagePropertiesRecord() returns? This could reduce one API interface. Also this makes the caller easier to write.

4. Insert the IMAGE_PROPERTIES_RECORD instance to ImageRecordList.
  Should we rename CreateImagePropertiesRecord() to InsertImagePropertiesRecord() so that the insertion is done before InsertImagePropertiesRecord() returns?

5. Sort the ImageRecordList.
  Should we update the CreateImagePropertiesRecord() /InsertImagePropertiesRecord() to sort the list internally before it returns?


In summary, we could design the library API in a way that caller could call as follows:

LIST_ENTRY mImageList;
VOID *ImagePropertiesRecord;

InitializeListHead (&mImageList);
InsertImagePropertiesRecord (&mImageList, ImageBase, ImageSize, &ImagePropertiesRecord);
// caller doesn't know what's stored for one image properties record. It only knows a "handle" to the image properties record.
Saves ImagePropertiesRecord in EFI_RUNTIME_IMAGE_ENTRY for later unload.

If any error happens later on:
  DeleteImagePropertiesRecord (ImagePropertiesRecord);


So, following APIs can be eliminated:
* Structure IMAGE_PROPERTIES_RECORD_CODE_SECTION
* Structure IMAGE_PROPERTIES_RECORD
* SortImageRecordCodeSection()
* IsImageRecordCodeSectionValid()
* SortImageRecord()
* SwapImageRecord()
* SwapImageRecordCodeSection()
* FindImageRecord()

And only following APIs are needed:
* InsertImagePropertiesRecord()
* DeleteImagePropertiesRecord()
* SplitMemoryDescriptors() or SplitTable()
* DumpImageRecords()



Thanks,
Ray
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Tuesday, November 28, 2023 2:40 AM
> To: Taylor Beebe <taylor.d.beebe@gmail.com>
> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Bi, Dandan <dandan.bi@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Dong,
> Guo <guo.dong@intel.com>; Guo, Gua <gua.guo@intel.com>; Lu, James
> <james.lu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Leif
> Lindholm <quic_llindhol@quicinc.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; Rhodes,
> Sean <sean@starlabs.systems>; Lazlo Ersek <lersek@redhat.com>
> Subject: Re: [PATCH v5 00/16] Add ImagePropertiesRecordLib and Fix MAT
> Bugs​
> 
> On Mon, 27 Nov 2023 at 19:18, Taylor Beebe <taylor.d.beebe@gmail.com>
> wrote:
> >
> > v5:
> > This version is purely an update of commit messages and orientation.
> > No code changes were made.
> > - Split patch 9 into 3 separate patches so each fix is in its own
> >   commit.
> > - Updated the commit comment of patch 11 to add context.
> >
> 
> Thanks for sticking with this.
> 
> For the series,
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> 
> 
> 
> > v4:
> > - Expose additional functions in the Library API
> > - Add NULL checks to library functions and return a
> >   status where applicable.
> >
> > v3:
> > - Refactor patch series so the transition of logic from the DXE
> >   MAT logic to the new library is more clear.
> > - Update function headers to improve clarity and follow EDK2
> >   standards.
> > - Add Create and Delete functions for Image Properties Records
> >   and redirect some of the SMM and DXE MAT code to use these
> >   functions.
> > - Update/Add DumpImageRecords() to print the image name and code
> >   sections of each runtime image which will be put in the MAT.
> >   The DXE and SMM MAT logic will now invoke the DumpImageRecords()
> >   on DEBUG builds at the EndOfDxe event to install the MAT.
> >
> > v2:
> > - A one-line change in patch 3 was moved to patch 9 for correctness.
> >
> > Reference: https://github.com/tianocore/edk2/pull/4590
> > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4492
> >
> > The UEFI and SMM MAT logic contains duplicate logic for manipulating
> image
> > properties records which is used to track runtime images.
> > This patch series adds a new library, ImagePropertiesRecordLib,
> > which consolidates this logic and fixes the bugs which currently exist in
> > the MAT logic.
> >
> > The first patch adds the ImagePropertiesRecordLib implementation which
> > is a copy of the UEFI MAT logic with minor modifications to remove the
> > reliance on globabl variables and make the code unit testable.
> >
> > The second patch adds a unit test for the ImagePropertiesRecordLib. The
> > logic tests various potential layouts of the EFI memory map and runtime
> > images. 3/4 of these tests will fail which demonstrates the MAT logic
> > bugs.
> >
> > The third patch fixes the logic in the ImagePropertiesRecordLib so
> > that all of the unit tests pass and the MAT logic can be fixed by
> > using the library.
> >
> > The remaining patches add library instances to DSC files and remove
> > the image properties record logic from the SMM and UEFI MAT logic.
> >
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Guo Dong <guo.dong@intel.com>
> > Cc: Gua Guo <gua.guo@intel.com>
> > Cc: James Lu <james.lu@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Cc: Sean Rhodes <sean@starlabs.systems>
> > Cc: Lazlo Ersek <lersek@redhat.com>
> >
> > Taylor Beebe (16):
> >   MdeModulePkg: Add ImagePropertiesRecordLib
> >   ArmVirtPkg: Add ImagePropertiesRecordLib Instance
> >   EmulatorPkg: Add ImagePropertiesRecordLib Instance
> >   OvmfPkg: Add ImagePropertiesRecordLib Instance
> >   UefiPayloadPkg: Add ImagePropertiesRecordLib Instance
> >   MdeModulePkg: Update MemoryAttributesTable.c to Reduce Global
> Variable
> >     Use
> >   MdeModulePkg: Move Some DXE MAT Logic to
> ImagePropertiesRecordLib
> >   MdeModulePkg: Add ImagePropertiesRecordLib Host-Based Unit Test
> >   MdeModulePkg: Fix MAT Descriptor Count Calculation
> >   MdeModulePkg: Fix MAT SplitRecord() Logic
> >   MdeModulePkg: Fix MAT SplitTable() Logic
> >   MdeModulePkg: Add NULL checks and Return Status to
> >     ImagePropertiesRecordLib
> >   UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero
> >   MdeModulePkg: Transition SMM MAT Logic to Use
> ImagePropertiesRecordLib
> >   MdeModulePkg: Add Logic to Create/Delete Image Properties Records
> >   MdeModulePkg: Update DumpImageRecord() in
> ImagePropertiesRecordLib
> >
> >  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> |  967 +----------------
> >  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> |   24 +-
> >  MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> |  958 +---------------
> >
> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLi
> b.c                        | 1144 ++++++++++++++++++++
> >
> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImageProperties
> RecordLibUnitTestHost.c   |  938 ++++++++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> |   19 +-
> >  ArmVirtPkg/ArmVirt.dsc.inc
> |    1 +
> >  EmulatorPkg/EmulatorPkg.dsc
> |    1 +
> >  MdeModulePkg/Core/Dxe/DxeMain.h
> |   20 -
> >  MdeModulePkg/Core/Dxe/DxeMain.inf
> |    1 +
> >  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> |    1 +
> >  MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
> |  234 ++++
> >
> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLi
> b.inf                      |   31 +
> >
> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImageProperties
> RecordLibUnitTestHost.inf |   35 +
> >  MdeModulePkg/MdeModulePkg.dec
> |    5 +
> >  MdeModulePkg/MdeModulePkg.dsc
> |    2 +
> >  MdeModulePkg/Test/MdeModulePkgHostTest.dsc
> |    6 +
> >  OvmfPkg/AmdSev/AmdSevX64.dsc
> |    1 +
> >  OvmfPkg/Bhyve/BhyveX64.dsc
> |    1 +
> >  OvmfPkg/CloudHv/CloudHvX64.dsc
> |    1 +
> >  OvmfPkg/IntelTdx/IntelTdxX64.dsc
> |    1 +
> >  OvmfPkg/Microvm/MicrovmX64.dsc
> |    1 +
> >  OvmfPkg/OvmfPkgIa32.dsc
> |    1 +
> >  OvmfPkg/OvmfPkgIa32X64.dsc
> |    1 +
> >  OvmfPkg/OvmfPkgX64.dsc
> |    1 +
> >  OvmfPkg/OvmfXen.dsc
> |    1 +
> >  OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
> |    1 +
> >  UefiPayloadPkg/UefiPayloadPkg.dsc
> |    1 +
> >  28 files changed, 2524 insertions(+), 1874 deletions(-)
> >  create mode 100644
> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLi
> b.c
> >  create mode 100644
> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImageProperties
> RecordLibUnitTestHost.c
> >  create mode 100644
> MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
> >  create mode 100644
> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLi
> b.inf
> >  create mode 100644
> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImageProperties
> RecordLibUnitTestHost.inf
> >
> > --
> > 2.42.0.windows.2
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111804): https://edk2.groups.io/g/devel/message/111804
Mute This Topic: https://groups.io/mt/102834905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-