From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 098CF81B5A for ; Wed, 11 Jan 2017 01:55:44 -0800 (PST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP; 11 Jan 2017 01:55:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,345,1477983600"; d="scan'208";a="47762209" Received: from shzintpr02.sh.intel.com (HELO [10.7.209.38]) ([10.239.4.160]) by orsmga004.jf.intel.com with ESMTP; 11 Jan 2017 01:55:42 -0800 To: Linson Augustine , edk2-devel@lists.01.org References: <20170111085703.8180-1-linson.augustine@hpe.com> Cc: jaben.carsey@intel.com, ruiyu.ni@intel.com, star.zeng@intel.com From: "Zeng, Star" Message-ID: <4a32ae19-ab57-9715-df71-831e677a7356@intel.com> Date: Wed, 11 Jan 2017 17:55:12 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170111085703.8180-1-linson.augustine@hpe.com> Subject: Re: [PATCH v2 1/1] ShellPkg/SmbiosView: Add decoding of SMBIOS record type 43 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Jan 2017 09:55:44 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Linson, Add minor comments inline. :) On 2017/1/11 16:57, Linson Augustine wrote: > Added decoding of the new SMBIOS Type 43 record. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Augustine Linson P > --- > ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c | 72 +++++++++++++++++++- > ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.h | 13 ++++ > ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c | 6 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosViewStrings.uni | 8 ++- > 4 files changed, 96 insertions(+), 3 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c > index 7e17b69d5a..8d4a613c40 100644 > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c > @@ -3,7 +3,7 @@ > > Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> (C) Copyright 2014 Hewlett-Packard Development Company, L.P.
> - (C) Copyright 2015 Hewlett Packard Enterprise Development LP
> + (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -1083,6 +1083,20 @@ SmbiosPrintStructure ( > break; > > // > + // TPM Device (Type 43) The comment here is using "TPM Device (Type 43)", no comments to it. :) > + // > + case 43: > + PRINT_BIT_FIELD (Struct, Type43, VendorID, 4); > + PRINT_STRUCT_VALUE_H (Struct, Type43, MajorSpecVersion); > + PRINT_STRUCT_VALUE_H (Struct, Type43, MinorSpecVersion); > + PRINT_STRUCT_VALUE_H (Struct, Type43, FirmwareVersion1); > + PRINT_STRUCT_VALUE_H (Struct, Type43, FirmwareVersion2); > + PRINT_PENDING_STRING (Struct, Type43, Description); > + DisplayTpmCharacteristics (ReadUnaligned64 ((UINT64 *) (UINTN) &(Struct->Type43->Characteristics)), Option); > + PRINT_STRUCT_VALUE_H (Struct, Type43, OemDefined); Thanks for the code update for this. > + break; > + > + // > // Inactive (Type 126) > // > case 126: > @@ -3225,3 +3239,59 @@ DisplaySPSCharacteristics ( > ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_POWER_SUPPLY_NOT_REPLACE), gShellDebug1HiiHandle); > } > } > + > +/** > + Display TPM (Type 43) Device Characteristics. How about use "Display TPM Device (Type 43) Characteristics" to align with the comment above? > + > + @param[in] Chara The information bits. > + @param[in] Option The optional information. > +**/ > +VOID > +DisplayTpmCharacteristics ( How about to use function name "DisplayTpmDeviceCharacteristics"? > + IN UINT64 Chara, > + IN UINT8 Option > + ) > +{ > + // > + // Print header > + // > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_TPM_CHAR), gShellDebug1HiiHandle); > + // > + // print option > + // > + PRINT_INFO_OPTION (Chara, Option); > + > + // > + // Check all the bits and print information > + // This function does not use Table because table of bits > + // are designed not to deal with UINT64 > + // > + if (BIT (Chara, 0) != 0) { > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_RESERVED_BIT), gShellDebug1HiiHandle); > + } > + > + if (BIT (Chara, 1) != 0) { > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_RESERVED_BIT), gShellDebug1HiiHandle); > + } > + if (BIT (Chara, 2) != 0) { > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_TPM_CHAR_NOT_SUPPORTED), gShellDebug1HiiHandle); > + } > + > + if (BIT (Chara, 3) != 0) { > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_TPM_CONFIG_FWU), gShellDebug1HiiHandle); > + } > + > + if (BIT (Chara, 4) != 0) { > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_TPM_CONFIG_PLAT_SW), gShellDebug1HiiHandle); > + } > + > + if (BIT (Chara, 5) != 0) { > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_TPM_CONFIG_OEM), gShellDebug1HiiHandle); > + } > + > + // > + // Just print the Reserved > + // > + ShellPrintHiiEx(-1,-1,NULL,STRING_TOKEN (STR_SMBIOSVIEW_PRINTINFO_BITS_06_63), gShellDebug1HiiHandle); > + > +} > diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.h b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.h > index 50667b684c..e5017990aa 100644 > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.h > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.h > @@ -2,6 +2,7 @@ > Module to clarify the element info of the smbios structure. > > Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.
> + (C) Copyright 2017 Hewlett Packard Enterprise Development LP
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -420,4 +421,16 @@ DisplaySPSCharacteristics ( > IN UINT8 Option > ); > > +/** > + Display TPM (Type 43) Device Characteristics. > + > + @param[in] Chara The information bits. > + @param[in] Option The optional information. > +**/ > +VOID > +DisplayTpmCharacteristics ( > + IN UINT64 Chara, > + IN UINT8 Option > + ); > + > #endif > diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c > index d0106c0b55..e1e79c4410 100644 > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c > @@ -3,7 +3,7 @@ > And give a interface of query a string out of a table. > > Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.
> - (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> + (C) Copyright 2016-2017 Hewlett Packard Enterprise Development LP
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -3190,6 +3190,10 @@ TABLE_ITEM StructureTypeInfoTable[] = { > L" Management Controller Host Interface" > }, > { > + 43, > + L" TPM Device" > + }, > + { > 0x7E, > L" Inactive" > }, > diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosViewStrings.uni b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosViewStrings.uni > index 9811542013..6c10db6993 100644 > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosViewStrings.uni > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosViewStrings.uni > @@ -2,7 +2,7 @@ > // > // Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.
> // (C) Copyright 2014-2015 Hewlett-Packard Development Company, L.P.
> -// (C) Copyright 2015 Hewlett Packard Enterprise Development LP
> +// (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP
> // This program and the accompanying materials > // are licensed and made available under the terms and conditions of the BSD License > // which accompanies this distribution. The full text of the license may be found at > @@ -488,4 +488,10 @@ > #string STR_SMBIOSVIEW_SMBIOSVIEW_SMBIOS_TABLE #language en-US "SmbiosView: SMBIOS table damaged\r\n" > #string STR_SMBIOSVIEW_SMBIOSVIEW_OUT_OF_MEM #language en-US "SmbiosView: Out of memory\r\n" > #string STR_SMBIOSVIEW_SMBIOSVIEW_CANNOT_ACCESS_STATS #language en-US "SmbiosView: Cannot access statistics table\r\n" > +#string STR_SMBIOSVIEW_PRINTINFO_TPM_CHAR #language en-US "TPM Characteristics: \r\n" > +#string STR_SMBIOSVIEW_PRINTINFO_TPM_CHAR_NOT_SUPPORTED #language en-US "TPM Characteristics Not Supported\r\n" For the two strings above, how about use "TPM_DEVICE" instead "TPM" in STR_SMBIOSVIEW_PRINTINFO_XXX and use "TPM Device" instead of "TPM" in string content? > +#string STR_SMBIOSVIEW_PRINTINFO_TPM_CONFIG_FWU #language en-US "TPM Family configurable via firmware update\r\n" > +#string STR_SMBIOSVIEW_PRINTINFO_TPM_CONFIG_PLAT_SW #language en-US "TPM Family configurable via platform software support\r\n" > +#string STR_SMBIOSVIEW_PRINTINFO_TPM_CONFIG_OEM #language en-US "TPM Family configurable via OEM proprietary mechanism\r\n" For the three strings above, how about use "TPM_DEVICE" instead "TPM" in STR_SMBIOSVIEW_PRINTINFO_XXX and *remove* TPM in string content as the function should have printed "TPM Device Characteristics:" before printing the Characteristics? Thanks, Star > +#string STR_SMBIOSVIEW_PRINTINFO_BITS_06_63 #language en-US "Bits 6:63 are reserved\r\n" > >