From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.477.1616518109130984788 for ; Tue, 23 Mar 2021 09:48:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IXkVC9cC; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1616518108; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RuRRTN6we48xLK2M/4gI2DnnRPDVjYSOAXKwvT1qKos=; b=IXkVC9cC9M+ufioAhc57JooTbZIAYU4nhR5JZDTPv66tTQRMZxYpca0mfBc5bJhUhP1F1w CvFUiyhX0gH/8Ed8maccrQ6e80s0EtPr0RgCU8TCx5vIKKogWgkkeuDj8GvZkcnoVlu/w+ HsxEglXmTvuuP/LcvcT6kgPuwjJDLgo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-179-CFRaxVxhPmWrxdK4LqaZEw-1; Tue, 23 Mar 2021 12:48:25 -0400 X-MC-Unique: CFRaxVxhPmWrxdK4LqaZEw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C028F800D53; Tue, 23 Mar 2021 16:48:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-240.ams2.redhat.com [10.36.114.240]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4EDBF60877; Tue, 23 Mar 2021 16:48:21 +0000 (UTC) Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob To: devel@edk2.groups.io, guo.dong@intel.com, "Liu, Zhiguang" , "Ni, Ray" Cc: "Wang, Jian J" , "Wu, Hao A" , "Bi, Dandan" , Liming Gao , Andrew Fish References: <20210323032438.950-1-zhiguang.liu@intel.com> <20210323032438.950-3-zhiguang.liu@intel.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 23 Mar 2021 17:48:20 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 On Behalf Of Laszlo >> Ersek >> Sent: Tuesday, March 23, 2021 5:40 AM >> To: Liu, Zhiguang ; Ni, Ray ; Dong, >> Guo >> Cc: devel@edk2.groups.io; Wang, Jian J ; Wu, Hao A >> ; Bi, Dandan ; Liming Gao >> >> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver 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 >> >> >> >> >> > > > > > >