| Message ID | 20171213122630.17023-2-leif.lindholm@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use central definitions for EFI_VARIABLE_* | expand |
On 12/13/2017 8:26 PM, Leif Lindholm wrote: > Turns out all .vfr files in the tree interacting with DynamicPcds > manually copy the same set of EFI_VARIABLE_* definitions, since the rest > of UefiMultiPhase.h is incompatible with VfrCompile. > > Split these out into a separate header file UefiMultiPhaseDefinitions.h > in order to make it possible to include just that portion into .vfr > files. Then include that from UefiMultiPhase.h. > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > MdePkg/Include/Uefi/UefiMultiPhase.h | 23 +----------- > MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h | 39 ++++++++++++++++++++ > 2 files changed, 41 insertions(+), 21 deletions(-) > > diff --git a/MdePkg/Include/Uefi/UefiMultiPhase.h b/MdePkg/Include/Uefi/UefiMultiPhase.h > index 0dcbb1b9ee..b360c9513b 100644 > --- a/MdePkg/Include/Uefi/UefiMultiPhase.h > +++ b/MdePkg/Include/Uefi/UefiMultiPhase.h > @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > #ifndef __UEFI_MULTIPHASE_H__ > #define __UEFI_MULTIPHASE_H__ > > +#include "UefiMultiPhaseDefinitions.h" > + > #include <Guid/WinCertificate.h> > /// > /// Enumeration of memory types introduced in UEFI. > @@ -156,27 +158,6 @@ typedef struct { > } EFI_TABLE_HEADER; > > /// > -/// Attributes of variable. > -/// > -#define EFI_VARIABLE_NON_VOLATILE 0x00000001 > -#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002 > -#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004 > -/// > -/// This attribute is identified by the mnemonic 'HR' > -/// elsewhere in this specification. > -/// > -#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x00000008 > -/// > -/// Attributes of Authenticated Variable > -/// > -#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x00000020 > -#define EFI_VARIABLE_APPEND_WRITE 0x00000040 > -/// > -/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved. > -/// > -#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x00000010 > - > -/// > /// AuthInfo is a WIN_CERTIFICATE using the wCertificateType > /// WIN_CERTIFICATE_UEFI_GUID and the CertType > /// EFI_CERT_TYPE_RSA2048_SHA256_GUID. If the attribute specifies > diff --git a/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h > new file mode 100644 > index 0000000000..df55a92dfa > --- /dev/null > +++ b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h > @@ -0,0 +1,39 @@ > +/** @file > + This includes some definitions introduced in UEFI that will be used in both PEI and DXE phases. > + > +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR> > +This program and the accompanying materials are licensed and made available under > +the terms and conditions of the BSD License that accompanies this distribution. > +The full text of the license may be found at > +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 __UEFI_MULTIPHASE_DEFS_H__ > +#define __UEFI_MULTIPHASE_DEFS_H__ > + > +/// > +/// Attributes of variable. > +/// > +#define EFI_VARIABLE_NON_VOLATILE 0x00000001 > +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002 > +#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004 > +/// > +/// This attribute is identified by the mnemonic 'HR' > +/// elsewhere in this specification. > +/// > +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x00000008 > +/// > +/// Attributes of Authenticated Variable > +/// > +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x00000020 > +#define EFI_VARIABLE_APPEND_WRITE 0x00000040 > +/// > +/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved. > +/// > +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x00000010 > + > +#endif > Can we just move the definitions to UefiBaseTypes.h? Even vfrcompiler still complains, the syntax enhancement to vfrcompiler should be simple. I personally do not like creating more and more files. -- Thanks, Ray _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, Dec 14, 2017 at 10:44:04AM +0800, Ni, Ruiyu wrote: > On 12/13/2017 8:26 PM, Leif Lindholm wrote: > > Turns out all .vfr files in the tree interacting with DynamicPcds > > manually copy the same set of EFI_VARIABLE_* definitions, since the rest > > of UefiMultiPhase.h is incompatible with VfrCompile. > > > > Split these out into a separate header file UefiMultiPhaseDefinitions.h > > in order to make it possible to include just that portion into .vfr > > files. Then include that from UefiMultiPhase.h. > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > --- > > MdePkg/Include/Uefi/UefiMultiPhase.h | 23 +----------- > > MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h | 39 ++++++++++++++++++++ > > 2 files changed, 41 insertions(+), 21 deletions(-) > > > > diff --git a/MdePkg/Include/Uefi/UefiMultiPhase.h b/MdePkg/Include/Uefi/UefiMultiPhase.h > > index 0dcbb1b9ee..b360c9513b 100644 > > --- a/MdePkg/Include/Uefi/UefiMultiPhase.h > > +++ b/MdePkg/Include/Uefi/UefiMultiPhase.h > > @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > #ifndef __UEFI_MULTIPHASE_H__ > > #define __UEFI_MULTIPHASE_H__ > > +#include "UefiMultiPhaseDefinitions.h" > > + > > #include <Guid/WinCertificate.h> > > /// > > /// Enumeration of memory types introduced in UEFI. > > @@ -156,27 +158,6 @@ typedef struct { > > } EFI_TABLE_HEADER; > > /// > > -/// Attributes of variable. > > -/// > > -#define EFI_VARIABLE_NON_VOLATILE 0x00000001 > > -#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002 > > -#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004 > > -/// > > -/// This attribute is identified by the mnemonic 'HR' > > -/// elsewhere in this specification. > > -/// > > -#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x00000008 > > -/// > > -/// Attributes of Authenticated Variable > > -/// > > -#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x00000020 > > -#define EFI_VARIABLE_APPEND_WRITE 0x00000040 > > -/// > > -/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved. > > -/// > > -#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x00000010 > > - > > -/// > > /// AuthInfo is a WIN_CERTIFICATE using the wCertificateType > > /// WIN_CERTIFICATE_UEFI_GUID and the CertType > > /// EFI_CERT_TYPE_RSA2048_SHA256_GUID. If the attribute specifies > > diff --git a/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h > > new file mode 100644 > > index 0000000000..df55a92dfa > > --- /dev/null > > +++ b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h > > @@ -0,0 +1,39 @@ > > +/** @file > > + This includes some definitions introduced in UEFI that will be used in both PEI and DXE phases. > > + > > +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR> > > +This program and the accompanying materials are licensed and made available under > > +the terms and conditions of the BSD License that accompanies this distribution. > > +The full text of the license may be found at > > +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 __UEFI_MULTIPHASE_DEFS_H__ > > +#define __UEFI_MULTIPHASE_DEFS_H__ > > + > > +/// > > +/// Attributes of variable. > > +/// > > +#define EFI_VARIABLE_NON_VOLATILE 0x00000001 > > +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002 > > +#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004 > > +/// > > +/// This attribute is identified by the mnemonic 'HR' > > +/// elsewhere in this specification. > > +/// > > +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x00000008 > > +/// > > +/// Attributes of Authenticated Variable > > +/// > > +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x00000020 > > +#define EFI_VARIABLE_APPEND_WRITE 0x00000040 > > +/// > > +/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved. > > +/// > > +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x00000010 > > + > > +#endif > > > Can we just move the definitions to UefiBaseTypes.h? There is nothing in UefiBaseTypes.h that is fundamentally easier to deal with than what is in UefiMultiPhase.h to begin with. > Even vfrcompiler still complains, the syntax enhancement to > vfrcompiler should be simple. It's (probably) not VfrCompile that would need to change. The problem is that the build runs the C preprocessor on the .vfr source files before passing them to VfrCompile. #defines that just replace text work fine, but typedefs and struct definitions make no sense in vfr source. It runs the preprocessor in C mode (for gpp, -x c) which means we cannot use __ASSEMBLER__ to filter out typedefs and struct definitions. We could perhaps switch to -x assembler-with-cpp (and filter UefiMultiPhase.h on __ASSEMBLER__), like we do for DTC. It _seems_ to work in my test, but I do not know what a corresponding change for VS would be. > I personally do not like creating more and more files. I am sort of with you on that one. But I consider it a much lesser evil than duplicated definitions. / Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, Dec 14, 2017 at 12:03:56PM +0000, Leif Lindholm wrote: > On Thu, Dec 14, 2017 at 10:44:04AM +0800, Ni, Ruiyu wrote: > > Can we just move the definitions to UefiBaseTypes.h? > > There is nothing in UefiBaseTypes.h that is fundamentally easier to > deal with than what is in UefiMultiPhase.h to begin with. > > > Even vfrcompiler still complains, the syntax enhancement to > > vfrcompiler should be simple. > > It's (probably) not VfrCompile that would need to change. > The problem is that the build runs the C preprocessor on the .vfr > source files before passing them to VfrCompile. #defines that just > replace text work fine, but typedefs and struct definitions make no > sense in vfr source. > > It runs the preprocessor in C mode (for gpp, -x c) which means we > cannot use __ASSEMBLER__ to filter out typedefs and struct > definitions. > > We could perhaps switch to -x assembler-with-cpp (and filter > UefiMultiPhase.h on __ASSEMBLER__), like we do for DTC. > It _seems_ to work in my test, but I do not know what a corresponding > change for VS would be. Well, this turns out to be unworkable anyway, since the affected .vfr source files depend on certain typedefs and struct definitions. I was under the misconception that only #defines were used in .vfr. So, yes, the better solution would be to teach VfrCompile to understand enums and other things, but I wouldn't really know where to start (and I'm about to disappear for a month). Symptoms when building MdeModulePkg DriverSampleDxe with UefiMultiPhase.h include ERROR 12288: EFI_GUID : undefined ERROR 12288: enum : unexpected token If I then add UefiBaseTypes.h, we insted have issues from the inclusion of ProcessorBind.h: ERROR 12288: unsigned : unexpected token ERROR 12288: long : unexpected token and so on. An alternative less invasive solution, which would avoid adding new files, would be to add #ifndef VFRCOMPILE around everything other than the EFI_VARIABLE_* #defines in UefiMultiPhase.h. Would you be happier with that as a temporary measure until someone gets around to looking at VfrCompile? / Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Leif: Right. Current VfrCompiler supports the limited C syntax. Many C syntax are not recognized. So, you will meet with the below build error. For this change, I agree to share the definition between C and VFR source files. The temp solution is to use #ifndef VFRCOMPILE around the definition. The long term solution is to enhance VfrCompiler tool to recognize more C syntax so that C and VFR can share more definitions. For long term solution, you can submit feature request in bugzillia to catch it first. Thanks Liming > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Friday, December 15, 2017 1:00 AM > To: Ni, Ruiyu <ruiyu.ni@intel.com> > Cc: edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star > <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> > Subject: Re: [edk2] [PATCH 1/3] MdePkg: break #defines out of Uefi/UefiMultiPhase.h > > On Thu, Dec 14, 2017 at 12:03:56PM +0000, Leif Lindholm wrote: > > On Thu, Dec 14, 2017 at 10:44:04AM +0800, Ni, Ruiyu wrote: > > > Can we just move the definitions to UefiBaseTypes.h? > > > > There is nothing in UefiBaseTypes.h that is fundamentally easier to > > deal with than what is in UefiMultiPhase.h to begin with. > > > > > Even vfrcompiler still complains, the syntax enhancement to > > > vfrcompiler should be simple. > > > > It's (probably) not VfrCompile that would need to change. > > The problem is that the build runs the C preprocessor on the .vfr > > source files before passing them to VfrCompile. #defines that just > > replace text work fine, but typedefs and struct definitions make no > > sense in vfr source. > > > > It runs the preprocessor in C mode (for gpp, -x c) which means we > > cannot use __ASSEMBLER__ to filter out typedefs and struct > > definitions. > > > > We could perhaps switch to -x assembler-with-cpp (and filter > > UefiMultiPhase.h on __ASSEMBLER__), like we do for DTC. > > It _seems_ to work in my test, but I do not know what a corresponding > > change for VS would be. > > Well, this turns out to be unworkable anyway, since the affected > .vfr source files depend on certain typedefs and struct definitions. > I was under the misconception that only #defines were used in .vfr. > > So, yes, the better solution would be to teach VfrCompile to > understand enums and other things, but I wouldn't really know where to > start (and I'm about to disappear for a month). > > Symptoms when building MdeModulePkg DriverSampleDxe with > UefiMultiPhase.h include > > ERROR 12288: EFI_GUID > : undefined > ERROR 12288: enum > : unexpected token > > If I then add UefiBaseTypes.h, we insted have issues from the > inclusion of ProcessorBind.h: > > ERROR 12288: unsigned > : unexpected token > ERROR 12288: long > : unexpected token > > and so on. > > An alternative less invasive solution, which would avoid adding new > files, would be to add #ifndef VFRCOMPILE around everything other > than the EFI_VARIABLE_* #defines in UefiMultiPhase.h. > > Would you be happier with that as a temporary measure until someone > gets around to looking at VfrCompile? > > / > Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdePkg/Include/Uefi/UefiMultiPhase.h b/MdePkg/Include/Uefi/UefiMultiPhase.h index 0dcbb1b9ee..b360c9513b 100644 --- a/MdePkg/Include/Uefi/UefiMultiPhase.h +++ b/MdePkg/Include/Uefi/UefiMultiPhase.h @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #ifndef __UEFI_MULTIPHASE_H__ #define __UEFI_MULTIPHASE_H__ +#include "UefiMultiPhaseDefinitions.h" + #include <Guid/WinCertificate.h> /// /// Enumeration of memory types introduced in UEFI. @@ -156,27 +158,6 @@ typedef struct { } EFI_TABLE_HEADER; /// -/// Attributes of variable. -/// -#define EFI_VARIABLE_NON_VOLATILE 0x00000001 -#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002 -#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004 -/// -/// This attribute is identified by the mnemonic 'HR' -/// elsewhere in this specification. -/// -#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x00000008 -/// -/// Attributes of Authenticated Variable -/// -#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x00000020 -#define EFI_VARIABLE_APPEND_WRITE 0x00000040 -/// -/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved. -/// -#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x00000010 - -/// /// AuthInfo is a WIN_CERTIFICATE using the wCertificateType /// WIN_CERTIFICATE_UEFI_GUID and the CertType /// EFI_CERT_TYPE_RSA2048_SHA256_GUID. If the attribute specifies diff --git a/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h new file mode 100644 index 0000000000..df55a92dfa --- /dev/null +++ b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h @@ -0,0 +1,39 @@ +/** @file + This includes some definitions introduced in UEFI that will be used in both PEI and DXE phases. + +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR> +This program and the accompanying materials are licensed and made available under +the terms and conditions of the BSD License that accompanies this distribution. +The full text of the license may be found at +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 __UEFI_MULTIPHASE_DEFS_H__ +#define __UEFI_MULTIPHASE_DEFS_H__ + +/// +/// Attributes of variable. +/// +#define EFI_VARIABLE_NON_VOLATILE 0x00000001 +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002 +#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004 +/// +/// This attribute is identified by the mnemonic 'HR' +/// elsewhere in this specification. +/// +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x00000008 +/// +/// Attributes of Authenticated Variable +/// +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x00000020 +#define EFI_VARIABLE_APPEND_WRITE 0x00000040 +/// +/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved. +/// +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x00000010 + +#endif
Turns out all .vfr files in the tree interacting with DynamicPcds manually copy the same set of EFI_VARIABLE_* definitions, since the rest of UefiMultiPhase.h is incompatible with VfrCompile. Split these out into a separate header file UefiMultiPhaseDefinitions.h in order to make it possible to include just that portion into .vfr files. Then include that from UefiMultiPhase.h. Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Star Zeng <star.zeng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> --- MdePkg/Include/Uefi/UefiMultiPhase.h | 23 +----------- MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h | 39 ++++++++++++++++++++ 2 files changed, 41 insertions(+), 21 deletions(-) -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel