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.551.1616693607067518020 for ; Thu, 25 Mar 2021 10:33:27 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=AaTcbZgM; 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=1616693606; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Su5kWaCeGAdVl4YlZEkdqRNbq5qRnspCRqTKyDQKMnw=; b=AaTcbZgM7eJ/sPS73mSSbo+YvCYIHoKoYmXs2lFmp/mnmz4658NQAylZ9/VZ6152zVGygG j7hpQ8X0ZAtWxZ20mSsJ/eQhOwrDMaejBIhZ6Z0M8bDFNnrJ8/+uorDL34dsRnMP8bIKgI ICFoDNcepojvgYKf0xMalq7BTcWouJQ= 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-587-U-wQUbj6NLGpVpvXN2gfbw-1; Thu, 25 Mar 2021 13:33:23 -0400 X-MC-Unique: U-wQUbj6NLGpVpvXN2gfbw-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 39CFD100F76A; Thu, 25 Mar 2021 17:33:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-200.ams2.redhat.com [10.36.114.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2CF285D6D5; Thu, 25 Mar 2021 17:33:20 +0000 (UTC) Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob To: "Ni, Ray" , "devel@edk2.groups.io" , Benjamin Doron References: <8ecc6ec1-f7a0-7878-ef57-b58f99bc3af2@redhat.com> <8402.1616604928827766426@groups.io> <2e602df8-b1fc-193b-f61a-86802ace9108@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 25 Mar 2021 18:33:20 +0100 MIME-Version: 1.0 In-Reply-To: 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/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 On Behalf Of Laszlo Ersek >> Sent: Thursday, March 25, 2021 2:33 AM >> To: Benjamin Doron ; 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 >> >> >> >> >> >