[PATCH 0/3] Use central definitions for EFI_VARIABLE_*

[PATCH 0/3] Use central definitions for EFI_VARIABLE_*

[Leif Lindholm]

The set of variable attribute definitions in <Uefi/UefiMultiPhase.h> is
used by C code, but VfrCompile has no way of dealing with structs or
typedefs, and the VFRPP rules generate (and depend on) preprocessing with
C rules.

There may be neater ways of dealing with this, but a simple solution is
to break the #defines into a separate header and include this both in
UefiMultiPhase.h and directly in .vfr source.

Leif Lindholm (3):
  MdePkg: break #defines out of Uefi/UefiMultiPhase.h
  MdeModulePkg: use central variable definitions in DriverSampleDxe
  EmbeddedPkg: use central variable definitions in .vfr files

 EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr |  9 +----
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr   |  9 +----
 MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr        |  9 +----
 MdePkg/Include/Uefi/UefiMultiPhase.h                  | 23 +-----------
 MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h       | 39 ++++++++++++++++++++
 5 files changed, 44 insertions(+), 45 deletions(-)
 create mode 100644 MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.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>

-- 
2.11.0

[PATCH 1/3] MdePkg: break #defines out of Uefi/UefiMultiPhase.h

[Leif Lindholm]

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

Re: [PATCH 1/3] MdePkg: break #defines out of Uefi/UefiMultiPhase.h

[Ni, Ruiyu]

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

Re: [PATCH 1/3] MdePkg: break #defines out of Uefi/UefiMultiPhase.h

[Leif Lindholm]

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

Re: [PATCH 1/3] MdePkg: break #defines out of Uefi/UefiMultiPhase.h

[Leif Lindholm]

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

Re: [PATCH 1/3] MdePkg: break #defines out of Uefi/UefiMultiPhase.h

[Gao, Liming]

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

[PATCH 2/3] MdeModulePkg: use central variable definitions in DriverSampleDxe

[Leif Lindholm]

Use UefiMultiPhaseDefinitions.h in Vfr.vfr instead of duplicating
the definitions.

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>
---
 MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr b/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr
index c1682913fa..551d78c15f 100644
--- a/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr
+++ b/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr
@@ -14,6 +14,7 @@
 //**/
 
 
+#include <Uefi/UefiMultiPhaseDefinitions.h>
 #include "NVDataStruc.h"
 
 //
@@ -35,14 +36,6 @@
 #define EFI_FRONT_PAGE_SUBCLASS           0x02
 #define EFI_SINGLE_USE_SUBCLASS           0x03
 
-//
-// EFI Variable attributes
-//
-#define EFI_VARIABLE_NON_VOLATILE       0x00000001
-#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
-#define EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
-#define EFI_VARIABLE_READ_ONLY          0x00000008
-
 #define EFI_USER_INFO_ACCESS_SETUP_ADMIN_GUID \
   { 0x85b75607, 0xf7ce, 0x471e, { 0xb7, 0xe4, 0x2a, 0xea, 0x5f, 0x72, 0x32, 0xee } }
 
-- 
2.11.0

Re: [PATCH 2/3] MdeModulePkg: use central variable definitions in DriverSampleDxe

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com> to MdeModulePkg change.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif Lindholm
Sent: Wednesday, December 13, 2017 8:26 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH 2/3] MdeModulePkg: use central variable definitions in DriverSampleDxe

Use UefiMultiPhaseDefinitions.h in Vfr.vfr instead of duplicating the definitions.

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>
---
 MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr b/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr
index c1682913fa..551d78c15f 100644
--- a/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr
+++ b/MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr
@@ -14,6 +14,7 @@
 //**/
 
 
+#include <Uefi/UefiMultiPhaseDefinitions.h>
 #include "NVDataStruc.h"
 
 //
@@ -35,14 +36,6 @@
 #define EFI_FRONT_PAGE_SUBCLASS           0x02
 #define EFI_SINGLE_USE_SUBCLASS           0x03
 
-//
-// EFI Variable attributes
-//
-#define EFI_VARIABLE_NON_VOLATILE       0x00000001
-#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
-#define EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
-#define EFI_VARIABLE_READ_ONLY          0x00000008
-
 #define EFI_USER_INFO_ACCESS_SETUP_ADMIN_GUID \
   { 0x85b75607, 0xf7ce, 0x471e, { 0xb7, 0xe4, 0x2a, 0xea, 0x5f, 0x72, 0x32, 0xee } }
 
--
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 3/3] EmbeddedPkg: use central variable definitions in .vfr files

[Leif Lindholm]

Use UefiMultiPhaseDefinitions.h in Vfr.vfr instead of duplicating
the definitions.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr | 9 +--------
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr   | 9 +--------
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr
index a1e603abf0..19371b3157 100644
--- a/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr
+++ b/EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr
@@ -12,16 +12,9 @@
 *
 **/
 
+#include <Uefi/UefiMultiPhaseDefinitions.h>
 #include "ConsolePrefDxe.h"
 
-//
-// EFI Variable attributes
-//
-#define EFI_VARIABLE_NON_VOLATILE       0x00000001
-#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
-#define EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
-#define EFI_VARIABLE_READ_ONLY          0x00000008
-
 formset
   guid      = CONSOLE_PREF_FORMSET_GUID,
   title     = STRING_TOKEN(STR_FORM_SET_TITLE),
diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr
index 3516746c4d..8e5d34dadb 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr
@@ -12,16 +12,9 @@
 *
 **/
 
+#include <Uefi/UefiMultiPhaseDefinitions.h>
 #include "DtPlatformDxe.h"
 
-//
-// EFI Variable attributes
-//
-#define EFI_VARIABLE_NON_VOLATILE       0x00000001
-#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
-#define EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
-#define EFI_VARIABLE_READ_ONLY          0x00000008
-
 formset
   guid      = DT_PLATFORM_FORMSET_GUID,
   title     = STRING_TOKEN(STR_FORM_SET_TITLE),
-- 
2.11.0

Re: [PATCH 0/3] Use central definitions for EFI_VARIABLE_*

[Ard Biesheuvel]

On 13 December 2017 at 12:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> The set of variable attribute definitions in <Uefi/UefiMultiPhase.h> is
> used by C code, but VfrCompile has no way of dealing with structs or
> typedefs, and the VFRPP rules generate (and depend on) preprocessing with
> C rules.
>
> There may be neater ways of dealing with this, but a simple solution is
> to break the #defines into a separate header and include this both in
> UefiMultiPhase.h and directly in .vfr source.
>
> Leif Lindholm (3):
>   MdePkg: break #defines out of Uefi/UefiMultiPhase.h
>   MdeModulePkg: use central variable definitions in DriverSampleDxe
>   EmbeddedPkg: use central variable definitions in .vfr files
>

For the series

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefHii.vfr |  9 +----
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr   |  9 +----
>  MdeModulePkg/Universal/DriverSampleDxe/Vfr.vfr        |  9 +----
>  MdePkg/Include/Uefi/UefiMultiPhase.h                  | 23 +-----------
>  MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h       | 39 ++++++++++++++++++++
>  5 files changed, 44 insertions(+), 45 deletions(-)
>  create mode 100644 MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.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>
>
> --
> 2.11.0
>