This code is gratuitous on typecasting and pointer use. As this change is making the use of these fields more confusing, I would like to see it simplified.
e.g. *(UINT32 *)&FitEntry[FitIndex].Size[0] = This is introducing possible errors. More correct would be FitEntry[FitIndex].Size[0] = (UINT8) gFitTableContext.FitEntryNumber; FitEntry[FitIndex].Size[1] = (UINT8) (gFitTableContext.FitEntryNumber >> 8); FitEntry[FitIndex].Size[2] = (UINT8) (gFitTableContext.FitEntryNumber >> 16); FitEntry[FitIndex].Rsvd = 0; But I think that we can do better by redefining the structs and using unions to make the field use clear. Or different structs for different entry types would be fine too. Thanks, Isaac -----Original Message----- From: Lin, Jason1 <jason1....@intel.com> Sent: Thursday, June 23, 2022 7:49 AM To: devel@edk2.groups.io Cc: Lin, Jason1 <jason1....@intel.com>; Feng, Bob C <bob.c.f...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Chen, Christine <yuwei.c...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>; Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Chiang, Dakota <dakota.chi...@intel.com> Subject: [PATCH v2 2/2] [edk2-platforms] Silicon/Intel/FitGen: Support to override the MBZ byte in Startup ACM entries for other usages From: Jason1 Lin <jason1....@intel.com> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3959 As per FIT BIOS Specification 1.2 Rules, the size bytes (3 bytes)/ reserved byte (1 byte) / CheckSum byte (1 byte) in type 2 are must-be-zero (MBZ). These bytes could be override for the other usages. In the future, if these bytes field have other meanings, there would be no need to change the FitGen. Command: [-S <StartupAcmAddress StartupAcmSize>|<StartupAcmGuid>] [-I <StartupAcmSizeByte0 StartupAcmSizeByte1 StartupAcmSizeByte2 StartupAcmRsvd StartupAcmChksum>] [-V <StartupAcmVersion>] With "-I" input the default Type 2 entry version would be 0x200. Signed-off-by: Jason1 Lin <jason1....@intel.com> Cc: Bob Feng <bob.c.f...@intel.com> Cc: Liming Gao <gaolim...@byosoft.com.cn> Cc: Yuwei Chen <yuwei.c...@intel.com> Cc: Isaac W Oram <isaac.w.o...@intel.com> Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com> Cc: Dakota Chiang <dakota.chi...@intel.com> --- Silicon/Intel/Tools/FitGen/FitGen.c | 105 ++++++++++++++++---- Silicon/Intel/Tools/FitGen/FitGen.h | 2 +- 2 files changed, 88 insertions(+), 19 deletions(-) diff --git a/Silicon/Intel/Tools/FitGen/FitGen.c b/Silicon/Intel/Tools/FitGen/FitGen.c index eac8fa8715..bdda8dfd5a 100644 --- a/Silicon/Intel/Tools/FitGen/FitGen.c +++ b/Silicon/Intel/Tools/FitGen/FitGen.c @@ -210,6 +210,7 @@ typedef struct { #define DEFAULT_FIT_TABLE_POINTER_OFFSET 0x40 #define DEFAULT_FIT_ENTRY_VERSION 0x0100+#define STARTUP_ACM_FIT_ENTRY_200_VERSION 0x0200 #define TOP_FLASH_ADDRESS (gFitTableContext.TopFlashAddressRemapValue) @@ -247,6 +248,12 @@ typedef struct { UINT8 *Buffer; // Used by OptionalModule only UINT32 Size; UINT32 Version; // Used by OptionalModule and PortModule only+ UINT32 MaxSize;+ UINT8 SizeByte0; // Used by S-ACM only+ UINT8 SizeByte1; // Used by S-ACM only+ UINT8 SizeByte2; // Used by S-ACM only+ UINT8 Rsvd; // Used by S-ACM only+ UINT8 ChkSum; // Used by S-ACM only } FIT_TABLE_CONTEXT_ENTRY; typedef struct {@@ -262,7 +269,7 @@ typedef struct { UINT32 GlobalVersion; UINT32 FitHeaderVersion; FIT_TABLE_CONTEXT_ENTRY StartupAcm[MAX_STARTUP_ACM_ENTRY];- UINT32 StartupAcmVersion;+ UINT32 StartupAcmVersion[MAX_STARTUP_ACM_ENTRY]; FIT_TABLE_CONTEXT_ENTRY DiagnstAcm; UINT32 DiagnstAcmVersion; FIT_TABLE_CONTEXT_ENTRY BiosModule[MAX_BIOS_MODULE_ENTRY];@@ -341,7 +348,7 @@ Returns: "\t[-L <MicrocodeSlotSize> <MicrocodeFfsGuid>]\n" "\t[-LF <MicrocodeSlotSize>]\n" "\t[-I <BiosInfoGuid>]\n"- "\t[-S <StartupAcmAddress StartupAcmSize>|<StartupAcmGuid>] [-V <StartupAcmVersion>]\n"+ "\t[-S <StartupAcmAddress StartupAcmSize>|<StartupAcmGuid>] [-I <StartupAcmSizeByte0 StartupAcmSizeByte1 StartupAcmSizeByte2 StartupAcmRsvd StartupAcmChksum>] [-V <StartupAcmVersion>]\n" "\t[-U <DiagnstAcmAddress>|<DiagnstAcmGuid>]\n" "\t[-B <BiosModuleAddress BiosModuleSize>] [-B ...] [-V <BiosModuleVersion>]\n" "\t[-M <MicrocodeAddress MicrocodeSize>] [-M ...]|[-U <MicrocodeFv MicrocodeBase>|<MicrocodeRegionOffset MicrocodeRegionSize>|<MicrocodeGuid>] [-V <MicrocodeVersion>]\n"@@ -357,7 +364,13 @@ Returns: printf ("\tBiosInfoGuid - Guid of BiosInfo Module. If this module exists, StartupAcm/Bios/Microcode can be optional.\n"); printf ("\tStartupAcmAddress - Address of StartupAcm.\n"); printf ("\tStartupAcmSize - Size of StartupAcm.\n");- printf ("\tStartupAcmGuid - Guid of StartupAcm Module, if StartupAcm is in a BiosModule, it will be excluded form that.\n");+ printf ("\tStartupAcmGuid - Guid of StartupAcm Module.\n");+ printf ("\tStartupAcmMaxSize - The maximum size value that could place the StartupAcm in.\n");+ printf ("\tStartupAcmSizeByte0 - The value for Size byte 0 (Override default value).\n");+ printf ("\tStartupAcmSizeByte1 - The value for Size byte 1 (Override default value).\n");+ printf ("\tStartupAcmSizeByte2 - The value for Size byte 2 (Override default value).\n");+ printf ("\tStartupAcmRsvd - The value for Reserved byte (Override default value).\n");+ printf ("\tStartupAcmChkSum - The value for CheckSum byte (Override default value).\n"); printf ("\tDiagnstAcmAddress - Address of DiagnstAcm.\n"); printf ("\tDiagnstAcmGuid - Guid of DiagnstAcm Module, if DiagnstAcm is in a BiosModule, it will be excluded from that.\n"); printf ("\tBiosModuleAddress - Address of BiosModule. User should ensure there is no overlap.\n");@@ -913,6 +926,9 @@ Returns: UINT32 FileSize; UINT32 Type; UINT32 SubType;+ UINT8 StartupAcmSizeByte0;+ UINT8 StartupAcmSizeByte1;+ UINT8 StartupAcmSizeByte2; UINT8 *MicrocodeFileBuffer; UINT8 *MicrocodeFileBufferRaw; UINT32 MicrocodeFileSize;@@ -1155,9 +1171,12 @@ Returns: Error (NULL, 0, 0, "-I Parameter incorrect, too many StartupAcm!", NULL); return 0; }+ //+ // NOTE: BIOS INFO structure not support to override the Size/Rsvd/ChkSum byte value.+ // gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Type = FIT_TABLE_TYPE_STARTUP_ACM; gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Address = (UINT32)BiosInfoStruct[BiosInfoIndex].Address;- gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Size = (UINT32)BiosInfoStruct[BiosInfoIndex].Size;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].MaxSize = (UINT32)BiosInfoStruct[BiosInfoIndex].Size; gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Version = BiosInfoStruct[BiosInfoIndex].Version; gFitTableContext.StartupAcmNumber ++; gFitTableContext.FitEntryNumber ++;@@ -1389,10 +1408,52 @@ Returns: } gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Type = FIT_TABLE_TYPE_STARTUP_ACM; gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Address = (UINT32) (UINTN) FileBuffer;- gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Size = FileSize;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].MaxSize = FileSize; //- // 1.1 StartupAcm version+ // 1.1 Support 0x200 StartupAcm Information+ //+ if ((Index + 1 >= argc) ||+ ((strcmp (argv[Index], "-I") != 0) &&+ (strcmp (argv[Index], "-i") != 0)) ) {+ //+ // Bypass+ //+ gFitTableContext.StartupAcmVersion[gFitTableContext.StartupAcmNumber] = gFitTableContext.GlobalVersion;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Size = 0;+ } else {+ //+ // Should use the multiple StartupACM+ //+ if (Index + 5 >= argc) {+ //+ // Should get five input value, but not sufficient+ //+ Error (NULL, 0, 0, "-I Parameter incorrect, Require five inputs value!", NULL);+ return 0;+ } else {+ //+ // Should assign this as 0x200+ //+ gFitTableContext.StartupAcmVersion[gFitTableContext.StartupAcmNumber] = STARTUP_ACM_FIT_ENTRY_200_VERSION;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte0 = (UINT8)xtoi (argv[Index + 1]);+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte1 = (UINT8)xtoi (argv[Index + 2]);+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte2 = (UINT8)xtoi (argv[Index + 3]);+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Rsvd = (UINT8)xtoi (argv[Index + 4]);+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].ChkSum = (UINT8)xtoi (argv[Index + 5]);++ StartupAcmSizeByte0 = (UINT32)gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte0;+ StartupAcmSizeByte1 = (UINT32)gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte1;+ StartupAcmSizeByte2 = (UINT32)gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte2;++ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Size = (StartupAcmSizeByte2 << 16) + (StartupAcmSizeByte1 << 8) + (StartupAcmSizeByte0);++ Index += 6;+ }+ }++ //+ // 1.2 StartupAcm version // if ((Index + 1 >= argc) || ((strcmp (argv[Index], "-V") != 0) &&@@ -1400,18 +1461,17 @@ Returns: // // Bypass //- gFitTableContext.StartupAcmVersion = gFitTableContext.GlobalVersion; } else { // // Get offset from parameter //- gFitTableContext.StartupAcmVersion = xtoi (argv[Index + 1]);+ gFitTableContext.StartupAcmVersion[gFitTableContext.StartupAcmNumber] = xtoi (argv[Index + 1]); Index += 2; } gFitTableContext.StartupAcmNumber ++; gFitTableContext.FitEntryNumber ++;- }+ }; // // 1.5. DiagnosticsAcm@@ -1895,7 +1955,7 @@ Returns: // Final: Check StartupAcm in BiosModule. // for (Index = 0; Index < (INTN)gFitTableContext.StartupAcmNumber; Index++) {- CheckOverlap (gFitTableContext.StartupAcm[Index].Address, gFitTableContext.StartupAcm[Index].Size);+ CheckOverlap (gFitTableContext.StartupAcm[Index].Address, gFitTableContext.StartupAcm[Index].MaxSize); } FitEntryNumber = gFitTableContext.FitEntryNumber; for (Index = 0; Index < (INTN)gFitTableContext.OptionalModuleNumber; Index++) {@@ -2185,7 +2245,7 @@ Returns: printf ("Total FIT Entry number: 0x%x\n", gFitTableContext.FitEntryNumber); printf ("FitHeader version: 0x%04x\n", gFitTableContext.FitHeaderVersion); for (Index = 0; Index < gFitTableContext.StartupAcmNumber; Index++) {- printf ("StartupAcm[%d] - (0x%08x, 0x%08x, 0x%04x)\n", Index, gFitTableContext.StartupAcm[Index].Address, gFitTableContext.StartupAcm[Index].Size, gFitTableContext.StartupAcmVersion);+ printf ("StartupAcm[%d] - (0x%08x, 0x%08x, 0x%08x, 0x%04x)\n", Index, gFitTableContext.StartupAcm[Index].Address, gFitTableContext.StartupAcm[Index].Size, gFitTableContext.StartupAcm[Index].MaxSize, gFitTableContext.StartupAcmVersion[Index]); } if (gFitTableContext.DiagnstAcm.Address != 0) { printf ("DiagnosticAcm - (0x%08x, 0x%08x, 0x%04x)\n", gFitTableContext.DiagnstAcm.Address, gFitTableContext.DiagnstAcm.Size, gFitTableContext.DiagnstAcmVersion);@@ -2817,11 +2877,12 @@ Returns: // for (Index = 0; Index < gFitTableContext.StartupAcmNumber; Index++) { FitEntry[FitIndex].Address = gFitTableContext.StartupAcm[Index].Address;- *(UINT32 *)&FitEntry[FitIndex].Size[0] = 0; //gFitTableContext.StartupAcm.Size / 16;- FitEntry[FitIndex].Version = (UINT16)gFitTableContext.StartupAcmVersion;+ *(UINT32 *)&FitEntry[FitIndex].Size[0] = gFitTableContext.StartupAcm[Index].Size;+ FitEntry[FitIndex].Version = (UINT16)gFitTableContext.StartupAcmVersion[Index]; FitEntry[FitIndex].Type = FIT_TABLE_TYPE_STARTUP_ACM; FitEntry[FitIndex].C_V = 0;- FitEntry[FitIndex].Checksum = 0;+ FitEntry[FitIndex].Rsvd = gFitTableContext.StartupAcm[Index].Rsvd;+ FitEntry[FitIndex].Checksum = gFitTableContext.StartupAcm[Index].ChkSum; FitIndex++; } @@ -3170,8 +3231,16 @@ Returns: gFitTableContext.MicrocodeNumber ++; break; case FIT_TABLE_TYPE_STARTUP_ACM:- gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Address = (UINT32)FitEntry[FitIndex].Address;- gFitTableContext.StartupAcmVersion = FitEntry[FitIndex].Version;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Address = (UINT32)FitEntry[FitIndex].Address;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Type = FitEntry[FitIndex].Type;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Size = *(UINT32 *)&FitEntry[FitIndex].Size[0];+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte0 = *(UINT8 *)&FitEntry[FitIndex].Size[0];+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte1 = *(UINT8 *)&FitEntry[FitIndex].Size[1];+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].SizeByte2 = *(UINT8 *)&FitEntry[FitIndex].Size[2];+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].Rsvd = FitEntry[FitIndex].Rsvd;+ gFitTableContext.StartupAcm[gFitTableContext.StartupAcmNumber].ChkSum = FitEntry[FitIndex].Checksum;+ gFitTableContext.StartupAcmVersion[gFitTableContext.StartupAcmNumber] = FitEntry[FitIndex].Version;+ gFitTableContext.StartupAcmNumber ++; break; case FIT_TABLE_TYPE_BIOS_MODULE: gFitTableContext.BiosModule[gFitTableContext.BiosModuleNumber].Address = (UINT32)FitEntry[FitIndex].Address;@@ -3333,13 +3402,13 @@ Returns: for (Index = 0; Index < (INTN)gFitTableContext.StartupAcmNumber; Index ++) { if (gFitTableContext.StartupAcm[Index].Address != 0) { AcmBuffer = FLASH_TO_MEMORY(gFitTableContext.StartupAcm[Index].Address, FdFileBuffer, FdFileSize);- if ((AcmBuffer < FdFileBuffer) || (AcmBuffer + gFitTableContext.StartupAcm[Index].Size > FdFileBuffer + FdFileSize)) {+ if ((AcmBuffer < FdFileBuffer) || (AcmBuffer + gFitTableContext.StartupAcm[Index].MaxSize > FdFileBuffer + FdFileSize)) { printf ("ACM out of range - can not validate it\n"); AcmBuffer = NULL; } if (AcmBuffer != NULL) {- if (CheckAcm ((ACM_FORMAT *)AcmBuffer, gFitTableContext.StartupAcm[Index].Size)) {+ if (CheckAcm ((ACM_FORMAT *)AcmBuffer, gFitTableContext.StartupAcm[Index].MaxSize)) { DumpAcm ((ACM_FORMAT *)AcmBuffer); } else { Status = STATUS_ERROR;diff --git a/Silicon/Intel/Tools/FitGen/FitGen.h b/Silicon/Intel/Tools/FitGen/FitGen.h index b7de0a6b2d..80a1423ceb 100644 --- a/Silicon/Intel/Tools/FitGen/FitGen.h +++ b/Silicon/Intel/Tools/FitGen/FitGen.h @@ -31,7 +31,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent // Utility version information // #define UTILITY_MAJOR_VERSION 0-#define UTILITY_MINOR_VERSION 65+#define UTILITY_MINOR_VERSION 66 #define UTILITY_DATE __DATE__ //-- 2.26.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90734): https://edk2.groups.io/g/devel/message/90734 Mute This Topic: https://groups.io/mt/91944670/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
