Re: [edk2-devel] [PATCH v2 0/9] Add ImagePropertiesRecordLib and Fix MAT Bugs

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

Taylor,
Thank you for your effort for removing the duplicated logic in Dxe/Smm Core and fixing the bugs.

Two general comments:
#1. Can you refactor your patch series in a way that the new lib code is like a "git move" instead of "git add"? For example, you could add an empty lib first and update all DSC to depend on the new lib. Then move the lib code from DxeCore to the lib folder. So that when reviewing the code changes, they are relative smaller.

#2. I see that you directly move the code to lib and update consumer to call the lib APIs. Do you think it's feasible to refine the code further such that the responsibilities of DxeCore and the lib can be clearer and with that the lib APIs can be more meaningful?

I provided thoughts for #1 but haven't thought about #2 specifically.

Thanks,
Ray

> -----Original Message-----
> From: Taylor Beebe <t@taylorbeebe.com>
> Sent: Thursday, July 20, 2023 8:06 AM
> To: devel@edk2.groups.io
> Cc: 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>
> Subject: [PATCH v2 0/9] Add ImagePropertiesRecordLib and Fix MAT Bugs
> 
> 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>
> Taylor Beebe (9):
>   MdeModulePkg: Add ImagePropertiesRecordLib
>   MdeModulePkg: Add ImagePropertiesRecordLib Host-Based Unit Test
>   MdeModulePkg: Fix Bugs in MAT Logic
>   ArmVirtPkg: Add ImagePropertiesRecordLib Instance
>   EmulatorPkg: Add ImagePropertiesRecordLib Instance
>   OvmfPkg: Add ImagePropertiesRecordLib Instance
>   UefiPayloadPkg: Add ImagePropertiesRecordLib Instance
>   UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero
>   MdeModulePkg: Update UEFI and SMM MAT Logic To Use
>     ImagePropertiesRecordLib
> 
>  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> | 786 +---------------
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> |  24 +-
>  MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> | 785 +---------------
> 
> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLi
> b.c                        | 805 +++++++++++++++++
> 
> 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
> | 151 ++++
> 
> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLi
> b.inf                      |  28 +
> 
> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImageProperties
> RecordLibUnitTestHost.inf |  35 +
>  MdeModulePkg/MdeModulePkg.dec                                                                   |   5 +
>  MdeModulePkg/MdeModulePkg.dsc                                                                   |   2 +
>  MdeModulePkg/Test/MdeModulePkgHostTest.dsc
> |   5 +
>  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, 2039 insertions(+), 1579 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.41.0.windows.2



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