From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x235.google.com (mail-wr0-x235.google.com [IPv6:2a00:1450:400c:c0c::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 74DB0803D7 for ; Mon, 20 Mar 2017 04:59:19 -0700 (PDT) Received: by mail-wr0-x235.google.com with SMTP id u48so90611138wrc.0 for ; Mon, 20 Mar 2017 04:59:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=GB3J64WUTZfoTi2eUJ2TfddRvCLFw4ayrXIFAtY9I7M=; b=cUuxYxJuBmYktLTiMQ2Hfs5F3WLk7garZuQZ05zzgErpxmNjLW++RSROyZyJOj/ujv taDDTegUVESKpgjXy1vSJgmlBcTp8b/j121RUYa95ULUjniBdhifl1I+BlsxViW6n7UE lUAFSHzB9n1OEfMGIyKpFHoG2Bs8q5JKVQ1Z0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=GB3J64WUTZfoTi2eUJ2TfddRvCLFw4ayrXIFAtY9I7M=; b=hbg2n1rzj++J35G0sHl2dl6ZuZCljJn12/6QcvHI7HweVcEjm7wAOaWsqvx0X9541c WQWhekss/gJ6/Wm4ujbiCfgPf9NGyyL4zVHfJZzwmBr95UV/eO8UZgrIpF2m+4Pr0q25 7txWzpq8BElRYzpMARkrAQgMJO8kdri+Nvw6WRqRyfBOEQaZLR0zyqRnBWepKyTyDRmD HnZFT/HceS2meQLroXH+LrstZgwkq2eLgBEnTcFjVXFUSUM8Fx93d97wBtoqyb9VRQHe EP3fjiQH2rwSPYNIKF4XHhGo5jwgWH7cBzFwcmFHlXa+3aatyqnQjRReBU555ajA7sl9 04sA== X-Gm-Message-State: AFeK/H2XOIi9pXRM2t78/joB7Q3W5mEylSRz4seYgKya3XbRbx7VstZWOjoolEd/+VGZDde7 X-Received: by 10.223.179.216 with SMTP id x24mr24978830wrd.171.1490011157456; Mon, 20 Mar 2017 04:59:17 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id x69sm13235827wma.15.2017.03.20.04.59.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 20 Mar 2017 04:59:16 -0700 (PDT) Date: Mon, 20 Mar 2017 11:59:15 +0000 From: Leif Lindholm To: Laszlo Ersek Cc: edk2-devel-01 , Ard Biesheuvel , Star Zeng , Feng Tian Message-ID: <20170320115915.GN16034@bivouac.eciton.net> References: <20170317204731.31488-1-lersek@redhat.com> <20170317204731.31488-6-lersek@redhat.com> <20170318150041.GL16034@bivouac.eciton.net> <806495eb-d22b-f9e9-d215-935fdedc17a5@redhat.com> MIME-Version: 1.0 In-Reply-To: <806495eb-d22b-f9e9-d215-935fdedc17a5@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Mar 2017 11:59:20 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Mar 20, 2017 at 10:01:09AM +0100, Laszlo Ersek wrote: > >> Because this protocol is not standard, it is prefixed with EDKII / Edkii, > >> as seen elsewhere in MdeModulePkg and SecurityPkg, for example. (ARM / Arm > >> doesn't look future-proof enough; future UEFI platforms could face the > >> same issue.) In addition, an effort is made to avoid the phrase > >> "AcpiPlatform", as that belongs to drivers / libraries that produce > >> platform specific ACPI content (as opposed to deciding whether the entire > >> firmware will have access to EFI_ACPI_TABLE_PROTOCOL). > > > > I greatly approve, but since you've already done this generically - is > > there any good reason to keep this in ArmPkg? > > I am strongly looking to get rid of "things that happen to have been > > implemented for ARM" from there and pruning it down to contain only > > things that are architecturelly ARM-specific. > > This patch is not specific to ARM architecturally, but it is specific to > the ARM ecosystem / community. I'm unaware of another platform (that > would affect edk2 ATM anyway) where such a conflict in beliefs has not > been sorted out for years. Indeed. However, I have developed a mild allergy towards things that are not fundamentally ARM-specific, but are treated as such. It tends to introduce a mental barrier towards code unification and sharing, since it makes it look like an architecture-specific component. > The somewhat speculative generality in the naming (i.e., Edkii prefix > rather than Arm) is there only because I understand that DT / libfdt is > used on other platforms as well, and I expect once those see UEFI (and > edk2) enablement & porting, the same DT vs. ACPI conflict in Linux space > will extend to those platforms as well. > > IOW, at the moment the patch is specific to ARM, it is not random, but > it could change in the future. And, I wouldn't like to force client > modules to rename the GUID at that time. Oh, I perfectly agree. I'm just saying I don't think that a feature only being applicable to ARM at the current moment should be a barrier for it being introduced in Mde*Pkg. > > MdeModulePkg? > > Not a bad idea, but the point of this approach was to avoid touching > core modules. If we modify MdeModulePkg *logic* (as opposed to the > trivial typo fix elsewhere in this series), then the simplest solution > is to just add a dynamic PCD to MdeModulePkg.dec, which forces > AcpiTableDxe to exit immediately with EFI_ABORTED or EFI_UNSUPPORTED. That actually sounds more palatable to me than this set, which I still prefer over Ard's solution (I'll expand on my reservations about that one on the correct thread). > As I mentioned earlier, PcdAcpiS3Enable had been introduced very > similarly to this. It controls partial or full functionality of several > DXE drivers: > > 11a291a4d279 MdeModulePkg: Introduce new PCD PcdAcpiS3Enable > a1726e308903 OvmfPkg: Set PcdAcpiS3Enable according to > QemuFwCfgS3Enabled() > 125e09387641 MdeModulePkg S3SaveStateDxe: Consume PcdAcpiS3Enable to > control the code > e96708de8837 IntelFrameworkModulePkg AcpiS3SaveDxe: Consume > PcdAcpiS3Enable to control the code > d2d38610603f MdeModulePkg SmmS3SaveStateDxe: Consume PcdAcpiS3Enable > to control the code > 800c02fbe2da MdeModulePkg BootScriptExecutorDxe: Consume > PcdAcpiS3Enable to control the code > ca98f6038294 UefiCpuPkg/CpuS3DataDxe: Consume PcdAcpiS3Enable to > control the code > b10d5ddc0385 UefiCpuPkg/PiSmmCpuDxeSmm: Consume PcdAcpiS3Enable to > control the code > > If the AcpiTableDxe maintainers aren't opposed to another dynamic PCD > like PcdAcpiS3Enable -- but in this case for controlling AcpiTableDxe -- > then I'm fine with it too. > > ... I knew that new PCDs in MdeModulePkg needed strong justification, so > I figured I'd try another route first. Yes, I understand. But I think it is well motivated here, and the solution that feels the most UEFI to me. Regards, Leif > Thanks > Laszlo > > > > > Regards, > > > > Leif > > > >> Cc: Ard Biesheuvel > >> Cc: Leif Lindholm > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek > >> --- > >> ArmPkg/ArmPkg.dec | 4 ++ > >> ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf | 40 ++++++++++++++++++++ > >> ArmPkg/Include/Protocol/PlatformHasAcpi.h | 34 +++++++++++++++++ > >> ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c | 36 ++++++++++++++++++ > >> 4 files changed, 114 insertions(+) > >> > >> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec > >> index c4b4da2f95bb..0e49360a386a 100644 > >> --- a/ArmPkg/ArmPkg.dec > >> +++ b/ArmPkg/ArmPkg.dec > >> @@ -52,6 +52,10 @@ [Ppis] > >> ## Include/Ppi/ArmMpCoreInfo.h > >> gArmMpCoreInfoPpiGuid = { 0x6847cc74, 0xe9ec, 0x4f8f, {0xa2, 0x9d, 0xab, 0x44, 0xe7, 0x54, 0xa8, 0xfc} } > >> > >> +[Protocols] > >> + ## Include/Protocol/PlatformHasAcpi.h > >> + gEdkiiPlatformHasAcpiProtocolGuid = { 0xf0966b41, 0xc23f, 0x41b9, { 0x96, 0x04, 0x0f, 0xf7, 0xe1, 0x11, 0x96, 0x5a } } > >> + > >> [PcdsFeatureFlag.common] > >> gArmTokenSpaceGuid.PcdCpuDxeProduceDebugSupport|FALSE|BOOLEAN|0x00000001 > >> > >> diff --git a/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf > >> new file mode 100644 > >> index 000000000000..c83da4d8e98a > >> --- /dev/null > >> +++ b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf > >> @@ -0,0 +1,40 @@ > >> +## @file > >> +# A hook-in library for MdeModulePkg/Universal/Acpi/AcpiTableDxe. > >> +# > >> +# Plugging this library instance into AcpiTableDxe makes > >> +# EFI_ACPI_TABLE_PROTOCOL and (if enabled) EFI_ACPI_SDT_PROTOCOL depend on the > >> +# platform's dynamic decision whether to expose an ACPI-based hardware > >> +# description to the operating system. > >> +# > >> +# Universal and platform specific DXE drivers that produce ACPI tables depend > >> +# on EFI_ACPI_TABLE_PROTOCOL / EFI_ACPI_SDT_PROTOCOL in turn. > >> +# > >> +# Copyright (C) 2017, Red Hat, Inc. > >> +# > >> +# This program and the accompanying materials are licensed and made available > >> +# under the terms and conditions of the BSD License which accompanies this > >> +# distribution. The full text of the license may be found at > >> +# http://opensource.org/licenses/bsd-license.php > >> +# > >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > >> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > >> +## > >> + > >> +[Defines] > >> + INF_VERSION = 1.25 > >> + BASE_NAME = PlatformHasAcpiLib > >> + FILE_GUID = 29beb028-0958-447b-be0a-12229235d77d > >> + MODULE_TYPE = BASE > >> + VERSION_STRING = 1.0 > >> + LIBRARY_CLASS = PlatformHasAcpiLib|DXE_DRIVER > >> + CONSTRUCTOR = PlatformHasAcpiInitialize > >> + > >> +[Sources] > >> + PlatformHasAcpiLib.c > >> + > >> +[Packages] > >> + ArmPkg/ArmPkg.dec > >> + MdePkg/MdePkg.dec > >> + > >> +[Depex] > >> + gEdkiiPlatformHasAcpiProtocolGuid > >> diff --git a/ArmPkg/Include/Protocol/PlatformHasAcpi.h b/ArmPkg/Include/Protocol/PlatformHasAcpi.h > >> new file mode 100644 > >> index 000000000000..3cd0cfe4515d > >> --- /dev/null > >> +++ b/ArmPkg/Include/Protocol/PlatformHasAcpi.h > >> @@ -0,0 +1,34 @@ > >> +/** @file > >> + EDKII Platform Has ACPI Protocol > >> + > >> + The presence of this protocol in the DXE protocol database implies that the > >> + platform provides the operating system with an ACPI-based hardware > >> + description. Note that this is not necessarily mutually exclusive with a > >> + Device Tree-based hardware description. A platform driver is supposed to > >> + produce a single instance of the protocol (with NULL contents), if > >> + appropriate. > >> + > >> + Copyright (C) 2017, Red Hat, Inc. > >> + > >> + This program and the accompanying materials are licensed and made available > >> + under the terms and conditions of the BSD License that accompanies this > >> + distribution. The full text of the license may be found at > >> + http://opensource.org/licenses/bsd-license.php. > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > >> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > >> +**/ > >> + > >> + > >> +#ifndef __EDKII_PLATFORM_HAS_ACPI_PROTOCOL_H__ > >> +#define __EDKII_PLATFORM_HAS_ACPI_PROTOCOL_H__ > >> + > >> +#define EDKII_PLATFORM_HAS_ACPI_PROTOCOL_GUID \ > >> + { \ > >> + 0xf0966b41, 0xc23f, 0x41b9, \ > >> + { 0x96, 0x04, 0x0f, 0xf7, 0xe1, 0x11, 0x96, 0x5a } \ > >> + } > >> + > >> +extern EFI_GUID gEdkiiPlatformHasAcpiProtocolGuid; > >> + > >> +#endif > >> diff --git a/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c > >> new file mode 100644 > >> index 000000000000..79072da21c2b > >> --- /dev/null > >> +++ b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c > >> @@ -0,0 +1,36 @@ > >> +/** @file > >> + A hook-in library for MdeModulePkg/Universal/Acpi/AcpiTableDxe. > >> + > >> + Plugging this library instance into AcpiTableDxe makes > >> + EFI_ACPI_TABLE_PROTOCOL and (if enabled) EFI_ACPI_SDT_PROTOCOL depend on the > >> + platform's dynamic decision whether to expose an ACPI-based hardware > >> + description to the operating system. > >> + > >> + Universal and platform specific DXE drivers that produce ACPI tables depend > >> + on EFI_ACPI_TABLE_PROTOCOL / EFI_ACPI_SDT_PROTOCOL in turn. > >> + > >> + Copyright (C) 2017, Red Hat, Inc. > >> + > >> + This program and the accompanying materials are licensed and made available > >> + under the terms and conditions of the BSD License which accompanies this > >> + distribution. The full text of the license may be found at > >> + http://opensource.org/licenses/bsd-license.php > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > >> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > >> +**/ > >> + > >> +#include > >> + > >> +RETURN_STATUS > >> +EFIAPI > >> +PlatformHasAcpiInitialize ( > >> + VOID > >> + ) > >> +{ > >> + // > >> + // Do nothing, just imbue AcpiTableDxe with an > >> + // EDKII_PLATFORM_HAS_ACPI_PROTOCOL dependency. > >> + // > >> + return RETURN_SUCCESS; > >> +} > >> -- > >> 2.9.3 > >> > >> >