public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Taylor Beebe" <t@taylorbeebe.com>
To: devel@edk2.groups.io, ray.ni@intel.com
Cc: Andrew Fish <afish@apple.com>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>,
	"Bi, Dandan" <dandan.bi@intel.com>,
	"Dong, Eric" <eric.dong@intel.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	"Dong, Guo" <guo.dong@intel.com>, "Guo, Gua" <gua.guo@intel.com>,
	"Lu, James" <james.lu@intel.com>,
	"Wang, Jian J" <jian.j.wang@intel.com>,
	"Yao, Jiewen" <jiewen.yao@intel.com>,
	"Justen, Jordan L" <jordan.l.justen@intel.com>,
	Leif Lindholm <quic_llindhol@quicinc.com>,
	"Gao, Liming" <gaoliming@byosoft.com.cn>,
	"Kumar, Rahul R" <rahul.r.kumar@intel.com>,
	Sami Mujawar <sami.mujawar@arm.com>,
	"Rhodes, Sean" <sean@starlabs.systems>
Subject: Re: [edk2-devel] [PATCH v2 0/9] Add ImagePropertiesRecordLib and Fix MAT Bugs
Date: Thu, 20 Jul 2023 11:40:06 -0700	[thread overview]
Message-ID: <a3e35c6a-f266-29d2-50f5-579a075e3872@taylorbeebe.com> (raw)
In-Reply-To: <MN6PR11MB824410A6544A9B10D29D74838C3EA@MN6PR11MB8244.namprd11.prod.outlook.com>



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?

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()


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



  reply	other threads:[~2023-07-20 18:40 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-20  0:05 [edk2-devel] [PATCH v2 0/9] Add ImagePropertiesRecordLib and Fix MAT Bugs Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 1/9] MdeModulePkg: Add ImagePropertiesRecordLib Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 2/9] MdeModulePkg: Add ImagePropertiesRecordLib Host-Based Unit Test Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 3/9] MdeModulePkg: Fix Bugs in MAT Logic Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 4/9] ArmVirtPkg: Add ImagePropertiesRecordLib Instance Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 5/9] EmulatorPkg: " Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 6/9] OvmfPkg: " Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 7/9] UefiPayloadPkg: " Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 8/9] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero Taylor Beebe
2023-07-20  0:05 ` [edk2-devel] [PATCH v2 9/9] MdeModulePkg: Update UEFI and SMM MAT Logic To Use ImagePropertiesRecordLib Taylor Beebe
2023-07-20  5:19 ` [edk2-devel] [PATCH v2 0/9] Add ImagePropertiesRecordLib and Fix MAT Bugs Ni, Ray
2023-07-20 18:40   ` Taylor Beebe [this message]
2023-07-21  3:34     ` Ni, Ray

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a3e35c6a-f266-29d2-50f5-579a075e3872@taylorbeebe.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox