[edk2-devel] [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator and Xor field names
Michael D Kinney
michael.d.kinney at intel.com
Tue Jun 6 17:56:59 UTC 2023
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 at intel.com>
> Sent: Thursday, June 1, 2023 8:02 AM
> To: Pedro Falcato <pedro.falcato at gmail.com>
> Cc: devel at edk2.groups.io; Gao, Liming <gaoliming at byosoft.com.cn>; Liu,
> Zhiguang <zhiguang.liu at intel.com>; Oliver Smith-Denny
> <osde at linux.microsoft.com>; Pop, Aaron <aaronpop at microsoft.com>; Kinney,
> Michael D <michael.d.kinney at 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 at intel.com>
> > Sent: Wednesday, May 31, 2023 11:42 AM
> > To: Pedro Falcato <pedro.falcato at gmail.com>
> > Cc: devel at edk2.groups.io; Gao, Liming <gaoliming at byosoft.com.cn>; Liu,
> > Zhiguang <zhiguang.liu at intel.com>; Oliver Smith-Denny
> > <osde at linux.microsoft.com>; Pop, Aaron <aaronpop at microsoft.com>; Kinney,
> > Michael D <michael.d.kinney at intel.com>
> > Subject: RE: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator
> > and Xor field names
> >
> >
> >
> > > -----Original Message-----
> > > From: Pedro Falcato <pedro.falcato at gmail.com>
> > > Sent: Wednesday, May 31, 2023 11:17 AM
> > > To: Kinney, Michael D <michael.d.kinney at intel.com>
> > > Cc: devel at edk2.groups.io; Gao, Liming <gaoliming at byosoft.com.cn>; Liu,
> > > Zhiguang <zhiguang.liu at intel.com>; Oliver Smith-Denny
> > > <osde at linux.microsoft.com>; Pop, Aaron <aaronpop at 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 at 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 at byosoft.com.cn>
> > > > Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> > > > Cc: Oliver Smith-Denny <osde at linux.microsoft.com>
> > > > Cc: Pedro Falcato <pedro.falcato at gmail.com>
> > > > Cc: Aaron Pop <aaronpop at microsoft.com>
> > > > Signed-off-by: Michael D Kinney <michael.d.kinney at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list