Re: [PATCH 5/6] MdeModulePkg/Smbios: Add TCG PFP rev 105 support.

["Zeng, Star" <star.zeng@intel.com>]

Minor comments.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Tuesday, December 31, 2019 2:44 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 5/6] MdeModulePkg/Smbios: Add TCG PFP rev 105 support.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2439
> 
> Report EV_EFI_HANDOFF_TABLES2 if the platform chooses PFP >= 105.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  .../SmbiosMeasurementDxe.c                    | 35 +++++++++++++++++--
>  .../SmbiosMeasurementDxe.inf                  |  3 ++
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasuremen
> tDxe.c
> b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasureme
> ntDxe.c
> index 5ec2aca095..a5839c09f1 100644
> ---
> a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasuremen
> tDxe.c
> +++
> b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasureme
> ntDxe.c
> @@ -108,6 +108,18 @@ SMBIOS_FILTER_STRUCT
> mSmbiosFilterStandardTableBlackList[] = {  EFI_SMBIOS_PROTOCOL
> *mSmbios;
>  UINTN               mMaxLen;
> 
> +#pragma pack (1)
> +
> +#define SMBIOS_HANDOFF_TABLE_DESC  "SmbiosTable"
> +typedef struct {
> +  UINT8                             TableDescriptionSize;
> +  UINT8
> TableDescription[sizeof(SMBIOS_HANDOFF_TABLE_DESC)];
> +  UINT64                            NumberOfTables;
> +  EFI_CONFIGURATION_TABLE           TableEntry[1];
> +} SMBIOS_HANDOFF_TABLE_POINTERS2;

Curious about that there is no standard structure defined in public header instead of defining it internal here?

> +
> +#pragma pack ()
> +
>  /**
> 
>    This function dump raw data.
> @@ -460,6 +472,10 @@ MeasureSmbiosTable (  {
>    EFI_STATUS                        Status;
>    EFI_HANDOFF_TABLE_POINTERS        HandoffTables;
> +  SMBIOS_HANDOFF_TABLE_POINTERS2    SmbiosHandoffTables2;
> +  UINT32                            EventType;
> +  VOID                              *EventLog;
> +  UINT32                            EventLogSize;
>    SMBIOS_TABLE_ENTRY_POINT          *SmbiosTable;
>    SMBIOS_TABLE_3_0_ENTRY_POINT      *Smbios3Table;
>    VOID                              *SmbiosTableAddress;
> @@ -569,11 +585,24 @@ MeasureSmbiosTable (
>        CopyGuid (&(HandoffTables.TableEntry[0].VendorGuid),
> &gEfiSmbiosTableGuid);
>        HandoffTables.TableEntry[0].VendorTable = SmbiosTable;
>      }
> +    EventType = EV_EFI_HANDOFF_TABLES;
> +    EventLog = &HandoffTables;
> +    EventLogSize = sizeof (HandoffTables);

How about making them into the else condition in the if condition below?

> +
> +    if (PcdGet32(PcdTcgPfpMeasurementRevision) >=
> TCG_EfiSpecIDEventStruct_SPEC_ERRATA_TPM2_REV_105) {
> +      SmbiosHandoffTables2.TableDescriptionSize =
> sizeof(SmbiosHandoffTables2.TableDescription);
> +      CopyMem (SmbiosHandoffTables2.TableDescription,
> SMBIOS_HANDOFF_TABLE_DESC,
> sizeof(SmbiosHandoffTables2.TableDescription));
> +      SmbiosHandoffTables2.NumberOfTables =
> HandoffTables.NumberOfTables;
> +      CopyMem (&(SmbiosHandoffTables2.TableEntry[0]),
> &(HandoffTables.TableEntry[0]),
> sizeof(SmbiosHandoffTables2.TableEntry[0]));
> +      EventType = EV_EFI_HANDOFF_TABLES2;
> +      EventLog = &SmbiosHandoffTables2;
> +      EventLogSize = sizeof (SmbiosHandoffTables2);
> +    }
>      Status = TpmMeasureAndLogData (
>                 1,                       // PCRIndex
> -               EV_EFI_HANDOFF_TABLES,   // EventType
> -               &HandoffTables,          // EventLog
> -               sizeof (HandoffTables),  // LogLen
> +               EventType,               // EventType
> +               EventLog,                // EventLog
> +               EventLogSize,            // LogLen
>                 TableAddress,            // HashData
>                 TableLength              // HashDataLen
>                 );
> diff --git
> a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasuremen
> tDxe.inf
> b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasureme
> ntDxe.inf
> index a074044c84..81d3655dc7 100644
> ---
> a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasuremen
> tDxe.inf
> +++
> b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasureme
> ntDxe.i
> +++ nf
> @@ -57,6 +57,9 @@
>    gEfiSmbiosTableGuid                               ## SOMETIMES_CONSUMES ##
> SystemTable
>    gEfiSmbios3TableGuid                              ## SOMETIMES_CONSUMES ##
> SystemTable
> 
> +[Pcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdTcgPfpMeasurementRevision
> ## CONSUMES

To be explicit, how about adding PcdLib in inf and PcdLib.h in c?


Thanks,
Star

> +
>  [Depex]
>    gEfiSmbiosProtocolGuid
> 
> --
> 2.19.2.windows.1