Re: [PATCH v2 1/1] ShellPkg/SmbiosView: Add decoding of SMBIOS record type 43

["Augustine, Linson P" <linson.augustine@hpe.com>]

Hi Star,
  This is regarding the following comments.

>    //
> +  // TPM Device (Type 43)

The comment here is using "TPM Device (Type 43)", no comments to it. :)

> +  //

All the comments in the function are using the heading of corresponding table in SMBIOS spec. And for Type 43 it is " TPM Device (Type 43)".
Please let me know if you like to change it.
Based on this I will send the updated patch (addressing other comments as well).

Regards,
Linson.



-----Original Message-----
From: Zeng, Star [mailto:star.zeng@intel.com] 
Sent: Wednesday, January 11, 2017 3:25 PM
To: Augustine, Linson P <linson.augustine@hpe.com>; edk2-devel@lists.01.org
Cc: jaben.carsey@intel.com; ruiyu.ni@intel.com; star.zeng@intel.com
Subject: Re: [edk2] [PATCH v2 1/1] ShellPkg/SmbiosView: Add decoding of SMBIOS record type 43

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 <linson.augustine@hpe.com>
> ---
>  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/SmbiosViewStrin
> gs.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.<BR>
>    (C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>
> -  (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
> +  (C) Copyright 2015-2017 Hewlett Packard Enterprise Development 
> + LP<BR>
>    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.<BR>
> +  (C) Copyright 2017 Hewlett Packard Enterprise Development LP<BR>
>    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/QueryTabl
> +++ e.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.<BR>
> -  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +  (C) Copyright 2016-2017 Hewlett Packard Enterprise Development 
> + LP<BR>
>    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/SmbiosViewStr
> ings.uni 
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosViewStr
> ings.uni
> index 9811542013..6c10db6993 100644
> --- 
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosViewStr
> ings.uni
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosVie
> +++ wStrings.uni
> @@ -2,7 +2,7 @@
>  //
>  // Copyright (c) 2005 - 2015, Intel Corporation. All rights 
> reserved.<BR>  // (C) Copyright 2014-2015 Hewlett-Packard Development 
> Company, L.P.<BR> -// (C) Copyright 2015 Hewlett Packard Enterprise 
> Development LP<BR>
> +// (C) Copyright 2015-2017 Hewlett Packard Enterprise Development 
> +LP<BR>
>  // 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"
>
>