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(-)
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 HOB. This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-) -- 2.30.0.windows.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73174): https://edk2.groups.io/g/devel/message/73174 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 HOB. > This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-) > Where does this idea come from? (1) There is no bugzilla for this, apparently (not 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 better. (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 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? 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. 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 table protocol method (InstallAcpiTable()) with the individual tables. I disagree with the code complexity / platform quirk in AcpiTableDxe. At 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 logic. 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. Of course if you can show that this HOB usage is standard / publicly specified, that's different. Please do not merge this series. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73185): https://edk2.groups.io/g/devel/message/73185 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms. Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support. Thanks, Guo > -----Original Message----- > From: 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.dong@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@intel.com>; Liming Gao > <gaoliming@byosoft.com.cn> > Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi > table 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 HOB. > > This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-) > > > > Where does this idea come from? > > (1) There is no bugzilla for this, apparently (not 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 better. > > (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 > 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? > > 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. 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 table protocol method (InstallAcpiTable()) with the > individual tables. > > I disagree with the code complexity / platform quirk in AcpiTableDxe. At > 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 logic. > > 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. > > Of course if you can show that this HOB usage is standard / publicly > specified, that's different. > > Please do not merge this series. > > Laszlo > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73188): https://edk2.groups.io/g/devel/message/73188 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label usage should probably 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 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 the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms. > > Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support. > > Thanks, > Guo > >> -----Original Message----- >> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Laszlo >> Ersek >> Sent: Tuesday, March 23, 2021 5:40 AM >> To: Liu, Zhiguang <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Dong, >> Guo <guo.dong@intel.com <mailto:guo.dong@intel.com>> >> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A >> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com <mailto:dandan.bi@intel.com>>; Liming Gao >> <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>> >> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi >> table 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 HOB. >>> This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-) >>> >> >> Where does this idea come from? >> >> (1) There is no bugzilla for this, apparently (not 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 better. >> >> (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 >> 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? >> >> 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. 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 table protocol method (InstallAcpiTable()) with the >> individual tables. >> >> I disagree with the code complexity / platform quirk in AcpiTableDxe. At >> 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 logic. >> >> 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. >> >> Of course if you can show that this HOB usage is standard / publicly >> specified, that's different. >> >> Please do not merge this series. >> >> Laszlo >> >> >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73189): https://edk2.groups.io/g/devel/message/73189 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
The universal payload specification<https://universalpayload.github.io/documentation/spec/spec.html> proposes to re-use the existing GUID from UEFI for the HOB. I don't know if we have to update the UEFI spec firstly in order to reuse the GUID. Any idea? Thanks, Guo From: Andrew Fish <afish@apple.com> Sent: Tuesday, March 23, 2021 9:13 AM To: edk2-devel-groups-io <devel@edk2.groups.io>; Dong, Guo <guo.dong@intel.com> Cc: lersek@redhat.com; Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label usage should probably 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<mailto:guo.dong@intel.com>> wrote: 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 the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms. Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support. Thanks, Guo -----Original Message----- From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo Ersek Sent: Tuesday, March 23, 2021 5:40 AM To: Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table 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 HOB. This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-) Where does this idea come from? (1) There is no bugzilla for this, apparently (not 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 better. (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 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? 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. 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 table protocol method (InstallAcpiTable()) with the individual tables. I disagree with the code complexity / platform quirk in AcpiTableDxe. At 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 logic. 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. Of course if you can show that this HOB usage is standard / publicly specified, that's different. Please do not merge this series. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73192): https://edk2.groups.io/g/devel/message/73192 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Andrew, If the change is to use the gEfiAcpiTableGuid to identify another entry in the EFI configuration table, I agree it's a violation. We position this as a pure implementation that reuses a spec defined GUID. We didn't realize that it's a violation to the spec. We could define a new GUID for the HOB data. But using the same GUID avoids introducing new GUID for the similar purpose. Thanks, Ray From: Andrew Fish <afish@apple.com> Sent: Wednesday, March 24, 2021 12:13 AM To: edk2-devel-groups-io <devel@edk2.groups.io>; Dong, Guo <guo.dong@intel.com> Cc: lersek@redhat.com; Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label usage should probably 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<mailto:guo.dong@intel.com>> wrote: 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 the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms. Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support. Thanks, Guo -----Original Message----- From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo Ersek Sent: Tuesday, March 23, 2021 5:40 AM To: Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table 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 HOB. This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-) Where does this idea come from? (1) There is no bugzilla for this, apparently (not 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 better. (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 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? 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. 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 table protocol method (InstallAcpiTable()) with the individual tables. I disagree with the code complexity / platform quirk in AcpiTableDxe. At 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 logic. 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. Of course if you can show that this HOB usage is standard / publicly specified, that's different. Please do not merge this series. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73196): https://edk2.groups.io/g/devel/message/73196 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 03/23/21 16:45, Guo Dong wrote:
>
> 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 the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms.
>
> Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support.
I don't understand this argument.
(1) Currently, BlSupportDxe expects the ACPI content to come from
"SYSTEM_TABLE_INFO.AcpiTableBase" [Include/Guid/SystemTableInfoGuid.h].
That header file is at least *moderately* documented (it's better than
nothing). Additionally, BlSupportDxe is a DXE-phase component.
The patch set removes the handling of "SYSTEM_TABLE_INFO.AcpiTableBase"
from BlSupportDxe. That means that platforms currently relying on
BlSupportDxe to expose the ACPI content will break (until they start
producing the new HOB). I don't see the HOB (with this particular GUID)
being produced in UefiPayloadPkg anywhere.
(2) The UefiPayloadEntry module ("This is the first module for UEFI
payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
various pieces of information into the "AcpiBoardInfo" structure. So
even if the HOB producer phase exposes the ACPI payload via a dedicated
HOB, it will only create inconsistency between the information parsed by
UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
(which will the ACPI contents from the dedicated HOB).
(3) The new HOB's structure (regardless of GUID) is not declared in any
MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
we have is hidden in the source code:
Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
(UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
If a platform's PEI phase actually inteded to produce this new HOB, it
couldn't rely on a header file / DEC file.
This is actually a *step back* from the SystemTableInfoGuid declaration
-- header file and DEC file -- that we currently have in UefiPayloadPkg.
So how can this be called "standardizing and modularizing"?
You need a new GUID, a new GUID HOB structure (declared in MdeModulePkg
DEC and GUID header); you need to spell out the priority order between
the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
you need to update all driver in UefiPayloadPkg accordingly.
I will also not make a secret of my annoyance that, the first time Intel
needs such a core extension for some platform feature, it immediately
gets all approvals. Whereas, when we needed the exact same feature in
OVMF, we struggled for months, if not *years*, to reliably split the
ACPI content that OVMF downloaded from QEMU, into blobs that were
suitable for the standard ACPI table protocol interfaces. For years I've
been telling my colleagues that all this complexity in OVMF's ACPI
platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
implementation in edk2 cannot simply accept a "root pointer", to the
ACPI table "forest" that's already laid out in memory. Now I find it
just a little bit too convenient that the first time Intel needs the
same, we immediately call it "standardizing and modularizing" -- without
as much as a header file describing the actual contents of the new GUID HOB.
(Meanwhile we argue for months about actual, proven spec breakage in
edk2, such as signaling ready to boot around recovery options or
whatever. Standardization matters as long as *you* need it, huh?)
Laszlo
>
> Thanks,
> Guo
>
>> -----Original Message-----
>> From: 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.dong@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@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>> table 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 HOB.
>>> This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-)
>>>
>>
>> Where does this idea come from?
>>
>> (1) There is no bugzilla for this, apparently (not 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 better.
>>
>> (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
>> 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?
>>
>> 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. 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 table protocol method (InstallAcpiTable()) with the
>> individual tables.
>>
>> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
>> 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 logic.
>>
>> 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.
>>
>> Of course if you can show that this HOB usage is standard / publicly
>> specified, that's different.
>>
>> Please do not merge this series.
>>
>> Laszlo
>>
>>
>>
>>
>>
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73190): https://edk2.groups.io/g/devel/message/73190
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo,
I don't mean currently UEFI payload is already standardized and modularized.
There are still lots of things to do and I think the AcpiTableDxe patch is the one it needed.
More information on ideas behind could be found from https://universalpayload.github.io/documentation/spec/spec.html.
A universal payload interface is proposed between the bootloader and the payload to allow reuse for the same payload for different boot firmware solutions on different platforms. It describes the interface between the bootloader and the payload, including what parameters need pass to payload, how to pass parameters to payload, the parameters format, the payload image format, and the payload boot mode, etc.
I am not saying UefiPayloadPkg would fully follow this proposed interface for now. But we are trying to align with the proposed interface under EDKII framework.
Thanks,
Guo
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, March 23, 2021 9:48 AM
> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Andrew Fish <afish@apple.com>
> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
> table from Hob
>
> On 03/23/21 16:45, Guo Dong wrote:
> >
> > 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 the
> payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2
> bootloader) on different platforms.
> >
> > Just like other DXE modules (e.g. variable DXE driver, firmware performance
> DXE driver), standardizing and modularizing platform independent modules
> through a HOB as the AcpiTableDxe patch did to support potential data from
> PEI/bootloader is a nature way for EDKII modules reuse. Understood the
> concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code
> reuse by adding PEI/bootloader HOB support.
>
> I don't understand this argument.
>
> (1) Currently, BlSupportDxe expects the ACPI content to come from
> "SYSTEM_TABLE_INFO.AcpiTableBase" [Include/Guid/SystemTableInfoGuid.h].
> That header file is at least *moderately* documented (it's better than
> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>
> The patch set removes the handling of "SYSTEM_TABLE_INFO.AcpiTableBase"
> from BlSupportDxe. That means that platforms currently relying on
> BlSupportDxe to expose the ACPI content will break (until they start
> producing the new HOB). I don't see the HOB (with this particular GUID)
> being produced in UefiPayloadPkg anywhere.
>
> (2) The UefiPayloadEntry module ("This is the first module for UEFI
> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
> various pieces of information into the "AcpiBoardInfo" structure. So
> even if the HOB producer phase exposes the ACPI payload via a dedicated
> HOB, it will only create inconsistency between the information parsed by
> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
> (which will the ACPI contents from the dedicated HOB).
>
> (3) The new HOB's structure (regardless of GUID) is not declared in any
> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
> we have is hidden in the source code:
>
> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>
> If a platform's PEI phase actually inteded to produce this new HOB, it
> couldn't rely on a header file / DEC file.
>
> This is actually a *step back* from the SystemTableInfoGuid declaration
> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
>
>
> So how can this be called "standardizing and modularizing"?
>
> You need a new GUID, a new GUID HOB structure (declared in MdeModulePkg
> DEC and GUID header); you need to spell out the priority order between
> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
> you need to update all driver in UefiPayloadPkg accordingly.
>
>
> I will also not make a secret of my annoyance that, the first time Intel
> needs such a core extension for some platform feature, it immediately
> gets all approvals. Whereas, when we needed the exact same feature in
> OVMF, we struggled for months, if not *years*, to reliably split the
> ACPI content that OVMF downloaded from QEMU, into blobs that were
> suitable for the standard ACPI table protocol interfaces. For years I've
> been telling my colleagues that all this complexity in OVMF's ACPI
> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
> implementation in edk2 cannot simply accept a "root pointer", to the
> ACPI table "forest" that's already laid out in memory. Now I find it
> just a little bit too convenient that the first time Intel needs the
> same, we immediately call it "standardizing and modularizing" -- without
> as much as a header file describing the actual contents of the new GUID HOB.
>
> (Meanwhile we argue for months about actual, proven spec breakage in
> edk2, such as signaling ready to boot around recovery options or
> whatever. Standardization matters as long as *you* need it, huh?)
>
> Laszlo
>
> >
> > Thanks,
> > Guo
> >
> >> -----Original Message-----
> >> From: 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.dong@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@intel.com>; Liming Gao
> >> <gaoliming@byosoft.com.cn>
> >> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
> >> table 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 HOB.
> >>> This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-)
> >>>
> >>
> >> Where does this idea come from?
> >>
> >> (1) There is no bugzilla for this, apparently (not 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 better.
> >>
> >> (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
> >> 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?
> >>
> >> 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. 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 table protocol method (InstallAcpiTable()) with the
> >> individual tables.
> >>
> >> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
> >> 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 logic.
> >>
> >> 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.
> >>
> >> Of course if you can show that this HOB usage is standard / publicly
> >> specified, that's different.
> >>
> >> Please do not merge this series.
> >>
> >> Laszlo
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73191): https://edk2.groups.io/g/devel/message/73191
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 03/23/21 18:15, Dong, Guo wrote:
>
> Hi Laszlo,
>
> I don't mean currently UEFI payload is already standardized and modularized.
> There are still lots of things to do and I think the AcpiTableDxe patch is the one it needed.
>
> More information on ideas behind could be found from https://universalpayload.github.io/documentation/spec/spec.html.
> A universal payload interface is proposed between the bootloader and the payload to allow reuse for the same payload for different boot firmware solutions on different platforms. It describes the interface between the bootloader and the payload, including what parameters need pass to payload, how to pass parameters to payload, the parameters format, the payload image format, and the payload boot mode, etc.
Sounds good, but then I would say a new GUID is needed even more.
Laszlo
>
> I am not saying UefiPayloadPkg would fully follow this proposed interface for now. But we are trying to align with the proposed interface under EDKII framework.
>
> Thanks,
> Guo
>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, March 23, 2021 9:48 AM
>> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Liu, Zhiguang
>> <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
>> Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
>> Andrew Fish <afish@apple.com>
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>> table from Hob
>>
>> On 03/23/21 16:45, Guo Dong wrote:
>>>
>>> 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 the
>> payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2
>> bootloader) on different platforms.
>>>
>>> Just like other DXE modules (e.g. variable DXE driver, firmware performance
>> DXE driver), standardizing and modularizing platform independent modules
>> through a HOB as the AcpiTableDxe patch did to support potential data from
>> PEI/bootloader is a nature way for EDKII modules reuse. Understood the
>> concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code
>> reuse by adding PEI/bootloader HOB support.
>>
>> I don't understand this argument.
>>
>> (1) Currently, BlSupportDxe expects the ACPI content to come from
>> "SYSTEM_TABLE_INFO.AcpiTableBase" [Include/Guid/SystemTableInfoGuid.h].
>> That header file is at least *moderately* documented (it's better than
>> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>>
>> The patch set removes the handling of "SYSTEM_TABLE_INFO.AcpiTableBase"
>> from BlSupportDxe. That means that platforms currently relying on
>> BlSupportDxe to expose the ACPI content will break (until they start
>> producing the new HOB). I don't see the HOB (with this particular GUID)
>> being produced in UefiPayloadPkg anywhere.
>>
>> (2) The UefiPayloadEntry module ("This is the first module for UEFI
>> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
>> various pieces of information into the "AcpiBoardInfo" structure. So
>> even if the HOB producer phase exposes the ACPI payload via a dedicated
>> HOB, it will only create inconsistency between the information parsed by
>> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
>> (which will the ACPI contents from the dedicated HOB).
>>
>> (3) The new HOB's structure (regardless of GUID) is not declared in any
>> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
>> we have is hidden in the source code:
>>
>> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
>> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>>
>> If a platform's PEI phase actually inteded to produce this new HOB, it
>> couldn't rely on a header file / DEC file.
>>
>> This is actually a *step back* from the SystemTableInfoGuid declaration
>> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
>>
>>
>> So how can this be called "standardizing and modularizing"?
>>
>> You need a new GUID, a new GUID HOB structure (declared in MdeModulePkg
>> DEC and GUID header); you need to spell out the priority order between
>> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
>> you need to update all driver in UefiPayloadPkg accordingly.
>>
>>
>> I will also not make a secret of my annoyance that, the first time Intel
>> needs such a core extension for some platform feature, it immediately
>> gets all approvals. Whereas, when we needed the exact same feature in
>> OVMF, we struggled for months, if not *years*, to reliably split the
>> ACPI content that OVMF downloaded from QEMU, into blobs that were
>> suitable for the standard ACPI table protocol interfaces. For years I've
>> been telling my colleagues that all this complexity in OVMF's ACPI
>> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
>> implementation in edk2 cannot simply accept a "root pointer", to the
>> ACPI table "forest" that's already laid out in memory. Now I find it
>> just a little bit too convenient that the first time Intel needs the
>> same, we immediately call it "standardizing and modularizing" -- without
>> as much as a header file describing the actual contents of the new GUID HOB.
>>
>> (Meanwhile we argue for months about actual, proven spec breakage in
>> edk2, such as signaling ready to boot around recovery options or
>> whatever. Standardization matters as long as *you* need it, huh?)
>>
>> Laszlo
>>
>>>
>>> Thanks,
>>> Guo
>>>
>>>> -----Original Message-----
>>>> From: 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.dong@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@intel.com>; Liming Gao
>>>> <gaoliming@byosoft.com.cn>
>>>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>>>> table 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 HOB.
>>>>> This way, for UefiPayloadPkg, there is no need to specially 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 | 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(-)
>>>>>
>>>>
>>>> Where does this idea come from?
>>>>
>>>> (1) There is no bugzilla for this, apparently (not 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 better.
>>>>
>>>> (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
>>>> 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?
>>>>
>>>> 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. 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 table protocol method (InstallAcpiTable()) with the
>>>> individual tables.
>>>>
>>>> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
>>>> 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 logic.
>>>>
>>>> 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.
>>>>
>>>> Of course if you can show that this HOB usage is standard / publicly
>>>> specified, that's different.
>>>>
>>>> Please do not merge this series.
>>>>
>>>> Laszlo
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73202): https://edk2.groups.io/g/devel/message/73202
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
>
> (1) Currently, BlSupportDxe expects the ACPI content to come from
> "SYSTEM_TABLE_INFO.AcpiTableBase"
> [Include/Guid/SystemTableInfoGuid.h].
> That header file is at least *moderately* documented (it's better than
> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>
> The patch set removes the handling of
> "SYSTEM_TABLE_INFO.AcpiTableBase"
> from BlSupportDxe. That means that platforms currently relying on
> BlSupportDxe to expose the ACPI content will break (until they start
> producing the new HOB). I don't see the HOB (with this particular GUID)
> being produced in UefiPayloadPkg anywhere.
The concern is about PEI passing ACPI Table location through two kinds
of HOBs. It looks like a flaw in the design. I agree.
The HOB is produced by PEI phase, by some code that doesn't belong
to edk2 repo.
>
> (2) The UefiPayloadEntry module ("This is the first module for UEFI
> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
> various pieces of information into the "AcpiBoardInfo" structure. So
> even if the HOB producer phase exposes the ACPI payload via a dedicated
> HOB, it will only create inconsistency between the information parsed by
> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
> (which will the ACPI contents from the dedicated HOB).
I agree. At least the SYSTEM_TABLE_INFO.AcpiTableBase field should be removed
and the accordingly code that consumes ACPI Table in BlSupportDxe should
be updated to consume the new HOB.
>
> (3) The new HOB's structure (regardless of GUID) is not declared in any
> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
> we have is hidden in the source code:
>
> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>
> If a platform's PEI phase actually inteded to produce this new HOB, it
> couldn't rely on a header file / DEC file.
>
> This is actually a *step back* from the SystemTableInfoGuid declaration
> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
gEfiAcpiTableGuid is defined in MdePkg/Include/Guid/Acpi.h.
The file header says the GUID is used for entry in EFI system table.
Now we reuse this GUID for HOB data.
I think it's ok to use a spec defined GUID for another implementation purpose.
I can create a file MdeModulePkg/Include/Guid/Acpi.h to define the HOB structure.
Do you think it's ok?
>
>
> So how can this be called "standardizing and modularizing"?
>
> You need a new GUID, a new GUID HOB structure (declared in
> MdeModulePkg
> DEC and GUID header); you need to spell out the priority order between
> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
> you need to update all driver in UefiPayloadPkg accordingly.
Again. I agree it's a flaw in the design. We should remove AcpiTableBase field.
>
>
> I will also not make a secret of my annoyance that, the first time Intel
> needs such a core extension for some platform feature, it immediately
> gets all approvals. Whereas, when we needed the exact same feature in
> OVMF, we struggled for months, if not *years*, to reliably split the
> ACPI content that OVMF downloaded from QEMU, into blobs that were
> suitable for the standard ACPI table protocol interfaces. For years I've
> been telling my colleagues that all this complexity in OVMF's ACPI
> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
> implementation in edk2 cannot simply accept a "root pointer", to the
> ACPI table "forest" that's already laid out in memory. Now I find it
> just a little bit too convenient that the first time Intel needs the
> same, we immediately call it "standardizing and modularizing" -- without
> as much as a header file describing the actual contents of the new GUID HOB.
I am not aware of the similar OVMF requirement.
The requirement here is to support different bootloaders that may already
create the essential ACPI table and DXE phase (payload) may use AcpiTable
protocol to install/update tables.
>
> (Meanwhile we argue for months about actual, proven spec breakage in
> edk2, such as signaling ready to boot around recovery options or
> whatever. Standardization matters as long as *you* need it, huh?)
The definition of spec breakage to me is we cannot do anything that's
conflict with the spec. But we can do things that spec doesn't define.
Please correct if I am wrong.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73195): https://edk2.groups.io/g/devel/message/73195
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 03/24/21 05:09, Ni, Ray wrote:
>>
>> (1) Currently, BlSupportDxe expects the ACPI content to come from
>> "SYSTEM_TABLE_INFO.AcpiTableBase"
>> [Include/Guid/SystemTableInfoGuid.h].
>> That header file is at least *moderately* documented (it's better than
>> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>>
>> The patch set removes the handling of
>> "SYSTEM_TABLE_INFO.AcpiTableBase"
>> from BlSupportDxe. That means that platforms currently relying on
>> BlSupportDxe to expose the ACPI content will break (until they start
>> producing the new HOB). I don't see the HOB (with this particular GUID)
>> being produced in UefiPayloadPkg anywhere.
>
> The concern is about PEI passing ACPI Table location through two kinds
> of HOBs. It looks like a flaw in the design. I agree.
>
> The HOB is produced by PEI phase, by some code that doesn't belong
> to edk2 repo.
>
>>
>> (2) The UefiPayloadEntry module ("This is the first module for UEFI
>> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
>> various pieces of information into the "AcpiBoardInfo" structure. So
>> even if the HOB producer phase exposes the ACPI payload via a dedicated
>> HOB, it will only create inconsistency between the information parsed by
>> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
>> (which will the ACPI contents from the dedicated HOB).
>
> I agree. At least the SYSTEM_TABLE_INFO.AcpiTableBase field should be removed
> and the accordingly code that consumes ACPI Table in BlSupportDxe should
> be updated to consume the new HOB.
>
>>
>> (3) The new HOB's structure (regardless of GUID) is not declared in any
>> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
>> we have is hidden in the source code:
>>
>> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
>> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>>
>> If a platform's PEI phase actually inteded to produce this new HOB, it
>> couldn't rely on a header file / DEC file.
>>
>> This is actually a *step back* from the SystemTableInfoGuid declaration
>> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
>
> gEfiAcpiTableGuid is defined in MdePkg/Include/Guid/Acpi.h.
> The file header says the GUID is used for entry in EFI system table.
> Now we reuse this GUID for HOB data.
> I think it's ok to use a spec defined GUID for another implementation purpose.
>
> I can create a file MdeModulePkg/Include/Guid/Acpi.h to define the HOB structure.
> Do you think it's ok?
I'd prefer a new GUID. New GUIDs are very easy to introduce; we won't
run out of them. It's always possible to use a new GUID for some purpose
that relates to an existent GUID. However, in the other direction, it's
difficult to split one use of a GUID from another use of the same GUID.
It's OK if the PI and UEFI specs agree on reusing gEfiAcpiTableGuid for
a new GUIDed HOB -- because they are managed by the same organization.
There's going to be coordination between those two spec working groups.
But the use case here is different.
It's not a problem to use "Acpi" in the GUID's symbolic name, in edk2.
An example for that is "gEfiAcpiVariableGuid" in
"MdeModulePkg/Include/Guid/AcpiS3Context.h". (Well, I think it should
not have used "Efi" in the name, but "Acpi" was fine.
"gEdkiiAcpiVariableGuid" would have been better.)
Especially if this is going to be documented in the universal boot
loader spec, then that spec should propose some C-language prefixes
anyway, such as (just a guess) "Ubl", and then one idea would be
"gUblAcpiTablesGuid", for the HOB.
>
>>
>>
>> So how can this be called "standardizing and modularizing"?
>>
>> You need a new GUID, a new GUID HOB structure (declared in
>> MdeModulePkg
>> DEC and GUID header); you need to spell out the priority order between
>> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
>> you need to update all driver in UefiPayloadPkg accordingly.
>
> Again. I agree it's a flaw in the design. We should remove AcpiTableBase field.
>
>>
>>
>> I will also not make a secret of my annoyance that, the first time Intel
>> needs such a core extension for some platform feature, it immediately
>> gets all approvals. Whereas, when we needed the exact same feature in
>> OVMF, we struggled for months, if not *years*, to reliably split the
>> ACPI content that OVMF downloaded from QEMU, into blobs that were
>> suitable for the standard ACPI table protocol interfaces. For years I've
>> been telling my colleagues that all this complexity in OVMF's ACPI
>> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
>> implementation in edk2 cannot simply accept a "root pointer", to the
>> ACPI table "forest" that's already laid out in memory. Now I find it
>> just a little bit too convenient that the first time Intel needs the
>> same, we immediately call it "standardizing and modularizing" -- without
>> as much as a header file describing the actual contents of the new GUID HOB.
>
> I am not aware of the similar OVMF requirement.
>
> The requirement here is to support different bootloaders that may already
> create the essential ACPI table and DXE phase (payload) may use AcpiTable
> protocol to install/update tables.
The requirement has been the same with QEMU, for a very long time. QEMU
generates ACPI tables (more precisely, ACPI blobs) at runtime, in a
particular format. The guest firmware allocates memory, downloads the
blobs into them, and then performs a number of steps on them that QEMU
dictates in a "command script". This command script has commands like
"download this blob into memory you allocate", "update this pointer in
this ACPI blob to point at a specific offset in another ACPI blob",
"recalculate checksum over this range, and store the result at this
offset", and so on. The idea is that QEMU generates the ACPI content,
but the guest firmware places it the content into guest RAM, and then
(according to the QEMU instructions) fixes up the blobs, so they are
valid ACPI tables in the end. This allows the guest firmware to remain
independent of (or, put differently, "blind to") the ACPI content that
QEMU generates. This is a very important design goal, because the
platform hardware in QEMU changes frequently, and with it, the ACPI
content has to change together. If we kept the ACPI content in the guest
firmware (SeaBIOS and OVMF), then SeaBIOS and OVMF would have to be
updated in lock-step with QEMU. That's unsustainable.
What OVMF does is that interprets the command script, laying out the
tables / blobs in guest RAM as QEMU requires. However, when it's done,
it doesn't just use the RSDP / RSDT / XSDT from QEMU as the root tables,
circumventing AcpiTableDxe. Instead, OVMF traverses the tables itself,
"structurally", and identifies memory addresses that are "most likely"
the start offsets of ACPI tables. And then OVMF passes those to
EFI_ACPI_TABLE_PROTOCOL.InstallTable(). In the end, the originally laid
out tables (the result of the command script execution) is freed, and
only those ACPI tables will exist that EFI_ACPI_TABLE_PROTOCOL created
internally.
If the HOB you are now trying to introduce had existed 6-8 years ago,
the work I had to do in OvmfPkg's AcpiPlatformDxe would have been a
*fraction* of what I ended up doing.
This is one reason why I'm so annoyed. There's a double standard in edk2
core module maintenance. Extending the core of edk2 is more or less
*equally useful* for different platform owners, but those platform
owners face *very different difficulties* when they actually attempt the
core extensions. I never even attempted extending AcpiTableDxe in
MdeModulePkg, because I knew it was tied to the spec, and was convinced
that my QEMU-motivated request would be rejected anyway.
>> (Meanwhile we argue for months about actual, proven spec breakage in
>> edk2, such as signaling ready to boot around recovery options or
>> whatever. Standardization matters as long as *you* need it, huh?)
>
> The definition of spec breakage to me is we cannot do anything that's
> conflict with the spec. But we can do things that spec doesn't define.
> Please correct if I am wrong.
Sure, here's a counter-example: when I proposed adding new status codes
to UefiBootManagerLib, so that boot option loading and boot option
starting would be broadcast to the system using the "report status code"
facility, with rich data (boot option device path, and EFI_STATUS
result), you said that you didn't want to add such extensions to
UefiBootManagerLib; you wanted it to do what the spec required, and
*only* what the spec required. You told me to go change the spec first.
My proposal was entirely transparent to the PI and UEFI specs, both the
introduction of the new status codes (and the associated information
structure(s)), and the logic to emit them.
* [edk2] [PATCH 0/5]
MdeModulePkg, OvmfPkg: more easily visible boot progress reporting
http://mid.mail-archive.com/20171122235849.4177-1-lersek@redhat.com
We have a new process for that now ("code first", although I'm unsure
how it works in practice).
My point is: don't cut corners. It's already *much easier* for Intel to
extend core modules than for other contributors -- because the core
module maintainers mostly come from Intel, and they can negotiate and
get aligned with the Intel *platform owners* before anything happens on
the list! --, so at least *how* you implement the extension should be
squeaky clean. Don't make other platforms pay a steep price (in
complexity or code size that cannot be compiled out), don't reuse
standard GUIDs, don't shirk documentation requirements (new header for
the GUID structure), and so on.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73205): https://edk2.groups.io/g/devel/message/73205
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi all, Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP and then install the tables? It's a solution that uses the regular UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP is in the configuration table, we probably always want those tables). Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in DSC) but not added to a FV (not listed in FDF). So, how has this been tested? Regards, Benjamin Doron -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73208): https://edk2.groups.io/g/devel/message/73208 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 03/24/21 00:52, Benjamin Doron wrote: > Hi all, > Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in > MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP > and then install the tables? It's a solution that uses the regular > UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP > is in the configuration table, we probably always want those tables). I'm sorry, I don't understand how this would help. > Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in > DSC) but not added to a FV (not listed in FDF). So, how has this been > tested? I assume through an out-of-tree platform. Many such core modules exist in edk2 that are not consumed by any of the virtual platforms in the edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg). Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73204): https://edk2.groups.io/g/devel/message/73204 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> > >> Hi all, >> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in >> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP >> and then install the tables? It's a solution that uses the regular >> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP >> is in the configuration table, we probably always want those tables). > > I'm sorry, I don't understand how this would help. As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing. Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe. This allows MdeModulePkg to abstract away the parsing, only installing tables available to it. (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls `gBS->InstallConfigurationTable` with the address of RSDP). I understand that this may not work for OVMF if tables are located differently in memory. > > >> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in >> DSC) but not added to a FV (not listed in FDF). So, how has this been >> tested? > > I assume through an out-of-tree platform. Many such core modules exist > in edk2 that are not consumed by any of the virtual platforms in the > edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg). This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73213): https://edk2.groups.io/g/devel/message/73213 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 03/24/21 17:55, Benjamin Doron wrote: >> >> >>> Hi all, >>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in >>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP >>> and then install the tables? It's a solution that uses the regular >>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP >>> is in the configuration table, we probably always want those tables). >> >> I'm sorry, I don't understand how this would help. > > As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing. > > Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add > RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe. > This allows MdeModulePkg to abstract away the parsing, only installing tables > available to it. But this is already the best approach, and already what's happening -- when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in MdeModulePkg to pick up the table and hook it into the RSDT / XSDT / wherever, and also to manage RSD PTR as a UEFI configuration table. Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member function for taking a forest of ACPI tables, passed in by RSD PTR? Sorry about being dense. :) > (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls > `gBS->InstallConfigurationTable` with the address of RSDP). > > I understand that this may not work for OVMF if tables are located differently in memory. > >> >> >>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in >>> DSC) but not added to a FV (not listed in FDF). So, how has this been >>> tested? >> >> I assume through an out-of-tree platform. Many such core modules exist >> in edk2 that are not consumed by any of the virtual platforms in the >> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg). > > This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF > if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised. > Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF at all.) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73215): https://edk2.groups.io/g/devel/message/73215 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Ben,
I understand your point.
The purpose of changing the AcpiTableDxe to directly consume the HOB is to reduce the overall system complexity.
The complexity I see with your option is:
1. platform needs to include that driver to consume the ACPI table from bootloader
2. that driver (consume HOB and install table through AcpiTable protocol) needs to register a protocol notification on AcpiTable protocol and do the table installation in the callback.
Laszlo,
The change here is to meet the requirement that bootloader provides the ACPI table.
In OVMF case, it's not the bootloader but the virtual hardware who provides the ACPI table.
I can see the similarity between the two: the ACPI table is produced before the UEFI Payload runs.
Can you review the new option below and see whether it can meet the OVMF needs as well?
1. Define a new GUID gPldHobAcpiTable in MdeModulePkg.dec and a structure as below in MdeModulePkg/Include/Guid/Acpi.h.
typedef struct {
UINT64 TableAddress;
} PLD_HOB_ACPI_TABLE;
2. Change AcpiTableDxe driver to consume the HOB
3. Remove SYSTEM_TABLE_INFO. AcpiTableBase/AcpiTableSize.
4. Remove the BlSupportDxe code that consume the ACPI table in SYSTEM_TABLE_INFO.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Thursday, March 25, 2021 2:33 AM
> To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
>
> On 03/24/21 17:55, Benjamin Doron wrote:
> >>
> >>
> >>> Hi all,
> >>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
> >>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
> >>> and then install the tables? It's a solution that uses the regular
> >>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
> >>> is in the configuration table, we probably always want those tables).
> >>
> >> I'm sorry, I don't understand how this would help.
> >
> > As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
> >
> > Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
> > RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
> > This allows MdeModulePkg to abstract away the parsing, only installing tables
> > available to it.
>
> But this is already the best approach, and already what's happening --
> when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
> platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
> MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
> wherever, and also to manage RSD PTR as a UEFI configuration table.
>
> Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
> function for taking a forest of ACPI tables, passed in by RSD PTR?
>
> Sorry about being dense. :)
>
> > (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
> > `gBS->InstallConfigurationTable` with the address of RSDP).
> >
> > I understand that this may not work for OVMF if tables are located differently in memory.
> >
> >>
> >>
> >>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
> >>> DSC) but not added to a FV (not listed in FDF). So, how has this been
> >>> tested?
> >>
> >> I assume through an out-of-tree platform. Many such core modules exist
> >> in edk2 that are not consumed by any of the virtual platforms in the
> >> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
> >
> > This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
> > if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
> >
>
> Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
> at all.)
>
> Laszlo
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73243): https://edk2.groups.io/g/devel/message/73243
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Ray,
The other option would be to define a Library Class for the AcpiTableDxe driver to consume and have a NULL version of it in the MdeModulePkg. That would allow more flexibility? It kind of depends if you want to make the implementation depend on a HOB or a LibraryClass? It at least gives the non pure PI implementations more potential options? I’m not sure with the problem you are trying to solve that helps or hurts, but I though I would throw out another option. The other advantage about the lib is could let you define the HOB in another package, if that makes more sense.
Not trying to slow down the solution, but just giving some other options in case another partitioning of this problem would be helpful?
Thanks,
Andrew Fish
> On Mar 24, 2021, at 6:10 PM, Ni, Ray <ray.ni@intel.com> wrote:
>
> Ben,
> I understand your point.
> The purpose of changing the AcpiTableDxe to directly consume the HOB is to reduce the overall system complexity.
> The complexity I see with your option is:
> 1. platform needs to include that driver to consume the ACPI table from bootloader
> 2. that driver (consume HOB and install table through AcpiTable protocol) needs to register a protocol notification on AcpiTable protocol and do the table installation in the callback.
>
> Laszlo,
> The change here is to meet the requirement that bootloader provides the ACPI table.
> In OVMF case, it's not the bootloader but the virtual hardware who provides the ACPI table.
> I can see the similarity between the two: the ACPI table is produced before the UEFI Payload runs.
> Can you review the new option below and see whether it can meet the OVMF needs as well?
> 1. Define a new GUID gPldHobAcpiTable in MdeModulePkg.dec and a structure as below in MdeModulePkg/Include/Guid/Acpi.h.
> typedef struct {
> UINT64 TableAddress;
> } PLD_HOB_ACPI_TABLE;
> 2. Change AcpiTableDxe driver to consume the HOB
> 3. Remove SYSTEM_TABLE_INFO. AcpiTableBase/AcpiTableSize.
> 4. Remove the BlSupportDxe code that consume the ACPI table in SYSTEM_TABLE_INFO.
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Thursday, March 25, 2021 2:33 AM
>> To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
>>
>> On 03/24/21 17:55, Benjamin Doron wrote:
>>>>
>>>>
>>>>> Hi all,
>>>>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
>>>>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
>>>>> and then install the tables? It's a solution that uses the regular
>>>>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
>>>>> is in the configuration table, we probably always want those tables).
>>>>
>>>> I'm sorry, I don't understand how this would help.
>>>
>>> As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
>>>
>>> Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
>>> RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
>>> This allows MdeModulePkg to abstract away the parsing, only installing tables
>>> available to it.
>>
>> But this is already the best approach, and already what's happening --
>> when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
>> platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
>> MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
>> wherever, and also to manage RSD PTR as a UEFI configuration table.
>>
>> Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
>> function for taking a forest of ACPI tables, passed in by RSD PTR?
>>
>> Sorry about being dense. :)
>>
>>> (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
>>> `gBS->InstallConfigurationTable` with the address of RSDP).
>>>
>>> I understand that this may not work for OVMF if tables are located differently in memory.
>>>
>>>>
>>>>
>>>>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
>>>>> DSC) but not added to a FV (not listed in FDF). So, how has this been
>>>>> tested?
>>>>
>>>> I assume through an out-of-tree platform. Many such core modules exist
>>>> in edk2 that are not consumed by any of the virtual platforms in the
>>>> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
>>>
>>> This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
>>> if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
>>>
>>
>> Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
>> at all.)
>>
>> Laszlo
>>
>>
>>
>>
>>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73255): https://edk2.groups.io/g/devel/message/73255
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 03/25/21 04:55, Andrew Fish wrote: > Ray, > > The other option would be to define a Library Class for the AcpiTableDxe driver to consume and have a NULL version of it in the MdeModulePkg. That would allow more flexibility? It kind of depends if you want to make the implementation depend on a HOB or a LibraryClass? It at least gives the non pure PI implementations more potential options? I’m not sure with the problem you are trying to solve that helps or hurts, but I though I would throw out another option. The other advantage about the lib is could let you define the HOB in another package, if that makes more sense. > > Not trying to slow down the solution, but just giving some other options in case another partitioning of this problem would be helpful? I agree this is another option for separating out the HOB consumption logic, in case it is complex or large. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73284): https://edk2.groups.io/g/devel/message/73284 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 03/25/21 02:10, Ni, Ray wrote:
> Ben,
> I understand your point.
> The purpose of changing the AcpiTableDxe to directly consume the HOB is to reduce the overall system complexity.
> The complexity I see with your option is:
> 1. platform needs to include that driver to consume the ACPI table from bootloader
> 2. that driver (consume HOB and install table through AcpiTable protocol) needs to register a protocol notification on AcpiTable protocol and do the table installation in the callback.
>
> Laszlo,
> The change here is to meet the requirement that bootloader provides the ACPI table.
> In OVMF case, it's not the bootloader but the virtual hardware who provides the ACPI table.
> I can see the similarity between the two: the ACPI table is produced before the UEFI Payload runs.
> Can you review the new option below and see whether it can meet the OVMF needs as well?
Let me clarify: there's no way I'm touching that part of OVMF. I don't
want any potential regressions there. It's been working stably for years.
I'd just like to avoid a duplication of functionality -- if the new HOB
logic in MdeModulePkg is heavy, then I'd like a possibility for
platforms to separate it out.
> 1. Define a new GUID gPldHobAcpiTable in MdeModulePkg.dec and a structure as below in MdeModulePkg/Include/Guid/Acpi.h.
> typedef struct {
> UINT64 TableAddress;
> } PLD_HOB_ACPI_TABLE;
Looks good (but maybe use EFI_PHYSICAL_ADDRESS for type, and a more
telling name than "TableAddress" -- name precisely what ACPI table type
the pointer refers to?)
> 2. Change AcpiTableDxe driver to consume the HOB
Yes. And this is the part that, if it's complex or large, should go into
a separate source file (together with a new INF file), or be controlled
by a Feature PCD.
If it's not complex / large, and you can refactor AcpiTableDxe first
such that the HOB-based functionality is not littered over a bunch of
functions, then it's OK to stick with just one INF file (and no Feature
PCD).
> 3. Remove SYSTEM_TABLE_INFO. AcpiTableBase/AcpiTableSize.
Won't that break other systems that currently depend on it? Just asking.
I'm neutral, personally.
> 4. Remove the BlSupportDxe code that consume the ACPI table in SYSTEM_TABLE_INFO.
Same compatibility question for existent, dependent platforms.
Thanks
Laszlo
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Thursday, March 25, 2021 2:33 AM
>> To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
>>
>> On 03/24/21 17:55, Benjamin Doron wrote:
>>>>
>>>>
>>>>> Hi all,
>>>>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
>>>>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
>>>>> and then install the tables? It's a solution that uses the regular
>>>>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
>>>>> is in the configuration table, we probably always want those tables).
>>>>
>>>> I'm sorry, I don't understand how this would help.
>>>
>>> As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
>>>
>>> Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
>>> RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
>>> This allows MdeModulePkg to abstract away the parsing, only installing tables
>>> available to it.
>>
>> But this is already the best approach, and already what's happening --
>> when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
>> platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
>> MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
>> wherever, and also to manage RSD PTR as a UEFI configuration table.
>>
>> Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
>> function for taking a forest of ACPI tables, passed in by RSD PTR?
>>
>> Sorry about being dense. :)
>>
>>> (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
>>> `gBS->InstallConfigurationTable` with the address of RSDP).
>>>
>>> I understand that this may not work for OVMF if tables are located differently in memory.
>>>
>>>>
>>>>
>>>>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
>>>>> DSC) but not added to a FV (not listed in FDF). So, how has this been
>>>>> tested?
>>>>
>>>> I assume through an out-of-tree platform. Many such core modules exist
>>>> in edk2 that are not consumed by any of the virtual platforms in the
>>>> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
>>>
>>> This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
>>> if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
>>>
>>
>> Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
>> at all.)
>>
>> Laszlo
>>
>>
>>
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73283): https://edk2.groups.io/g/devel/message/73283
Mute This Topic: https://groups.io/mt/81543419/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, Mar 24, 2021 at 02:33 PM, Laszlo Ersek wrote: > > >> >>> >>>> Hi all, >>>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in >>>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP >>>> and then install the tables? It's a solution that uses the regular >>>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP >>>> is in the configuration table, we probably always want those tables). >>> >>> I'm sorry, I don't understand how this would help. >> >> As I understand it, the issue is that this patchset changes MdeModulePkg >> to do platform-specific parsing. >> >> Perhaps it would be an acceptable solution for platforms to retrieve the >> tables, then add >> RSDP/them to the configuration table to be installed by >> AcpiTableDxe/AcpiPlatformDxe. >> This allows MdeModulePkg to abstract away the parsing, only installing >> tables >> available to it. > > But this is already the best approach, and already what's happening -- > when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the > platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in > MdeModulePkg to pick up the table and hook it into the RSDT / XSDT / > wherever, and also to manage RSD PTR as a UEFI configuration table. > > Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member > function for taking a forest of ACPI tables, passed in by RSD PTR? Yes. I thought the implementation of AcpiPlatformDxe in MdeModulePkg could take it, so it will install ACPI tables from flash or from memory. Regarding UefiPayloadPkg: gEfiAcpiTableGuid is not a HOB. It's an entry in gUefiSystemTableInfoGuid (which is a HOB) that was installed in the config table. Therefore, retrieving its GUID as a HOB in AcpiTableDxe fails. To make this patchset work in UefiPayloadPkg (actually a fork), I have to add AcpiTableDxe to its FDF, drop patch 2/2 and make `InstallAcpiTableFromHob` do `Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID **) &Rsdp);`. Then, this patchset works: "acpiview" shell command shows tables from QEMU + coreboot, as well as a BGRT. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73244): https://edk2.groups.io/g/devel/message/73244 Mute This Topic: https://groups.io/mt/81543419/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.