From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from rn-mailsvcp-ppex-lapp35.apple.com (rn-mailsvcp-ppex-lapp35.apple.com [17.179.253.44]) by mx.groups.io with SMTP id smtpd.web10.96.1616515976326258263 for ; Tue, 23 Mar 2021 09:12:56 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=H3yQWcYX; spf=pass (domain: apple.com, ip: 17.179.253.44, mailfrom: afish@apple.com) Received: from pps.filterd (rn-mailsvcp-ppex-lapp35.rno.apple.com [127.0.0.1]) by rn-mailsvcp-ppex-lapp35.rno.apple.com (8.16.0.43/8.16.0.43) with SMTP id 12NG3X2q029822; Tue, 23 Mar 2021 09:12:37 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=wItI9HvFYyLPMDqHdcdLGISdL4HVNm2+1WBoHuzVEUY=; b=H3yQWcYXcYmCxBmvLbzlXrS10+17cjo0fyRd8pZ8aeYQB79FDXVplRudiEmjFpU0Eism FVcsKG3rZr5NEPoJ7srw3PfsV1m5wobZMCJGEUebx3kuqWZLX+/sa15SawD249l2kTxQ QYq7gJqBDwOnzE43xzoJBh7zJ//6LIODQYLKOtOe7EUSl5TjzQYJyRvUhR72zabug+bi 6pi5+AHhSstcAYN1yM+BF5mY9Q9NSbyKZ213SZK8wCq/27w0cKp3gsLDQwbjZUHfFsNf 96CfZ2pXzIcmHUfsvmEMsCXM2sYViy00Kr4OXf4MBCY/VOP1H10aSGofyt/jL1zPfqSh og== Received: from rn-mailsvcp-mta-lapp04.rno.apple.com (rn-mailsvcp-mta-lapp04.rno.apple.com [10.225.203.152]) by rn-mailsvcp-ppex-lapp35.rno.apple.com with ESMTP id 37dck5t8h0-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Tue, 23 Mar 2021 09:12:37 -0700 Received: from rn-mailsvcp-mmp-lapp04.rno.apple.com (rn-mailsvcp-mmp-lapp04.rno.apple.com [17.179.253.17]) by rn-mailsvcp-mta-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPS id <0QQF00E56JP0I9C0@rn-mailsvcp-mta-lapp04.rno.apple.com>; Tue, 23 Mar 2021 09:12:37 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp04.rno.apple.com by rn-mailsvcp-mmp-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) id <0QQF00N00JLYCO00@rn-mailsvcp-mmp-lapp04.rno.apple.com>; Tue, 23 Mar 2021 09:12:36 -0700 (PDT) X-Va-A: X-Va-T-CD: e46d62b7dc3f91464ba9fc842ece509f X-Va-E-CD: b5f30b9d65bd1c0f3fb8f4c9dc4cd7af X-Va-R-CD: 15345fbd519b77a3bf708ef7fea543cf X-Va-CD: 0 X-Va-ID: 1f848ec0-52f8-4601-8ab6-b7e27661d291 X-V-A: X-V-T-CD: e46d62b7dc3f91464ba9fc842ece509f X-V-E-CD: b5f30b9d65bd1c0f3fb8f4c9dc4cd7af X-V-R-CD: 15345fbd519b77a3bf708ef7fea543cf X-V-CD: 0 X-V-ID: c3ee3e9c-c994-45c6-86bd-ff562c1afb56 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761 definitions=2021-03-23_07:2021-03-22,2021-03-23 signatures=0 Received: from [17.235.4.130] (unknown [17.235.4.130]) by rn-mailsvcp-mmp-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPSA id <0QQF004FMJOZ0T00@rn-mailsvcp-mmp-lapp04.rno.apple.com>; Tue, 23 Mar 2021 09:12:36 -0700 (PDT) From: "Andrew Fish" Message-id: <41831DD2-0EBC-4B29-B6A0-BE56F322F478@apple.com> MIME-version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.1\)) Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob Date: Tue, 23 Mar 2021 09:12:34 -0700 In-reply-to: Cc: "lersek@redhat.com" , "Liu, Zhiguang" , "Ni, Ray" , "Wang, Jian J" , "Wu, Hao A" , "Bi, Dandan" , Liming Gao To: edk2-devel-groups-io , "Dong, Guo" References: <20210323032438.950-1-zhiguang.liu@intel.com> <20210323032438.950-3-zhiguang.liu@intel.com> X-Mailer: Apple Mail (2.3654.20.0.2.1) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761 definitions=2021-03-23_07:2021-03-22,2021-03-23 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_894B3980-FF6E-4B45-8676-275C0868762B" --Apple-Mail=_894B3980-FF6E-4B45-8676-275C0868762B Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label= usage should probably be defined by the PI Spec.=20 Is this a code 1st proposal or just an implementation?=20 Thanks, Andrew Fish > On Mar 23, 2021, at 8:45 AM, Guo Dong wrote: >=20 >=20 > Add my comments on the ideas behind. > UefiPayloadPkg is not a platform specific package, it tries to provide a= generic payload using platform independent Modules. This allows to reuse t= he payload for different boot firmware solutions (Slim Bootloader, Coreboot= , EDK2 bootloader) on different platforms. >=20 > Just like other DXE modules (e.g. variable DXE driver, firmware perform= ance DXE driver), standardizing and modularizing platform independent modul= es through a HOB as the AcpiTableDxe patch did to support potential data fr= om PEI/bootloader is a nature way for EDKII modules reuse. Understood the c= oncerns to keep AcpiTableDxe as simple as possible. I think it deserve for = code reuse by adding PEI/bootloader HOB support. >=20 > Thanks, > Guo >=20 >> -----Original Message----- >> From: devel@edk2.groups.io > On Behalf Of Laszlo >> Ersek >> Sent: Tuesday, March 23, 2021 5:40 AM >> To: Liu, Zhiguang >; Ni, Ray >; Dong, >> Guo > >> Cc: devel@edk2.groups.io ; Wang, Jian J >; Wu, Hao A >> >; Bi, Dandan >; Liming Gao >> > >> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver instal= l Acpi >> table from Hob >>=20 >> On 03/23/21 04:24, Zhiguang Liu wrote: >>> If HOB contains APCI table information, entry point of AcpiTableDxe.in= f >>> should parse the APCI table from HOB, and install these tables. >>> We assume the whole ACPI table (starting with >> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER) >>> is contained by a single gEfiAcpiTableGuid HOB. >>> This way, for UefiPayloadPkg, there is no need to specially hanle acpi= table. >>>=20 >>> Zhiguang Liu (2): >>> MdeModulePkg/ACPI: Install ACPI table from HOB. >>> UefiPayloadPkg: Remove code that installs APCI >>>=20 >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++- >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- >> ---- >>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-= ---------- >>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +--= -- >>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++-= -- >>> 5 files changed, 133 insertions(+), 27 deletions(-) >>>=20 >>=20 >> Where does this idea come from? >>=20 >> (1) There is no bugzilla for this, apparently (not linked in the commit >> messages anyway). >>=20 >> (2) Also, I'm not sure if reusing an existing (and standardized) GUID >> for this purpose is a good idea. I think an edk2-only >> (MdeModulePkg-defined), brand new GUID, for the HOB, would be better. >>=20 >> (3) I'm also not convinced at all that this *whole approach* is sound. >>=20 >> The fact that UefiPayloadPkg has the ACPI content available to it in a >> particular data representation (as a HOB, for example) is just a >> platform trait. Why should that platform trait leak into the central >> implementation of the ACPI table protocol? >>=20 >> UefiPayloadPkg can call the ACPI table protocol interfaces to install >> its tables. OVMF does the same -- OVMF also does not construct its own >> ACPI tables, but downloads them in a quirky representation from QEMU. W= e >> didn't hack the central AcpiTableDxe driver for that use case; instead, >> we dissected the blob (in OvmfPkg) into individual tables, and called >> the proper ACPI table protocol method (InstallAcpiTable()) with the >> individual tables. >>=20 >> I disagree with the code complexity / platform quirk in AcpiTableDxe. A= t >> the bare minimum, this feature should be possible to compile out >> altogether. I don't necessarily mean a FeaturePCD; there could be a new >> INF file too, that shares most of the functionality with the current >> core driver, but also includes (from a different source file) the new l= ogic. >>=20 >> But I'd really like if this mess were kept out of MdeModulePkg >> altogether. It's the job of the platform ACPI driver to use the ACPI >> table protocol. >>=20 >> Of course if you can show that this HOB usage is standard / publicly >> specified, that's different. >>=20 >> Please do not merge this series. >>=20 >> Laszlo >>=20 >>=20 >>=20 >>=20 >>=20 >=20 >=20 >=20 >=20 --Apple-Mail=_894B3980-FF6E-4B45-8676-275C0868762B Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii My concern is gEf= iAcpiTableGuid is owned by the UEFI Spec and any off label usage should pro= bably be defined by the PI Spec. 

=
Is this a code 1st proposal or just an implementation? = ;

Thanks= ,

Andrew Fish

On Mar 23, 2021, at 8:45 AM, Guo Dong <guo.dong@intel.com> wrote:


Add my comments on th= e ideas behind.
UefiPay= loadPkg is not a platform specific package, it tries to provide a generic p= ayload using platform independent Modules. This allows to reuse the payload= for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 boo= tloader) on different platforms.

Just like other DXE module= s (e.g. variable DXE driver,  firmware performance DXE driver), standa= rdizing and modularizing platform independent modules through a HOB as the = AcpiTableDxe patch did to support potential data from PEI/bootloader is a n= ature way for EDKII modules reuse. Understood the concerns to keep AcpiTabl= eDxe as simple as possible. I think it deserve for code reuse by adding PEI= /bootloader HOB support.

Thanks,
Guo
=
-----Original Me= ssage-----
From: <= /span>devel@edk2.groups.= io <devel@edk2.groups.io> On Behalf= Of Laszlo
Ersek
Sent: Tuesday, March 23, 2021 = 5:40 AM
To: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong,Guo <guo.do= ng@intel.com>
Cc: devel@edk2= .groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
<hao.a.wu@intel.com>; Bi, = Dandan <dandan.bi@inte= l.com>; Liming Gao
<gaoliming@byosoft.com.cn>
Subj= ect: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpitable from Hob

On 03/23/21 04:24,= Zhiguang Liu wrote:
If = HOB contains APCI table information, entry point of AcpiTableDxe.inf
should parse the APCI table from HOB, and install these tables.
We assume the whole ACPI table (starting with
EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
is contained by a single gEfiAcpiTableGuid H= OB.
This way, for UefiPayloadPkg, there is no need to special= ly hanle acpi table.

Zhiguang Liu (2):
 MdeModulePkg/ACPI: Install ACPI table from HOB.
=  UefiPayloadPkg: Remove code that installs APCI

MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf  &nbs= p; |   3 ++-
MdeModulePkg/Universal/Acpi/AcpiT= ableDxe/AcpiTableProtocol.c | 134
++++++++++++++= ++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++= ++++++++++++++++++++++++++++++++++++++++++++++++++++----
----=
UefiPayloadPkg/BlSuppor= tDxe/BlSupportDxe.c          &= nbsp;        |  13 ++---------= --
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h   &nb= sp;            =    |   5 +----
UefiPayloadPkg/BlSupp= ortDxe/BlSupportDxe.inf         &nb= sp;       |   5 ++---
5 files changed, 133 insertions(+), 27 deletions(-)

Where does this idea come from?
(1) There is no bugzilla for this, apparently (n= ot linked in the commit
messages anyway).

(2) Also, I'm not sure if reusing an existing (and standardized) = GUID
for this purpose is a good idea. I think an edk2-only(MdeModulePkg-defined), brand new GUID, for the HOB, would be b= etter.

(3) I'm also not convinced at all that = this *whole approach* is sound.

The fact that = UefiPayloadPkg has the ACPI content available to it in a
part= icular data representation (as a HOB, for example) is just a
= platform trait. Why should that platform trait leak into the central
implementation of the ACPI table protocol?

UefiPayloadPkg can call the ACPI table protocol interfaces to instal= l
its tables. OVMF does the same -- OVMF also does not constr= uct its own
ACPI tables, but downloads them in a quirky repre= sentation from QEMU. We
didn't hack the central AcpiTableDxe = driver for that use case; instead,
we dissected the blob (in = OvmfPkg) into individual tables, and called
the proper ACPI t= able protocol method (InstallAcpiTable()) with the
individual= tables.

I disagree with the code complexity /= platform quirk in AcpiTableDxe. At
the bare minimum, this fe= ature should be possible to compile out
altogether. I don't n= ecessarily mean a FeaturePCD; there could be a new
INF file t= oo, that shares most of the functionality with the current
co= re driver, but also includes (from a different source file) the new logic.<= br class=3D"">
But I'd really like if this mess were kept out= of MdeModulePkg
altogether. It's the job of the platform ACP= I driver to use the ACPI
table protocol.

Of course if you can show that this HOB usage is standard / public= ly
specified, that's different.

= Please do not merge this series.

Laszlo









--Apple-Mail=_894B3980-FF6E-4B45-8676-275C0868762B--