Update Tpm12.h and Tpm20.h and not use c++ reserved keywords
operator and xor in C structures to support use of these
include files when building with a C++ compiler.

This patch temporarily introduces an anonymous union to add
Operator and Xor fields to support migration from the current
field names to the new field names.

Warning 4201 is disabled for VS20xx tool chains is a temporary
change to allow the use of anonymous unions.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Aaron Pop <aaronpop@microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdePkg/Include/IndustryStandard/Tpm12.h | 22 ++++++++++++++++++++--
 MdePkg/Include/IndustryStandard/Tpm20.h | 25 +++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h b/MdePkg/Include/IndustryStandard/Tpm12.h
index 155dcc9f5f99..56e89d9d0835 100644
--- a/MdePkg/Include/IndustryStandard/Tpm12.h
+++ b/MdePkg/Include/IndustryStandard/Tpm12.h
@@ -9,6 +9,14 @@
 #ifndef _TPM12_H_
 #define _TPM12_H_
 
+///
+/// Temporary disable 4201 to support anonymous unions
+///
+#if defined (_MSC_EXTENSIONS)
+#pragma warning( push )
+#pragma warning ( disable : 4201 )
+#endif
+
 ///
 /// The start of TPM return codes
 ///
@@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS {
   BOOLEAN              TPMpost;
   BOOLEAN              TPMpostLock;
   BOOLEAN              FIPS;
-  BOOLEAN                           operator;
-  BOOLEAN                           enableRevokeEK;
+  union {
+    BOOLEAN            operator;
+    BOOLEAN            Operator;
+  };
+  BOOLEAN              enableRevokeEK;
   BOOLEAN              nvLocked;
   BOOLEAN              readSRKPub;
   BOOLEAN              tpmEstablished;
@@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR {
 
 #pragma pack ()
 
+///
+/// Temporary disable 4201 to support anonymous unions
+///
+#if defined (_MSC_EXTENSIONS)
+#pragma warning( pop )
+#endif
+
 #endif
diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h b/MdePkg/Include/IndustryStandard/Tpm20.h
index 4440f3769f26..a602c0d9c289 100644
--- a/MdePkg/Include/IndustryStandard/Tpm20.h
+++ b/MdePkg/Include/IndustryStandard/Tpm20.h
@@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <IndustryStandard/Tpm12.h>
 
+///
+/// Temporary disable 4201 to support anonymous unions
+///
+#if defined (_MSC_EXTENSIONS)
+#pragma warning( push )
+#pragma warning ( disable : 4201 )
+#endif
+
 #pragma pack (1)
 
 // Annex A Algorithm Constants
@@ -1247,7 +1255,10 @@ typedef union {
   TPMI_AES_KEY_BITS    aes;
   TPMI_SM4_KEY_BITS    SM4;
   TPM_KEY_BITS         sym;
-  TPMI_ALG_HASH     xor;
+  union {
+    TPMI_ALG_HASH      xor;
+    TPMI_ALG_HASH      Xor;
+  };
 } TPMU_SYM_KEY_BITS;
 
 // Table 123 - TPMU_SYM_MODE Union
@@ -1320,7 +1331,10 @@ typedef struct {
 // Table 136 - TPMU_SCHEME_KEYEDHASH Union
 typedef union {
   TPMS_SCHEME_HMAC    hmac;
-  TPMS_SCHEME_XOR  xor;
+  union {
+    TPMS_SCHEME_XOR   xor;
+    TPMS_SCHEME_XOR   Xor;
+  };
 } TPMU_SCHEME_KEYEDHASH;
 
 // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
@@ -1809,4 +1823,11 @@ typedef struct {
 #define HASH_ALG_SHA512   0x00000008
 #define HASH_ALG_SM3_256  0x00000010
 
+///
+/// Temporary disable 4201 to support anonymous unions
+///
+#if defined (_MSC_EXTENSIONS)
+#pragma warning( pop )
+#endif
+
 #endif
-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105466): https://edk2.groups.io/g/devel/message/105466
Mute This Topic: https://groups.io/mt/99226543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, May 30, 2023 at 7:53 PM Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Update Tpm12.h and Tpm20.h and not use c++ reserved keywords
> operator and xor in C structures to support use of these
> include files when building with a C++ compiler.
>
> This patch temporarily introduces an anonymous union to add
> Operator and Xor fields to support migration from the current
> field names to the new field names.
>
> Warning 4201 is disabled for VS20xx tool chains is a temporary
> change to allow the use of anonymous unions.
>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Aaron Pop <aaronpop@microsoft.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdePkg/Include/IndustryStandard/Tpm12.h | 22 ++++++++++++++++++++--
>  MdePkg/Include/IndustryStandard/Tpm20.h | 25 +++++++++++++++++++++++--
>  2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h b/MdePkg/Include/IndustryStandard/Tpm12.h
> index 155dcc9f5f99..56e89d9d0835 100644
> --- a/MdePkg/Include/IndustryStandard/Tpm12.h
> +++ b/MdePkg/Include/IndustryStandard/Tpm12.h
> @@ -9,6 +9,14 @@
>  #ifndef _TPM12_H_
>  #define _TPM12_H_
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( push )
> +#pragma warning ( disable : 4201 )
> +#endif
> +
>  ///
>  /// The start of TPM return codes
>  ///
> @@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS {
>    BOOLEAN              TPMpost;
>    BOOLEAN              TPMpostLock;
>    BOOLEAN              FIPS;
> -  BOOLEAN                           operator;
> -  BOOLEAN                           enableRevokeEK;
> +  union {
> +    BOOLEAN            operator;
> +    BOOLEAN            Operator;
> +  };

Do you know if this works cleanly for the other toolchains? i.e
supported GCCs and CLANGs?
I don't *think* there's a warning for anonymous unions beyond passing
-pedantic + -std=c<something>, but it'd be good to know.
If so, we may need a pragma for this.

> +  BOOLEAN              enableRevokeEK;
>    BOOLEAN              nvLocked;
>    BOOLEAN              readSRKPub;
>    BOOLEAN              tpmEstablished;
> @@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR {
>
>  #pragma pack ()
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( pop )
> +#endif
> +
>  #endif
> diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h b/MdePkg/Include/IndustryStandard/Tpm20.h
> index 4440f3769f26..a602c0d9c289 100644
> --- a/MdePkg/Include/IndustryStandard/Tpm20.h
> +++ b/MdePkg/Include/IndustryStandard/Tpm20.h
> @@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  #include <IndustryStandard/Tpm12.h>
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( push )
> +#pragma warning ( disable : 4201 )
> +#endif
> +
>  #pragma pack (1)
>
>  // Annex A Algorithm Constants
> @@ -1247,7 +1255,10 @@ typedef union {
>    TPMI_AES_KEY_BITS    aes;
>    TPMI_SM4_KEY_BITS    SM4;
>    TPM_KEY_BITS         sym;
> -  TPMI_ALG_HASH     xor;
> +  union {
> +    TPMI_ALG_HASH      xor;
> +    TPMI_ALG_HASH      Xor;
> +  };
>  } TPMU_SYM_KEY_BITS;
>
>  // Table 123 - TPMU_SYM_MODE Union
> @@ -1320,7 +1331,10 @@ typedef struct {
>  // Table 136 - TPMU_SCHEME_KEYEDHASH Union
>  typedef union {
>    TPMS_SCHEME_HMAC    hmac;
> -  TPMS_SCHEME_XOR  xor;
> +  union {
> +    TPMS_SCHEME_XOR   xor;
> +    TPMS_SCHEME_XOR   Xor;
> +  };
>  } TPMU_SCHEME_KEYEDHASH;
>
>  // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
> @@ -1809,4 +1823,11 @@ typedef struct {
>  #define HASH_ALG_SHA512   0x00000008
>  #define HASH_ALG_SM3_256  0x00000010
>
> +///
> +/// Temporary disable 4201 to support anonymous unions
> +///
> +#if defined (_MSC_EXTENSIONS)
> +#pragma warning( pop )
> +#endif
> +
>  #endif
> --
> 2.40.1.windows.1
>

All in all, this looks ok to me. Although I have to say, I'm not a
huge fan of the naming style inconsistency introduced here (i.e Xor vs
hmac).
What if we made all the struct members MixedCase? Or what if we did
something like calling them xor_ and operator_?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105510): https://edk2.groups.io/g/devel/message/105510
Mute This Topic: https://groups.io/mt/99226543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Wednesday, May 31, 2023 11:17 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; Oliver Smith-Denny
> <osde@linux.microsoft.com>; Pop, Aaron <aaronpop@microsoft.com>
> Subject: Re: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator
> and Xor field names
> 
> On Tue, May 30, 2023 at 7:53 PM Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > Update Tpm12.h and Tpm20.h and not use c++ reserved keywords
> > operator and xor in C structures to support use of these
> > include files when building with a C++ compiler.
> >
> > This patch temporarily introduces an anonymous union to add
> > Operator and Xor fields to support migration from the current
> > field names to the new field names.
> >
> > Warning 4201 is disabled for VS20xx tool chains is a temporary
> > change to allow the use of anonymous unions.
> >
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
> > Cc: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: Aaron Pop <aaronpop@microsoft.com>
> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  MdePkg/Include/IndustryStandard/Tpm12.h | 22 ++++++++++++++++++++--
> >  MdePkg/Include/IndustryStandard/Tpm20.h | 25 +++++++++++++++++++++++--
> >  2 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h
> b/MdePkg/Include/IndustryStandard/Tpm12.h
> > index 155dcc9f5f99..56e89d9d0835 100644
> > --- a/MdePkg/Include/IndustryStandard/Tpm12.h
> > +++ b/MdePkg/Include/IndustryStandard/Tpm12.h
> > @@ -9,6 +9,14 @@
> >  #ifndef _TPM12_H_
> >  #define _TPM12_H_
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( push )
> > +#pragma warning ( disable : 4201 )
> > +#endif
> > +
> >  ///
> >  /// The start of TPM return codes
> >  ///
> > @@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS {
> >    BOOLEAN              TPMpost;
> >    BOOLEAN              TPMpostLock;
> >    BOOLEAN              FIPS;
> > -  BOOLEAN                           operator;
> > -  BOOLEAN                           enableRevokeEK;
> > +  union {
> > +    BOOLEAN            operator;
> > +    BOOLEAN            Operator;
> > +  };
> 
> Do you know if this works cleanly for the other toolchains? i.e
> supported GCCs and CLANGs?
> I don't *think* there's a warning for anonymous unions beyond passing
> -pedantic + -std=c<something>, but it'd be good to know.
> If so, we may need a pragma for this.

I did not see any issues with my local testing or with EDK II CI.

> 
> > +  BOOLEAN              enableRevokeEK;
> >    BOOLEAN              nvLocked;
> >    BOOLEAN              readSRKPub;
> >    BOOLEAN              tpmEstablished;
> > @@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR {
> >
> >  #pragma pack ()
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( pop )
> > +#endif
> > +
> >  #endif
> > diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h
> b/MdePkg/Include/IndustryStandard/Tpm20.h
> > index 4440f3769f26..a602c0d9c289 100644
> > --- a/MdePkg/Include/IndustryStandard/Tpm20.h
> > +++ b/MdePkg/Include/IndustryStandard/Tpm20.h
> > @@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  #include <IndustryStandard/Tpm12.h>
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( push )
> > +#pragma warning ( disable : 4201 )
> > +#endif
> > +
> >  #pragma pack (1)
> >
> >  // Annex A Algorithm Constants
> > @@ -1247,7 +1255,10 @@ typedef union {
> >    TPMI_AES_KEY_BITS    aes;
> >    TPMI_SM4_KEY_BITS    SM4;
> >    TPM_KEY_BITS         sym;
> > -  TPMI_ALG_HASH     xor;
> > +  union {
> > +    TPMI_ALG_HASH      xor;
> > +    TPMI_ALG_HASH      Xor;
> > +  };
> >  } TPMU_SYM_KEY_BITS;
> >
> >  // Table 123 - TPMU_SYM_MODE Union
> > @@ -1320,7 +1331,10 @@ typedef struct {
> >  // Table 136 - TPMU_SCHEME_KEYEDHASH Union
> >  typedef union {
> >    TPMS_SCHEME_HMAC    hmac;
> > -  TPMS_SCHEME_XOR  xor;
> > +  union {
> > +    TPMS_SCHEME_XOR   xor;
> > +    TPMS_SCHEME_XOR   Xor;
> > +  };
> >  } TPMU_SCHEME_KEYEDHASH;
> >
> >  // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
> > @@ -1809,4 +1823,11 @@ typedef struct {
> >  #define HASH_ALG_SHA512   0x00000008
> >  #define HASH_ALG_SM3_256  0x00000010
> >
> > +///
> > +/// Temporary disable 4201 to support anonymous unions
> > +///
> > +#if defined (_MSC_EXTENSIONS)
> > +#pragma warning( pop )
> > +#endif
> > +
> >  #endif
> > --
> > 2.40.1.windows.1
> >
> 
> All in all, this looks ok to me. Although I have to say, I'm not a
> huge fan of the naming style inconsistency introduced here (i.e Xor vs
> hmac).
> What if we made all the struct members MixedCase? Or what if we did
> something like calling them xor_ and operator_?

The more we change, the greater the potential impact to downstream use of
these structures.  I prefer to do the smallest possible change to address
the c++ reserved keyword name collisions in this patch series.

I do not have strong opinion between 'xor_' vs 'Xor'.  These files are 
based on the TCG TPM Specifications that typically start field names with
lower case and camel case after that for multi-word field names.

https://trustedcomputinggroup.org/resource/tpm-library-specification/
https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part2_Structures_pub.pdf

> 
> --
> Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105511): https://edk2.groups.io/g/devel/message/105511
Mute This Topic: https://groups.io/mt/99226543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Pedro,

It appears other projects have run into this same issue with the
TPM specifications and have changed the field names by appending '_'.

https://github.com/MIPS/external-tpm2/blob/d704926273cf17498c95ff0dc50b4b17e523c109/generator/structure_generator.py#L1183

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, May 31, 2023 11:42 AM
> To: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; Oliver Smith-Denny
> <osde@linux.microsoft.com>; Pop, Aaron <aaronpop@microsoft.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: RE: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator
> and Xor field names
> 
> 
> 
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Wednesday, May 31, 2023 11:17 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> > Zhiguang <zhiguang.liu@intel.com>; Oliver Smith-Denny
> > <osde@linux.microsoft.com>; Pop, Aaron <aaronpop@microsoft.com>
> > Subject: Re: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator
> > and Xor field names
> >
> > On Tue, May 30, 2023 at 7:53 PM Michael D Kinney
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > Update Tpm12.h and Tpm20.h and not use c++ reserved keywords
> > > operator and xor in C structures to support use of these
> > > include files when building with a C++ compiler.
> > >
> > > This patch temporarily introduces an anonymous union to add
> > > Operator and Xor fields to support migration from the current
> > > field names to the new field names.
> > >
> > > Warning 4201 is disabled for VS20xx tool chains is a temporary
> > > change to allow the use of anonymous unions.
> > >
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
> > > Cc: Pedro Falcato <pedro.falcato@gmail.com>
> > > Cc: Aaron Pop <aaronpop@microsoft.com>
> > > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > > ---
> > >  MdePkg/Include/IndustryStandard/Tpm12.h | 22 ++++++++++++++++++++--
> > >  MdePkg/Include/IndustryStandard/Tpm20.h | 25 +++++++++++++++++++++++--
> > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h
> > b/MdePkg/Include/IndustryStandard/Tpm12.h
> > > index 155dcc9f5f99..56e89d9d0835 100644
> > > --- a/MdePkg/Include/IndustryStandard/Tpm12.h
> > > +++ b/MdePkg/Include/IndustryStandard/Tpm12.h
> > > @@ -9,6 +9,14 @@
> > >  #ifndef _TPM12_H_
> > >  #define _TPM12_H_
> > >
> > > +///
> > > +/// Temporary disable 4201 to support anonymous unions
> > > +///
> > > +#if defined (_MSC_EXTENSIONS)
> > > +#pragma warning( push )
> > > +#pragma warning ( disable : 4201 )
> > > +#endif
> > > +
> > >  ///
> > >  /// The start of TPM return codes
> > >  ///
> > > @@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS {
> > >    BOOLEAN              TPMpost;
> > >    BOOLEAN              TPMpostLock;
> > >    BOOLEAN              FIPS;
> > > -  BOOLEAN                           operator;
> > > -  BOOLEAN                           enableRevokeEK;
> > > +  union {
> > > +    BOOLEAN            operator;
> > > +    BOOLEAN            Operator;
> > > +  };
> >
> > Do you know if this works cleanly for the other toolchains? i.e
> > supported GCCs and CLANGs?
> > I don't *think* there's a warning for anonymous unions beyond passing
> > -pedantic + -std=c<something>, but it'd be good to know.
> > If so, we may need a pragma for this.
> 
> I did not see any issues with my local testing or with EDK II CI.
> 
> >
> > > +  BOOLEAN              enableRevokeEK;
> > >    BOOLEAN              nvLocked;
> > >    BOOLEAN              readSRKPub;
> > >    BOOLEAN              tpmEstablished;
> > > @@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR {
> > >
> > >  #pragma pack ()
> > >
> > > +///
> > > +/// Temporary disable 4201 to support anonymous unions
> > > +///
> > > +#if defined (_MSC_EXTENSIONS)
> > > +#pragma warning( pop )
> > > +#endif
> > > +
> > >  #endif
> > > diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h
> > b/MdePkg/Include/IndustryStandard/Tpm20.h
> > > index 4440f3769f26..a602c0d9c289 100644
> > > --- a/MdePkg/Include/IndustryStandard/Tpm20.h
> > > +++ b/MdePkg/Include/IndustryStandard/Tpm20.h
> > > @@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  #include <IndustryStandard/Tpm12.h>
> > >
> > > +///
> > > +/// Temporary disable 4201 to support anonymous unions
> > > +///
> > > +#if defined (_MSC_EXTENSIONS)
> > > +#pragma warning( push )
> > > +#pragma warning ( disable : 4201 )
> > > +#endif
> > > +
> > >  #pragma pack (1)
> > >
> > >  // Annex A Algorithm Constants
> > > @@ -1247,7 +1255,10 @@ typedef union {
> > >    TPMI_AES_KEY_BITS    aes;
> > >    TPMI_SM4_KEY_BITS    SM4;
> > >    TPM_KEY_BITS         sym;
> > > -  TPMI_ALG_HASH     xor;
> > > +  union {
> > > +    TPMI_ALG_HASH      xor;
> > > +    TPMI_ALG_HASH      Xor;
> > > +  };
> > >  } TPMU_SYM_KEY_BITS;
> > >
> > >  // Table 123 - TPMU_SYM_MODE Union
> > > @@ -1320,7 +1331,10 @@ typedef struct {
> > >  // Table 136 - TPMU_SCHEME_KEYEDHASH Union
> > >  typedef union {
> > >    TPMS_SCHEME_HMAC    hmac;
> > > -  TPMS_SCHEME_XOR  xor;
> > > +  union {
> > > +    TPMS_SCHEME_XOR   xor;
> > > +    TPMS_SCHEME_XOR   Xor;
> > > +  };
> > >  } TPMU_SCHEME_KEYEDHASH;
> > >
> > >  // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
> > > @@ -1809,4 +1823,11 @@ typedef struct {
> > >  #define HASH_ALG_SHA512   0x00000008
> > >  #define HASH_ALG_SM3_256  0x00000010
> > >
> > > +///
> > > +/// Temporary disable 4201 to support anonymous unions
> > > +///
> > > +#if defined (_MSC_EXTENSIONS)
> > > +#pragma warning( pop )
> > > +#endif
> > > +
> > >  #endif
> > > --
> > > 2.40.1.windows.1
> > >
> >
> > All in all, this looks ok to me. Although I have to say, I'm not a
> > huge fan of the naming style inconsistency introduced here (i.e Xor vs
> > hmac).
> > What if we made all the struct members MixedCase? Or what if we did
> > something like calling them xor_ and operator_?
> 
> The more we change, the greater the potential impact to downstream use of
> these structures.  I prefer to do the smallest possible change to address
> the c++ reserved keyword name collisions in this patch series.
> 
> I do not have strong opinion between 'xor_' vs 'Xor'.  These files are
> based on the TCG TPM Specifications that typically start field names with
> lower case and camel case after that for multi-word field names.
> 
> https://trustedcomputinggroup.org/resource/tpm-library-specification/
> https://trustedcomputinggroup.org/wp-
> content/uploads/TCG_TPM2_r1p59_Part2_Structures_pub.pdf
> 
> >
> > --
> > Pedro



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105569): https://edk2.groups.io/g/devel/message/105569
Mute This Topic: https://groups.io/mt/99226543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Pedro,

Based on this info, would you prefer we use xor_ style instead of Xor?

I can update the PR with that change.

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, June 1, 2023 8:02 AM
> To: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; Oliver Smith-Denny
> <osde@linux.microsoft.com>; Pop, Aaron <aaronpop@microsoft.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: RE: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator
> and Xor field names
> 
> Hi Pedro,
> 
> It appears other projects have run into this same issue with the
> TPM specifications and have changed the field names by appending '_'.
> 
> https://github.com/MIPS/external-
> tpm2/blob/d704926273cf17498c95ff0dc50b4b17e523c109/generator/structure_gene
> rator.py#L1183
> 
> Mike
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Wednesday, May 31, 2023 11:42 AM
> > To: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> > Zhiguang <zhiguang.liu@intel.com>; Oliver Smith-Denny
> > <osde@linux.microsoft.com>; Pop, Aaron <aaronpop@microsoft.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator
> > and Xor field names
> >
> >
> >
> > > -----Original Message-----
> > > From: Pedro Falcato <pedro.falcato@gmail.com>
> > > Sent: Wednesday, May 31, 2023 11:17 AM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> > > Zhiguang <zhiguang.liu@intel.com>; Oliver Smith-Denny
> > > <osde@linux.microsoft.com>; Pop, Aaron <aaronpop@microsoft.com>
> > > Subject: Re: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add
> Operator
> > > and Xor field names
> > >
> > > On Tue, May 30, 2023 at 7:53 PM Michael D Kinney
> > > <michael.d.kinney@intel.com> wrote:
> > > >
> > > > Update Tpm12.h and Tpm20.h and not use c++ reserved keywords
> > > > operator and xor in C structures to support use of these
> > > > include files when building with a C++ compiler.
> > > >
> > > > This patch temporarily introduces an anonymous union to add
> > > > Operator and Xor fields to support migration from the current
> > > > field names to the new field names.
> > > >
> > > > Warning 4201 is disabled for VS20xx tool chains is a temporary
> > > > change to allow the use of anonymous unions.
> > > >
> > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > > Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
> > > > Cc: Pedro Falcato <pedro.falcato@gmail.com>
> > > > Cc: Aaron Pop <aaronpop@microsoft.com>
> > > > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > > > ---
> > > >  MdePkg/Include/IndustryStandard/Tpm12.h | 22 ++++++++++++++++++++--
> > > >  MdePkg/Include/IndustryStandard/Tpm20.h | 25
> +++++++++++++++++++++++--
> > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h
> > > b/MdePkg/Include/IndustryStandard/Tpm12.h
> > > > index 155dcc9f5f99..56e89d9d0835 100644
> > > > --- a/MdePkg/Include/IndustryStandard/Tpm12.h
> > > > +++ b/MdePkg/Include/IndustryStandard/Tpm12.h
> > > > @@ -9,6 +9,14 @@
> > > >  #ifndef _TPM12_H_
> > > >  #define _TPM12_H_
> > > >
> > > > +///
> > > > +/// Temporary disable 4201 to support anonymous unions
> > > > +///
> > > > +#if defined (_MSC_EXTENSIONS)
> > > > +#pragma warning( push )
> > > > +#pragma warning ( disable : 4201 )
> > > > +#endif
> > > > +
> > > >  ///
> > > >  /// The start of TPM return codes
> > > >  ///
> > > > @@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS {
> > > >    BOOLEAN              TPMpost;
> > > >    BOOLEAN              TPMpostLock;
> > > >    BOOLEAN              FIPS;
> > > > -  BOOLEAN                           operator;
> > > > -  BOOLEAN                           enableRevokeEK;
> > > > +  union {
> > > > +    BOOLEAN            operator;
> > > > +    BOOLEAN            Operator;
> > > > +  };
> > >
> > > Do you know if this works cleanly for the other toolchains? i.e
> > > supported GCCs and CLANGs?
> > > I don't *think* there's a warning for anonymous unions beyond passing
> > > -pedantic + -std=c<something>, but it'd be good to know.
> > > If so, we may need a pragma for this.
> >
> > I did not see any issues with my local testing or with EDK II CI.
> >
> > >
> > > > +  BOOLEAN              enableRevokeEK;
> > > >    BOOLEAN              nvLocked;
> > > >    BOOLEAN              readSRKPub;
> > > >    BOOLEAN              tpmEstablished;
> > > > @@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR {
> > > >
> > > >  #pragma pack ()
> > > >
> > > > +///
> > > > +/// Temporary disable 4201 to support anonymous unions
> > > > +///
> > > > +#if defined (_MSC_EXTENSIONS)
> > > > +#pragma warning( pop )
> > > > +#endif
> > > > +
> > > >  #endif
> > > > diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h
> > > b/MdePkg/Include/IndustryStandard/Tpm20.h
> > > > index 4440f3769f26..a602c0d9c289 100644
> > > > --- a/MdePkg/Include/IndustryStandard/Tpm20.h
> > > > +++ b/MdePkg/Include/IndustryStandard/Tpm20.h
> > > > @@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > >  #include <IndustryStandard/Tpm12.h>
> > > >
> > > > +///
> > > > +/// Temporary disable 4201 to support anonymous unions
> > > > +///
> > > > +#if defined (_MSC_EXTENSIONS)
> > > > +#pragma warning( push )
> > > > +#pragma warning ( disable : 4201 )
> > > > +#endif
> > > > +
> > > >  #pragma pack (1)
> > > >
> > > >  // Annex A Algorithm Constants
> > > > @@ -1247,7 +1255,10 @@ typedef union {
> > > >    TPMI_AES_KEY_BITS    aes;
> > > >    TPMI_SM4_KEY_BITS    SM4;
> > > >    TPM_KEY_BITS         sym;
> > > > -  TPMI_ALG_HASH     xor;
> > > > +  union {
> > > > +    TPMI_ALG_HASH      xor;
> > > > +    TPMI_ALG_HASH      Xor;
> > > > +  };
> > > >  } TPMU_SYM_KEY_BITS;
> > > >
> > > >  // Table 123 - TPMU_SYM_MODE Union
> > > > @@ -1320,7 +1331,10 @@ typedef struct {
> > > >  // Table 136 - TPMU_SCHEME_KEYEDHASH Union
> > > >  typedef union {
> > > >    TPMS_SCHEME_HMAC    hmac;
> > > > -  TPMS_SCHEME_XOR  xor;
> > > > +  union {
> > > > +    TPMS_SCHEME_XOR   xor;
> > > > +    TPMS_SCHEME_XOR   Xor;
> > > > +  };
> > > >  } TPMU_SCHEME_KEYEDHASH;
> > > >
> > > >  // Table 137 - TPMT_KEYEDHASH_SCHEME Structure
> > > > @@ -1809,4 +1823,11 @@ typedef struct {
> > > >  #define HASH_ALG_SHA512   0x00000008
> > > >  #define HASH_ALG_SM3_256  0x00000010
> > > >
> > > > +///
> > > > +/// Temporary disable 4201 to support anonymous unions
> > > > +///
> > > > +#if defined (_MSC_EXTENSIONS)
> > > > +#pragma warning( pop )
> > > > +#endif
> > > > +
> > > >  #endif
> > > > --
> > > > 2.40.1.windows.1
> > > >
> > >
> > > All in all, this looks ok to me. Although I have to say, I'm not a
> > > huge fan of the naming style inconsistency introduced here (i.e Xor vs
> > > hmac).
> > > What if we made all the struct members MixedCase? Or what if we did
> > > something like calling them xor_ and operator_?
> >
> > The more we change, the greater the potential impact to downstream use of
> > these structures.  I prefer to do the smallest possible change to address
> > the c++ reserved keyword name collisions in this patch series.
> >
> > I do not have strong opinion between 'xor_' vs 'Xor'.  These files are
> > based on the TCG TPM Specifications that typically start field names with
> > lower case and camel case after that for multi-word field names.
> >
> > https://trustedcomputinggroup.org/resource/tpm-library-specification/
> > https://trustedcomputinggroup.org/wp-
> > content/uploads/TCG_TPM2_r1p59_Part2_Structures_pub.pdf
> >
> > >
> > > --
> > > Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105811): https://edk2.groups.io/g/devel/message/105811
Mute This Topic: https://groups.io/mt/99226543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Jun 6, 2023 at 6:57 PM Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Hi Pedro,
>
> Based on this info, would you prefer we use xor_ style instead of Xor?
>
> I can update the PR with that change.
>
> Mike
Hi Mike,

Sorry, I didn't know you were waiting for my input. Yes, I would
prefer xor_ style, but I'll leave it up to you.
You can add my

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

For the whole series, regardless of the style choice here.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105817): https://edk2.groups.io/g/devel/message/105817
Mute This Topic: https://groups.io/mt/99226543/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-