public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
@ 2023-08-04 19:46 Taylor Beebe
  0 siblings, 0 replies; 15+ messages in thread
From: Taylor Beebe @ 2023-08-04 19:46 UTC (permalink / raw)
  To: devel
  Cc: 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, Liming Gao, Rahul Kumar, Ray Ni,
	Sami Mujawar, Sean Rhodes

From: Taylor Beebe <taylor.d.beebe@gmail.com>

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>

Taylor Beebe (14):
  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 Bugs in MAT 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/ImagePropertiesRecordLib.c                        | 1144 ++++++++++++++++++++
 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf                      |   31 +
 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
 create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
 create mode 100644 MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
 create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
 create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf

-- 
2.41.0.windows.3



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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
       [not found] <177845D072580598.19347@groups.io>
@ 2023-08-16 18:14 ` Taylor Beebe
       [not found] ` <177BEFB7EF58B50B.8497@groups.io>
  1 sibling, 0 replies; 15+ messages in thread
From: Taylor Beebe @ 2023-08-16 18:14 UTC (permalink / raw)
  To: devel
  Cc: 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, Liming Gao, Rahul Kumar, Ray Ni,
	Sami Mujawar, Sean Rhodes

Can I please get reviews/feedback on this patch series?

On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
> From: Taylor Beebe <taylor.d.beebe@gmail.com>
> 
> 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>
> 
> Taylor Beebe (14):
>    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 Bugs in MAT 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/ImagePropertiesRecordLib.c                        | 1144 ++++++++++++++++++++
>   MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf                      |   31 +
>   MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
>   create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
>   create mode 100644 MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
>   create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
>   create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf
> 

-- 
Taylor Beebe
Software Engineer @ Microsoft


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
       [not found] ` <177BEFB7EF58B50B.8497@groups.io>
@ 2023-08-28 16:27   ` Taylor Beebe
  2023-08-28 16:38     ` Ard Biesheuvel
  0 siblings, 1 reply; 15+ messages in thread
From: Taylor Beebe @ 2023-08-28 16:27 UTC (permalink / raw)
  To: devel
  Cc: 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, Liming Gao, Rahul Kumar, Ray Ni,
	Sami Mujawar, Sean Rhodes

Can I please get reviews/feedback on this patch series?

On 8/16/23 11:14 AM, Taylor Beebe via groups.io wrote:
> Can I please get reviews/feedback on this patch series?
>
> On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
>> From: Taylor Beebe <taylor.d.beebe@gmail.com>
>>
>> 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>
>>
>> Taylor Beebe (14):
>>    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 Bugs in MAT 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/ImagePropertiesRecordLib.c 
>> | 1144 ++++++++++++++++++++
>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf 
>> |   31 +
>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
>>   create mode 100644 
>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
>>   create mode 100644 
>> MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
>>   create mode 100644 
>> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
>>   create mode 100644 
>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf
>>
>


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-08-28 16:27   ` Taylor Beebe
@ 2023-08-28 16:38     ` Ard Biesheuvel
  2023-09-26 16:02       ` Taylor Beebe
       [not found]       ` <17887E55BF3885BC.16687@groups.io>
  0 siblings, 2 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2023-08-28 16:38 UTC (permalink / raw)
  To: Taylor Beebe
  Cc: devel, 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, Liming Gao, Rahul Kumar,
	Ray Ni, Sami Mujawar, Sean Rhodes

I was hoping to get around to this before the end of the week (but I
am not a MdeModulePkg maintainer)


On Mon, 28 Aug 2023 at 18:27, Taylor Beebe <taylor.d.beebe@gmail.com> wrote:
>
> Can I please get reviews/feedback on this patch series?
>
> On 8/16/23 11:14 AM, Taylor Beebe via groups.io wrote:
> > Can I please get reviews/feedback on this patch series?
> >
> > On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
> >> From: Taylor Beebe <taylor.d.beebe@gmail.com>
> >>
> >> 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>
> >>
> >> Taylor Beebe (14):
> >>    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 Bugs in MAT 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/ImagePropertiesRecordLib.c
> >> | 1144 ++++++++++++++++++++
> >> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf
> >> |   31 +
> >> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
> >>   create mode 100644
> >> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
> >>   create mode 100644
> >> MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
> >>   create mode 100644
> >> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
> >>   create mode 100644
> >> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf
> >>
> >


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-08-28 16:38     ` Ard Biesheuvel
@ 2023-09-26 16:02       ` Taylor Beebe
       [not found]       ` <17887E55BF3885BC.16687@groups.io>
  1 sibling, 0 replies; 15+ messages in thread
From: Taylor Beebe @ 2023-09-26 16:02 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: devel, 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, Liming Gao, Rahul Kumar,
	Ray Ni, Sami Mujawar, Sean Rhodes

Reviews on this patch series would be much appreciated :)

On 8/28/23 9:38 AM, Ard Biesheuvel wrote:
> I was hoping to get around to this before the end of the week (but I
> am not a MdeModulePkg maintainer)
>
>
> On Mon, 28 Aug 2023 at 18:27, Taylor Beebe <taylor.d.beebe@gmail.com> wrote:
>> Can I please get reviews/feedback on this patch series?
>>
>> On 8/16/23 11:14 AM, Taylor Beebe via groups.io wrote:
>>> Can I please get reviews/feedback on this patch series?
>>>
>>> On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
>>>> From: Taylor Beebe <taylor.d.beebe@gmail.com>
>>>>
>>>> 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>
>>>>
>>>> Taylor Beebe (14):
>>>>     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 Bugs in MAT 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/ImagePropertiesRecordLib.c
>>>> | 1144 ++++++++++++++++++++
>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf
>>>> |   31 +
>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
>>>>    create mode 100644
>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
>>>>    create mode 100644
>>>> MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
>>>>    create mode 100644
>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
>>>>    create mode 100644
>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf
>>>>


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
       [not found]       ` <17887E55BF3885BC.16687@groups.io>
@ 2023-10-03 17:56         ` Taylor Beebe
  2023-10-03 18:57           ` Ard Biesheuvel
  0 siblings, 1 reply; 15+ messages in thread
From: Taylor Beebe @ 2023-10-03 17:56 UTC (permalink / raw)
  To: devel
  Cc: 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, Liming Gao, Rahul Kumar, Ray Ni,
	Sami Mujawar, Sean Rhodes

Have we given up on this patch series and Bug 4492?

On 9/26/2023 9:02 AM, Taylor Beebe via groups.io wrote:
> Reviews on this patch series would be much appreciated :)
>
> On 8/28/23 9:38 AM, Ard Biesheuvel wrote:
>> I was hoping to get around to this before the end of the week (but I
>> am not a MdeModulePkg maintainer)
>>
>>
>> On Mon, 28 Aug 2023 at 18:27, Taylor Beebe <taylor.d.beebe@gmail.com> 
>> wrote:
>>> Can I please get reviews/feedback on this patch series?
>>>
>>> On 8/16/23 11:14 AM, Taylor Beebe via groups.io wrote:
>>>> Can I please get reviews/feedback on this patch series?
>>>>
>>>> On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
>>>>> From: Taylor Beebe <taylor.d.beebe@gmail.com>
>>>>>
>>>>> 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>
>>>>>
>>>>> Taylor Beebe (14):
>>>>>     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 Bugs in MAT 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/ImagePropertiesRecordLib.c 
>>>>>
>>>>> | 1144 ++++++++++++++++++++
>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf 
>>>>>
>>>>> |   31 +
>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c 
>>>>>
>>>>>    create mode 100644
>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c 
>>>>>
>>>>>    create mode 100644
>>>>> MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
>>>>>    create mode 100644
>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf 
>>>>>
>>>>>    create mode 100644
>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf 
>>>>>
>>>>>
>
>
> 
>
>


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-03 17:56         ` Taylor Beebe
@ 2023-10-03 18:57           ` Ard Biesheuvel
  2023-10-03 19:04             ` Taylor Beebe
  0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2023-10-03 18:57 UTC (permalink / raw)
  To: Taylor Beebe
  Cc: devel, 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, Liming Gao, Rahul Kumar,
	Ray Ni, Sami Mujawar, Sean Rhodes

On Tue, 3 Oct 2023 at 17:56, Taylor Beebe <taylor.d.beebe@gmail.com> wrote:
>
> Have we given up on this patch series and Bug 4492?
>

I haven't, I promise. I will get to this on Thursday or Friday.


> On 9/26/2023 9:02 AM, Taylor Beebe via groups.io wrote:
> > Reviews on this patch series would be much appreciated :)
> >
> > On 8/28/23 9:38 AM, Ard Biesheuvel wrote:
> >> I was hoping to get around to this before the end of the week (but I
> >> am not a MdeModulePkg maintainer)
> >>
> >>
> >> On Mon, 28 Aug 2023 at 18:27, Taylor Beebe <taylor.d.beebe@gmail.com>
> >> wrote:
> >>> Can I please get reviews/feedback on this patch series?
> >>>
> >>> On 8/16/23 11:14 AM, Taylor Beebe via groups.io wrote:
> >>>> Can I please get reviews/feedback on this patch series?
> >>>>
> >>>> On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
> >>>>> From: Taylor Beebe <taylor.d.beebe@gmail.com>
> >>>>>
> >>>>> 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>
> >>>>>
> >>>>> Taylor Beebe (14):
> >>>>>     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 Bugs in MAT 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/ImagePropertiesRecordLib.c
> >>>>>
> >>>>> | 1144 ++++++++++++++++++++
> >>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf
> >>>>>
> >>>>> |   31 +
> >>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
> >>>>>
> >>>>>    create mode 100644
> >>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
> >>>>>
> >>>>>    create mode 100644
> >>>>> MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
> >>>>>    create mode 100644
> >>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
> >>>>>
> >>>>>    create mode 100644
> >>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf
> >>>>>
> >>>>>
> >
> >
> > 
> >
> >


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-03 18:57           ` Ard Biesheuvel
@ 2023-10-03 19:04             ` Taylor Beebe
  2023-10-07  5:57               ` 回复: " gaoliming via groups.io
  0 siblings, 1 reply; 15+ messages in thread
From: Taylor Beebe @ 2023-10-03 19:04 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: devel, 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, Liming Gao, Rahul Kumar,
	Ray Ni, Sami Mujawar, Sean Rhodes

Thank you , Ard. I very much appreciate your responsiveness.

The majority of these patches fall under MdeModulePkg maintainers so 
I'll also need help from them to drive this forward.

On 10/3/2023 11:57 AM, Ard Biesheuvel wrote:
> On Tue, 3 Oct 2023 at 17:56, Taylor Beebe <taylor.d.beebe@gmail.com> wrote:
>> Have we given up on this patch series and Bug 4492?
>>
> I haven't, I promise. I will get to this on Thursday or Friday.
>
>
>> On 9/26/2023 9:02 AM, Taylor Beebe via groups.io wrote:
>>> Reviews on this patch series would be much appreciated :)
>>>
>>> On 8/28/23 9:38 AM, Ard Biesheuvel wrote:
>>>> I was hoping to get around to this before the end of the week (but I
>>>> am not a MdeModulePkg maintainer)
>>>>
>>>>
>>>> On Mon, 28 Aug 2023 at 18:27, Taylor Beebe <taylor.d.beebe@gmail.com>
>>>> wrote:
>>>>> Can I please get reviews/feedback on this patch series?
>>>>>
>>>>> On 8/16/23 11:14 AM, Taylor Beebe via groups.io wrote:
>>>>>> Can I please get reviews/feedback on this patch series?
>>>>>>
>>>>>> On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
>>>>>>> From: Taylor Beebe <taylor.d.beebe@gmail.com>
>>>>>>>
>>>>>>> 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>
>>>>>>>
>>>>>>> Taylor Beebe (14):
>>>>>>>      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 Bugs in MAT 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/ImagePropertiesRecordLib.c
>>>>>>>
>>>>>>> | 1144 ++++++++++++++++++++
>>>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf
>>>>>>>
>>>>>>> |   31 +
>>>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
>>>>>>>
>>>>>>>     create mode 100644
>>>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
>>>>>>>
>>>>>>>     create mode 100644
>>>>>>> MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
>>>>>>>     create mode 100644
>>>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
>>>>>>>
>>>>>>>     create mode 100644
>>>>>>> MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf
>>>>>>>
>>>>>>>
>>>
>>> 
>>>
>>>


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-03 19:04             ` Taylor Beebe
@ 2023-10-07  5:57               ` gaoliming via groups.io
  2023-10-08 19:20                 ` Taylor Beebe
  0 siblings, 1 reply; 15+ messages in thread
From: gaoliming via groups.io @ 2023-10-07  5:57 UTC (permalink / raw)
  To: devel, taylor.d.beebe, 'Ard Biesheuvel'
  Cc: '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'

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?

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Taylor Beebe
> 发送时间: 2023年10月4日 3:05
> 收件人: Ard Biesheuvel <ardb@kernel.org>
> 抄送: devel@edk2.groups.io; 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>; Liming Gao
> <gaoliming@byosoft.com.cn>; 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
> 
> Thank you , Ard. I very much appreciate your responsiveness.
> 
> The majority of these patches fall under MdeModulePkg maintainers so
> I'll also need help from them to drive this forward.
> 
> On 10/3/2023 11:57 AM, Ard Biesheuvel wrote:
> > On Tue, 3 Oct 2023 at 17:56, Taylor Beebe <taylor.d.beebe@gmail.com>
> wrote:
> >> Have we given up on this patch series and Bug 4492?
> >>
> > I haven't, I promise. I will get to this on Thursday or Friday.
> >
> >
> >> On 9/26/2023 9:02 AM, Taylor Beebe via groups.io wrote:
> >>> Reviews on this patch series would be much appreciated :)
> >>>
> >>> On 8/28/23 9:38 AM, Ard Biesheuvel wrote:
> >>>> I was hoping to get around to this before the end of the week (but I
> >>>> am not a MdeModulePkg maintainer)
> >>>>
> >>>>
> >>>> On Mon, 28 Aug 2023 at 18:27, Taylor Beebe
> <taylor.d.beebe@gmail.com>
> >>>> wrote:
> >>>>> Can I please get reviews/feedback on this patch series?
> >>>>>
> >>>>> On 8/16/23 11:14 AM, Taylor Beebe via groups.io wrote:
> >>>>>> Can I please get reviews/feedback on this patch series?
> >>>>>>
> >>>>>> On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:
> >>>>>>> From: Taylor Beebe <taylor.d.beebe@gmail.com>
> >>>>>>>
> >>>>>>> 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>
> >>>>>>>
> >>>>>>> Taylor Beebe (14):
> >>>>>>>      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 Bugs in MAT 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
> >>>>>>>
> >>>>>>>
> >>>
> >>>
> >>>
> >>>
> 
> 
> 
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109381): https://edk2.groups.io/g/devel/message/109381
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-07  5:57               ` 回复: " gaoliming via groups.io
@ 2023-10-08 19:20                 ` Taylor Beebe
  2023-10-11  2:14                   ` 回复: " gaoliming via groups.io
  0 siblings, 1 reply; 15+ messages in thread
From: Taylor Beebe @ 2023-10-08 19:20 UTC (permalink / raw)
  To: devel, gaoliming, 'Ard Biesheuvel'
  Cc: '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'

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* 回复: 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-08 19:20                 ` Taylor Beebe
@ 2023-10-11  2:14                   ` gaoliming via groups.io
  2023-10-13 22:43                     ` Taylor Beebe
  0 siblings, 1 reply; 15+ messages in thread
From: gaoliming via groups.io @ 2023-10-11  2:14 UTC (permalink / raw)
  To: devel, taylor.d.beebe, 'Ard Biesheuvel'
  Cc: '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'

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

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 (#109511): https://edk2.groups.io/g/devel/message/109511
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]
-=-=-=-=-=-=-=-=-=-=-=-



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: 回复: 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-11  2:14                   ` 回复: " gaoliming via groups.io
@ 2023-10-13 22:43                     ` Taylor Beebe
  2023-10-24  0:14                       ` Michael D Kinney
  0 siblings, 1 reply; 15+ messages in thread
From: Taylor Beebe @ 2023-10-13 22:43 UTC (permalink / raw)
  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

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

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) 
<https://github.com/tianocore/edk2/pull/4900>

Bugzilla: 4492 – MAT Logic Incorrectly Reports Runtime Images 
(tianocore.org) <https://bugzilla.tianocore.org/show_bug.cgi?id=4492>

-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): 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]
-=-=-=-=-=-=-=-=-=-=-=-



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: 回复: 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-13 22:43                     ` Taylor Beebe
@ 2023-10-24  0:14                       ` Michael D Kinney
  2023-10-24  0:36                         ` Taylor Beebe
  0 siblings, 1 reply; 15+ messages in thread
From: Michael D Kinney @ 2023-10-24  0:14 UTC (permalink / raw)
  To: Taylor Beebe, 'Ard Biesheuvel', Ni, Ray,
	'Leif Lindholm', Yao, Jiewen, 'Sami Mujawar',
	'Gerd Hoffmann', 'Andrew Fish', Justen, Jordan L,
	Kumar, Rahul R, Dong, Eric, devel@edk2.groups.io
  Cc: Kinney, Michael D

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

Hi Taylor,

I did notice that the Signed-off-by tags in some of the commit messages do not look right.


Signed-off-by: Taylor Beebe <t@taylorbeebe.com<mailto:t@taylorbeebe.com>>

Please make sure those are updated.

Thanks,

Mike



From: Taylor Beebe <taylor.d.beebe@gmail.com>
Sent: Friday, October 13, 2023 3:43 PM
To: 'Ard Biesheuvel' <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; 'Leif Lindholm' <quic_llindhol@quicinc.com>; Yao, Jiewen <jiewen.yao@intel.com>; 'Sami Mujawar' <sami.mujawar@arm.com>; 'Gerd Hoffmann' <kraxel@redhat.com>; 'Andrew Fish' <afish@apple.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: 回复: 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs


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)<https://github.com/tianocore/edk2/pull/4900>

Bugzilla: 4492 – MAT Logic Incorrectly Reports Runtime Images (tianocore.org)<https://bugzilla.tianocore.org/show_bug.cgi?id=4492>

-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><mailto:gaoliming@byosoft.com.cn>

Thanks
Liming
发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io><mailto:devel@edk2.groups.io> 代表 Taylor Beebe
发送时间: 2023年10月9日 3:21
收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>; 'Ard Biesheuvel' <ardb@kernel.org><mailto:ardb@kernel.org>
抄送: 'Andrew Fish' <afish@apple.com><mailto:afish@apple.com>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org><mailto:ardb+tianocore@kernel.org>; 'Dandan Bi' <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; 'Eric Dong' <eric.dong@intel.com><mailto:eric.dong@intel.com>; 'Gerd Hoffmann' <kraxel@redhat.com><mailto:kraxel@redhat.com>; 'Guo Dong' <guo.dong@intel.com><mailto:guo.dong@intel.com>; 'Gua Guo' <gua.guo@intel.com><mailto:gua.guo@intel.com>; 'James Lu' <james.lu@intel.com><mailto:james.lu@intel.com>; 'Jian J Wang' <jian.j.wang@intel.com><mailto:jian.j.wang@intel.com>; 'Jiewen Yao' <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>; 'Jordan Justen' <jordan.l.justen@intel.com><mailto:jordan.l.justen@intel.com>; 'Leif Lindholm' <quic_llindhol@quicinc.com><mailto:quic_llindhol@quicinc.com>; 'Rahul Kumar' <rahul1.kumar@intel.com><mailto:rahul1.kumar@intel.com>; 'Ray Ni' <ray.ni@intel.com><mailto:ray.ni@intel.com>; 'Sami Mujawar' <sami.mujawar@arm.com><mailto:sami.mujawar@arm.com>; 'Sean Rhodes' <sean@starlabs.systems><mailto: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 (#109961): https://edk2.groups.io/g/devel/message/109961
Mute This Topic: https://groups.io/mt/101889424/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: 回复: 回复: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
  2023-10-24  0:14                       ` Michael D Kinney
@ 2023-10-24  0:36                         ` Taylor Beebe
  0 siblings, 0 replies; 15+ messages in thread
From: Taylor Beebe @ 2023-10-24  0:36 UTC (permalink / raw)
  To: Kinney, Michael D, 'Ard Biesheuvel', Ni, Ray,
	'Leif Lindholm', Yao, Jiewen, 'Sami Mujawar',
	'Gerd Hoffmann', 'Andrew Fish', Justen, Jordan L,
	Kumar, Rahul R, Dong, Eric, devel@edk2.groups.io

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

Ah, I switched my EDK2 mailing list email to this one a couple of months 
ago after

this series was sent. I'll update them in the PR.


Thanks for the heads-up :)

-Taylor

On 10/23/2023 5:14 PM, Kinney, Michael D wrote:
>
> Hi Taylor,
>
> I did notice that the Signed-off-by tags in some of the commit 
> messages do not look right.
>
> Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
>
> Please make sure those are updated.
>
> Thanks,
>
> Mike
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109965): https://edk2.groups.io/g/devel/message/109965
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]
-=-=-=-=-=-=-=-=-=-=-=-



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs
@ 2023-11-03 17:16 Taylor Beebe
  0 siblings, 0 replies; 15+ messages in thread
From: Taylor Beebe @ 2023-11-03 17:16 UTC (permalink / raw)
  To: devel
  Cc: 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, Liming Gao, Rahul Kumar, Ray Ni,
	Sami Mujawar, Sean Rhodes, Lazlo Ersek

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 (14):
  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 Bugs in MAT 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/ImagePropertiesRecordLib.c                        | 1144 ++++++++++++++++++++
 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.inf                      |   31 +
 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.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/ImagePropertiesRecordLib.c
 create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
 create mode 100644 MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h
 create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
 create mode 100644 MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf

-- 
2.42.0.windows.2



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



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-11-03 17:17 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <177845D072580598.19347@groups.io>
2023-08-16 18:14 ` [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs Taylor Beebe
     [not found] ` <177BEFB7EF58B50B.8497@groups.io>
2023-08-28 16:27   ` Taylor Beebe
2023-08-28 16:38     ` Ard Biesheuvel
2023-09-26 16:02       ` Taylor Beebe
     [not found]       ` <17887E55BF3885BC.16687@groups.io>
2023-10-03 17:56         ` Taylor Beebe
2023-10-03 18:57           ` Ard Biesheuvel
2023-10-03 19:04             ` Taylor Beebe
2023-10-07  5:57               ` 回复: " gaoliming via groups.io
2023-10-08 19:20                 ` Taylor Beebe
2023-10-11  2:14                   ` 回复: " gaoliming via groups.io
2023-10-13 22:43                     ` Taylor Beebe
2023-10-24  0:14                       ` Michael D Kinney
2023-10-24  0:36                         ` Taylor Beebe
2023-11-03 17:16 Taylor Beebe
  -- strict thread matches above, loose matches on Subject: below --
2023-08-04 19:46 Taylor Beebe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox