Re: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator and Xor field names

["Michael D Kinney" <michael.d.kinney@intel.com>]

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