> On May 23, 2016, at 10:09 AM, Kinney, Michael D <[email protected]> > wrote: > > Andrew, > > To clarify the status of the prototype I did for my proposal for Structured > PCD. > > I have all elements listed in the proposal working in the prototype including > field access macros. > > The current prototype uses Python bindings to CLANG front end library to get > AST into Python. It simulates what would be integrated into Autogen stage of > EDK II build. > > Based on feedback from Laszlo and you, I am evaluating how to perform the > equivalent work without Python bindings to CLANG and use the C compiler > from the active tool chain tag instead. > > One example you show is a device path. That type of data structure is > not what I have been focused on. I have been focused on structures that > contain a set of configuration elements for CPU/Chipset/SoC/HW component > that would typically be a well-defined static structure with option for a > flexible array member. > > With EDK II firmware based on UEFI/PI specs, there are a few data types > that are variable length that require parsing from beginning of the > buffer to process. The ones I can think of real quick are: > > * HOB List > * UEFI Boot Option > * UEFI Driver Option > * UEFI Device Path > > From your experience, what percentage of PCDs use these data types that > require parsing from the beginning of the structure when consumed? >
Mike, Today any Ptr/VOID* PCD is a variable length structure. Strings are a form of variable length structures in use today. The current PCD schemes supports variable length complex structures today, but they are not used as they are hard to initialize. If we are adding syntax that is C like to initialize PCD values it seems like it is reasonable to let it support arbitrary C structures, and I think my prototype points out it is easy to initialize an arbitrary C structure when using the C pre processor and the compiler. > In your specific example, is the PCD really treated as a pointer to a > buffer and the C sources that consume the PCD treat it as a generic > pointer to a device path? Or do the C sources access fields in the > various nodes of the device path directly by using the date type > USB_CLASS_FORMAT_DEVICE_PATH? > In my example I assume you would get back a pointer to an EFI_DEVICE_PATH_PROTOCOL, and the size, and then treat it like a device path. This is generally how device paths get used. > If C code only uses it as a generic pointer to a device path, then I do > not think your example may be a good candidate for a Structured PCD, > but I do see value in having a better mechanism to initialize these > types of data structures from a DSC file. > Well as I've stated from my point of view being able initialize complex PCD data structures using the C include infrastructure is really the big win in this proposal. Adding a structured PCD that supports per field access seems like a nice to have feature, as it is just a simplification of a read modify write of an entire structure. Is there some requirement to abstract the caller from the layout of the data structure? I don't really understand what problem you are trying to solve? > Would my suggesting of building in support for device paths as strings > as a value syntax make an even simpler platform porting experience? > > For example, we could add a DEVICE_PATH() operator to the value syntax > and the operand is converted from a string to a device path with an > implied end node. > Adding yet another abstraction does not seem to make it simpler for the developer, but more complex. I picked that device path example from the BDS PlatformLib as it seemed like a good candidate to replace all the globals in that file with PCD entries. There are other variable length arrays that act like lists in that file. I just picked the device path as it seemed like the hardest one to implement for the POC. In general the support for variable length arrays/structures seems useful for things like lists of PCI devices, black lists, white lists, etc. Also in our current scheme we have a syntax extension that supports a table of SMBIOS structures and their strings that is consumed by the platform SMBIOS driver that knows how to programmatically patch fields that are discoverable via an API. > DSC File > ========= > [PcdsFixedAtBuild.common] > gMdeModulePkgTokenGuid.PcdFakeDevicePathExample|DEVICE_PATH(UsbClass(0xffff,0xffff,3,1,1)) > > If a PCD is a single node (no end node), we could have a 2nd operator > called DEVICE_PATH_NODE(). > > I will follow up shortly with your 2 examples using the syntax from > my current proposal. > OK thanks that will help me understand it better. Thanks, Andrew Fish > Thanks, > > Mike > > >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf Of >> Andrew Fish >> Sent: Monday, May 23, 2016 8:55 AM >> To: Gao, Liming <[email protected]> >> Cc: Kinney, Michael D <[email protected]>; [email protected] >> <edk2- >> [email protected]> >> Subject: Re: [edk2] [RFC] Structured PCD Proposal >> >> >>> On May 23, 2016, at 7:19 AM, Gao, Liming <[email protected]> wrote: >>> >>> Andrew: >>> Sorry, I have not fully understood your prototype. In this prototype, after >>> the >> module with sample.pcd and sample.h is built, the output AutoGenTool file is >> generated. It includes below contents for PCD value initialization. The >> driver can >> directly consumes PcdFakePcdExample, and convert it to structure >> FAKE_PCD_EXAMPLE >> type, then access its field. There is no change in PcdLib and Pcd Driver. >> Right? >>> >> >> Liming, >> >> I just used the C pre processor to produce a syntax similar to what we use >> today in >> the DSC file. I assume that the DSC grammar would get extended in a way that >> this >> code could be invoked prior to the PCD processing and the output would be >> processed >> by the DSC PCD processing logic. >> >> I just echoed the output to the console to make it easy to see what the >> prototype >> code was doing. >> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> [PcdsFixedAtBuild.common] >>> gMdeModulePkgTokenGuid.PcdFakePcdExample| { 0x53, 0x47, 0xc1, 0xe0, 0xbe, >>> 0xf9, >> 0xd2, 0x11, 0x9a, 0x0c, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d, 0x21, 0x30, >> 0x04, 0x00, >> 0x03, 0x00, 0x00, 0x00,} | FAKE_PCD_EXAMPLE >>> gMdeModulePkgTokenGuid.PcdFakeDevicePathExample| { 0x03, 0x0f, 0x0b, 0x00, >>> 0xff, >> 0xff, 0xff, 0xff, 0x03, 0x01, 0x01, 0x7f, 0xff, 0x04, 0x00,} | >> EFI_DEVICE_PATH_PROTOCOL >>> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> >> >> I only added the type to the current syntax in this example. I'm not sure >> how the >> field accessors are going to work. One option is to use the C compiler on >> the calling >> side to figure out offsets and bit masks for bit fields. I think Mike is >> looking into >> how make field accessors work in more detail, so we need to wait and see >> what he is >> going to propose. >> >> I was trying to make the point that using the C pre processor and the >> current INF >> syntax is a developer friendly way to represent the data and relatively easy >> to >> implement. I only spent a couple of hours on the prototype on Sunday, and it >> supports >> structures, bit fields, and device paths. Since this scheme leverages the >> existing C >> infrastructure it should currently support any possible C construct that >> could be >> used. >> >> Thanks, >> >> Andrew Fish >> >>> Thanks >>> Liming <> >>> <>From: edk2-devel [mailto:[email protected]] On Behalf Of >>> Andrew >> Fish >>> Sent: Monday, May 23, 2016 6:04 AM >>> To: Kinney, Michael D <[email protected]> >>> Cc: [email protected] <[email protected]> >>> Subject: Re: [edk2] [RFC] Structured PCD Proposal >>> >>> Mike, >>> >>> I did a quick prototype that just outputs the current PCD DSC syntax with >>> an extra >> | C typedef name at the end when you build. The example includes a >> structure with >> bit-fields and a device path. >>> >>> >>> >>> build -p MdeModulePkg/MdeModulePkg.dsc -a X64 -t XCODE5 >>> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> [PcdsFixedAtBuild.common] >>> gMdeModulePkgTokenGuid.PcdFakePcdExample| { 0x53, 0x47, 0xc1, 0xe0, 0xbe, >>> 0xf9, >> 0xd2, 0x11, 0x9a, 0x0c, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d, 0x21, 0x30, >> 0x04, 0x00, >> 0x03, 0x00, 0x00, 0x00,} | FAKE_PCD_EXAMPLE >>> gMdeModulePkgTokenGuid.PcdFakeDevicePathExample| { 0x03, 0x0f, 0x0b, 0x00, >>> 0xff, >> 0xff, 0xff, 0xff, 0x03, 0x01, 0x01, 0x7f, 0xff, 0x04, 0x00,} | >> EFI_DEVICE_PATH_PROTOCOL >>> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> >>> There are a few hacks: >>> 1) The parser is not very robust from an error handling point of view. >>> 2) Some of the compiler flags are missing. The reason is I could not undo >>> the -E >> from the flags I was trying to reuse. So this may take a little flags >> cleanup to >> work. >>> 3) I only had enough time to do the XCODE version, and I set the flags >>> needed to >> make sure you get the EFI Spec structure packing. >>> >>> Thanks, >>> >>> Andrew Fish >>> >>> >>> >>> diff --git a/BaseTools/BinWrappers/PosixLike/StructuredPcdAutoGen.py >> b/BaseTools/BinWrappers/PosixLike/StructuredPcdAutoGen.py >>> new file mode 100755 >>> index 0000000..82a9b3b >>> --- /dev/null >>> +++ b/BaseTools/BinWrappers/PosixLike/StructuredPcdAutoGen.py >>> @@ -0,0 +1,85 @@ >>> +#!/usr/bin/python -u >>> +## @file >>> +# Prototype .pcd parser >>> +# >>> +# Parse the meta data >>> +#// @PCDTYPE [PcdsFixedAtBuild.common] >>> +#// @PCD >>> gMdeModulePkgTokenGuid.PcdFakePcdExample|FAKE_PCD_EXAMPLE|gFakePcdExample >>> +# >>> +# Create a C application to generate the PCD data. >>> +# >>> +# Copyright (c) 2016, Apple Inc. All rights reserved.<BR> >>> +# 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 >> <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. >>> +# >>> + >>> +import sys >>> + >>> +programStart = ''' >>> +// AutoGen file do not hand edit. >>> +#include <stdio.h> >>> + >>> +#ifdef NULL >>> +#undef NULL >>> +#endif >>> + >>> +#include "%s" >>> + >>> +int >>> +main (int argv, char **argc) >>> +{ >>> + unsigned char *c; >>> + int i; >>> + >>> +''' >>> + >>> +programWork = ''' >>> + printf (" @1| {"); >>> + for (i=0, c = (unsigned char *)(unsigned long long) &@2; i < sizeof >>> (@2); i++) { >>> + printf (" 0x%02x,", c[i]); >>> + } >>> + printf ("} | @3\\n"); >>> +''' >>> + >>> +"" >>> + >>> +programEnd = ''' >>> + printf ("\\n <file://///n>"); >>> + return 0; >>> +} >>> + >>> +''' >>> + >>> +def main (filename): >>> + ''' strip out lines that do not start with @, and the @''' >>> + print programStart % filename >>> + >>> + pcd = {} >>> + pcdType = '' >>> + with open(filename) as openfile: >>> + for line_of_c in openfile.readlines(): >>> + line = line_of_c.split() >>> + if len(line) >= 3 and line[0] == '//': >>> + if '@PCDTYPE' in line[1]: >>> + #// @PCDTYPE [PcdsFixedAtBuild.common] >>> + print ' printf ("%s\\n");' % line[2] >>> + if '@PCD' in line[1]: >>> + #// @PCD >> gMdeModulePkgTokenGuid.PcdFakePcdExample|FAKE_PCD_EXAMPLE|gFakePcdExample >>> + PcdEntry = line[2].split('|') >>> + if len (PcdEntry) >= 3: >>> + print programWork.replace('@1', >>> PcdEntry[0]).replace('@2', >> PcdEntry[2]).replace('@3', PcdEntry[1]) >>> + >>> + print programEnd >>> + >>> +if __name__ == "__main__": >>> + if len (sys.argv) < 2: >>> + sys.exit() >>> + >>> + main (sys.argv[1]) >>> + >>> + >>> diff --git a/BaseTools/Conf/build_rule.template >> b/BaseTools/Conf/build_rule.template >>> index 91bcc18..3d2c47f 100644 >>> --- a/BaseTools/Conf/build_rule.template >>> +++ b/BaseTools/Conf/build_rule.template >>> @@ -604,3 +604,21 @@ >>> GenFw -o $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.rc -g $(MODULE_GUID) -- >> hiibinpackage $(HII_BINARY_PACKAGES) >>> >>> >>> +[Pcd-Code-File] >>> + <InputFile> >>> + ?.pcd >>> + >>> + <OutputFile> >>> + $(OUTPUT_DIR)(+)${s_dir}(+)${s_base} >>> + >>> + <ExtraDependency> >>> + $(MAKE_FILE) >>> + >>> + <Command.XCODE> >>> + StructuredPcdAutoGen.py ${src} > >> $(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.AutoGen.c >>> + "$(CC)" -mms-bitfields -fshort-wchar -funsigned-char $(INC) -o >> $(OUTPUT_DIR)(+)${s_dir}(+)AutoGenTool >> $(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.AutoGen.c >>> + @echo >> "\n+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++" >>> + @$(OUTPUT_DIR)(+)${s_dir}(+)AutoGenTool >>> + @echo >> "+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n" >>> + >>> + >>> diff --git a/MdeModulePkg/Include/Pcd/FakeExample.h >> b/MdeModulePkg/Include/Pcd/FakeExample.h >>> new file mode 100644 >>> index 0000000..06d5526 >>> --- /dev/null >>> +++ b/MdeModulePkg/Include/Pcd/FakeExample.h >>> @@ -0,0 +1,33 @@ >>> +/** @file >>> + Example of Structured PCDs include file. >>> + >>> + Copyright (c) 2016, Apple Inc. All rights reserved.<BR> >>> + >>> + 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 >> <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 __FAKE_EXAMPLE_H__ >>> +#define __FAKE_EXAMPLE_H__ >>> + >>> +typedef struct { >>> + UINT32 BitsA:4; >>> + UINT32 BitsB:8; >>> + UINT32 BitsC:4; >>> + UINT32 BitsD:8; >>> +} EXAMPLE_BITFIELD; >>> + >>> +typedef struct { >>> + EFI_GUID Compression; >>> + EXAMPLE_BITFIELD BitField; >>> + UINT8 ClassCode[3]; >>> +} FAKE_PCD_EXAMPLE; >>> + >>> + >>> +#endif >>> + >>> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc >>> index 936f46d..4dbedd6 100644 >>> --- a/MdeModulePkg/MdeModulePkg.dsc >>> +++ b/MdeModulePkg/MdeModulePkg.dsc >>> @@ -187,6 +187,9 @@ >>> [PcdsFixedAtBuild.IPF] >>> gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000 >>> >>> +[Components] >>> + MdeModulePkg/Pcd/Example.inf >>> + >>> >> ##################################################################################### >> ############## >>> # >>> # Components Section - list of the modules and components that will be >>> processed >> by compilation >>> diff --git a/MdeModulePkg/Pcd/Example.inf b/MdeModulePkg/Pcd/Example.inf >>> new file mode 100644 >>> index 0000000..b12c383 >>> --- /dev/null >>> +++ b/MdeModulePkg/Pcd/Example.inf >>> @@ -0,0 +1,31 @@ >>> +## @file >>> +# Example of Structured PCDs set via C include infrastructure >>> +# >>> +# Portions copyright (c) 2016, Apple Inc. All rights reserved.<BR> >>> +# >>> +# 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 >> <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 = 0x00010005 >>> + BASE_NAME = ExamplePcd >>> + FILE_GUID = C8BE4EB8-2035-11E6-9969-B8E8562CBAFA >>> + MODULE_TYPE = USER_DEFINED >>> + VERSION_STRING = 1.0 >>> + >>> +[Sources] >>> + Example.pcd >>> + >>> +[Sources.IA32] >>> + YouMustCompileInX64Mode.h >>> + >>> +[Packages] >>> + MdePkg/MdePkg.dec >>> + MdeModulePkg/MdeModulePkg.dec >>> diff --git a/MdeModulePkg/Pcd/Example.pcd b/MdeModulePkg/Pcd/Example.pcd >>> new file mode 100644 >>> index 0000000..f01dcd8 >>> --- /dev/null >>> +++ b/MdeModulePkg/Pcd/Example.pcd >>> @@ -0,0 +1,74 @@ >>> +/** @file >>> + Example of Structured PCDs set via C include infrastructure >>> + >>> + Copyright (c) 2016, Apple Inc. All rights reserved.<BR> >>> + >>> + 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 >> <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 <Uefi.h> >>> +#include <IndustryStandard/Pci.h> >>> +#include <Guid/PcAnsi.h> >>> +#include <Protocol/DevicePath.h> >>> + >>> +#include <Pcd/FakeExample.h> >>> + >>> +#define A 1 >>> +#define B 2 >>> +#define C 3 >>> +#define D 4 >>> + >>> +typedef struct { >>> + USB_CLASS_DEVICE_PATH UsbClass; >>> + EFI_DEVICE_PATH_PROTOCOL End; >>> +} USB_CLASS_FORMAT_DEVICE_PATH; >>> + >>> +#define CLASS_HID 3 >>> +#define SUBCLASS_BOOT 1 >>> +#define PROTOCOL_KEYBOARD 1 >>> + >>> +// @PCDTYPE [PcdsFixedAtBuild.common] >>> + >>> + >>> +// @PCD >>> gMdeModulePkgTokenGuid.PcdFakePcdExample|FAKE_PCD_EXAMPLE|gFakePcdExample >>> +FAKE_PCD_EXAMPLE gFakePcdExample = { >>> + EFI_PC_ANSI_GUID, >>> + { A, B, C, D }, >>> + { PCI_CLASS_DISPLAY, PCI_CLASS_DISPLAY_VGA, PCI_IF_VGA_VGA } >>> +}; >>> + >>> + >>> +// @PCD >> gMdeModulePkgTokenGuid.PcdFakeDevicePathExample|EFI_DEVICE_PATH_PROTOCOL|gFakePcdDevi >> cePathExample >>> +USB_CLASS_FORMAT_DEVICE_PATH gFakePcdDevicePathExample = { >>> + { >>> + { >>> + MESSAGING_DEVICE_PATH, >>> + MSG_USB_CLASS_DP, >>> + { >>> + (UINT8) (sizeof (USB_CLASS_DEVICE_PATH)), >>> + (UINT8) ((sizeof (USB_CLASS_DEVICE_PATH)) >> 8) >>> + } >>> + }, >>> + 0xffff, // VendorId >>> + 0xffff, // ProductId >>> + CLASS_HID, // DeviceClass >>> + SUBCLASS_BOOT, // DeviceSubClass >>> + PROTOCOL_KEYBOARD // DeviceProtocol >>> + }, >>> + >>> + { >>> + END_DEVICE_PATH_TYPE, >>> + END_ENTIRE_DEVICE_PATH_SUBTYPE, >>> + { >>> + sizeof (EFI_DEVICE_PATH_PROTOCOL), >>> + 0 >>> + } >>> + } >>> +}; >>> + >>> >>> >>> >>>> On May 20, 2016, at 2:48 PM, Andrew Fish <[email protected] >> <mailto:[email protected]>> wrote: >>>> >>>>> >>>>> On May 20, 2016, at 12:34 PM, Kinney, Michael D >>>>> <[email protected] >> <mailto:[email protected]>> wrote: >>>>> >>>>> Andrew, >>>>> >>>>> I hope there is not a misunderstanding in the include file used by a DEC >>>>> file >> for a PCD. >>>>> >>>>> It is not a parallel include file. The same include file is used in DEC >>>>> parsing >> and >>>>> by modules/libs that access the same Structured PCD. This is especially >> important for >>>>> sharing a .h file between a PCD and VFR NV storage sources to eliminate >>>>> manual >> maintenance >>>>> of config value offsets in HII type PCD. >>>>> >>>> >>>> Mike, >>>> >>>> Sorry for the confusion I assume .h files are the same in both cases, and >>>> that is >> an absolute requirement. My point is the current DSC, INF, Conf/*.txt syntax >> supports >> figuring out the include pathing and passing the build generated compiler >> flags to >> the compiler, getting code auto generated, and built. Adding the C syntax >> info the >> .DEC and .DSC files means you have duplicate this mechanism. So I guess what >> I'm >> saying is if it is C like syntax use an INF, just like the C and ACPI code >> does today >> since you are really all sharing the C include infrastructure. If you try and >> duplicate build infrastructure with different code it is going to break (not >> get the >> same result as the INF path) in some subtle way at some point.Ssince it is a >> different syntax we could hit and end case where things are not symmetric >> (for >> example the INF files can have [BuildOptions] settings and new extensions to >> DEC/DSC >> don't support that). Also extending the DEC and DSC syntax is more new >> things for a >> devel >> oper to learn. >>>> >>>> Basically if it is a C like syntax (like C and ACPI) then we should use an >>>> INF >> file like today is what I'm advocating. >>>> >>>>> I only added syntax for some optional tags in comments for fields in the C >> structure. >>>>> I want developer to only enter information (C struct in this case) one >>>>> time. We >> also >>>>> want to focus platform configuration information in the one DSC file for >>>>> simpler >> platform >>>>> configuration and generate PCD reports that show PCD configuration >>>>> settings >> (including >>>>> all field values in a Structured PCD). >>>>> >>>>> At this time, bit fields are a must have requirement for Structured PCDs. >>>>> >>>>> Main use case is register values with bit fields with no translation from >>>>> PCD to >>>>> register access. As a result SIZE_OF and OFFSET_OF are not sufficient. >>>>> I have >> ideas >>>>> on how to get bit offset/bit length from C compiler without using >>>>> CLANG/AST. It >> was >>>>> just very easy to get this info from CLANG/AST. >>>>> >>>>> Additional reason is size reduction of configuration data. Single bit or >> BOOLEAN >>>>> like configuration setting takes up minimum of 8-bits today. If there >>>>> are many >>>>> configuration settings like these, then they can add up. >>>>> >>>> >>>> It is not so much as they are not supported, but you don't get an atomic >>>> write. >> You have to read modify write them, so 2 calls and a local variable. >> Actually we >> "solved that problem" in the IoLib by adding IoBitField* functions.... So >> that would >> imply what you need is an AND mask and you can hide the read modify write >> behind the >> library.... >>>> >>>> Thus if you could generate a local, global or #define with the bit filed >>>> value >> all 1's and all the other fields 0's you can automate the read modify write >> into a >> single call. So maybe it is possible? >>>> >>>> Do all the compilers support C99? Named structure initialization makes >>>> this is >> easy if you know the type of the bitfield and the member name. Well you have >> to >> suppress the warning. >>>> >>>> ~/work/Compiler>cat bitfield.c >>>> typedef struct { >>>> unsigned int a:2; >>>> unsigned int b:4; >>>> unsigned int c:2; >>>> } TEST; >>>> >>>> TEST Test0 = { 0 }; >>>> TEST Testa = { .a = 0xFFFFFFFF }; >>>> TEST Testb = { .b = 0xFFFFFFFF }; >>>> TEST Testc = { .c = 0xFFFFFFFF }; >>>> ~/work/Compiler>clang -S -Wno-bitfield-constant-conversion bitfield.c >>>> ~/work/Compiler>cat bitfield.S >>>> .section __TEXT,__text,regular,pure_instructions >>>> .macosx_version_min 10, 11 >>>> .section __DATA,__data >>>> .globl _Test0 ## @Test0 >>>> .align 2 >>>> _Test0: >>>> .byte 0 ## 0x0 >>>> .space 3 >>>> >>>> .globl _Testa ## @Testa >>>> .align 2 >>>> _Testa: >>>> .byte 3 ## 0x3 >>>> .space 3 >>>> >>>> .globl _Testb ## @Testb >>>> .align 2 >>>> _Testb: >>>> .byte 60 ## 0x3c >>>> .space 3 >>>> >>>> .globl _Testc ## @Testc >>>> .align 2 >>>> _Testc: >>>> .byte 192 ## 0xc0 >>>> .space 3 >>>> >>>> OK I think I'm convinced now it is likely we will be able to support >>>> bitfields. >>>> >>>> Thanks, >>>> >>>> Andrew Fish >>>> >>>> PS Mike if you look into this bitfield thing I can look at prototyping the >>>> INF >> based parsing with an example syntax from my previous mail. It will be >> easier to >> discuss with a prototype to look at. >>>> >>>> Well assuming I can track down the PACKAGES_PATH bug with ACPI tables. >> FfsInf.EfiOutputPath is missing the PACKAGES_PATH part and so I don't get >> any ACPI >> tables as the OUTPUT directory path is wrong so the search for the files >> fail :(. >>>> >>>>> Mike >>>>> >>>>>> -----Original Message----- >>>>>> From: [email protected] <mailto:[email protected]> [mailto:[email protected] >> <mailto:[email protected]>] >>>>>> Sent: Friday, May 20, 2016 12:15 PM >>>>>> To: Kinney, Michael D <[email protected] >> <mailto:[email protected]>> >>>>>> Cc: [email protected] <mailto:[email protected]> <edk2- >> [email protected] <mailto:[email protected]>> >>>>>> Subject: Re: [edk2] [RFC] Structured PCD Proposal >>>>>> >>>>>> >>>>>>> On May 20, 2016, at 10:48 AM, Kinney, Michael D >>>>>>> <[email protected] >> <mailto:[email protected]>> wrote: >>>>>>> >>>>>>> Andrew, >>>>>>> >>>>>>> 1) I think the strongest concern is on use of CLANG/AST. I will look >>>>>>> at your >> ideas >>>>>> and >>>>>>> some other ideas I have thought of as well to use standard C compiler >>>>>>> that is >>>>>> already >>>>>>> installed to build FW to support Structured PCD features. >>>>>>> >>>>>>> 2) The other concern I see here is extending PcdLib to access fields. >>>>>>> You >> prefer >>>>>> to >>>>>>> not add those APIs and always read/write entire structure. I have >>>>>>> requests to >>>>>> support >>>>>>> field access, so that is a bit of a requirement conflict. >>>>>>> >>>>>> >>>>>> Mike, >>>>>> >>>>>> Given the complexity we ended up with the basic edk2 design, that was >>>>>> also >> driven by >>>>>> complex requirements, I wanted to start the conversation of how some of >>>>>> the >>>>>> requirements maybe driving a disproportionate amount of the complexity. >>>>>> I would >> hate >>>>>> for a "nice to have" feature drive 80% of the complexity. I'm still >>>>>> feeling >> guilty >>>>>> about those PCD section names.... >>>>>> >>>>>> I think there are two vectors of complexity: >>>>>> 1) Implementation - Requiring clang/AST >>>>>> I think stepping away with the assumption of clang/AST being required >>>>>> helps >> this >>>>>> vector out a lot. >>>>>> >>>>>> 2) Workflow - how a developer uses the feature day to day. >>>>>> I think my comments about the default values in the .DEC relate to the >> complexities a >>>>>> developer will face. As you point out there is going to be a .DEC grammar >> extension >>>>>> required just to support this feature. Also if I understand correctly >>>>>> changing >> a >>>>>> #define or a structure definition in another package could change the >>>>>> default. >> We >>>>>> currently don't have this behavior and I think that will lead to more >>>>>> confusion >> and >>>>>> hard to track down bugs. Thus I think we should consider sticking with >>>>>> the >> current >>>>>> .DEC syntax for the default variable if it is required. We could advocate >> comments >>>>>> that explain the fields and maybe even output information like that in >>>>>> build >> log PCD >>>>>> section so you can copy paste into the .DEC. >>>>>> >>>>>> Using an INF file and new file type for the structured PCD is also >>>>>> trying to >> manage >>>>>> developer complexity by making it symmetric with C and ACPI code. You >>>>>> can make >> the >>>>>> argument that PCDs always worked a certain way, but I will make the a >>>>>> counter >>>>>> argument that managing the C include file infrastructure has always >>>>>> worked a >> certain >>>>>> way too, and that is the reason to consider not building a parallel >>>>>> include >>>>>> infrastructure definition for the .DSC (and .DEC) files and use an INF >>>>>> file, >> not to >>>>>> mention it is probably easier to implement since it is a solved problem. >>>>>> >>>>>> >>>>>> I was also thinking about the PcdLib field accessors. If you don't >>>>>> support bit >> fields >>>>>> and don't need pedantic type safety I think this idea may get you 80% of >>>>>> what >> you >>>>>> want (I'm guessing you might have already thought of this). If the build >>>>>> system >> knows >>>>>> the C type of the structure and places it in the AutoGen.h and you know >>>>>> the >> structure >>>>>> member name in the calling code the Code calling the PcdLib.h can derive >>>>>> the >>>>>> structure offset and size of the field. It may also be possible to >>>>>> verify the >> size >>>>>> of the argument via a sizeof() in a macro. >>>>>> >>>>>> I just double checked that nested structures/unions would work, and >>>>>> clang looks >>>>>> happy. >>>>>> >>>>>> #include <stdio.h> >>>>>> >>>>>> #define OFFSET_OF(TYPE, Field) __builtin_offsetof(TYPE, Field) >>>>>> #define SIZE_OF(TYPE, Field) (sizeof(((TYPE *)0)->Field)) >>>>>> >>>>>> typedef struct { >>>>>> int a; >>>>>> int b; >>>>>> int c; >>>>>> int leaf; >>>>>> } STRUCT_LEAF; >>>>>> >>>>>> typedef struct { >>>>>> STRUCT_LEAF Leaf; >>>>>> int i; >>>>>> } STRUCT; >>>>>> >>>>>> int >>>>>> main () >>>>>> { >>>>>> STRUCT S; >>>>>> >>>>>> printf ("Offset 0x%lx size 0x%lx\n", OFFSET_OF(STRUCT, Leaf.leaf), >> SIZE_OF(STRUCT, >>>>>> Leaf.leaf)); >>>>>> return 0; >>>>>> } >>>>>> >>>>>> So maybe the complexity tradeoff is how you support bitfields (union >>>>>> with a >> real type >>>>>> and granularity of access is that type) and how pedantic the type >>>>>> checking (C >> is not >>>>>> type safe, so this is no different than a cast in the C code) is going >>>>>> to be >> for the >>>>>> PcdLib structure field accessor. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Andrew Fish >>>>>> >>>>>>> More responses below. >>>>>>> >>>>>>> Mike >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: [email protected] <mailto:[email protected]> [mailto:[email protected] >> <mailto:[email protected]>] >>>>>>>> Sent: Thursday, May 19, 2016 3:25 PM >>>>>>>> To: Kinney, Michael D <[email protected] >> <mailto:[email protected]>> >>>>>>>> Cc: [email protected] <mailto:[email protected]> <edk2- >> [email protected] <mailto:[email protected]>> >>>>>>>> Subject: Re: [edk2] [RFC] Structured PCD Proposal >>>>>>>> >>>>>>>> >>>>>>>>> On May 19, 2016, at 12:16 PM, Kinney, Michael D >>>>>>>>> <[email protected] >> <mailto:[email protected]>> >>>>>> wrote: >>>>>>>>> >>>>>>>>> Andrew, >>>>>>>>> >>>>>>>>> My responses embedded below. >>>>>>>>> >>>>>>>> >>>>>>>> Mike, >>>>>>>> >>>>>>>> Likewise. >>>>>>>> >>>>>>>>> Mike >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: edk2-devel [mailto:[email protected] >>>>>>>>>> <mailto:edk2- >> [email protected]>] On Behalf Of Andrew >>>>>> Fish >>>>>>>>>> Sent: Thursday, May 19, 2016 10:53 AM >>>>>>>>>> To: Kinney, Michael D <[email protected] >> <mailto:[email protected]>>; [email protected] <mailto:edk2- >> [email protected]> >>>>>> <edk2- >>>>>>>>>> [email protected] <mailto:[email protected]>> >>>>>>>>>> Subject: Re: [edk2] [RFC] Structured PCD Proposal >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> On May 19, 2016, at 2:26 AM, Laszlo Ersek <[email protected] >> <mailto:[email protected]>> wrote: >>>>>>>>>>> >>>>>>>>>>> On 05/19/16 01:28, Kinney, Michael D wrote: >>>>>>>>>>> >>>>>>>>>>> [snip] >>>>>>>>>>> >>>>>>>>>>>> ## C Structure Syntax Proposal >>>>>>>>>>>> * Use C typedef syntax to describe complex C structures >>>>>>>>>>>> * Use a C language parser with support for struct/union/array/bit >> fields/pack >>>>>>>>>>>> * Recommend use of libclang C language parser into Abstract Syntax >>>>>>>>>>>> Tree >> (AST) >>>>>>>>>>>> - Included with LLVM release >>>>>>>>>>>> - http://llvm.org/releases/download.html >> <http://llvm.org/releases/download.html> >>>>>>>>>>>> - http://clang.llvm.org/doxygen/group__CINDEX.html >> <http://clang.llvm.org/doxygen/group__CINDEX.html> >>>>>>>>>>>> * Recommend use of Python bindings for CLANG for EDK II Python >>>>>>>>>>>> based >> BaseTools >>>>>>>>>>>> - pip install clang >>>>>>>>>>> >>>>>>>>>>> What versions are necessary? On RHEL-7, the following versions are >>>>>>>>>>> available (from EPEL only; RHEL does not provide clang): >>>>>>>>>>> >>>>>>>>>>> clang-devel: 3.4.2-8.el7 >>>>>>>>>>> python-pip: 7.1.0-1.el7 >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Will it be possible to check everything into the edk2 git tree? >>>>>>>>>> Currently >> we >>>>>> only >>>>>>>>>> require the developer to have Xcode installed and everything else is >>>>>>>>>> in the >> git >>>>>>>> tree. >>>>>>>>>> I'm not sure it is possible to run pip on our production build >>>>>>>>>> servers? >>>>>>>>> >>>>>>>>> Is clang and clang libs included in your environment? >>>>>>>> >>>>>>>> clang is the compiler for Xcode so we have that for sure. I'm not sure >>>>>>>> about >> the >>>>>> libs >>>>>>>> but they are likely accessible. >>>>>>>> >>>>>>>>> That is not edk2 git today. >>>>>>>>> The only other component is the Python wrapper on top of the clang >>>>>>>>> lib and >> that >>>>>>>>> is available from another github project if you want to pull python >>>>>>>>> sources >> to >>>>>> it. >>>>>>>>> >>>>>>>> >>>>>>>> If is just Python code it should not be too complex? >>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> ## DEC File Extensions >>>>>>>>>>>> * Declare link between PCD and C data structure and token range for >> fields >>>>>>>>>>>> * @Include path to include file with C typedef [Required] >>>>>>>>>>>> * Replace |VOID*| with name of C typedef >>>>>>>>>>>> * @TokenMap path to file with token number assignments [Optional] >>>>>>>>>>>> * Range of token numbers to assign to fields in C typedef >>>>>>>>>>>> [Required] >>>>>>>>>>>> * Example: >>>>>>>>>>>> >>>>>>>>>>>> ``` >>>>>>>>>>>> # @Include Include/Pcd/AcpiLowerPowerIdleTable.h >>>>>>>>>>>> # @TokenMap Include/Pcd/AcpiLowerPowerIdleTable.map >>>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >> gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable|{}|ACPI_LOW_POWER_IDLE_TABLE|0x00010080 >>>>>>>>>> |0x0E000200-0x0E0002FF >>>>>>>>>>>> ``` >>>>>>>>>>> >>>>>>>>>>> What does 0x00010080 mean here? >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> * Recommended File Paths >>>>>>>>>>>> - <PackageName>/Include/Pcd/<StructuredPcdName>.h >>>>>>>>>>>> - <PackageName>/Include/Pcd/<StructuredPcdName>.map [Optional] >>>>>>>>>>>> - <PackageName>/Include/Pcd/<StructuredPcdName>.uni [Optional] >>>>>>>>>>>> * C Pre-Processor Support >>>>>>>>>>>> - Use of #include, #define, #if supported >>>>>>>>>>>> - #include limited to files in same package >>>>>>>>>>>> - Including files from other packages being evaluated >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I think you need to share the C headers with the edk2 C and ASL >>>>>>>>>> code, so >> you >>>>>>>> should >>>>>>>>>> have full access including all the things set in the [BuildOptions] >>>>>>>>>> flags >> in the >>>>>>>> DSC. >>>>>>>>>> So use something like PP_FLAGS or ASLPP_FLAGS and allow >>>>>>>>>> customization via >> the >>>>>>>>>> Conf/build_rule.txt. I'd rather we not makeup a new thing and just >>>>>>>>>> use one >> of >>>>>> the >>>>>>>>>> existing pre processor schemes. >>>>>>>>> >>>>>>>>> I agree. Any flags set up in [BuildOptions] or -D flags on command >>>>>>>>> line >> would be >>>>>>>>> passed in, so the .h file associated with the Structured PCD is >>>>>>>>> treated the >> same >>>>>>>>> as any other .h file used to build modules/libs. There is no >>>>>>>>> intention to >> create >>>>>>>>> any new build rules to support Structured PCDs. >>>>>>>> >>>>>>>> OK I was confused by this statement. "Including files from other >>>>>>>> packages >> being >>>>>>>> evaluated". The more I think about it it is probably more related to >>>>>>>> this not >>>>>> being >>>>>>>> symmetric to how C and ASL works (you have an INF file to declare your >>>>>> dependencies). >>>>>>>> I'm worried that could cause issue. What if I needed a include file >>>>>>>> from a >> package >>>>>>>> that was not included any other way? If I was writing C or ASL I'd just >> update the >>>>>>>> INF file. >>>>>>> >>>>>>> Today, all of the content in a DEC file of a package does not have any >> dependencies >>>>>>> outside that package. Modules/library implementations within a package >>>>>>> may >> have >>>>>>> dependencies on content from other packages as defined by the [Packages] >> section >>>>>>> in an INF file. A PCD, declared in a package DEC files, that is >>>>>>> associated >> with >>>>>>> a typedef struct in an include file must follow the DEC file constraint >>>>>>> where >> there >>>>>>> are no dependencies on other packages. >>>>>>> >>>>>>> What is being evaluated is how to support a use case where an include >>>>>>> file >> used for >>>>>>> a Structured PCD needs definitions from include files in another package >>>>>>> (e.g. MdePkg for some industry standard defines/structs) >>>>>>> >>>>>>> We support multiple package dependencies in modules/library INF files >>>>>>> today >> with a >>>>>>> [Packages] section in the INF. One idea (not part of this proposal at >>>>>>> this >> time), >>>>>>> is to add a [Packages] section to a DEC file. The include paths used to >>>>>>> process a .h file associated with a Structured PCD can be appended with >>>>>>> all >> the >>>>>>> dependent package include paths. The [Packages] section in a DEC would >>>>>>> only >> be >>>>>>> used for DEC file dependencies. It would never be used to package >> dependencies of >>>>>>> modules/libraries within that package. >>>>>>> >>>>>>> Adding a [Packages] section to a DEC file has wider impacts to other >> BaseTools, >>>>>>> which is why I did not provide any more details than "under evaluation" >>>>>>> at >> this >>>>>>> time. My suggestion with the current proposal is to make sure all the >>>>>>> .h file >>>>>>> content required for a Structured PCD be within the package itself. >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> IMHO it will be easy enough to follow the proposed restrictive rules >>>>>>>>>> when >> you >>>>>> are >>>>>>>>>> starting from scratch, but I'm more worried about the case when you >>>>>>>>>> have an >>>>>>>> existing >>>>>>>>>> (very complex) include infrastructure that supports feature X in C >>>>>>>>>> and ASL >> and >>>>>> you >>>>>>>>>> decide to add a PCD for X. You don't want it to not work due to more >> restrictive >>>>>>>>>> rules around the PCD. >>>>>>>>> >>>>>>>>> A .h file can have lots of content in it. Only the typedef structure >>>>>>>>> name >>>>>>>> referenced >>>>>>>>> in the DEC file declaration has to follow any of the restrictions >>>>>>>>> associated >> with >>>>>>>>> a Structured PCD. The C Preprocessor statements above are there to >>>>>>>>> show >> that >>>>>> there >>>>>>>>> .h file for a Structured PCD can use the same C Preprocessor >>>>>>>>> directives that >> the >>>>>>>> rest >>>>>>>>> of the C code in modules/libs can use. The biggest restriction is in >>>>>>>>> the >> data >>>>>>>> types >>>>>>>>> of the leaf fields of the structures. Those have to be compatible >>>>>>>>> with the >> PCD >>>>>>>>> get/set APIs. >>>>>>>>> >>>>>>>> >>>>>>>> I think this makes my point about the complexity driving restrictions. >>>>>>>> If you >> did >>>>>> not >>>>>>>> have leaf field accessors you would not have that restriction. >>>>>>> >>>>>>> I agree that removing the requirement for leaf accessors reduces >>>>>>> complexity. >> I >>>>>> have >>>>>>> a request to support leaf accessors, so that is why they are part of >>>>>>> this >> proposal. >>>>>>> >>>>>>>> >>>>>>>>>> >>>>>>>>>>>> ## C Structure Syntax >>>>>>>>>>>> * Support struct/union/array with nesting >>>>>>>>>>>> * Support bit fields >>>>>>>>>>> >>>>>>>>>>> Bit fields will require new PcdLib interaces, right? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Yikes bit fileds. My over all feedback is this feature seems very >>>>>>>>>> complex >> and >>>>>>>>>> designed by committee (too many features). The complexity already >>>>>>>>>> seems to >> be >>>>>>>>>> introducing restrictions and will possible make it harder for >>>>>>>>>> developers to >>>>>>>>>> understand. >>>>>>>>> >>>>>>>>> The goal was to allow use of C syntax for a typedef struct with as few >>>>>> restrictions >>>>>>>>> as possible, so exiting C structs associated with VOID* PCDs could be >>>>>>>>> used >> "as >>>>>> is". >>>>>>>>> Yes. Supporting full C syntax can be complex, but by using CLANG >>>>>>>>> front end >> for >>>>>>>>> parsing into an AST, the complexity is handled by a well supported >>>>>>>>> parser. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> I'll take a quick stab at simplification. >>>>>>>>>> 1) Add the new syntax to a new file type: *.pcd. This file would >>>>>>>>>> contain >> [Pcd*] >>>>>>>>>> sections and the C like syntax. >>>>>>>>> >>>>>>>>> C like syntax. That is what concerns me. Now we have to tell >>>>>>>>> developers >> that >>>>>>>>> they can use full C syntax for modules/libs, but there are different >>>>>>>>> C rules >> for >>>>>>>>> PCDs. I think it is easier is developers can use came C syntax for >> modules/libs >>>>>>>>> and PCDs. >>>>>>>> >>>>>>>> I would assume the structure initialization is pure C, but there is >>>>>>>> meta data >>>>>>>> associated with the PCD that needs to get collected and associated >> initialized C >>>>>>>> structure. This is going to be true for any scheme. >>>>>>> >>>>>>> Yes. There is optional meta data in a comment block for a field of a >> structure. >>>>>>> I have used the same meta-data tags and syntax that is defined in DEC >>>>>>> file >>>>>>> comment blocks, to take advantage of the source style a developer >>>>>>> already >> knowns >>>>>>> when declaring PCDs in a DEC file. Only @TokenNumber (likely very >>>>>>> rarely >> used) and >>>>>>> @DefaultValue are new tags. But even the value syntax for these reuses >>>>>>> the >> value >>>>>>> syntax for a token number and default value in a PCD declaration in a >>>>>>> DEC >> file. >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>>> 2) Don't support default values in the .dec with the C like syntax. >> Recommend no >>>>>>>>>> defaults and a build break if a PCD is not set by the platform >>>>>>>>>> build for >>>>>> complex >>>>>>>>>> structures. >>>>>>>>> >>>>>>>>> Why not support defaults in scope of DEC file or associated .h file? >>>>>>>>> The >>>>>>>>> @DefaultValue tag in .h file is optional, so what you suggest here is >> supported. >>>>>>>>> >>>>>>>> >>>>>>>> To simplify the .DEC parser so it does not require AST. >>>>>>> >>>>>>> >>>>>>> This is pushing more work onto platform porting in DSC/FDF file. The >>>>>>> PCD >> concept >>>>>>> from the beginning included the idea that the developer that designs a >>>>>>> new PCD >>>>>>> and adds it to a DEC file gets to support a "preferred" default value >>>>>>> that is >>>>>>> applicable to most platforms, and the DSC/FDF files only need to provide >> override >>>>>>> values if the "preferred" default does not work for that specific >>>>>>> platform. >> Why >>>>>>> should we change this fundamental PCD concept just to simplify the >>>>>>> tools. >>>>>>> >>>>>>> I agree that dependency on CLANG/AST/Python bindings may not be the best >>>>>>> Implementation choice based on feedback received so far, but should not >>>>>>> mean >>>>>>> burden the platform port and don't implement in better tools. >>>>>>> >>>>>>>> >>>>>>>>>> 3) Use the C pre processor with PP_FLAGS or ASLPP_FLAGS to handle the >> include >>>>>>>>>> infrastructure for maximum compatibility. Just like other rules in >>>>>>>>>> Conf/build_rule.txt. ?.pcd could be added to build_rule.txt. >>>>>>>>> >>>>>>>>> That is part of the current proposal. >>>>>>>>> >>>>>>>>>> 4) Don't add new PcdLib APIs. Return a pointer to the structure and >>>>>>>>>> use C >> code >>>>>> to >>>>>>>>>> access the data. >>>>>>>>> >>>>>>>>> That is supported today without this proposal. >>>>>>>>> >>>>>>>>> By adding new APIs, we can support access to individual fields within >>>>>>>>> a >>>>>> Structured >>>>>>>>> PCD without having to require a local variable of the full C typedef. >>>>>>>> >>>>>>>> I don't see that as solving a real problem. From a walk up an use >>>>>>>> standpoint >> I >>>>>>>> initialize a C structure in a file, there is an API to return that >>>>>>>> structure >> and I >>>>>>>> dereference it with a pointer. That is how you write C code in any >> environment. >>>>>> Thus >>>>>>>> why do we need to invent another way to do this? >>>>>>>> >>>>>>>>> For Dynamic >>>>>>>>> or DynamicEx PCDs, individual fields can be accessed without having to >> read/write >>>>>>>>> the entire structure. >>>>>>>> >>>>>>>> That could be an implementation optimization. >>>>>>>> >>>>>>>>> >>>>>>>>>> 5) OK someone is going to complain about type checking so add a new >>>>>>>>>> version >> of >>>>>>>>>> PcdGetPtr() that casts the pointer to the C pointer type returned. >>>>>>>>>> The type >> is >>>>>> in >>>>>>>> the >>>>>>>>>> AutoGen.h file (_PCD_GET_PTR_TYPE_##TokenName). If you don't use the >>>>>>>>>> new >> .pcd >>>>>> file >>>>>>>>>> syntax then you get a build break. >>>>>>>>>> 6) Don't be too restrictive on type checking. Support something like >>>>>>>>>> a >> device >>>>>>>> path. >>>>>>>>>> For example the globals in PlatformData.c of PlatformBdsLib could be >>>>>>>>>> PCD >>>>>> entries. >>>>>>>>>> This could be phase 2, and require a modified syntax if needed. >>>>>>>>> >>>>>>>>> This could be supported today using a VOID* PCD for each of this >>>>>>>>> structures. >>>>>>>>> Configuring through PCDs in a DSC file today as list of hex bytes is >>>>>>>>> not >> very >>>>>>>>> developer friendly. I agree that a phase 2 could help improve the >>>>>>>>> developer >>>>>>>>> experience configuring a VOID* buffer that is a sequence of Device >>>>>>>>> Path >> nodes. >>>>>>>>> Maybe even support the Device Path To String syntax a value syntax >> extension. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> This simpler scheme only requires initializing a global variable and >>>>>>>>>> being >> able >>>>>> to >>>>>>>>>> extract that information vs AST. The real value in this scheme is >>>>>>>>>> unifying >> the >>>>>>>>>> include infrastructure for C, ASL, and PCD. C can already access C >> structure >>>>>>>> members, >>>>>>>>>> seems like reinventing the wheel to come up with a complex scheme to >> abstract >>>>>> how >>>>>>>> to >>>>>>>>>> access C structure elements when a C pointer solves that problem. >>>>>>>>> >>>>>>>>> How would you initialize the global for a nested structure of many >>>>>>>>> types >> without >>>>>>>>> a parser? >>>>>>>>> How would you handle unions for a corner case when multiple union >>>>>>>>> members from the same union need to be initialized? >>>>>>>> >>>>>>>> Use the compiler we already have installed on the system as it is >>>>>>>> already >>>>>> configured >>>>>>>> to do the correct thing, and the produce and consumer are using the >>>>>>>> same >> tools. >>>>>>>> >>>>>>>> How are you going to deal with something like long (it is not the same >>>>>>>> width >> on >>>>>> all >>>>>>>> the IA32 compilers supported by edk2)? Or even structure packing >>>>>>>> rules? The >>>>>> default >>>>>>>> output of clang in not compatible with the EFI ABI and we have >>>>>>>> compiler flags >> and >>>>>>>> triples that fix that. >>>>>>> >>>>>>> I believe I have resolved all of the issues you raise here using >>>>>>> CLANG/AST. >> Type >>>>>>> 'long' is not supported as a leaf field type and generated an error in >>>>>>> the >>>>>> prototype. >>>>>>> The supported leaf field types are in this proposal. I use the same >>>>>> ProcessorBind.h >>>>>>> mappings for CLANG for the supported leaf field types and it all works >>>>>>> for me. >>>>>>> CLANG/AST is only used to parse a C structure in a .h file. It is not >>>>>>> used >> for >>>>>>> compiling code, so there are never any EFI ABI issues with this use. >>>>>>> CLANG >>>>>> supports >>>>>>> byte packing using the same #pragma pack syntax that is used in FW >>>>>>> sources and >> the >>>>>>> byte/bit offsets computed by CLANG match the byte/bit offsets from the >> compiler >>>>>>> used to compile FW. >>>>>>> >>>>>>> As I mentioned at the top of email, I will evaluate using the current >>>>>>> tool >> chain >>>>>> tag >>>>>>> instead of CLANG/AST. I am confident the results will be the same for >>>>>>> the >> concerns >>>>>>> you raise here. >>>>>>> >>>>>>>> >>>>>>>> So the syntax probably needs some work as I'm making this up as I go >>>>>>>> along >> but you >>>>>>>> could have a *.pcd file that looks like: >>>>>>>> >>>>>>>> #include <BdsPlatform.h> >>>>>>>> >>>>>>>> >>>>>>>> // @ PCDTYPE [PcdsFixedAtBuild.common] >>>>>>>> >>>>>>>> >>>>>>>> // @PCD >>>>>>>> >>>>>> >> gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|gUsbClassDeviceP >>>>>>>> ath >>>>>>>> USB_CLASS_FORMAT_DEVICE_PATH gUsbClassDevicePath = { >>>>>>>> { >>>>>>>> { >>>>>>>> MESSAGING_DEVICE_PATH, >>>>>>>> MSG_USB_CLASS_DP, >>>>>>>> { >>>>>>>> (UINT8) (sizeof (USB_CLASS_DEVICE_PATH)), >>>>>>>> (UINT8) ((sizeof (USB_CLASS_DEVICE_PATH)) >> 8) >>>>>>>> } >>>>>>>> }, >>>>>>>> 0xffff, // VendorId >>>>>>>> 0xffff, // ProductId >>>>>>>> CLASS_HID, // DeviceClass >>>>>>>> SUBCLASS_BOOT, // DeviceSubClass >>>>>>>> PROTOCOL_KEYBOARD // DeviceProtocol >>>>>>>> }, >>>>>>>> >>>>>>>> { >>>>>>>> END_DEVICE_PATH_TYPE, >>>>>>>> END_ENTIRE_DEVICE_PATH_SUBTYPE, >>>>>>>> { >>>>>>>> END_DEVICE_PATH_LENGTH, >>>>>>>> 0 >>>>>>>> } >>>>>>>> } >>>>>>>> }; >>>>>>>> >>>>>>>> You do a quick parse to extract the meta data from the file: @ PCDTYPE >>>>>>>> [PcdsFixedAtBuild.common] and @PCD >>>>>>>> >>>>>> >> gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|gUsbClassDeviceP >>>>>>>> ath >>>>>>>> AutoGen a C program to do what you want and append it to the .pcd file. >>>>>>>> Compile and run the AutoGen code to create the output that could be as >>>>>>>> simple >> as >>>>>> the >>>>>>>> current PCD Pointer syntax. >>>>>>>> >>>>>>>> autogen code output looks like: >>>>>>>> // cat *.pcd file contents. >>>>>>>> // AutoGen Start. >>>>>>>> #include <stdio.h> >>>>>>>> >>>>>>>> int main() { >>>>>>>> char *c; >>>>>>>> int i; >>>>>>>> >>>>>>>> // @ PCDTYPE [PcdsFixedAtBuild.common] >>>>>>>> printf (" [PcdsFixedAtBuild.common]\n"); >>>>>>>> // @PCD >>>>>>>> >>>>>> >> gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|gUsbClassDeviceP >>>>>>>> ath >>>>>>>> printf >>>>>> ("gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|{"); >>>>>>>> for (i=0, c = (char *)(unsigned long long) &gUsbClassDevicePath; i < >>>>>>>> sizeof >>>>>>>> (gUsbClassDevicePath); i++) { >>>>>>>> printf (" 0x%02x,", c[i]); >>>>>>>> } >>>>>>>> printf (" }\n"); >>>>>>>> >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> Thus basically use the same compiler on both sides, and some slightly >> modified >>>>>> form >>>>>>>> of the current PCD syntax (add the type). You could actually do this >>>>>>>> today >> with an >>>>>>>> INF file and a custom rule in the Conf/build_rule.txt. The only >>>>>>>> modification >> to >>>>>> the >>>>>>>> build system would adding the output files to the PCD database >>>>>>>> creation. >> There are >>>>>>>> other ways to do it but I'm a fan of making it work like C and ASL, as >>>>>>>> I >> think the >>>>>>>> symmetry is intuitive for the developer, and you don't need to learn a >>>>>>>> new >> way to >>>>>>>> specify packages, BuildOptions etc. as you are using the existing INF >>>>>>>> syntax >> to >>>>>> build >>>>>>>> and process for all file types. >>>>>>>> >>>>>>>> Another option is to abstract all the meta data in a C macro and >>>>>>>> follow the >> macro >>>>>>>> with the = {}; data. That is what we did, basically 1st pass the macro >> expands to >>>>>> the >>>>>>>> global. 2nd pass the macro expands to the code to write out the date. >>>>>>>> You >> still >>>>>> have >>>>>>>> to strip to bits you don't want before you compile the program (you >>>>>>>> don't >> want 2 >>>>>>>> copies of all the preprocessed include stuff etc. >>>>>>>> >>>>>>>> So my point is you can get an 80% solution that is rock solid with zero >>>>>> constraints >>>>>>>> without going down the AST route, have it be a lot simpler, and not >>>>>>>> require a >>>>>> bunch >>>>>>>> of extra tools to be installed. I'm also a fan of using a new file >>>>>>>> type and >> an INF >>>>>>>> file vs. coming up with yet another edk2 unique syntax to do something >>>>>>>> the >> edk2 >>>>>>>> already has a syntax to support. >>>>>>>> >>>>>>>> Ironically we asked the clang developers (that wrote the AST stuff) >>>>>>>> what was >> the >>>>>>>> safest way to do something like this and they recommended using the >>>>>>>> compiler >> in >>>>>> this >>>>>>>> fashion of my example. Our example was slightly more complex as we >> constructed an >>>>>>>> entire database, but close enough (no issue with the packing between >> structures in >>>>>>>> the PCD case). >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Andrew Fish >>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> "The real art is knowing what to leave out, not what to put in" >>>>>>>>>> quote from >> Steve >>>>>>>>>> Jobs. >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> >>>>>>>>>> Andrew Fish >>>>>>>>>> >>>>>>>>>> _______________________________________________ >>>>>>>>>> edk2-devel mailing list >>>>>>>>>> [email protected] <mailto:[email protected]> >>>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >> <https://lists.01.org/mailman/listinfo/edk2-devel> >>>>>>>>> _______________________________________________ >>>>>>>>> edk2-devel mailing list >>>>>>>>> [email protected] <mailto:[email protected]> >>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >> <https://lists.01.org/mailman/listinfo/edk2-devel> >>> _______________________________________________ >>> edk2-devel mailing list >>> [email protected] <mailto:[email protected]> >>> https://lists.01.org/mailman/listinfo/edk2-devel >> <https://lists.01.org/mailman/listinfo/edk2-devel> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
