Re: [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library

[Laszlo Ersek <lersek@redhat.com>]

On 03/18/17 16:00, Leif Lindholm wrote:
> On Fri, Mar 17, 2017 at 09:47:24PM +0100, Laszlo Ersek wrote:
>> The presence of this protocol in the DXE protocol database implies that
>> the platform provides the operating system with an ACPI-based hardware
>> description. 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.
>>
>> The decision to produce the protocol is platform specific; for example, it
>> could depend on an HII checkbox / underlying non-volatile UEFI variable.
>>
>> The protocol is meant to be consumed by
>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe", through the
>> PlatformHasAcpiLib plug-in. By linking this library into AcpiTableDxe via
>> NULL resolution in the platform DSC, the platform makes
>> EFI_ACPI_TABLE_PROTOCOL and (if enabled) EFI_ACPI_SDT_PROTOCOL dependent
>> on the above dynamic decision.
>>
>> In turn, other (platform and universal) DXE drivers that produce ACPI
>> tables will wait for EFI_ACPI_TABLE_PROTOCOL / EFI_ACPI_SDT_PROTOCOL, via
>> DEPEX, protocol notify, or a simple gBS->LocateProtocol() in a "late
>> enough" callback (such as Ready To Boot).
>>
>> 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.

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.

> 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.

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.

Thanks
Laszlo

> 
> Regards,
> 
> Leif
> 
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@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
>>
>>