> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Taylor Beebe > Sent: Friday, July 21, 2023 2:40 AM > To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com> > Cc: Andrew Fish <af...@apple.com>; Ard Biesheuvel > <ardb+tianoc...@kernel.org>; Bi, Dandan <dandan...@intel.com>; Dong, Eric > <eric.d...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Dong, Guo > <guo.d...@intel.com>; Guo, Gua <gua....@intel.com>; Lu, James > <james...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Yao, Jiewen > <jiewen....@intel.com>; Justen, Jordan L <jordan.l.jus...@intel.com>; Leif > Lindholm <quic_llind...@quicinc.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; Kumar, Rahul R <rahul.r.ku...@intel.com>; Sami > Mujawar <sami.muja...@arm.com>; Rhodes, Sean <sean@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
