Re: [staging/dynamictables PATCH 0/2] Dynamic Tables Framework core

["Desimone, Nathaniel L" <nathaniel.l.desimone@intel.com>]

Hi Evan,

Sorry it didn't notice during my first read that SMBIOS/devicetree was also supported. This is a rather large code drop and it is taking some time to go through the entire thing. DynamicTablesDxe seems fine to me.

With regard to the EFI_CONFIGURATION_MANAGER_PROTOCOL, note that the only protocols that are allowed to be marked with the EFI_ prefix are architectural protocols defined by either the UEFI specification or the PI specification. Since this protocol is not spec defined, it cannot be called EFI_CONFIGUARTION_MANAGER_PROTOCOL. The convention is that protocols that we consider to be a standard feature for the EDKII implementation of the UEFI specification are given the EDKII_ prefix. For an example, see MdeModulePkg/Include/Ppi/UfsHostController.h, note that the UFS host controller PPI is not spec. defined so the EDKII_ prefix is used. I'm not sure if we are ready to call this an MdeModulePkg addition yet, so let's keep the prefixes off for now. I'd recommend the name DYNAMIC_TABLES_PLATFORM_PROTOCOL instead of EFI_CONFIGURATION_MANAGER_PROTOCOL.

One concern I have is there seems to be very tight coupling between the DynamicTablesPkg and the platform code. It doesn't seem like DynamicTablesPkg is able to much by itself and depends on a very large quantity of platform code.

I have not been able to find the implementation for GetEStdObjAcpiTableList() anywhere in the code provided either in the fork of edk2-staging or edk2-platforms. Perhaps you forgot to upload something?

Thanks,

Nate

-----Original Message-----
From: Evan Lloyd [mailto:Evan.Lloyd@arm.com] 
Sent: Wednesday, March 21, 2018 6:23 AM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
Cc: nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; leif.lindholm@linaro.org
Subject: RE: [edk2] [staging/dynamictables PATCH 0/2] Dynamic Tables Framework core

Hi Nathaniel.

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of 
> Desimone, Nathaniel L
> Sent: 20 March 2018 22:08
> To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
> Cc: nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes- 
> Fitt@arm.com>; leif.lindholm@linaro.org
> Subject: Re: [edk2] [staging/dynamictables PATCH 0/2] Dynamic Tables 
> Framework core
> 
> Please don't use the name DynamicTableManagerDxe. What does it manage?
> DynamicTables? How does adding the word "Manager" provide any useful 
> information for the reader? I recommend the name "DynamicAcpiTableDxe".
[[Evan Lloyd]]
The problem with " DynamicAcpiTableDxe" is that the table manager can be used for SMBIOS tables and even, should you be so inclined (perverse?) Device Tree.  The "DynamicTableManager" name relates to the fact that the component generates and submits tables (of whatever sort) as opposed to the ConfigurationManager component which obtains, collates and provides the Configuration information.

> It makes it clear this this code is specifically for generating ACPI 
> tables at runtime (as opposed to some sort of unnamed table), and it 
> removes the unnecessary prose.
[[Evan Lloyd]] As noted (and implied by the name), the code is not "specifically for generating ACPI tables"

That being said, we'd be happy to change the names if you can suggest something clearer.
DynamicTablesDxe might do - the "Manager" part is, as you indicate, pretty redundant.
That implies changing ConfigurationManager to something like "DynamicConfig".

Would that strike you as clearer?

Regards,
Evan



> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Sami Mujawar
> Sent: Monday, March 19, 2018 8:19 AM
> To: edk2-devel@lists.01.org
> Cc: nd@arm.com; leif.lindholm@linaro.org; Stephanie.Hughes- 
> Fitt@arm.com
> Subject: [edk2] [staging/dynamictables PATCH 0/2] Dynamic Tables 
> Framework core
> 
> The Dynamic Tables Framework is a prototyped as a solution for 
> automatically generating the firmware tables based on hardware 
> description.
> 
> This patchset is the Dynamic Tables Framework core and implement the 
> generic/standard modules for dynamically generating ACPI 6.2 tables 
> for ARM platform. The platform specific modules are in the devel- 
> dynamictables branch in the edk2-platforms repository at:
> https://github.com/tianocore/edk2-platforms/tree/devel-dynamictables
> 
> The first patch in this patchset 'MdePkg: SMMUv3 updates for IORT'
> is a precursor for the Dynamic Tables Framework and has been submitted 
> independently to the edk2-devel mailing list where it is currently 
> awaiting acceptance.
> 
> The sources for this patchset can be seen at:
> https://github.com/samimujawar/edk2-
> staging/tree/187_dynamictables_v1
> 
> Sami Mujawar (2):
>   MdePkg: SMMUv3 updates for IORT table definitions
>   DynamicTablesPkg: Dynamic Tables Framework
> 
> 
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/AcpiTableFactory/Acpi
> TableFactory.c             |  226 +++
> 
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DeviceTreeTableFactor
> y/DeviceTreeTableFactory.c |  225 +++
> 
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactory.
> h                           |  125 ++
> 
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactory
> Dxe.c                        |   84 +
> 
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactory
> Dxe.inf                      |   59 +
> 
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/SmbiosTableFactory/S
> mbiosTableFactory.c         |  226 +++
> 
> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManag
> erDxe.c                        |  533 +++++
> 
> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManag
> erDxe.inf                      |   48 +
>  DynamicTablesPkg/DynamicTables.dsc.inc
> |   46 +
>  DynamicTablesPkg/DynamicTables.fdf.inc
> |   35 +
>  DynamicTablesPkg/DynamicTablesPkg.dec
> |   42 +
>  DynamicTablesPkg/Include/AcpiTableGenerator.h
> |  282 +++
>  DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> |  587 ++++++
>  DynamicTablesPkg/Include/ConfigurationManagerHelper.h
> |  119 ++
>  DynamicTablesPkg/Include/ConfigurationManagerObject.h
> |  176 ++
>  DynamicTablesPkg/Include/DeviceTreeTableGenerator.h
> |  182 ++
>  DynamicTablesPkg/Include/Library/TableHelperLib.h
> |   70 +
>  DynamicTablesPkg/Include/Protocol/ConfigurationManagerProtocol.h
> |  128 ++
>  DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
> |  140 ++
>  DynamicTablesPkg/Include/SmbiosTableGenerator.h
> |  240 +++
>  DynamicTablesPkg/Include/StandardNameSpaceObjects.h
> |  116 ++
>  DynamicTablesPkg/Include/TableGenerator.h
> |  252 +++
> 
> DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/AcpiDbg2LibArm.inf
> |   47 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> |  440 +++++
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/AcpiFadtLibArm.inf
> |   41 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
> |  666 +++++++
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/AcpiGtdtLibArm.inf
> |   41 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
> |  670 +++++++
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
> |   41 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
> | 2046 ++++++++++++++++++++
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.h
> |   50 +
> 
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.in
> f                             |   41 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
> |  717 +++++++
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
> |   41 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
> |  342 ++++
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
> |   41 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
> |  142 ++
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
> |   41 +
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
> |  324 ++++
>  DynamicTablesPkg/Library/Common/TableHelperLib/TableHelper.c
> |  164 ++
>  DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
> |   35 +
>  MdePkg/Include/IndustryStandard/IoRemappingTable.h
> |   11 +-
>  42 files changed, 9881 insertions(+), 1 deletion(-)  create mode 
> 100644 
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/AcpiTableFactory/Acpi
> TableFactory.c
>  create mode 100644
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DeviceTreeTableFactor
> y/DeviceTreeTableFactory.c
>  create mode 100644
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactory.
> h
>  create mode 100644
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactory
> Dxe.c
>  create mode 100644
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactory
> Dxe.inf
>  create mode 100644
> DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/SmbiosTableFactory/S
> mbiosTableFactory.c
>  create mode 100644
> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManag
> erDxe.c
>  create mode 100644
> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManag
> erDxe.inf
>  create mode 100644 DynamicTablesPkg/DynamicTables.dsc.inc
>  create mode 100644 DynamicTablesPkg/DynamicTables.fdf.inc
>  create mode 100644 DynamicTablesPkg/DynamicTablesPkg.dec
>  create mode 100644 DynamicTablesPkg/Include/AcpiTableGenerator.h
>  create mode 100644 DynamicTablesPkg/Include/ArmNameSpaceObjects.h
>  create mode 100644
> DynamicTablesPkg/Include/ConfigurationManagerHelper.h
>  create mode 100644
> DynamicTablesPkg/Include/ConfigurationManagerObject.h
>  create mode 100644
> DynamicTablesPkg/Include/DeviceTreeTableGenerator.h
>  create mode 100644 DynamicTablesPkg/Include/Library/TableHelperLib.h
>  create mode 100644
> DynamicTablesPkg/Include/Protocol/ConfigurationManagerProtocol.h
>  create mode 100644
> DynamicTablesPkg/Include/Protocol/DynamicTableFactoryProtocol.h
>  create mode 100644 DynamicTablesPkg/Include/SmbiosTableGenerator.h
>  create mode 100644
> DynamicTablesPkg/Include/StandardNameSpaceObjects.h
>  create mode 100644 DynamicTablesPkg/Include/TableGenerator.h
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/AcpiDbg2LibArm.inf
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/AcpiFadtLibArm.inf
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/FadtGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/AcpiGtdtLibArm.inf
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/IortGenerator.h
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.in
> f
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/MadtGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/McfgGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/RawGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/SpcrGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Common/TableHelperLib/TableHelper.c
>  create mode 100644
> DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
> 
> --
> '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