Re: [PATCH 0/2] Dynamic Tables

[Evan Lloyd <Evan.Lloyd@arm.com>]

Hi Jiewen
(I hope that is your personal name, not your surname - but it is a bit hard for us barbarians to tell, sorry.)
Thank you for the very helpful feedback.
Responses are inline below.

> -----Original Message-----
> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> Sent: 10 October 2017 03:29
> To: Evan Lloyd <Evan.Lloyd@arm.com>; 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; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [edk2] [PATCH 0/2] Dynamic Tables
>
> 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.

[[Evan Lloyd]] This sounds sensible, and I will discuss it with Leif.  Another option might be to start up a new module.

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

[[Evan Lloyd]] You are quite right - we will put a document together on this.

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

[[Evan Lloyd]] Yes.

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

[[Evan Lloyd]] This is an initial submission for discussion, but there is not currently a relevant standard.  We are providing a "proof of concept" for discussion and review.  The ids are currently internal to edk2.  We do anticipate some sort of standard definition (for the information supplier) but that might be no more than a version of ConfigurationManagerObject.h

>
> +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)

[[Evan Lloyd]] We did not.  We have not previously paid much attention to IntelFrameworkPkg  😊
We will have a look at the DataHub stuff though, to see how it fits.

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

[[Evan Lloyd]]  From your description, there does seem to be an element of similarity with our submitted implementation.
However, our current code does not yet cover later options of obtaining the configuration data from a remote node, etc.
Our ultimate aim is to have a single UEFI image run on a range of platforms, taking guidance from the remote on what tables to publish with what content.  You say "People feel it is easier to fill industry defined SMBIOS record directly, than to fill the EDK defined datahub record."  I do not question your statement, in fact I am sure you are right.  However, might those people be the ones who make a living filling "industry defined SMBIOS record directly"?  Our experience is that there is significant overhead in building complete sets of platform dependent tables for each variant of a machine, and we would like to reduce that.

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

[[Evan Lloyd]] PCDs are very useful.  However they (especially FixedPcds) are build time options.  Given the aim of a single UEFI image, they do not quite work.  We want the UEFI to be able to get the variant config from an external entity.  In theory, a complex enough PCD database could be used to provide the config information, and could be loaded from a remote, but it would be a very elaborate solution.

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

[[Evan Lloyd]] We will need to study DataHub to see what is there, but we will make a response when we have.

>
>
> 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.
> >
> >
...
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.