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

[Laszlo Ersek <lersek@redhat.com>]

On 03/21/17 09:41, Zeng, Star wrote:
> It is not been implemented yet. Just for our discussion. I should use future tense in the sentence, sorry.

Ah, I see. Yeah, it makes sense.

(Although it still wouldn't allow us to avoid a NULL class library
plug-in for AcpiTableDxe, for setting the PCD just in time -- the
information source for the PCD is generally available to us only within
the DXE phase, not in PEI.)

Thanks
Laszlo

> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Tuesday, March 21, 2017 4:38 PM
> To: Zeng, Star <star.zeng@intel.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
> 
> On 03/21/17 03:27, Zeng, Star wrote:
>> 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.
> 
> Hm, I don't see that:
> 
> $ git grep EFI_UNSUPPORTED -- MdeModulePkg/Universal/Acpi/AcpiTableDxe/
> [no matches]
> 
> Thanks
> Laszlo
> 
>>
>>
>> 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|0x00000
>>> 0
>>> 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
>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>