Below:
> -----Original Message-----
> From: Zeng, Star <star.z...@intel.com>
> Sent: Thursday, January 2, 2020 7:09 PM
> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>;
> Bi, Dandan <dandan...@intel.com>; Zeng, Star <star.z...@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.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>;
> > Bi, Dandan <dandan...@intel.com>; Zeng, Star <star.z...@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.w...@intel.com>
> > Cc: Hao A Wu <hao.a...@intel.com>
> > Cc: Dandan Bi <dandan...@intel.com>
> > Cc: Star Zeng <star.z...@intel.com>
> > Signed-off-by: Jiewen Yao <jiewen....@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.
>
>
> Thanks,
> Star
>
> > +
> > [Depex]
> > gEfiSmbiosProtocolGuid
> >
> > --
> > 2.19.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52652): https://edk2.groups.io/g/devel/message/52652
Mute This Topic: https://groups.io/mt/69344972/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
