public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "evan.lloyd@arm.com" <evan.lloyd@arm.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "\"Matteo.Carlini@arm.com\"@arm.com"
	<"Matteo.Carlini@arm.com"@arm.com>,
	"\"nd@arm.com\"@arm.com" <"nd@arm.com"@arm.com>,
	"\"ard.biesheuvel@linaro.org\"@arm.com"
	<"ard.biesheuvel@linaro.org"@arm.com>,
	"\"Stephanie.Hughes-Fitt@arm.com\"@arm.com"
	<"Stephanie.Hughes-Fitt@arm.com"@arm.com>,
	"\"thomas.abraham@arm.com\"@arm.com"
	<"thomas.abraham@arm.com"@arm.com>,
	"\"Arvind.Chauhan@arm.com\"@arm.com"
	<"Arvind.Chauhan@arm.com"@arm.com>,
	"\"leif.lindholm@linaro.org\"@arm.com"
	<"leif.lindholm@linaro.org"@arm.com>,
	"\"Daniil.Egranov@arm.com\"@arm.com"
	<"Daniil.Egranov@arm.com"@arm.com>,
	"Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [PATCH 0/2] Dynamic Tables
Date: Tue, 10 Oct 2017 02:29:02 +0000	[thread overview]
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A9D7016@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <20171002194753.4316-1-evan.lloyd@arm.com>

HI Evan
Thanks for the contribution.

This is a very big feature. It may talk us more time to review and evaluate.
At same time, one of our key MdeModule package maintainer is in paternity leave. It may be longer than usual.

I notice you only defined ARM namespace in this patch, and implemented ARM library instance.
Also most consumers of ConfigurationManager are from ARM platform package. So if it urgent from ARM platform, you may consider to check into ArmPkg at first.



I only have a quick look at the patch. Would you please share more on the design philosophy?

1) It seems the final goal is still to generate ACPI table/SMBIOS table/DevTree. You just introduce a way to manage how these tables are generated, right?

2) Below definition is defined by the MdeModulePkg/Include/DynamicTables/ConfigurationManagerObject.h.
Is there any industry standard to define below index? Or it is just defined by EDKII, and anyone can add extension here?

+Object ID's in the ARM Namespace:
+   0 - Reserved
+   1 - Boot Architecture Info
+   2 - CPU Info
+   3 - Power Management Profile Info
+   4 - GICC Info
+   5 - GICD Info
+   6 - GIC MSI Frame Info
+   7 - GIC Redistributor Info
+   8 - GIC ITS Info
+   9 - Serial Console Port Info
+  10 - Serial Debug Port Info
+  12 - Generic Timer Info
+  13 - Platform GT Block Info
+  14 - Platform Generic Watchdog
+  15 - PCI Configuration Space Info
+  16 - Hypervisor Vendor Id

3) I am not sure if you have known about datahub protocol. (IntelFrameworkPkg\Include\Protocol\DataHub.h)
Long time ago, we have platform module filling the SMBIOS needed information to datahub (such as CPU INFO, Memory Info).
The SMBIOS table is derived from datahub protocol. The setup driver can also from datahub.
But later, we think it is an overdesign and datahub is no longer used in the new IA platform.
People feel it is easier to fill industry defined SMBIOS record directly, than to fill the EDK defined datahub record.
They do not need to learn 2 different styles of data record format.

To me, this seems similar to datahub. Please help us understand the key difference.

4) In addition, EDKII/PI defined PCD (platform configuration database). It is an architecture way to manage the configuration data.
We are also implementing structure PCD to let platform fill data easily. (https://github.com/tianocore/edk2-staging/tree/StructurePcd)

I found some configuration can be as simple as a PCD, such as
+  // Boot architecture information
+  { EFI_ACPI_6_1_ARM_PSCI_COMPLIANT },              // BootArchFlags
+
+  // Power management profile information
+  { EFI_ACPI_6_1_PM_PROFILE_ENTERPRISE_SERVER },    // PowerManagement Profile

With the new structure PCD design, below definition can also be a structure PCD.
+  // SPCR Serial Port
+  {
+    FixedPcdGet64 (PcdSerialRegisterBase),  // UINT64  BaseAddress
+    FixedPcdGet32 (PL011UartInterrupt),     // UINT32  Interrupt
+    FixedPcdGet64 (PcdUartDefaultBaudRate), // UINT64  BaudRate
+    FixedPcdGet32 (PL011UartClkInHz)        // UINT32  Clock
+  },

+  // Debug Serial Port
+  {
+    FixedPcdGet64 (PcdSerialDbgRegisterBase), // UINT64  BaseAddress
+    38,                                       // UINT32  Interrupt
+    FixedPcdGet64 (PcdSerialDbgUartBaudRate), // UINT64  BaudRate
+    FixedPcdGet32 (PcdSerialDbgUartClkInHz)   // UINT32  Clock
+  },

What if we just use PCD to define these CPU info, GICC info? Do we really another ConfigurationManager?


All in all, if we can compare the difference of below design with pros/cons, that will be great.
That will help us understand more about the new design.
A) DataHub (IntelFrameworkPkg, do not recommend to use.)
B) PCD (MdePkg, in PI specification) and structure PCD (EDKII staging)
C) ConfigurationManager (this patch)


Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> evan.lloyd@arm.com
> Sent: Tuesday, October 3, 2017 3:48 AM
> To: edk2-devel@lists.01.org
> Cc: "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com;
> "ard.biesheuvel@linaro.org"@arm.com;
> "Stephanie.Hughes-Fitt@arm.com"@arm.com;
> "thomas.abraham@arm.com"@arm.com;
> "Arvind.Chauhan@arm.com"@arm.com; "leif.lindholm@linaro.org"@arm.com;
> "Daniil.Egranov@arm.com"@arm.com
> Subject: [edk2] [PATCH 0/2] Dynamic Tables
> 
> From: EvanLloyd <evan.lloyd@arm.com>
> 
> Historically, ACPI code, SMBIOS tables, and UEFI firmware were
> often developed in isolation from each other.  This introduced
> several problems, not least of which was duplication of platform
> information between the various source trees.
> In addition, variants of platforms introduced a plethora of
> alternative builds of ACPI, SMBIOS and EDK2, with the concomitant
> risk of getting the mixture wrong in a build.
> 
> In the effort to resolve these problems, the solution prototyped
> here was devised.  The basic idea is to obtain the "variant"
> information from a management node.  That means the firmware image
> can be platform independent, with ACPI, SMBIOS (and potentially
> other) tables generated with information from the management
> node.  This example has the framework for that, but the
> configuration information is supplied directly, as an interim solution
> until a suitable management node implementation exists yet.
> 
> 
> Sami Mujawar (1):
>   MdeModulePkg: Dynamic Tables Framework
> 
>  MdeModulePkg/MdeModulePkg.dec
> |  13 +
>  MdeModulePkg/Universal/DynamicTables/DynamicTables.dsc.inc
> |  45 ++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiDbg2LibArm/AcpiDbg2Lib
> Arm.inf                       |  49 ++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiFadtLibArm/AcpiFadtLibAr
> m.inf                       |  47 ++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiGtdtLibArm/AcpiGtdtLibA
> rm.inf                       |  46 ++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLib
> Arm.inf                       |  47 ++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLib
> Arm.inf                       |  47 ++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiRawLibArm/AcpiRawLibAr
> m.inf                         |  44 ++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibAr
> m.inf                       |  44 ++
> 
> MdeModulePkg/Library/DynamicTables/Common/TableHelperLib/TableHelperLi
> b.inf                         |  39 ++
> 
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/DynamicTa
> bleFactoryDxe.inf              |  57 ++
> 
> MdeModulePkg/Universal/DynamicTables/DynamicTableManagerDxe/DynamicT
> ableManagerDxe.inf              |  47 ++
>  MdeModulePkg/Include/DynamicTables/AcpiTableGenerator.h
> | 280 ++++++++
>  MdeModulePkg/Include/DynamicTables/ArmNameSpaceObjects.h
> | 367 ++++++++++
>  MdeModulePkg/Include/DynamicTables/ConfigurationManagerHelper.h
> | 112 +++
>  MdeModulePkg/Include/DynamicTables/ConfigurationManagerObject.h
> | 158 +++++
>  MdeModulePkg/Include/DynamicTables/SmbiosTableGenerator.h
> | 235 +++++++
>  MdeModulePkg/Include/DynamicTables/StandardNameSpaceObjects.h
> |  93 +++
>  MdeModulePkg/Include/DynamicTables/TableGenerator.h
> | 235 +++++++
>  MdeModulePkg/Include/Library/TableHelperLib.h
> |  67 ++
>  MdeModulePkg/Include/Protocol/ConfigurationManagerProtocol.h
> | 121 ++++
>  MdeModulePkg/Include/Protocol/DynamicTableFactoryProtocol.h
> | 113 +++
> 
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/DynamicTa
> bleFactory.h                   |  91 +++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiDbg2LibArm/Dbg2Genera
> tor.c                          | 440 ++++++++++++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiFadtLibArm/FadtGenerat
> or.c                          | 562 +++++++++++++++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerat
> or.c                          | 652 +++++++++++++++++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMadtLibArm/MadtGener
> ator.c                          | 732 ++++++++++++++++++++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMcfgLibArm/McfgGenera
> tor.c                          | 336 +++++++++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiRawLibArm/RawGenerat
> or.c                            | 177 +++++
> 
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerat
> or.c                          | 323 +++++++++
>  MdeModulePkg/Library/DynamicTables/Common/TableHelperLib/TableHelper.c
> | 165 +++++
> 
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/AcpiTableFa
> ctory/AcpiTableFactory.c     | 227 ++++++
> 
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/DynamicTa
> bleFactoryDxe.c                |  84 +++
> 
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/SmbiosTabl
> eFactory/SmbiosTableFactory.c | 227 ++++++
> 
> MdeModulePkg/Universal/DynamicTables/DynamicTableManagerDxe/DynamicT
> ableManagerDxe.c                | 531 ++++++++++++++
>  MdeModulePkg/Universal/DynamicTables/DynamicTables.fdf.inc
> |  35 +
>  36 files changed, 6888 insertions(+)
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTables.dsc.inc
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiDbg2LibArm/AcpiDbg2Lib
> Arm.inf
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiFadtLibArm/AcpiFadtLibAr
> m.inf
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiGtdtLibArm/AcpiGtdtLibA
> rm.inf
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLib
> Arm.inf
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLib
> Arm.inf
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiRawLibArm/AcpiRawLibAr
> m.inf
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibAr
> m.inf
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Common/TableHelperLib/TableHelperLi
> b.inf
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/DynamicTa
> bleFactoryDxe.inf
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTableManagerDxe/DynamicT
> ableManagerDxe.inf
>  create mode 100644
> MdeModulePkg/Include/DynamicTables/AcpiTableGenerator.h
>  create mode 100644
> MdeModulePkg/Include/DynamicTables/ArmNameSpaceObjects.h
>  create mode 100644
> MdeModulePkg/Include/DynamicTables/ConfigurationManagerHelper.h
>  create mode 100644
> MdeModulePkg/Include/DynamicTables/ConfigurationManagerObject.h
>  create mode 100644
> MdeModulePkg/Include/DynamicTables/SmbiosTableGenerator.h
>  create mode 100644
> MdeModulePkg/Include/DynamicTables/StandardNameSpaceObjects.h
>  create mode 100644 MdeModulePkg/Include/DynamicTables/TableGenerator.h
>  create mode 100644 MdeModulePkg/Include/Library/TableHelperLib.h
>  create mode 100644
> MdeModulePkg/Include/Protocol/ConfigurationManagerProtocol.h
>  create mode 100644
> MdeModulePkg/Include/Protocol/DynamicTableFactoryProtocol.h
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/DynamicTa
> bleFactory.h
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiDbg2LibArm/Dbg2Genera
> tor.c
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiFadtLibArm/FadtGenerat
> or.c
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerat
> or.c
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMadtLibArm/MadtGener
> ator.c
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiMcfgLibArm/McfgGenera
> tor.c
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiRawLibArm/RawGenerat
> or.c
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerat
> or.c
>  create mode 100644
> MdeModulePkg/Library/DynamicTables/Common/TableHelperLib/TableHelper.c
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/AcpiTableFa
> ctory/AcpiTableFactory.c
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/DynamicTa
> bleFactoryDxe.c
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTableFactoryDxe/SmbiosTabl
> eFactory/SmbiosTableFactory.c
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTableManagerDxe/DynamicT
> ableManagerDxe.c
>  create mode 100644
> MdeModulePkg/Universal/DynamicTables/DynamicTables.fdf.inc
> 
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


  parent reply	other threads:[~2017-10-10  2:25 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-02 19:47 [PATCH 0/2] Dynamic Tables evan.lloyd
2017-10-02 19:47 ` [PATCH 1/2] MdeModulePkg: Dynamic Tables Framework evan.lloyd
2017-10-03 14:34   ` Zeng, Star
2017-10-03 16:03     ` Leif Lindholm
2017-10-03 18:44       ` Sean Brogan
2017-10-03 17:03     ` Evan Lloyd
2017-10-02 19:47 ` [PATCH 2/2] [edk2-platforms] Platform/ARM: Dynamic Tables support for FVP evan.lloyd
2017-10-03 17:12 ` [PATCH 0/2] Dynamic Tables Evan Lloyd
2017-10-10  2:29 ` Yao, Jiewen [this message]
2017-10-10 18:52   ` Evan Lloyd
2017-10-10 19:57     ` Laszlo Ersek
2017-10-11  1:25     ` Yao, Jiewen
2017-10-12 15:43     ` Leif Lindholm
2017-10-12 15:46       ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=74D8A39837DF1E4DA445A8C0B3885C503A9D7016@shsmsx102.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox