> -----Original Message----- > From: [email protected] <[email protected]> On Behalf Of Taylor Beebe > Sent: Friday, July 21, 2023 2:40 AM > To: [email protected]; Ni, Ray <[email protected]> > Cc: Andrew Fish <[email protected]>; Ard Biesheuvel > <[email protected]>; Bi, Dandan <[email protected]>; Dong, Eric > <[email protected]>; Gerd Hoffmann <[email protected]>; Dong, Guo > <[email protected]>; Guo, Gua <[email protected]>; Lu, James > <[email protected]>; Wang, Jian J <[email protected]>; Yao, Jiewen > <[email protected]>; Justen, Jordan L <[email protected]>; Leif > Lindholm <[email protected]>; Gao, Liming > <[email protected]>; Kumar, Rahul R <[email protected]>; Sami > Mujawar <[email protected]>; Rhodes, Sean <[email protected]> > 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: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
