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

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

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, January 2, 2020 10:16 PM
> To: Zeng, Star <star.zeng@intel.com>; 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>
> Subject: RE: [PATCH 5/6] MdeModulePkg/Smbios: Add TCG PFP rev 105
> support.
> 
> Below:
> 
> > -----Original Message-----
> > From: Zeng, Star <star.zeng@intel.com>
> > Sent: Thursday, January 2, 2020 7:09 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; 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: RE: [PATCH 5/6] MdeModulePkg/Smbios: Add TCG PFP rev 105
> support.
> >
> > 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?
> [Jiewen] Right. The first byte is the size of the following description.
> The table owner may define different description for the table.
> 
> >
> > > +
> > > +#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?
> [Jiewen] My purpose is that all the rev 105 content are scoped in following if.
> For rev 0, people can just see the above logic.
> It is more straight forward than putting partial of logic into else.
> 
> >
> > > +
> > > +    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?
> [Jiewen] Agree. I will add them.

Reviewed-by: Star Zeng <star.zeng@intel.com>
No need send new patch series just for this, you can do them when doing pull-request.

Star

> 
> 
> >
> >
> > Thanks,
> > Star
> >
> > > +
> > >  [Depex]
> > >    gEfiSmbiosProtocolGuid
> > >
> > > --
> > > 2.19.2.windows.1