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

[Ni, Ray]

> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Taylor Beebe
> Sent: Friday, July 21, 2023 2:40 AM
> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>
> Cc: Andrew Fish <afish at apple.com>; Ard Biesheuvel
> <ardb+tianocore at kernel.org>; Bi, Dandan <dandan.bi at intel.com>; Dong, Eric
> <eric.dong at intel.com>; Gerd Hoffmann <kraxel at redhat.com>; Dong, Guo
> <guo.dong at intel.com>; Guo, Gua <gua.guo at intel.com>; Lu, James
> <james.lu at intel.com>; Wang, Jian J <jian.j.wang at intel.com>; Yao, Jiewen
> <jiewen.yao at intel.com>; Justen, Jordan L <jordan.l.justen at intel.com>; Leif
> Lindholm <quic_llindhol at quicinc.com>; Gao, Liming
> <gaoliming at byosoft.com.cn>; Kumar, Rahul R <rahul.r.kumar at intel.com>; Sami
> Mujawar <sami.mujawar at arm.com>; Rhodes, Sean <sean at starlabs.systems>
> Subject: Re: [edk2-devel] [PATCH v2 0/9] Add ImagePropertiesRecordLib and Fix
> MAT Bugs
> 
> 
> 
> On 7/19/23 10:19 PM, Ni, Ray wrote:
> > Taylor,
> > Thank you for your effort for removing the duplicated logic in Dxe/Smm Core
> and fixing the bugs.
> >
> > Two general comments:
> > #1. Can you refactor your patch series in a way that the new lib code is like a
> "git move" instead of "git add"? For example, you could add an empty lib first and
> update all DSC to depend on the new lib. Then move the lib code from DxeCore to
> the lib folder. So that when reviewing the code changes, they are relative smaller.
> >
> 
> There are enough differences between the DXE and SMM MAT logic that the
> "git move" you suggest won't resolve as cleanly as you hope without
> first creating separate library instances then walking them on to a
> common implementation. Is there another way I can make this more
> digestible?

Even there are lots of differences between DXE and SMM logic, can you start with DXE
implementation first by moving it to a separate lib?
Then you could gradually modify the lib to make it suitable for SMM.


> 
> The MAT logic is very dense, but the most complex part is the
> SplitTable() routine. The host-based unit test should prove that the
> expected input/output of the SplitTable() function is correct in this
> update.
> 
> > #2. I see that you directly move the code to lib and update consumer to call the
> lib APIs. Do you think it's feasible to refine the code further such that the
> responsibilities of DxeCore and the lib can be clearer and with that the lib APIs
> can be more meaningful?
> >
> 
> Yes, the currently exposed functions can be renamed and have more
> descriptive function headers. I can also round out missing functionality
> to attempt to further reduce the code footprint in the
> MemoryAttributesTable.c files.
> 
> For example:
> 
> RemoveImagePropertiesRecord()
> InsertImagePropertiesRecord()
> DeleteImagePropertiesRecord()
> CreateImagePropertiesRecord()


Thanks!


> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107116): https://edk2.groups.io/g/devel/message/107116
Mute This Topic: https://groups.io/mt/100246933/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-