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 <ard.biesheu...@linaro.org> > >> Cc: Leif Lindholm <leif.lindh...@linaro.org> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> > >> --- > >> 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 <Base.h> > >> + > >> +RETURN_STATUS > >> +EFIAPI > >> +PlatformHasAcpiInitialize ( > >> + VOID > >> + ) > >> +{ > >> + // > >> + // Do nothing, just imbue AcpiTableDxe with an > >> + // EDKII_PLATFORM_HAS_ACPI_PROTOCOL dependency. > >> + // > >> + return RETURN_SUCCESS; > >> +} > >> -- > >> 2.9.3 > >> > >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel