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.7927.1616503233420062644 for ; Tue, 23 Mar 2021 05:40:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cUVXmg+/; 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=1616503232; 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=BOZmnsN22twADE4oVXZVfINdpmXyrqi4Y3gKyK9f1+w=; b=cUVXmg+/CBXu5ZKz4aBQstFnvFf+PJsY4oTliL/8QbZpIoTq1G5QOq4/C7yD0fo3dJL4vW PTpwydSjcQldEmwfNI8P7lh5sc3m2QGzigybWZtDnAgQUiL8p+PR/TMm75D5bT73lEpLS2 ioRxo3y2dJ0tdjHb/yHDbZh93g5Ltqo= 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-599-OZTuKubgO4uy9KSPOrDi1w-1; Tue, 23 Mar 2021 08:40:30 -0400 X-MC-Unique: OZTuKubgO4uy9KSPOrDi1w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 088CB1007474; Tue, 23 Mar 2021 12:40:29 +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 22FD15D6AD; Tue, 23 Mar 2021 12:40:26 +0000 (UTC) Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob To: zhiguang.liu@intel.com, Ray Ni , Dong Guo References: <20210323032438.950-1-zhiguang.liu@intel.com> <20210323032438.950-3-zhiguang.liu@intel.com> From: "Laszlo Ersek" Cc: devel@edk2.groups.io, Jian J Wang , Hao A Wu , Dandan Bi , Liming Gao Message-ID: Date: Tue, 23 Mar 2021 13:40:26 +0100 MIME-Version: 1.0 In-Reply-To: <20210323032438.950-3-zhiguang.liu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 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