1. Patchew
  2. EDK2
  3. [edk2] [PATCH v2 00/12] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI
  4. View patch

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

Laszlo Ersek posted 12 patches 7 years, 7 months ago
There is a newer version of this series
[edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
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|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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Leif Lindholm 7 years, 7 months ago 
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.

MdeModulePkg?

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
> 
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
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
>>
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Leif Lindholm 7 years, 7 months ago 
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.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
> >>
> >>
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
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.
> 
> MdeModulePkg?

It looks like Ard is okay with this series after all, and you (Leif)
also seem to tolerate it. The question is now if the MdeModulePkg owners
are okay to take this patch for MdeModulePkg.

Star, Feng, can I add the protocol definition and the library instance
in this patch to MdeModulePkg? (We've ruled out the dynamic PCD
discussed previously for AcpiTableDxe, because platforms would have even
greater trouble setting such a PCD than using this protocol.)

Can you guys please give a decisive answer in one or two days?

Thank you,
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
>>
>>
> _______________________________________________
> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Zeng, Star 7 years, 7 months ago 
I prefer to do not include the protocol definition and the library instance into MdeModulePkg at this moment until they need to be used by multiple platforms/archs.

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, March 22, 2017 11:02 PM
To: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.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/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.
> 
> MdeModulePkg?

It looks like Ard is okay with this series after all, and you (Leif) also seem to tolerate it. The question is now if the MdeModulePkg owners are okay to take this patch for MdeModulePkg.

Star, Feng, can I add the protocol definition and the library instance in this patch to MdeModulePkg? (We've ruled out the dynamic PCD discussed previously for AcpiTableDxe, because platforms would have even greater trouble setting such a PCD than using this protocol.)

Can you guys please give a decisive answer in one or two days?

Thank you,
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|0x00000
>> 001
>>  
>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
> I prefer to do not include the protocol definition and the library instance into MdeModulePkg at this moment until they need to be used by multiple platforms/archs.
>

I disagree. Nowhere in the PI or UEFI spec is there any mention (for
any architecture) that ACPI is mandatory. Given that EDK2 is the
reference platform UEFI/PI, and not for ACPI or the PC/x86 platform, I
think it is unreasonable to have lots and lots of PC quirks (i.e.,
allocations below 4 GB) in MdeModulePkg, but then disallow a clean
approach to make ACPI selectable, in a way that is true to the spirit
of PI (i.e., using protocol dependencies)

So I think at least the protocol definitions belong in MdeModulePkg.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Zeng, Star 7 years, 7 months ago 
I just think it seems a generic problem.
Some platforms may dynamically determine whether ACPI should be supported or not.
Some platforms may dynamically determine whether SMBIOS should be supported or not.
Some platforms may dynamically determine whether HII should be supported or not.
...

We are thinking whether we can have a generic NULL instance to support all this kind of cases, for example:
1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD point to a depex GUID.
2. Implement a NULL instance DepexLib.
[Defines]
  INF_VERSION                    = 0x00010005
  BASE_NAME                      = DepexLib
  FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
  MODULE_TYPE                    = BASE
  VERSION_STRING                 = 1.0
  LIBRARY_CLASS                  = NULL

[Sources]
  DepexLib.c

[Packages]
  XXXPkg/XXXPkg.dec

[Depex]
  PcdDepexGuid

3. Link NULL instance and configure PCD in dsc.
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
    <LibraryClasses>
      NULL|XXXPkg/Library/DepexLib/DepexLib.inf
    <PcdsFixedAtBuild>
      PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
}
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
    <LibraryClasses>
      NULL|XXXPkg/Library/DepexLib/DepexLib.inf
    <PcdsFixedAtBuild>
      PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
}

But current BaseTools does not support the syntax to declare PCD in [Depex] section of inf yet.

Based on the statements above, I have below comments.
1. I agree to add the GUID into MdeModulePkg, but how about using gEdkiiPlatformHasAcpiGuid instead of gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids] section of MdeModulePkg.dec? As Platform can install gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER. And another, no need to add a include file PlatformHasAcpi.h as BaseTools supports to add the GUID definitions to AutoGen files from package dec.
2. You can file bugzilla bug to request BaseTools syntax support to declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib could be finally added into core package MdeModulePkg or even MdePkg (I prefer). Before that, how about implementing the PlatformHasAcpiLib in none core package?


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, March 23, 2017 5:09 PM
To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library

On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
> I prefer to do not include the protocol definition and the library instance into MdeModulePkg at this moment until they need to be used by multiple platforms/archs.
>

I disagree. Nowhere in the PI or UEFI spec is there any mention (for any architecture) that ACPI is mandatory. Given that EDK2 is the reference platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in MdeModulePkg, but then disallow a clean approach to make ACPI selectable, in a way that is true to the spirit of PI (i.e., using protocol dependencies)

So I think at least the protocol definitions belong in MdeModulePkg.

Thanks,
Ard.
_______________________________________________
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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 24 March 2017 at 03:44, Zeng, Star <star.zeng@intel.com> wrote:
> I just think it seems a generic problem.
> Some platforms may dynamically determine whether ACPI should be supported or not.
> Some platforms may dynamically determine whether SMBIOS should be supported or not.
> Some platforms may dynamically determine whether HII should be supported or not.
> ...
>
> We are thinking whether we can have a generic NULL instance to support all this kind of cases, for example:
> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD point to a depex GUID.
> 2. Implement a NULL instance DepexLib.
> [Defines]
>   INF_VERSION                    = 0x00010005
>   BASE_NAME                      = DepexLib
>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>   MODULE_TYPE                    = BASE
>   VERSION_STRING                 = 1.0
>   LIBRARY_CLASS                  = NULL
>
> [Sources]
>   DepexLib.c
>
> [Packages]
>   XXXPkg/XXXPkg.dec
>
> [Depex]
>   PcdDepexGuid
>
> 3. Link NULL instance and configure PCD in dsc.
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>     <LibraryClasses>
>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>     <PcdsFixedAtBuild>
>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
> }
> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>     <LibraryClasses>
>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>     <PcdsFixedAtBuild>
>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
> }
>
> But current BaseTools does not support the syntax to declare PCD in [Depex] section of inf yet.
>
> Based on the statements above, I have below comments.
> 1. I agree to add the GUID into MdeModulePkg, but how about using gEdkiiPlatformHasAcpiGuid instead of gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids] section of MdeModulePkg.dec? As Platform can install gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER. And another, no need to add a include file PlatformHasAcpi.h as BaseTools supports to add the GUID definitions to AutoGen files from package dec.
> 2. You can file bugzilla bug to request BaseTools syntax support to declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib could be finally added into core package MdeModulePkg or even MdePkg (I prefer). Before that, how about implementing the PlatformHasAcpiLib in none core package?
>

Hello Star,

Thanks for taking the time to think about this. It is much appreciated.

I like the generic approach for dynamic dependencies. The more
architectures are supported in UEFI, the more often we are likely to
see the need for such things.

I will let Laszlo speak for himself, as the author of the patches, but
I am perfectly fine with
a) the outline above of how we address this in the long term,
b) taking anything we need for the short term into ArmPkg, and move it
to MdeModulePkg and/or MdePkg later.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/24/17 08:15, Ard Biesheuvel wrote:
> On 24 March 2017 at 03:44, Zeng, Star <star.zeng@intel.com> wrote:
>> I just think it seems a generic problem.
>> Some platforms may dynamically determine whether ACPI should be supported or not.
>> Some platforms may dynamically determine whether SMBIOS should be supported or not.
>> Some platforms may dynamically determine whether HII should be supported or not.
>> ...
>>
>> We are thinking whether we can have a generic NULL instance to support all this kind of cases, for example:
>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD point to a depex GUID.
>> 2. Implement a NULL instance DepexLib.
>> [Defines]
>>   INF_VERSION                    = 0x00010005
>>   BASE_NAME                      = DepexLib
>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>   MODULE_TYPE                    = BASE
>>   VERSION_STRING                 = 1.0
>>   LIBRARY_CLASS                  = NULL
>>
>> [Sources]
>>   DepexLib.c
>>
>> [Packages]
>>   XXXPkg/XXXPkg.dec
>>
>> [Depex]
>>   PcdDepexGuid
>>
>> 3. Link NULL instance and configure PCD in dsc.
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>     <LibraryClasses>
>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>     <PcdsFixedAtBuild>
>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>> }
>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>     <LibraryClasses>
>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>     <PcdsFixedAtBuild>
>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>> }
>>
>> But current BaseTools does not support the syntax to declare PCD in [Depex] section of inf yet.
>>
>> Based on the statements above, I have below comments.
>> 1. I agree to add the GUID into MdeModulePkg, but how about using gEdkiiPlatformHasAcpiGuid instead of gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids] section of MdeModulePkg.dec? As Platform can install gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER. And another, no need to add a include file PlatformHasAcpi.h as BaseTools supports to add the GUID definitions to AutoGen files from package dec.
>> 2. You can file bugzilla bug to request BaseTools syntax support to declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib could be finally added into core package MdeModulePkg or even MdePkg (I prefer). Before that, how about implementing the PlatformHasAcpiLib in none core package?
>>
> 
> Hello Star,
> 
> Thanks for taking the time to think about this. It is much appreciated.
> 
> I like the generic approach for dynamic dependencies. The more
> architectures are supported in UEFI, the more often we are likely to
> see the need for such things.
> 
> I will let Laszlo speak for himself, as the author of the patches, but
> I am perfectly fine with
> a) the outline above of how we address this in the long term,
> b) taking anything we need for the short term into ArmPkg, and move it
> to MdeModulePkg and/or MdePkg later.

To confirm what Leif requested and what we've since discussed on IRC:
since the Has-Acpi GUID is now going under MdeModulePkg, I'll move the
Has-DeviceTree GUID under EmbeddedPkg.

Thanks
Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 24 March 2017 at 17:10, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/24/17 08:15, Ard Biesheuvel wrote:
>> On 24 March 2017 at 03:44, Zeng, Star <star.zeng@intel.com> wrote:
>>> I just think it seems a generic problem.
>>> Some platforms may dynamically determine whether ACPI should be supported or not.
>>> Some platforms may dynamically determine whether SMBIOS should be supported or not.
>>> Some platforms may dynamically determine whether HII should be supported or not.
>>> ...
>>>
>>> We are thinking whether we can have a generic NULL instance to support all this kind of cases, for example:
>>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD point to a depex GUID.
>>> 2. Implement a NULL instance DepexLib.
>>> [Defines]
>>>   INF_VERSION                    = 0x00010005
>>>   BASE_NAME                      = DepexLib
>>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>>   MODULE_TYPE                    = BASE
>>>   VERSION_STRING                 = 1.0
>>>   LIBRARY_CLASS                  = NULL
>>>
>>> [Sources]
>>>   DepexLib.c
>>>
>>> [Packages]
>>>   XXXPkg/XXXPkg.dec
>>>
>>> [Depex]
>>>   PcdDepexGuid
>>>
>>> 3. Link NULL instance and configure PCD in dsc.
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>>     <LibraryClasses>
>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>     <PcdsFixedAtBuild>
>>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>>> }
>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>>     <LibraryClasses>
>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>     <PcdsFixedAtBuild>
>>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>>> }
>>>
>>> But current BaseTools does not support the syntax to declare PCD in [Depex] section of inf yet.
>>>
>>> Based on the statements above, I have below comments.
>>> 1. I agree to add the GUID into MdeModulePkg, but how about using gEdkiiPlatformHasAcpiGuid instead of gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids] section of MdeModulePkg.dec? As Platform can install gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER. And another, no need to add a include file PlatformHasAcpi.h as BaseTools supports to add the GUID definitions to AutoGen files from package dec.
>>> 2. You can file bugzilla bug to request BaseTools syntax support to declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib could be finally added into core package MdeModulePkg or even MdePkg (I prefer). Before that, how about implementing the PlatformHasAcpiLib in none core package?
>>>
>>
>> Hello Star,
>>
>> Thanks for taking the time to think about this. It is much appreciated.
>>
>> I like the generic approach for dynamic dependencies. The more
>> architectures are supported in UEFI, the more often we are likely to
>> see the need for such things.
>>
>> I will let Laszlo speak for himself, as the author of the patches, but
>> I am perfectly fine with
>> a) the outline above of how we address this in the long term,
>> b) taking anything we need for the short term into ArmPkg, and move it
>> to MdeModulePkg and/or MdePkg later.
>
> To confirm what Leif requested and what we've since discussed on IRC:
> since the Has-Acpi GUID is now going under MdeModulePkg, I'll move the
> Has-DeviceTree GUID under EmbeddedPkg.
>

Ack
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/24/17 04:44, Zeng, Star wrote:
> I just think it seems a generic problem.
> Some platforms may dynamically determine whether ACPI should be supported or not.
> Some platforms may dynamically determine whether SMBIOS should be supported or not.
> Some platforms may dynamically determine whether HII should be supported or not.
> ...
> 
> We are thinking whether we can have a generic NULL instance to support all this kind of cases, for example:
> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD point to a depex GUID.
> 2. Implement a NULL instance DepexLib.
> [Defines]
>   INF_VERSION                    = 0x00010005
>   BASE_NAME                      = DepexLib
>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>   MODULE_TYPE                    = BASE
>   VERSION_STRING                 = 1.0
>   LIBRARY_CLASS                  = NULL
> 
> [Sources]
>   DepexLib.c
> 
> [Packages]
>   XXXPkg/XXXPkg.dec
> 
> [Depex]
>   PcdDepexGuid
> 
> 3. Link NULL instance and configure PCD in dsc.
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>     <LibraryClasses>
>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>     <PcdsFixedAtBuild>
>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
> }
> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>     <LibraryClasses>
>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>     <PcdsFixedAtBuild>
>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
> }
> 
> But current BaseTools does not support the syntax to declare PCD in [Depex] section of inf yet.
> 
> Based on the statements above, I have below comments.

> 1. I agree to add the GUID into MdeModulePkg, but how about using
> gEdkiiPlatformHasAcpiGuid instead of
> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
> section of MdeModulePkg.dec? As Platform can install
> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.

Sounds reasonable, I'll do this.

> And another, no
> need to add a include file PlatformHasAcpi.h as BaseTools supports to
> add the GUID definitions to AutoGen files from package dec.

I disagree with this. I mean, I *personally* wouldn't mind, but this
would be inconsistent with existing MdeModulePkg practice. For example, see

MdeModulePkg/Include/Guid/TtyTerm.h

Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no data
structures associated with it; the header file only has an extern
declaration, and -- importantly -- an array initializer macro called
EFI_TTY_TERM_GUID, which can be used in the following syntax:

STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;

So, I think that the GUID header file is required. I will add it in v3.

Once we generalize this idea to the design that you laid out above, we
can perhaps eliminate the header file. But, for now, I think we should
remain consistent with other MdeModulePkg GUID definitions.

> 2. You can file bugzilla bug to request BaseTools syntax support to
> declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib
> could be finally added into core package MdeModulePkg or even MdePkg
> (I prefer).

I filed:

https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec

> Before that, how about implementing the
> PlatformHasAcpiLib in none core package?

Works for me; I'll split that part off and keep it under ArmPkg.

Thanks
Laszlo

> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Thursday, March 23, 2017 5:09 PM
> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
> 
> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>> I prefer to do not include the protocol definition and the library instance into MdeModulePkg at this moment until they need to be used by multiple platforms/archs.
>>
> 
> I disagree. Nowhere in the PI or UEFI spec is there any mention (for any architecture) that ACPI is mandatory. Given that EDK2 is the reference platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in MdeModulePkg, but then disallow a clean approach to make ACPI selectable, in a way that is true to the spirit of PI (i.e., using protocol dependencies)
> 
> So I think at least the protocol definitions belong in MdeModulePkg.
> 
> Thanks,
> Ard.
> _______________________________________________
> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Gao, Liming 7 years, 7 months ago 
Laszlo:
  On GUID header file, some header file are here, because they are added before BaseTools supports it. Now, we allow not to add header file if the header file only has GUID value definition. 
  On GUID C MACRO, we suggest to use GuidCName in every C source code. So, we don't suggest to add it.  As you know, some existing header files have GuidCName and GUID Macro. Now, we have no plan to clean up the existing one. But, we expect new added file follows this rule. 

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Saturday, March 25, 2017 1:09 AM
>To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>; Kinney, Michael D
><michael.d.kinney@intel.com>; afish@apple.com
>Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>;
>edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
><leif.lindholm@linaro.org>
>Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>ACPI Protocol, and plug-in library
>
>On 03/24/17 04:44, Zeng, Star wrote:
>> I just think it seems a generic problem.
>> Some platforms may dynamically determine whether ACPI should be
>supported or not.
>> Some platforms may dynamically determine whether SMBIOS should be
>supported or not.
>> Some platforms may dynamically determine whether HII should be
>supported or not.
>> ...
>>
>> We are thinking whether we can have a generic NULL instance to support all
>this kind of cases, for example:
>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD
>point to a depex GUID.
>> 2. Implement a NULL instance DepexLib.
>> [Defines]
>>   INF_VERSION                    = 0x00010005
>>   BASE_NAME                      = DepexLib
>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>   MODULE_TYPE                    = BASE
>>   VERSION_STRING                 = 1.0
>>   LIBRARY_CLASS                  = NULL
>>
>> [Sources]
>>   DepexLib.c
>>
>> [Packages]
>>   XXXPkg/XXXPkg.dec
>>
>> [Depex]
>>   PcdDepexGuid
>>
>> 3. Link NULL instance and configure PCD in dsc.
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>     <LibraryClasses>
>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>     <PcdsFixedAtBuild>
>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>> }
>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>     <LibraryClasses>
>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>     <PcdsFixedAtBuild>
>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>> }
>>
>> But current BaseTools does not support the syntax to declare PCD in [Depex]
>section of inf yet.
>>
>> Based on the statements above, I have below comments.
>
>> 1. I agree to add the GUID into MdeModulePkg, but how about using
>> gEdkiiPlatformHasAcpiGuid instead of
>> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
>> section of MdeModulePkg.dec? As Platform can install
>> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
>> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.
>
>Sounds reasonable, I'll do this.
>
>> And another, no
>> need to add a include file PlatformHasAcpi.h as BaseTools supports to
>> add the GUID definitions to AutoGen files from package dec.
>
>I disagree with this. I mean, I *personally* wouldn't mind, but this
>would be inconsistent with existing MdeModulePkg practice. For example,
>see
>
>MdeModulePkg/Include/Guid/TtyTerm.h
>
>Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no data
>structures associated with it; the header file only has an extern
>declaration, and -- importantly -- an array initializer macro called
>EFI_TTY_TERM_GUID, which can be used in the following syntax:
>
>STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;
>
>So, I think that the GUID header file is required. I will add it in v3.
>
>Once we generalize this idea to the design that you laid out above, we
>can perhaps eliminate the header file. But, for now, I think we should
>remain consistent with other MdeModulePkg GUID definitions.
>
>> 2. You can file bugzilla bug to request BaseTools syntax support to
>> declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib
>> could be finally added into core package MdeModulePkg or even MdePkg
>> (I prefer).
>
>I filed:
>
>https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
>https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec
>
>> Before that, how about implementing the
>> PlatformHasAcpiLib in none core package?
>
>Works for me; I'll split that part off and keep it under ArmPkg.
>
>Thanks
>Laszlo
>
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Ard Biesheuvel
>> Sent: Thursday, March 23, 2017 5:09 PM
>> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; afish@apple.com
>> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
><leif.lindholm@linaro.org>
>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>ACPI Protocol, and plug-in library
>>
>> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>>> I prefer to do not include the protocol definition and the library instance
>into MdeModulePkg at this moment until they need to be used by multiple
>platforms/archs.
>>>
>>
>> I disagree. Nowhere in the PI or UEFI spec is there any mention (for any
>architecture) that ACPI is mandatory. Given that EDK2 is the reference
>platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is
>unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in
>MdeModulePkg, but then disallow a clean approach to make ACPI selectable,
>in a way that is true to the spirit of PI (i.e., using protocol dependencies)
>>
>> So I think at least the protocol definitions belong in MdeModulePkg.
>>
>> Thanks,
>> Ard.
>> _______________________________________________
>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
> Laszlo:
>   On GUID header file, some header file are here, because they are added before BaseTools supports it. Now, we allow not to add header file if the header file only has GUID value definition.

I have to agree with Laszlo here. The BaseTools support is incomplete,
since it does not add a #define for the GUID to AutoGen.h. This makes
it impossible to initialize static structures containing GUIDs, such
as templates containing vendor device paths.

For instance, the following

typedef struct {
  EFI_GUID foo;
} TYPE;

TYPE mFoo {
  SOME_GUID
};

is not possible without a Guid/xxx.h include file containing a #define
for SOME_GUID.

>   On GUID C MACRO, we suggest to use GuidCName in every C source code. So, we don't suggest to add it.  As you know, some existing header files have GuidCName and GUID Macro. Now, we have no plan to clean up the existing one. But, we expect new added file follows this rule.
>
> Thanks
> Liming
>>-----Original Message-----
>>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>Laszlo Ersek
>>Sent: Saturday, March 25, 2017 1:09 AM
>>To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>><ard.biesheuvel@linaro.org>; Kinney, Michael D
>><michael.d.kinney@intel.com>; afish@apple.com
>>Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>;
>>edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>><leif.lindholm@linaro.org>
>>Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>>ACPI Protocol, and plug-in library
>>
>>On 03/24/17 04:44, Zeng, Star wrote:
>>> I just think it seems a generic problem.
>>> Some platforms may dynamically determine whether ACPI should be
>>supported or not.
>>> Some platforms may dynamically determine whether SMBIOS should be
>>supported or not.
>>> Some platforms may dynamically determine whether HII should be
>>supported or not.
>>> ...
>>>
>>> We are thinking whether we can have a generic NULL instance to support all
>>this kind of cases, for example:
>>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD
>>point to a depex GUID.
>>> 2. Implement a NULL instance DepexLib.
>>> [Defines]
>>>   INF_VERSION                    = 0x00010005
>>>   BASE_NAME                      = DepexLib
>>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>>   MODULE_TYPE                    = BASE
>>>   VERSION_STRING                 = 1.0
>>>   LIBRARY_CLASS                  = NULL
>>>
>>> [Sources]
>>>   DepexLib.c
>>>
>>> [Packages]
>>>   XXXPkg/XXXPkg.dec
>>>
>>> [Depex]
>>>   PcdDepexGuid
>>>
>>> 3. Link NULL instance and configure PCD in dsc.
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>>     <LibraryClasses>
>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>     <PcdsFixedAtBuild>
>>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>>> }
>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>>     <LibraryClasses>
>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>     <PcdsFixedAtBuild>
>>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>>> }
>>>
>>> But current BaseTools does not support the syntax to declare PCD in [Depex]
>>section of inf yet.
>>>
>>> Based on the statements above, I have below comments.
>>
>>> 1. I agree to add the GUID into MdeModulePkg, but how about using
>>> gEdkiiPlatformHasAcpiGuid instead of
>>> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
>>> section of MdeModulePkg.dec? As Platform can install
>>> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
>>> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.
>>
>>Sounds reasonable, I'll do this.
>>
>>> And another, no
>>> need to add a include file PlatformHasAcpi.h as BaseTools supports to
>>> add the GUID definitions to AutoGen files from package dec.
>>
>>I disagree with this. I mean, I *personally* wouldn't mind, but this
>>would be inconsistent with existing MdeModulePkg practice. For example,
>>see
>>
>>MdeModulePkg/Include/Guid/TtyTerm.h
>>
>>Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no data
>>structures associated with it; the header file only has an extern
>>declaration, and -- importantly -- an array initializer macro called
>>EFI_TTY_TERM_GUID, which can be used in the following syntax:
>>
>>STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;
>>
>>So, I think that the GUID header file is required. I will add it in v3.
>>
>>Once we generalize this idea to the design that you laid out above, we
>>can perhaps eliminate the header file. But, for now, I think we should
>>remain consistent with other MdeModulePkg GUID definitions.
>>
>>> 2. You can file bugzilla bug to request BaseTools syntax support to
>>> declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib
>>> could be finally added into core package MdeModulePkg or even MdePkg
>>> (I prefer).
>>
>>I filed:
>>
>>https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
>>https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec
>>
>>> Before that, how about implementing the
>>> PlatformHasAcpiLib in none core package?
>>
>>Works for me; I'll split that part off and keep it under ArmPkg.
>>
>>Thanks
>>Laszlo
>>
>>>
>>>
>>> Thanks,
>>> Star
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>Ard Biesheuvel
>>> Sent: Thursday, March 23, 2017 5:09 PM
>>> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>><michael.d.kinney@intel.com>; afish@apple.com
>>> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>><leif.lindholm@linaro.org>
>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>>ACPI Protocol, and plug-in library
>>>
>>> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>>>> I prefer to do not include the protocol definition and the library instance
>>into MdeModulePkg at this moment until they need to be used by multiple
>>platforms/archs.
>>>>
>>>
>>> I disagree. Nowhere in the PI or UEFI spec is there any mention (for any
>>architecture) that ACPI is mandatory. Given that EDK2 is the reference
>>platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is
>>unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in
>>MdeModulePkg, but then disallow a clean approach to make ACPI selectable,
>>in a way that is true to the spirit of PI (i.e., using protocol dependencies)
>>>
>>> So I think at least the protocol definitions belong in MdeModulePkg.
>>>
>>> Thanks,
>>> Ard.
>>> _______________________________________________
>>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/27/17 09:00, Ard Biesheuvel wrote:
> On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
>> Laszlo:
>>   On GUID header file, some header file are here, because they are added before BaseTools supports it. Now, we allow not to add header file if the header file only has GUID value definition.
> 
> I have to agree with Laszlo here. The BaseTools support is incomplete,
> since it does not add a #define for the GUID to AutoGen.h. This makes
> it impossible to initialize static structures containing GUIDs, such
> as templates containing vendor device paths.
> 
> For instance, the following
> 
> typedef struct {
>   EFI_GUID foo;
> } TYPE;
> 
> TYPE mFoo {
>   SOME_GUID
> };
> 
> is not possible without a Guid/xxx.h include file containing a #define
> for SOME_GUID.

I wonder if we can commit this series before end of April.

Or is that too soon? End of May perhaps?

The mind boggles.

Laszlo

> 
>>   On GUID C MACRO, we suggest to use GuidCName in every C source code. So, we don't suggest to add it.  As you know, some existing header files have GuidCName and GUID Macro. Now, we have no plan to clean up the existing one. But, we expect new added file follows this rule.
>>
>> Thanks
>> Liming
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Saturday, March 25, 2017 1:09 AM
>>> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; afish@apple.com
>>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>;
>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>> <leif.lindholm@linaro.org>
>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>>> ACPI Protocol, and plug-in library
>>>
>>> On 03/24/17 04:44, Zeng, Star wrote:
>>>> I just think it seems a generic problem.
>>>> Some platforms may dynamically determine whether ACPI should be
>>> supported or not.
>>>> Some platforms may dynamically determine whether SMBIOS should be
>>> supported or not.
>>>> Some platforms may dynamically determine whether HII should be
>>> supported or not.
>>>> ...
>>>>
>>>> We are thinking whether we can have a generic NULL instance to support all
>>> this kind of cases, for example:
>>>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD
>>> point to a depex GUID.
>>>> 2. Implement a NULL instance DepexLib.
>>>> [Defines]
>>>>   INF_VERSION                    = 0x00010005
>>>>   BASE_NAME                      = DepexLib
>>>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>>>   MODULE_TYPE                    = BASE
>>>>   VERSION_STRING                 = 1.0
>>>>   LIBRARY_CLASS                  = NULL
>>>>
>>>> [Sources]
>>>>   DepexLib.c
>>>>
>>>> [Packages]
>>>>   XXXPkg/XXXPkg.dec
>>>>
>>>> [Depex]
>>>>   PcdDepexGuid
>>>>
>>>> 3. Link NULL instance and configure PCD in dsc.
>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>>>     <LibraryClasses>
>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>     <PcdsFixedAtBuild>
>>>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>>>> }
>>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>>>     <LibraryClasses>
>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>     <PcdsFixedAtBuild>
>>>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>>>> }
>>>>
>>>> But current BaseTools does not support the syntax to declare PCD in [Depex]
>>> section of inf yet.
>>>>
>>>> Based on the statements above, I have below comments.
>>>
>>>> 1. I agree to add the GUID into MdeModulePkg, but how about using
>>>> gEdkiiPlatformHasAcpiGuid instead of
>>>> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
>>>> section of MdeModulePkg.dec? As Platform can install
>>>> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
>>>> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.
>>>
>>> Sounds reasonable, I'll do this.
>>>
>>>> And another, no
>>>> need to add a include file PlatformHasAcpi.h as BaseTools supports to
>>>> add the GUID definitions to AutoGen files from package dec.
>>>
>>> I disagree with this. I mean, I *personally* wouldn't mind, but this
>>> would be inconsistent with existing MdeModulePkg practice. For example,
>>> see
>>>
>>> MdeModulePkg/Include/Guid/TtyTerm.h
>>>
>>> Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no data
>>> structures associated with it; the header file only has an extern
>>> declaration, and -- importantly -- an array initializer macro called
>>> EFI_TTY_TERM_GUID, which can be used in the following syntax:
>>>
>>> STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;
>>>
>>> So, I think that the GUID header file is required. I will add it in v3.
>>>
>>> Once we generalize this idea to the design that you laid out above, we
>>> can perhaps eliminate the header file. But, for now, I think we should
>>> remain consistent with other MdeModulePkg GUID definitions.
>>>
>>>> 2. You can file bugzilla bug to request BaseTools syntax support to
>>>> declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib
>>>> could be finally added into core package MdeModulePkg or even MdePkg
>>>> (I prefer).
>>>
>>> I filed:
>>>
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec
>>>
>>>> Before that, how about implementing the
>>>> PlatformHasAcpiLib in none core package?
>>>
>>> Works for me; I'll split that part off and keep it under ArmPkg.
>>>
>>> Thanks
>>> Laszlo
>>>
>>>>
>>>>
>>>> Thanks,
>>>> Star
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Ard Biesheuvel
>>>> Sent: Thursday, March 23, 2017 5:09 PM
>>>> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; afish@apple.com
>>>> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>> <leif.lindholm@linaro.org>
>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>>> ACPI Protocol, and plug-in library
>>>>
>>>> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>>>>> I prefer to do not include the protocol definition and the library instance
>>> into MdeModulePkg at this moment until they need to be used by multiple
>>> platforms/archs.
>>>>>
>>>>
>>>> I disagree. Nowhere in the PI or UEFI spec is there any mention (for any
>>> architecture) that ACPI is mandatory. Given that EDK2 is the reference
>>> platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is
>>> unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in
>>> MdeModulePkg, but then disallow a clean approach to make ACPI selectable,
>>> in a way that is true to the spirit of PI (i.e., using protocol dependencies)
>>>>
>>>> So I think at least the protocol definitions belong in MdeModulePkg.
>>>>
>>>> Thanks,
>>>> Ard.
>>>> _______________________________________________
>>>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 27 March 2017 at 18:31, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/27/17 09:00, Ard Biesheuvel wrote:
>> On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
>>> Laszlo:
>>>   On GUID header file, some header file are here, because they are added before BaseTools supports it. Now, we allow not to add header file if the header file only has GUID value definition.
>>
>> I have to agree with Laszlo here. The BaseTools support is incomplete,
>> since it does not add a #define for the GUID to AutoGen.h. This makes
>> it impossible to initialize static structures containing GUIDs, such
>> as templates containing vendor device paths.
>>
>> For instance, the following
>>
>> typedef struct {
>>   EFI_GUID foo;
>> } TYPE;
>>
>> TYPE mFoo {
>>   SOME_GUID
>> };
>>
>> is not possible without a Guid/xxx.h include file containing a #define
>> for SOME_GUID.
>
> I wonder if we can commit this series before end of April.
>
> Or is that too soon? End of May perhaps?
>
> The mind boggles.
>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Gao, Liming 7 years, 7 months ago 
Ersek:
  I don't want to block your patch. You can still check in Guid header file if you think it is necessary. 

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, March 28, 2017 1:32 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
> <feng.tian@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
> 
> On 03/27/17 09:00, Ard Biesheuvel wrote:
> > On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
> >> Laszlo:
> >>   On GUID header file, some header file are here, because they are added before BaseTools supports it. Now, we allow not to add
> header file if the header file only has GUID value definition.
> >
> > I have to agree with Laszlo here. The BaseTools support is incomplete,
> > since it does not add a #define for the GUID to AutoGen.h. This makes
> > it impossible to initialize static structures containing GUIDs, such
> > as templates containing vendor device paths.
> >
> > For instance, the following
> >
> > typedef struct {
> >   EFI_GUID foo;
> > } TYPE;
> >
> > TYPE mFoo {
> >   SOME_GUID
> > };
> >
> > is not possible without a Guid/xxx.h include file containing a #define
> > for SOME_GUID.
> 
> I wonder if we can commit this series before end of April.
> 
> Or is that too soon? End of May perhaps?
> 
> The mind boggles.
> 
> Laszlo
> 
> >
> >>   On GUID C MACRO, we suggest to use GuidCName in every C source code. So, we don't suggest to add it.  As you know, some
> existing header files have GuidCName and GUID Macro. Now, we have no plan to clean up the existing one. But, we expect new added
> file follows this rule.
> >>
> >> Thanks
> >> Liming
> >>> -----Original Message-----
> >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >>> Laszlo Ersek
> >>> Sent: Saturday, March 25, 2017 1:09 AM
> >>> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
> >>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
> >>> <michael.d.kinney@intel.com>; afish@apple.com
> >>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>;
> >>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
> >>> <leif.lindholm@linaro.org>
> >>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
> >>> ACPI Protocol, and plug-in library
> >>>
> >>> On 03/24/17 04:44, Zeng, Star wrote:
> >>>> I just think it seems a generic problem.
> >>>> Some platforms may dynamically determine whether ACPI should be
> >>> supported or not.
> >>>> Some platforms may dynamically determine whether SMBIOS should be
> >>> supported or not.
> >>>> Some platforms may dynamically determine whether HII should be
> >>> supported or not.
> >>>> ...
> >>>>
> >>>> We are thinking whether we can have a generic NULL instance to support all
> >>> this kind of cases, for example:
> >>>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD
> >>> point to a depex GUID.
> >>>> 2. Implement a NULL instance DepexLib.
> >>>> [Defines]
> >>>>   INF_VERSION                    = 0x00010005
> >>>>   BASE_NAME                      = DepexLib
> >>>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
> >>>>   MODULE_TYPE                    = BASE
> >>>>   VERSION_STRING                 = 1.0
> >>>>   LIBRARY_CLASS                  = NULL
> >>>>
> >>>> [Sources]
> >>>>   DepexLib.c
> >>>>
> >>>> [Packages]
> >>>>   XXXPkg/XXXPkg.dec
> >>>>
> >>>> [Depex]
> >>>>   PcdDepexGuid
> >>>>
> >>>> 3. Link NULL instance and configure PCD in dsc.
> >>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
> >>>>     <LibraryClasses>
> >>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
> >>>>     <PcdsFixedAtBuild>
> >>>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
> >>>> }
> >>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
> >>>>     <LibraryClasses>
> >>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
> >>>>     <PcdsFixedAtBuild>
> >>>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
> >>>> }
> >>>>
> >>>> But current BaseTools does not support the syntax to declare PCD in [Depex]
> >>> section of inf yet.
> >>>>
> >>>> Based on the statements above, I have below comments.
> >>>
> >>>> 1. I agree to add the GUID into MdeModulePkg, but how about using
> >>>> gEdkiiPlatformHasAcpiGuid instead of
> >>>> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
> >>>> section of MdeModulePkg.dec? As Platform can install
> >>>> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
> >>>> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.
> >>>
> >>> Sounds reasonable, I'll do this.
> >>>
> >>>> And another, no
> >>>> need to add a include file PlatformHasAcpi.h as BaseTools supports to
> >>>> add the GUID definitions to AutoGen files from package dec.
> >>>
> >>> I disagree with this. I mean, I *personally* wouldn't mind, but this
> >>> would be inconsistent with existing MdeModulePkg practice. For example,
> >>> see
> >>>
> >>> MdeModulePkg/Include/Guid/TtyTerm.h
> >>>
> >>> Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no data
> >>> structures associated with it; the header file only has an extern
> >>> declaration, and -- importantly -- an array initializer macro called
> >>> EFI_TTY_TERM_GUID, which can be used in the following syntax:
> >>>
> >>> STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;
> >>>
> >>> So, I think that the GUID header file is required. I will add it in v3.
> >>>
> >>> Once we generalize this idea to the design that you laid out above, we
> >>> can perhaps eliminate the header file. But, for now, I think we should
> >>> remain consistent with other MdeModulePkg GUID definitions.
> >>>
> >>>> 2. You can file bugzilla bug to request BaseTools syntax support to
> >>>> declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib
> >>>> could be finally added into core package MdeModulePkg or even MdePkg
> >>>> (I prefer).
> >>>
> >>> I filed:
> >>>
> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec
> >>>
> >>>> Before that, how about implementing the
> >>>> PlatformHasAcpiLib in none core package?
> >>>
> >>> Works for me; I'll split that part off and keep it under ArmPkg.
> >>>
> >>> Thanks
> >>> Laszlo
> >>>
> >>>>
> >>>>
> >>>> Thanks,
> >>>> Star
> >>>> -----Original Message-----
> >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >>> Ard Biesheuvel
> >>>> Sent: Thursday, March 23, 2017 5:09 PM
> >>>> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
> >>> <michael.d.kinney@intel.com>; afish@apple.com
> >>>> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> >>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
> >>> <leif.lindholm@linaro.org>
> >>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
> >>> ACPI Protocol, and plug-in library
> >>>>
> >>>> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
> >>>>> I prefer to do not include the protocol definition and the library instance
> >>> into MdeModulePkg at this moment until they need to be used by multiple
> >>> platforms/archs.
> >>>>>
> >>>>
> >>>> I disagree. Nowhere in the PI or UEFI spec is there any mention (for any
> >>> architecture) that ACPI is mandatory. Given that EDK2 is the reference
> >>> platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is
> >>> unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in
> >>> MdeModulePkg, but then disallow a clean approach to make ACPI selectable,
> >>> in a way that is true to the spirit of PI (i.e., using protocol dependencies)
> >>>>
> >>>> So I think at least the protocol definitions belong in MdeModulePkg.
> >>>>
> >>>> Thanks,
> >>>> Ard.
> >>>> _______________________________________________
> >>>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/28/17 07:25, Gao, Liming wrote:
> Ersek:
>   I don't want to block your patch. You can still check in Guid header file if you think it is necessary. 

I don't want to check in without formal MdeModulePkg maintainer
approval. If I check in a patch without formal approval, that will only
lead to chaos. I don't want to circumvent the process; I want the
process to *work*.

If MdeModulePkg maintainers agree with my

  [PATCH v3 04/12] MdeModulePkg: introduce EDKII Platform Has ACPI GUID

then they should please give their Reviewed-by for it.

If they disagree with it, they should please explain why.

The specific argument you raised, namely that a BaseTools utility
generates the necessary C language artifacts for GUID use, is not
precise. BaseTools generate *some* artifacts, but they do not generate a
macro that is usable for initializing a GUID object of static storage
duration (= global variable GUID, or a GUID field in a global variable
structure).

This is what the ISO C99 standard says:

  6.7.8 Initialization

  [...]

  Constraints

  [...]

  4 All the expressions in an initializer for an object that has static
    storage duration shall be constant expressions or string literals.

  [...]

  16 Otherwise, the initializer for an object that has aggregate or
     union type shall be a brace-enclosed list of initializers for the
     elements or named members.

  [...]

  20 If the aggregate or union contains elements or members that are
     aggregates or unions, these rules apply recursively to the
     subaggregates or contained unions. [...]

This is the GUID type definition:

typedef struct {
  UINT32  Data1;
  UINT16  Data2;
  UINT16  Data3;
  UINT8   Data4[8];
} GUID;

For this structure, the above standardese implies that we have to
provide integer constant expressions in the initializer.

  6.6 Constant expressions

  [...]

  6 An /integer constant expression/ shall have integer type and shall
    only have operands that are integer constants, enumeration
    constants, character constants, /sizeof/ expressions whose results
    are integer constants, and floating constants that are the
    immediate operands of casts. Cast operators in an integer constant
    expression shall only convert arithmetic types to integer types,
    except as part of an operand to the /sizeof/ operator.

The end result is that "gEdkiiPlatformHasAcpiGuid" is not usable in such
contexts, but the EDKII_PLATFORM_HAS_ACPI_GUID macro is.


If you want, I can file a TianoCore feature request BZ for generating
such macros as well in BaseTools, but for now, I don't think it should
either block my patch noted above, or force me to drop the Guid/ header
file from the patch. Right now, the Guid/ header adds something that is
not available from BaseTools, and I wouldn't like to delay the v3 series
any longer.

Do you agree to the above method? I file a TianoCore Feature Request BZ
for BaseTools, and MdeModulePkg maintainers formally approve the v3
04/12 patch?


(Side point: I think the Guid/ header file has another benefit:
documentation. I don't think we can add the amount of documentation that
is acceptable in a standalone Guid/ header to an in-line comment in the
.DEC file. So, I think that the new rule for not adding Guid/ headers is
immature at this point -- and, worse, I don't recall an open
announcement or discussion about this rule, so that we could have raised
concerns before turning the proposal into an actual rule.)

Thanks
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, March 28, 2017 1:32 AM
>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>
>> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
>> <feng.tian@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm <leif.lindholm@linaro.org>
>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
>>
>> On 03/27/17 09:00, Ard Biesheuvel wrote:
>>> On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
>>>> Laszlo:
>>>>   On GUID header file, some header file are here, because they are added before BaseTools supports it. Now, we allow not to add
>> header file if the header file only has GUID value definition.
>>>
>>> I have to agree with Laszlo here. The BaseTools support is incomplete,
>>> since it does not add a #define for the GUID to AutoGen.h. This makes
>>> it impossible to initialize static structures containing GUIDs, such
>>> as templates containing vendor device paths.
>>>
>>> For instance, the following
>>>
>>> typedef struct {
>>>   EFI_GUID foo;
>>> } TYPE;
>>>
>>> TYPE mFoo {
>>>   SOME_GUID
>>> };
>>>
>>> is not possible without a Guid/xxx.h include file containing a #define
>>> for SOME_GUID.
>>
>> I wonder if we can commit this series before end of April.
>>
>> Or is that too soon? End of May perhaps?
>>
>> The mind boggles.
>>
>> Laszlo
>>
>>>
>>>>   On GUID C MACRO, we suggest to use GuidCName in every C source code. So, we don't suggest to add it.  As you know, some
>> existing header files have GuidCName and GUID Macro. Now, we have no plan to clean up the existing one. But, we expect new added
>> file follows this rule.
>>>>
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>>> Laszlo Ersek
>>>>> Sent: Saturday, March 25, 2017 1:09 AM
>>>>> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>; afish@apple.com
>>>>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>;
>>>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>>>> <leif.lindholm@linaro.org>
>>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>>>>> ACPI Protocol, and plug-in library
>>>>>
>>>>> On 03/24/17 04:44, Zeng, Star wrote:
>>>>>> I just think it seems a generic problem.
>>>>>> Some platforms may dynamically determine whether ACPI should be
>>>>> supported or not.
>>>>>> Some platforms may dynamically determine whether SMBIOS should be
>>>>> supported or not.
>>>>>> Some platforms may dynamically determine whether HII should be
>>>>> supported or not.
>>>>>> ...
>>>>>>
>>>>>> We are thinking whether we can have a generic NULL instance to support all
>>>>> this kind of cases, for example:
>>>>>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD
>>>>> point to a depex GUID.
>>>>>> 2. Implement a NULL instance DepexLib.
>>>>>> [Defines]
>>>>>>   INF_VERSION                    = 0x00010005
>>>>>>   BASE_NAME                      = DepexLib
>>>>>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>>>>>   MODULE_TYPE                    = BASE
>>>>>>   VERSION_STRING                 = 1.0
>>>>>>   LIBRARY_CLASS                  = NULL
>>>>>>
>>>>>> [Sources]
>>>>>>   DepexLib.c
>>>>>>
>>>>>> [Packages]
>>>>>>   XXXPkg/XXXPkg.dec
>>>>>>
>>>>>> [Depex]
>>>>>>   PcdDepexGuid
>>>>>>
>>>>>> 3. Link NULL instance and configure PCD in dsc.
>>>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>>>>>     <LibraryClasses>
>>>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>>>     <PcdsFixedAtBuild>
>>>>>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>>>>>> }
>>>>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>>>>>     <LibraryClasses>
>>>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>>>     <PcdsFixedAtBuild>
>>>>>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>>>>>> }
>>>>>>
>>>>>> But current BaseTools does not support the syntax to declare PCD in [Depex]
>>>>> section of inf yet.
>>>>>>
>>>>>> Based on the statements above, I have below comments.
>>>>>
>>>>>> 1. I agree to add the GUID into MdeModulePkg, but how about using
>>>>>> gEdkiiPlatformHasAcpiGuid instead of
>>>>>> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
>>>>>> section of MdeModulePkg.dec? As Platform can install
>>>>>> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
>>>>>> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.
>>>>>
>>>>> Sounds reasonable, I'll do this.
>>>>>
>>>>>> And another, no
>>>>>> need to add a include file PlatformHasAcpi.h as BaseTools supports to
>>>>>> add the GUID definitions to AutoGen files from package dec.
>>>>>
>>>>> I disagree with this. I mean, I *personally* wouldn't mind, but this
>>>>> would be inconsistent with existing MdeModulePkg practice. For example,
>>>>> see
>>>>>
>>>>> MdeModulePkg/Include/Guid/TtyTerm.h
>>>>>
>>>>> Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no data
>>>>> structures associated with it; the header file only has an extern
>>>>> declaration, and -- importantly -- an array initializer macro called
>>>>> EFI_TTY_TERM_GUID, which can be used in the following syntax:
>>>>>
>>>>> STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;
>>>>>
>>>>> So, I think that the GUID header file is required. I will add it in v3.
>>>>>
>>>>> Once we generalize this idea to the design that you laid out above, we
>>>>> can perhaps eliminate the header file. But, for now, I think we should
>>>>> remain consistent with other MdeModulePkg GUID definitions.
>>>>>
>>>>>> 2. You can file bugzilla bug to request BaseTools syntax support to
>>>>>> declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib
>>>>>> could be finally added into core package MdeModulePkg or even MdePkg
>>>>>> (I prefer).
>>>>>
>>>>> I filed:
>>>>>
>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec
>>>>>
>>>>>> Before that, how about implementing the
>>>>>> PlatformHasAcpiLib in none core package?
>>>>>
>>>>> Works for me; I'll split that part off and keep it under ArmPkg.
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Star
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>>> Ard Biesheuvel
>>>>>> Sent: Thursday, March 23, 2017 5:09 PM
>>>>>> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>; afish@apple.com
>>>>>> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>>>> <leif.lindholm@linaro.org>
>>>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>>>>> ACPI Protocol, and plug-in library
>>>>>>
>>>>>> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>>>>>>> I prefer to do not include the protocol definition and the library instance
>>>>> into MdeModulePkg at this moment until they need to be used by multiple
>>>>> platforms/archs.
>>>>>>>
>>>>>>
>>>>>> I disagree. Nowhere in the PI or UEFI spec is there any mention (for any
>>>>> architecture) that ACPI is mandatory. Given that EDK2 is the reference
>>>>> platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is
>>>>> unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in
>>>>> MdeModulePkg, but then disallow a clean approach to make ACPI selectable,
>>>>> in a way that is true to the spirit of PI (i.e., using protocol dependencies)
>>>>>>
>>>>>> So I think at least the protocol definitions belong in MdeModulePkg.
>>>>>>
>>>>>> Thanks,
>>>>>> Ard.
>>>>>> _______________________________________________
>>>>>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Gao, Liming 7 years, 7 months ago 
Laszlo:
  I agree GuidCName can't be used to initialize the global variable. If there is such requirement, GUID C MACRO will have to be defined. Then, its header file will be required. 
  Now, we have no rule to forbid to add the header file if it has no other definitions except for GUID value, and we have no such discussion to define those rules. It is the developer choice to add it or not. 
  Here, I want to mention that BaseTools has added "extern gGuidCName" into AutoGen header file. The consumer code can directly use gGuidCName without include its header file. If no GUID MACRO usage, its header file is not necessary. 

  For this case gEdkiiPlatformHasAcpiGuid, I know it will be as protocol dependency. I don't see its GUID C MACRO usage. So, I suggest not to add its header file. This is just my opinion. 
  For BaseTools enhancement to generated GUID C MACRO in autogen.h, I think it is valuable. To avoid the generated MACRO conflict with the existing MACRO, the generated MACRO will still have "G" prefix, such as gEdkiiPlatformHasAcpiGuid ==> G_EDKII_PLATFORM_HAS_ACPI_GUID.

Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Tuesday, March 28, 2017 3:50 PM
>To: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
><feng.tian@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif
>Lindholm <leif.lindholm@linaro.org>
>Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>ACPI Protocol, and plug-in library
>
>On 03/28/17 07:25, Gao, Liming wrote:
>> Ersek:
>>   I don't want to block your patch. You can still check in Guid header file if you
>think it is necessary.
>
>I don't want to check in without formal MdeModulePkg maintainer
>approval. If I check in a patch without formal approval, that will only
>lead to chaos. I don't want to circumvent the process; I want the
>process to *work*.
>
>If MdeModulePkg maintainers agree with my
>
>  [PATCH v3 04/12] MdeModulePkg: introduce EDKII Platform Has ACPI GUID
>
>then they should please give their Reviewed-by for it.
>
>If they disagree with it, they should please explain why.
>
>The specific argument you raised, namely that a BaseTools utility
>generates the necessary C language artifacts for GUID use, is not
>precise. BaseTools generate *some* artifacts, but they do not generate a
>macro that is usable for initializing a GUID object of static storage
>duration (= global variable GUID, or a GUID field in a global variable
>structure).
>
>This is what the ISO C99 standard says:
>
>  6.7.8 Initialization
>
>  [...]
>
>  Constraints
>
>  [...]
>
>  4 All the expressions in an initializer for an object that has static
>    storage duration shall be constant expressions or string literals.
>
>  [...]
>
>  16 Otherwise, the initializer for an object that has aggregate or
>     union type shall be a brace-enclosed list of initializers for the
>     elements or named members.
>
>  [...]
>
>  20 If the aggregate or union contains elements or members that are
>     aggregates or unions, these rules apply recursively to the
>     subaggregates or contained unions. [...]
>
>This is the GUID type definition:
>
>typedef struct {
>  UINT32  Data1;
>  UINT16  Data2;
>  UINT16  Data3;
>  UINT8   Data4[8];
>} GUID;
>
>For this structure, the above standardese implies that we have to
>provide integer constant expressions in the initializer.
>
>  6.6 Constant expressions
>
>  [...]
>
>  6 An /integer constant expression/ shall have integer type and shall
>    only have operands that are integer constants, enumeration
>    constants, character constants, /sizeof/ expressions whose results
>    are integer constants, and floating constants that are the
>    immediate operands of casts. Cast operators in an integer constant
>    expression shall only convert arithmetic types to integer types,
>    except as part of an operand to the /sizeof/ operator.
>
>The end result is that "gEdkiiPlatformHasAcpiGuid" is not usable in such
>contexts, but the EDKII_PLATFORM_HAS_ACPI_GUID macro is.
>
>
>If you want, I can file a TianoCore feature request BZ for generating
>such macros as well in BaseTools, but for now, I don't think it should
>either block my patch noted above, or force me to drop the Guid/ header
>file from the patch. Right now, the Guid/ header adds something that is
>not available from BaseTools, and I wouldn't like to delay the v3 series
>any longer.
>
>Do you agree to the above method? I file a TianoCore Feature Request BZ
>for BaseTools, and MdeModulePkg maintainers formally approve the v3
>04/12 patch?
>
>
>(Side point: I think the Guid/ header file has another benefit:
>documentation. I don't think we can add the amount of documentation that
>is acceptable in a standalone Guid/ header to an in-line comment in the
>.DEC file. So, I think that the new rule for not adding Guid/ headers is
>immature at this point -- and, worse, I don't recall an open
>announcement or discussion about this rule, so that we could have raised
>concerns before turning the proposal into an actual rule.)
>
>Thanks
>Laszlo
>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Tuesday, March 28, 2017 1:32 AM
>>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming
><liming.gao@intel.com>
>>> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
>>> <feng.tian@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif
>Lindholm <leif.lindholm@linaro.org>
>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>ACPI Protocol, and plug-in library
>>>
>>> On 03/27/17 09:00, Ard Biesheuvel wrote:
>>>> On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
>>>>> Laszlo:
>>>>>   On GUID header file, some header file are here, because they are
>added before BaseTools supports it. Now, we allow not to add
>>> header file if the header file only has GUID value definition.
>>>>
>>>> I have to agree with Laszlo here. The BaseTools support is incomplete,
>>>> since it does not add a #define for the GUID to AutoGen.h. This makes
>>>> it impossible to initialize static structures containing GUIDs, such
>>>> as templates containing vendor device paths.
>>>>
>>>> For instance, the following
>>>>
>>>> typedef struct {
>>>>   EFI_GUID foo;
>>>> } TYPE;
>>>>
>>>> TYPE mFoo {
>>>>   SOME_GUID
>>>> };
>>>>
>>>> is not possible without a Guid/xxx.h include file containing a #define
>>>> for SOME_GUID.
>>>
>>> I wonder if we can commit this series before end of April.
>>>
>>> Or is that too soon? End of May perhaps?
>>>
>>> The mind boggles.
>>>
>>> Laszlo
>>>
>>>>
>>>>>   On GUID C MACRO, we suggest to use GuidCName in every C source
>code. So, we don't suggest to add it.  As you know, some
>>> existing header files have GuidCName and GUID Macro. Now, we have no
>plan to clean up the existing one. But, we expect new added
>>> file follows this rule.
>>>>>
>>>>> Thanks
>>>>> Liming
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>Of
>>>>>> Laszlo Ersek
>>>>>> Sent: Saturday, March 25, 2017 1:09 AM
>>>>>> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>>>>>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
>>>>>> <michael.d.kinney@intel.com>; afish@apple.com
>>>>>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming
><liming.gao@intel.com>;
>>>>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>>>>> <leif.lindholm@linaro.org>
>>>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform
>Has
>>>>>> ACPI Protocol, and plug-in library
>>>>>>
>>>>>> On 03/24/17 04:44, Zeng, Star wrote:
>>>>>>> I just think it seems a generic problem.
>>>>>>> Some platforms may dynamically determine whether ACPI should be
>>>>>> supported or not.
>>>>>>> Some platforms may dynamically determine whether SMBIOS should
>be
>>>>>> supported or not.
>>>>>>> Some platforms may dynamically determine whether HII should be
>>>>>> supported or not.
>>>>>>> ...
>>>>>>>
>>>>>>> We are thinking whether we can have a generic NULL instance to
>support all
>>>>>> this kind of cases, for example:
>>>>>>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type
>PCD
>>>>>> point to a depex GUID.
>>>>>>> 2. Implement a NULL instance DepexLib.
>>>>>>> [Defines]
>>>>>>>   INF_VERSION                    = 0x00010005
>>>>>>>   BASE_NAME                      = DepexLib
>>>>>>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>>>>>>   MODULE_TYPE                    = BASE
>>>>>>>   VERSION_STRING                 = 1.0
>>>>>>>   LIBRARY_CLASS                  = NULL
>>>>>>>
>>>>>>> [Sources]
>>>>>>>   DepexLib.c
>>>>>>>
>>>>>>> [Packages]
>>>>>>>   XXXPkg/XXXPkg.dec
>>>>>>>
>>>>>>> [Depex]
>>>>>>>   PcdDepexGuid
>>>>>>>
>>>>>>> 3. Link NULL instance and configure PCD in dsc.
>>>>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>>>>>>     <LibraryClasses>
>>>>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>>>>     <PcdsFixedAtBuild>
>>>>>>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>>>>>>> }
>>>>>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>>>>>>     <LibraryClasses>
>>>>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>>>>     <PcdsFixedAtBuild>
>>>>>>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>>>>>>> }
>>>>>>>
>>>>>>> But current BaseTools does not support the syntax to declare PCD in
>[Depex]
>>>>>> section of inf yet.
>>>>>>>
>>>>>>> Based on the statements above, I have below comments.
>>>>>>
>>>>>>> 1. I agree to add the GUID into MdeModulePkg, but how about using
>>>>>>> gEdkiiPlatformHasAcpiGuid instead of
>>>>>>> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
>>>>>>> section of MdeModulePkg.dec? As Platform can install
>>>>>>> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
>>>>>>> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.
>>>>>>
>>>>>> Sounds reasonable, I'll do this.
>>>>>>
>>>>>>> And another, no
>>>>>>> need to add a include file PlatformHasAcpi.h as BaseTools supports to
>>>>>>> add the GUID definitions to AutoGen files from package dec.
>>>>>>
>>>>>> I disagree with this. I mean, I *personally* wouldn't mind, but this
>>>>>> would be inconsistent with existing MdeModulePkg practice. For
>example,
>>>>>> see
>>>>>>
>>>>>> MdeModulePkg/Include/Guid/TtyTerm.h
>>>>>>
>>>>>> Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no
>data
>>>>>> structures associated with it; the header file only has an extern
>>>>>> declaration, and -- importantly -- an array initializer macro called
>>>>>> EFI_TTY_TERM_GUID, which can be used in the following syntax:
>>>>>>
>>>>>> STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;
>>>>>>
>>>>>> So, I think that the GUID header file is required. I will add it in v3.
>>>>>>
>>>>>> Once we generalize this idea to the design that you laid out above, we
>>>>>> can perhaps eliminate the header file. But, for now, I think we should
>>>>>> remain consistent with other MdeModulePkg GUID definitions.
>>>>>>
>>>>>>> 2. You can file bugzilla bug to request BaseTools syntax support to
>>>>>>> declare PCD in [Depex] section of inf. Then PcdDepexGuid and
>DepexLib
>>>>>>> could be finally added into core package MdeModulePkg or even
>MdePkg
>>>>>>> (I prefer).
>>>>>>
>>>>>> I filed:
>>>>>>
>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec
>>>>>>
>>>>>>> Before that, how about implementing the
>>>>>>> PlatformHasAcpiLib in none core package?
>>>>>>
>>>>>> Works for me; I'll split that part off and keep it under ArmPkg.
>>>>>>
>>>>>> Thanks
>>>>>> Laszlo
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Star
>>>>>>> -----Original Message-----
>>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
>Behalf Of
>>>>>> Ard Biesheuvel
>>>>>>> Sent: Thursday, March 23, 2017 5:09 PM
>>>>>>> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>>>>>> <michael.d.kinney@intel.com>; afish@apple.com
>>>>>>> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek
><lersek@redhat.com>;
>>>>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>>>>> <leif.lindholm@linaro.org>
>>>>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII
>Platform Has
>>>>>> ACPI Protocol, and plug-in library
>>>>>>>
>>>>>>> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>>>>>>>> I prefer to do not include the protocol definition and the library
>instance
>>>>>> into MdeModulePkg at this moment until they need to be used by
>multiple
>>>>>> platforms/archs.
>>>>>>>>
>>>>>>>
>>>>>>> I disagree. Nowhere in the PI or UEFI spec is there any mention (for
>any
>>>>>> architecture) that ACPI is mandatory. Given that EDK2 is the reference
>>>>>> platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is
>>>>>> unreasonable to have lots and lots of PC quirks (i.e., allocations below 4
>GB) in
>>>>>> MdeModulePkg, but then disallow a clean approach to make ACPI
>selectable,
>>>>>> in a way that is true to the spirit of PI (i.e., using protocol dependencies)
>>>>>>>
>>>>>>> So I think at least the protocol definitions belong in MdeModulePkg.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ard.
>>>>>>> _______________________________________________
>>>>>>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/29/17 11:49, Gao, Liming wrote:
> Laszlo:

> I agree GuidCName can't be used to initialize the global variable. If
> there is such requirement, GUID C MACRO will have to be defined.
> Then, its header file will be required.

> Now, we have no rule to forbid to add the header file if it has no
> other definitions except for GUID value, and we have no such
> discussion to define those rules. It is the developer choice to add
> it or not.

> Here, I want to mention that BaseTools has added "extern gGuidCName"
> into AutoGen header file. The consumer code can directly use
> gGuidCName without include its header file. If no GUID MACRO usage,
> its header file is not necessary.
> 
> For this case gEdkiiPlatformHasAcpiGuid, I know it will be as
> protocol dependency. I don't see its GUID C MACRO usage. So, I
> suggest not to add its header file. This is just my opinion.

Thank you for explaining the situation.

For now we've moved all of the generic/core patches in this series to
EmbeddedPkg (and the v4 series has been committed). Once Leif returns,
we can perhaps think about moving those GUIDs / lib instances to
MdeModulePkg, and we could agree at that point to drop the Guid/ header.

> For BaseTools enhancement to generated GUID C MACRO in autogen.h, I
> think it is valuable. To avoid the generated MACRO conflict with the
> existing MACRO, the generated MACRO will still have "G" prefix, such
> as gEdkiiPlatformHasAcpiGuid ==> G_EDKII_PLATFORM_HAS_ACPI_GUID.

Thanks -- I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=449>.
I marked it as a whishlist item.

Cheers
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, March 28, 2017 3:50 PM
>> To: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
>> <feng.tian@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif
>> Lindholm <leif.lindholm@linaro.org>
>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>> ACPI Protocol, and plug-in library
>>
>> On 03/28/17 07:25, Gao, Liming wrote:
>>> Ersek:
>>>   I don't want to block your patch. You can still check in Guid header file if you
>> think it is necessary.
>>
>> I don't want to check in without formal MdeModulePkg maintainer
>> approval. If I check in a patch without formal approval, that will only
>> lead to chaos. I don't want to circumvent the process; I want the
>> process to *work*.
>>
>> If MdeModulePkg maintainers agree with my
>>
>>  [PATCH v3 04/12] MdeModulePkg: introduce EDKII Platform Has ACPI GUID
>>
>> then they should please give their Reviewed-by for it.
>>
>> If they disagree with it, they should please explain why.
>>
>> The specific argument you raised, namely that a BaseTools utility
>> generates the necessary C language artifacts for GUID use, is not
>> precise. BaseTools generate *some* artifacts, but they do not generate a
>> macro that is usable for initializing a GUID object of static storage
>> duration (= global variable GUID, or a GUID field in a global variable
>> structure).
>>
>> This is what the ISO C99 standard says:
>>
>>  6.7.8 Initialization
>>
>>  [...]
>>
>>  Constraints
>>
>>  [...]
>>
>>  4 All the expressions in an initializer for an object that has static
>>    storage duration shall be constant expressions or string literals.
>>
>>  [...]
>>
>>  16 Otherwise, the initializer for an object that has aggregate or
>>     union type shall be a brace-enclosed list of initializers for the
>>     elements or named members.
>>
>>  [...]
>>
>>  20 If the aggregate or union contains elements or members that are
>>     aggregates or unions, these rules apply recursively to the
>>     subaggregates or contained unions. [...]
>>
>> This is the GUID type definition:
>>
>> typedef struct {
>>  UINT32  Data1;
>>  UINT16  Data2;
>>  UINT16  Data3;
>>  UINT8   Data4[8];
>> } GUID;
>>
>> For this structure, the above standardese implies that we have to
>> provide integer constant expressions in the initializer.
>>
>>  6.6 Constant expressions
>>
>>  [...]
>>
>>  6 An /integer constant expression/ shall have integer type and shall
>>    only have operands that are integer constants, enumeration
>>    constants, character constants, /sizeof/ expressions whose results
>>    are integer constants, and floating constants that are the
>>    immediate operands of casts. Cast operators in an integer constant
>>    expression shall only convert arithmetic types to integer types,
>>    except as part of an operand to the /sizeof/ operator.
>>
>> The end result is that "gEdkiiPlatformHasAcpiGuid" is not usable in such
>> contexts, but the EDKII_PLATFORM_HAS_ACPI_GUID macro is.
>>
>>
>> If you want, I can file a TianoCore feature request BZ for generating
>> such macros as well in BaseTools, but for now, I don't think it should
>> either block my patch noted above, or force me to drop the Guid/ header
>> file from the patch. Right now, the Guid/ header adds something that is
>> not available from BaseTools, and I wouldn't like to delay the v3 series
>> any longer.
>>
>> Do you agree to the above method? I file a TianoCore Feature Request BZ
>> for BaseTools, and MdeModulePkg maintainers formally approve the v3
>> 04/12 patch?
>>
>>
>> (Side point: I think the Guid/ header file has another benefit:
>> documentation. I don't think we can add the amount of documentation that
>> is acceptable in a standalone Guid/ header to an in-line comment in the
>> .DEC file. So, I think that the new rule for not adding Guid/ headers is
>> immature at this point -- and, worse, I don't recall an open
>> announcement or discussion about this rule, so that we could have raised
>> concerns before turning the proposal into an actual rule.)
>>
>> Thanks
>> Laszlo
>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Tuesday, March 28, 2017 1:32 AM
>>>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming
>> <liming.gao@intel.com>
>>>> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
>>>> <feng.tian@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif
>> Lindholm <leif.lindholm@linaro.org>
>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has
>> ACPI Protocol, and plug-in library
>>>>
>>>> On 03/27/17 09:00, Ard Biesheuvel wrote:
>>>>> On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
>>>>>> Laszlo:
>>>>>>   On GUID header file, some header file are here, because they are
>> added before BaseTools supports it. Now, we allow not to add
>>>> header file if the header file only has GUID value definition.
>>>>>
>>>>> I have to agree with Laszlo here. The BaseTools support is incomplete,
>>>>> since it does not add a #define for the GUID to AutoGen.h. This makes
>>>>> it impossible to initialize static structures containing GUIDs, such
>>>>> as templates containing vendor device paths.
>>>>>
>>>>> For instance, the following
>>>>>
>>>>> typedef struct {
>>>>>   EFI_GUID foo;
>>>>> } TYPE;
>>>>>
>>>>> TYPE mFoo {
>>>>>   SOME_GUID
>>>>> };
>>>>>
>>>>> is not possible without a Guid/xxx.h include file containing a #define
>>>>> for SOME_GUID.
>>>>
>>>> I wonder if we can commit this series before end of April.
>>>>
>>>> Or is that too soon? End of May perhaps?
>>>>
>>>> The mind boggles.
>>>>
>>>> Laszlo
>>>>
>>>>>
>>>>>>   On GUID C MACRO, we suggest to use GuidCName in every C source
>> code. So, we don't suggest to add it.  As you know, some
>>>> existing header files have GuidCName and GUID Macro. Now, we have no
>> plan to clean up the existing one. But, we expect new added
>>>> file follows this rule.
>>>>>>
>>>>>> Thanks
>>>>>> Liming
>>>>>>> -----Original Message-----
>>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>> Of
>>>>>>> Laszlo Ersek
>>>>>>> Sent: Saturday, March 25, 2017 1:09 AM
>>>>>>> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>>>>>>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
>>>>>>> <michael.d.kinney@intel.com>; afish@apple.com
>>>>>>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming
>> <liming.gao@intel.com>;
>>>>>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>>>>>> <leif.lindholm@linaro.org>
>>>>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform
>> Has
>>>>>>> ACPI Protocol, and plug-in library
>>>>>>>
>>>>>>> On 03/24/17 04:44, Zeng, Star wrote:
>>>>>>>> I just think it seems a generic problem.
>>>>>>>> Some platforms may dynamically determine whether ACPI should be
>>>>>>> supported or not.
>>>>>>>> Some platforms may dynamically determine whether SMBIOS should
>> be
>>>>>>> supported or not.
>>>>>>>> Some platforms may dynamically determine whether HII should be
>>>>>>> supported or not.
>>>>>>>> ...
>>>>>>>>
>>>>>>>> We are thinking whether we can have a generic NULL instance to
>> support all
>>>>>>> this kind of cases, for example:
>>>>>>>> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type
>> PCD
>>>>>>> point to a depex GUID.
>>>>>>>> 2. Implement a NULL instance DepexLib.
>>>>>>>> [Defines]
>>>>>>>>   INF_VERSION                    = 0x00010005
>>>>>>>>   BASE_NAME                      = DepexLib
>>>>>>>>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>>>>>>>>   MODULE_TYPE                    = BASE
>>>>>>>>   VERSION_STRING                 = 1.0
>>>>>>>>   LIBRARY_CLASS                  = NULL
>>>>>>>>
>>>>>>>> [Sources]
>>>>>>>>   DepexLib.c
>>>>>>>>
>>>>>>>> [Packages]
>>>>>>>>   XXXPkg/XXXPkg.dec
>>>>>>>>
>>>>>>>> [Depex]
>>>>>>>>   PcdDepexGuid
>>>>>>>>
>>>>>>>> 3. Link NULL instance and configure PCD in dsc.
>>>>>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>>>>>>>>     <LibraryClasses>
>>>>>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>>>>>     <PcdsFixedAtBuild>
>>>>>>>>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
>>>>>>>> }
>>>>>>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>>>>>>>>     <LibraryClasses>
>>>>>>>>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>>>>>>>>     <PcdsFixedAtBuild>
>>>>>>>>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
>>>>>>>> }
>>>>>>>>
>>>>>>>> But current BaseTools does not support the syntax to declare PCD in
>> [Depex]
>>>>>>> section of inf yet.
>>>>>>>>
>>>>>>>> Based on the statements above, I have below comments.
>>>>>>>
>>>>>>>> 1. I agree to add the GUID into MdeModulePkg, but how about using
>>>>>>>> gEdkiiPlatformHasAcpiGuid instead of
>>>>>>>> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
>>>>>>>> section of MdeModulePkg.dec? As Platform can install
>>>>>>>> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
>>>>>>>> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.
>>>>>>>
>>>>>>> Sounds reasonable, I'll do this.
>>>>>>>
>>>>>>>> And another, no
>>>>>>>> need to add a include file PlatformHasAcpi.h as BaseTools supports to
>>>>>>>> add the GUID definitions to AutoGen files from package dec.
>>>>>>>
>>>>>>> I disagree with this. I mean, I *personally* wouldn't mind, but this
>>>>>>> would be inconsistent with existing MdeModulePkg practice. For
>> example,
>>>>>>> see
>>>>>>>
>>>>>>> MdeModulePkg/Include/Guid/TtyTerm.h
>>>>>>>
>>>>>>> Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no
>> data
>>>>>>> structures associated with it; the header file only has an extern
>>>>>>> declaration, and -- importantly -- an array initializer macro called
>>>>>>> EFI_TTY_TERM_GUID, which can be used in the following syntax:
>>>>>>>
>>>>>>> STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;
>>>>>>>
>>>>>>> So, I think that the GUID header file is required. I will add it in v3.
>>>>>>>
>>>>>>> Once we generalize this idea to the design that you laid out above, we
>>>>>>> can perhaps eliminate the header file. But, for now, I think we should
>>>>>>> remain consistent with other MdeModulePkg GUID definitions.
>>>>>>>
>>>>>>>> 2. You can file bugzilla bug to request BaseTools syntax support to
>>>>>>>> declare PCD in [Depex] section of inf. Then PcdDepexGuid and
>> DepexLib
>>>>>>>> could be finally added into core package MdeModulePkg or even
>> MdePkg
>>>>>>>> (I prefer).
>>>>>>>
>>>>>>> I filed:
>>>>>>>
>>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
>>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec
>>>>>>>
>>>>>>>> Before that, how about implementing the
>>>>>>>> PlatformHasAcpiLib in none core package?
>>>>>>>
>>>>>>> Works for me; I'll split that part off and keep it under ArmPkg.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Laszlo
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Star
>>>>>>>> -----Original Message-----
>>>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
>> Behalf Of
>>>>>>> Ard Biesheuvel
>>>>>>>> Sent: Thursday, March 23, 2017 5:09 PM
>>>>>>>> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>>>>>>> <michael.d.kinney@intel.com>; afish@apple.com
>>>>>>>> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek
>> <lersek@redhat.com>;
>>>>>>> edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm
>>>>>>> <leif.lindholm@linaro.org>
>>>>>>>> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII
>> Platform Has
>>>>>>> ACPI Protocol, and plug-in library
>>>>>>>>
>>>>>>>> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>>>>>>>>> I prefer to do not include the protocol definition and the library
>> instance
>>>>>>> into MdeModulePkg at this moment until they need to be used by
>> multiple
>>>>>>> platforms/archs.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I disagree. Nowhere in the PI or UEFI spec is there any mention (for
>> any
>>>>>>> architecture) that ACPI is mandatory. Given that EDK2 is the reference
>>>>>>> platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is
>>>>>>> unreasonable to have lots and lots of PC quirks (i.e., allocations below 4
>> GB) in
>>>>>>> MdeModulePkg, but then disallow a clean approach to make ACPI
>> selectable,
>>>>>>> in a way that is true to the spirit of PI (i.e., using protocol dependencies)
>>>>>>>>
>>>>>>>> So I think at least the protocol definitions belong in MdeModulePkg.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Ard.
>>>>>>>> _______________________________________________
>>>>>>>> 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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Zeng, Star 7 years, 7 months ago 
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?

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|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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
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|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
>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Zeng, Star 7 years, 7 months ago 
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
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Zeng, Star 7 years, 7 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 21 March 2017 at 02:27, Zeng, Star <star.zeng@intel.com> wrote:
> Just an idea.
>

I llike it!

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

So could we then upgrade the definition so it is possible to use a
dynamic PCD for this?

> -----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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 21 March 2017 at 07:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 21 March 2017 at 02:27, Zeng, Star <star.zeng@intel.com> wrote:
>> Just an idea.
>>
>
> I like it!
>
>> 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.
>>
>
> So could we then upgrade the definition so it is possible to use a
> dynamic PCD for this?
>

Oh, and how is the ordering ensured in this case? We need to set the
dynamic PCD at runtime, but obviously before AcpiTableDxe gets a
chance to load. If the only way to achieve this is another NULL class
library, then this is not much of an improvement. Or could we detect
the table-loader node from PEI as well?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/21/17 09:43, Ard Biesheuvel wrote:
> On 21 March 2017 at 07:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 21 March 2017 at 02:27, Zeng, Star <star.zeng@intel.com> wrote:
>>> Just an idea.
>>>
>>
>> I like it!
>>
>>> 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.
>>>
>>
>> So could we then upgrade the definition so it is possible to use a
>> dynamic PCD for this?
>>
> 
> Oh, and how is the ordering ensured in this case? We need to set the
> dynamic PCD at runtime, but obviously before AcpiTableDxe gets a
> chance to load. If the only way to achieve this is another NULL class
> library, then this is not much of an improvement.

I've been silently waiting for this to get noticed :)

In the ArmVirtQemu case, the PCD value would be set from fw_cfg. But in
ArmVirtQemu, fw_cfg is not available in PEI, only in DXE, so yes, a NULL
class library would be needed.

In physical firmware (also in the planned "showcase QEMU" case), the
toggle would be exposed by another DXE driver (a platform driver) using
an HII form. Generally such a driver translates an nvvar to a PcdSet in
its entry point, plus registers HII stuff that allows the nvvar to be
toggled interactively in the setup browser, for the next boot. Either
way, the PcdSet would again only happen in DXE.

> Or could we detect
> the table-loader node from PEI as well?

Not at the moment; fw_cfg is currently not available in PEI, in ArmVirtQemu.

ArmVirtQemu's PEI phase executes in-place from pflash. That means no
writeable global variables (unless you make a PEIM, or the PEI library
instance, dependent on the "memory discovered" PPI). Even using just the
MMIO (no DMA) interface to fw_cfg, you'd have to parse the DTB on every
invocation. (No writeable global variables imply you can't pre-parse the
stuff in the library constructor.)

A similar example is
"ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c",
where each serial port write has to re-parse the base address from the DT.

(OVMF is different because OVMF's PEI runs from memory.)

So, whatever you saved on the PcdSet ordering in the DXE phase, you
would triply lose in fw_cfg complexity in PEI. A new PEI library
instance would be necessary for fw_cfg, which would either have to parse
the DT on each invocation, or else depend on permanent PEI RAM
availability (which would be the same (or worse) kind of ordering
restriction that you're trying to avoid in DXE in the first place).

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/21/17 10:02, Laszlo Ersek wrote:
> On 03/21/17 09:43, Ard Biesheuvel wrote:
>> On 21 March 2017 at 07:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 21 March 2017 at 02:27, Zeng, Star <star.zeng@intel.com> wrote:
>>>> Just an idea.
>>>>
>>>
>>> I like it!
>>>
>>>> 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.
>>>>
>>>
>>> So could we then upgrade the definition so it is possible to use a
>>> dynamic PCD for this?
>>>
>>
>> Oh, and how is the ordering ensured in this case? We need to set the
>> dynamic PCD at runtime, but obviously before AcpiTableDxe gets a
>> chance to load. If the only way to achieve this is another NULL class
>> library, then this is not much of an improvement.
> 
> I've been silently waiting for this to get noticed :)
> 
> In the ArmVirtQemu case, the PCD value would be set from fw_cfg. But in
> ArmVirtQemu, fw_cfg is not available in PEI, only in DXE, so yes, a NULL
> class library would be needed.
> 
> In physical firmware (also in the planned "showcase QEMU" case), the
> toggle would be exposed by another DXE driver (a platform driver) using
> an HII form. Generally such a driver translates an nvvar to a PcdSet in
> its entry point, plus registers HII stuff that allows the nvvar to be
> toggled interactively in the setup browser, for the next boot. Either
> way, the PcdSet would again only happen in DXE.
> 
>> Or could we detect
>> the table-loader node from PEI as well?
> 
> Not at the moment; fw_cfg is currently not available in PEI, in ArmVirtQemu.
> 
> ArmVirtQemu's PEI phase executes in-place from pflash. That means no
> writeable global variables (unless you make a PEIM, or the PEI library
> instance, dependent on the "memory discovered" PPI). Even using just the
> MMIO (no DMA) interface to fw_cfg, you'd have to parse the DTB on every
> invocation. (No writeable global variables imply you can't pre-parse the
> stuff in the library constructor.)
> 
> A similar example is
> "ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c",
> where each serial port write has to re-parse the base address from the DT.
> 
> (OVMF is different because OVMF's PEI runs from memory.)
> 
> So, whatever you saved on the PcdSet ordering in the DXE phase, you
> would triply lose in fw_cfg complexity in PEI. A new PEI library
> instance would be necessary for fw_cfg, which would either have to parse
> the DT on each invocation, or else depend on permanent PEI RAM
> availability (which would be the same (or worse) kind of ordering
> restriction that you're trying to avoid in DXE in the first place).

So... does that make this patch set (relatively) less "unwieldy" to you?

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Ard Biesheuvel 7 years, 7 months ago 
On 21 March 2017 at 18:00, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/21/17 10:02, Laszlo Ersek wrote:
>> On 03/21/17 09:43, Ard Biesheuvel wrote:
>>> On 21 March 2017 at 07:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 21 March 2017 at 02:27, Zeng, Star <star.zeng@intel.com> wrote:
>>>>> Just an idea.
>>>>>
>>>>
>>>> I like it!
>>>>
>>>>> 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.
>>>>>
>>>>
>>>> So could we then upgrade the definition so it is possible to use a
>>>> dynamic PCD for this?
>>>>
>>>
>>> Oh, and how is the ordering ensured in this case? We need to set the
>>> dynamic PCD at runtime, but obviously before AcpiTableDxe gets a
>>> chance to load. If the only way to achieve this is another NULL class
>>> library, then this is not much of an improvement.
>>
>> I've been silently waiting for this to get noticed :)
>>
>> In the ArmVirtQemu case, the PCD value would be set from fw_cfg. But in
>> ArmVirtQemu, fw_cfg is not available in PEI, only in DXE, so yes, a NULL
>> class library would be needed.
>>
>> In physical firmware (also in the planned "showcase QEMU" case), the
>> toggle would be exposed by another DXE driver (a platform driver) using
>> an HII form. Generally such a driver translates an nvvar to a PcdSet in
>> its entry point, plus registers HII stuff that allows the nvvar to be
>> toggled interactively in the setup browser, for the next boot. Either
>> way, the PcdSet would again only happen in DXE.
>>
>>> Or could we detect
>>> the table-loader node from PEI as well?
>>
>> Not at the moment; fw_cfg is currently not available in PEI, in ArmVirtQemu.
>>
>> ArmVirtQemu's PEI phase executes in-place from pflash. That means no
>> writeable global variables (unless you make a PEIM, or the PEI library
>> instance, dependent on the "memory discovered" PPI). Even using just the
>> MMIO (no DMA) interface to fw_cfg, you'd have to parse the DTB on every
>> invocation. (No writeable global variables imply you can't pre-parse the
>> stuff in the library constructor.)
>>
>> A similar example is
>> "ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c",
>> where each serial port write has to re-parse the base address from the DT.
>>
>> (OVMF is different because OVMF's PEI runs from memory.)
>>
>> So, whatever you saved on the PcdSet ordering in the DXE phase, you
>> would triply lose in fw_cfg complexity in PEI. A new PEI library
>> instance would be necessary for fw_cfg, which would either have to parse
>> the DT on each invocation, or else depend on permanent PEI RAM
>> availability (which would be the same (or worse) kind of ordering
>> restriction that you're trying to avoid in DXE in the first place).
>
> So... does that make this patch set (relatively) less "unwieldy" to you?
>

I suppose, yes :-)

I guess we will never have a perfect solution for this, and using
library classes and protocol dependencies is much preferred over
having to reason about when a dynamic PCD assumes its correct value.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
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|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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Zeng, Star 7 years, 7 months ago 
It is not been implemented yet. Just for our discussion. I should use future tense in the sentence, sorry.

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
Posted by Laszlo Ersek 7 years, 7 months ago 
Mike,

considering time zones and to expedite progress on this series, can you
please offer an opinion whether you would be okay with this one patch
(see full context below) if I posted it for MdeModulePkg, rather than
for ArmPkg?

MdeModulePkg was suggested by Leif.

Thanks!
Laszlo

On 03/17/17 21:47, 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).
> 
> 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;
> +}
> 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.