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-boun...@lists.01.org] On Behalf Of > evan.ll...@arm.com > Sent: Tuesday, October 3, 2017 3:48 AM > To: edk2-devel@lists.01.org > Cc: "matteo.carl...@arm.com"@arm.com; "n...@arm.com"@arm.com; > "ard.biesheu...@linaro.org"@arm.com; > "stephanie.hughes-f...@arm.com"@arm.com; > "thomas.abra...@arm.com"@arm.com; > "arvind.chau...@arm.com"@arm.com; "leif.lindh...@linaro.org"@arm.com; > "daniil.egra...@arm.com"@arm.com > Subject: [edk2] [PATCH 0/2] Dynamic Tables > > From: EvanLloyd <evan.ll...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
