Re: [PATCH V3 5/9] OvmfPkg/TdxHelperLib: Implement TdxHelperBuildGuidHobForTdxMeasurement

["Min Xu" <min.m.xu@intel.com>]

On January 25, 2023 8:22 PM, Gerd Hoffmann wrote:
> > +#define FV_HANDOFF_TABLE_DESC  "Fv(XXXXXXXX-XXXX-XXXX-XXXX-
> XXXXXXXXXXXX)"
> > +typedef PLATFORM_FIRMWARE_BLOB2_STRUCT
> CFV_HANDOFF_TABLE_POINTERS2;
> 
> > -#define FV_HANDOFF_TABLE_DESC  "Fv(XXXXXXXX-XXXX-XXXX-XXXX-
> XXXXXXXXXXXX)"
> > -typedef struct {
> > -  UINT8                   BlobDescriptionSize;
> > -  UINT8                   BlobDescription[sizeof (FV_HANDOFF_TABLE_DESC)];
> > -  EFI_PHYSICAL_ADDRESS    BlobBase;
> > -  UINT64                  BlobLength;
> > -} FV_HANDOFF_TABLE_POINTERS2;
> 
> That update makes sense, but can you please split this (and possibly
> other) code changes to a separate patch so this patch does nothing but
> moving code and the absolute necessary changes needed to make it work
> (update library references, function renames).
> 
Some of the data struct and functions in TdxMeasurementHob.c are moved from PeilessStartupLib/IntelTdx.c
 - TDX_HANDOFF_TABLE_POINTERS2
 - CFV_HANDOFF_TABLE_POINTERS2
 - GetFvName

BuildTdxMeasurementGuidHob is partially copied from TpmMeasureAndLogData@SecTpmMeasurementLibTdx.c.
InternalBuildGuidHobForTdxMeasurement is newly added.

That is because in the previous PeilessStartupLib/IntelTdx.c the measurement uses the TpmMeasureAndLogData which is exported by SecTpmMeasurementLibTdx.c. It does not only measurement/extending, but also builds GuidHob of the measurement.

Now in this patch-set, the measurement/extending and the building of GuidHob are split.
Firstly HobList and CFV image are measured and extended (at the same time the measurement values are stored in WorkArea because at that time the Hob service is not ready).
Then after hob service is ready, GuidHob of the measurements are built.

So it's hard to separate the patch into the one that does nothing but moving code. I would put below change in a sperate patch.
 +#define FV_HANDOFF_TABLE_DESC  "Fv(XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX)"
 +typedef PLATFORM_FIRMWARE_BLOB2_STRUCT  CFV_HANDOFF_TABLE_POINTERS2;
Then keep the other changes in another patch.

What's your thought?

Thanks
Min