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

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

Star,
   Thanks for the clarification. I will send the patch shortly.

Regards,
Linson

-----Original Message-----
From: Zeng, Star [mailto:star.zeng@intel.com] 
Sent: Thursday, January 12, 2017 11:39 AM
To: Augustine, Linson P <linson.augustine@hpe.com>; edk2-devel@lists.01.org
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH v2 1/1] ShellPkg/SmbiosView: Add decoding of SMBIOS record type 43

Linson,

" no comments to it" I said meant I have no comments to it, I just put a mark at there as my following comments "How about use "Display TPM Device (Type 43) Characteristics" to align with the comment above?" will refer it.
Sorry for confusing.
Please update patch.

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Augustine, Linson P
Sent: Thursday, January 12, 2017 1:59 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [edk2] [PATCH v2 1/1] ShellPkg/SmbiosView: Add decoding of SMBIOS record type 43

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel