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

["Zeng, Star" <star.zeng@intel.com>]

Just an idea.

There is a way to do not introduce a new PCD, but reuse PcdAcpiExposedTableVersions.
When PcdAcpiExposedTableVersions is configured to *0* (means no ANY ACPI table should be exposed), AcpiTableDxe returns EFI_UNSUPPORTED in the driver entrypoint.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Zeng, Star
Sent: Monday, March 20, 2017 5:57 PM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library

Oh got it, returning unsuccessfully in constructor of library does not work. :(

For (1), I definitely have no any comments. :) For (2), I got no strong reason to support or reject it. We'd better could get comments from Mike and Jiewen.

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Monday, March 20, 2017 5:18 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library

On 03/20/17 03:23, Zeng, Star wrote:
> Laszlo,
>
> Could the implementation have multiple instances of PlatformHasAcpiLib 
> to return unsuccessfully in the constructor when 
> EFI_ACPI_TABLE_PROTOCOL is not needed? And then eliminate 
> PlatformHasAcpiDtDxe, gEdkiiPlatformHasAcpiProtocolGuid and 
> gEdkiiPlatformHasDeviceTreeProtocolGuid?

If a library constructor fails, then the client module does not see an explicit error (or automatic unloading); instead, an assertion failure is triggered in auto-generated code.

For example, in the
"Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe/DEBUG/AutoGen.c"
file, with this series applied, we find:

> VOID
> EFIAPI
> ProcessLibraryConstructorList (
>   IN EFI_HANDLE        ImageHandle,
>   IN EFI_SYSTEM_TABLE  *SystemTable
>   )
> {
>   EFI_STATUS  Status;
>
>   Status = HobLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = FdtPL011SerialPortLibInitialize ();
>   ASSERT_EFI_ERROR (Status);
>
>   Status = BaseDebugLibSerialPortConstructor ();
>   ASSERT_EFI_ERROR (Status);
>
>   Status = UefiBootServicesTableLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = UefiRuntimeServicesTableLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = UefiLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = PlatformHasAcpiInitialize (); <--------------- constructor of PlatformHasAcpiLib plug-in lib
>   ASSERT_EFI_ERROR (Status);
>
> }

What we could do instead would be a new library class, with an explicit initialization function. However, for that to work, AcpiTableDxe would have to be extended with a new library class dependency and a call to the init function. Additionally, out-of-tree platform DSC files would have to resolve this library class to the default instance.

Introducing a boolean dynamic PCD for the same would be much simpler then; the DEC default would cover out-of-tree platform DSCs without any change in behavior.

In summary, there are two alternatives in my opinion:

(1) If we prefer not to modify MdeModulePkg: stick with this series. The only update I'd see justified would be to move gEdkiiPlatformHasDeviceTreeProtocolGuid to EmbeddedPkg as Leif requested.

(2) If we are willing to modify MdeModulePkg for this problem: introduce a new dynamic PCD such as "PcdAcpiTableEnable", and make AcpiTableDxe exit early if the PCD is FALSE.

Thanks,
Laszlo

>
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Laszlo Ersek
> Sent: Saturday, March 18, 2017 4:47 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel 
> <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has 
> ACPI Protocol, and plug-in library
>
> 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).
>
> 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|0x000000
> 01
>
> 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
>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel