I was wondering what would happen if we converted the CHAR8 arrays in to strings with multiple nulls [1]. Looks like it saves space in uncompressed FVs, but not in compressed FVs. Here are the Xcode numbers for X64 DEBUG Ovmf: With this Patch: SECFV [14%Full] 212992 total, 30128 used, 182864 free PEIFV [29%Full] 917504 total, 273192 used, 644312 free DXEFV [39%Full] 12582912 total, 4997120 used, 7585792 free FVMAIN_COMPACT [36%Full] 3440640 total, 1271264 used, 2169376 free Vs my patch: SECFV [14%Full] 212992 total, 29872 used, 183120 free PEIFV [29%Full] 917504 total, 271048 used, 646456 free DXEFV [39%Full] 12582912 total, 4979552 used, 7603360 free FVMAIN_COMPACT [36%Full] 3440640 total, 1271824 used, 2168816 free SEC: -256 PEI: -2,144 Dxe: -17,568 Compact: +560 [1] $ git diff diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c index 50c6e8559c43..db2533e7affb 100644 --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c @@ -30,53 +30,73 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] = {'0','1','2','3','4','5',' // // Longest string: RETURN_WARN_BUFFER_TOO_SMALL => 24 characters plus NUL byte // -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mWarningString[][24+1] = { - "Success", // RETURN_SUCCESS = 0 - "Warning Unknown Glyph", // RETURN_WARN_UNKNOWN_GLYPH = 1 - "Warning Delete Failure", // RETURN_WARN_DELETE_FAILURE = 2 - "Warning Write Failure", // RETURN_WARN_WRITE_FAILURE = 3 - "Warning Buffer Too Small", // RETURN_WARN_BUFFER_TOO_SMALL = 4 - "Warning Stale Data", // RETURN_WARN_STALE_DATA = 5 -}; +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mWarningString = \ + "Success\0" /* RETURN_SUCCESS = 0 */ \ + "Warning Unknown Glyph\0" /* RETURN_WARN_UNKNOWN_GLYPH = 1 */ \ + "Warning Delete Failure\0" /* RETURN_WARN_DELETE_FAILURE = 2 */ \ + "Warning Write Failure\0" /* RETURN_WARN_WRITE_FAILURE = 3 */ \ + "Warning Buffer Too Small\0" /* RETURN_WARN_BUFFER_TOO_SMALL = 4 */ \ + "Warning Stale Data"; /* RETURN_WARN_STALE_DATA = 5 */ // // Longest string: RETURN_INCOMPATIBLE_VERSION => 20 characters plus NUL byte // -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mErrorString[][20+1] = { - "Load Error", // RETURN_LOAD_ERROR = 1 | MAX_BIT - "Invalid Parameter", // RETURN_INVALID_PARAMETER = 2 | MAX_BIT - "Unsupported", // RETURN_UNSUPPORTED = 3 | MAX_BIT - "Bad Buffer Size", // RETURN_BAD_BUFFER_SIZE = 4 | MAX_BIT - "Buffer Too Small", // RETURN_BUFFER_TOO_SMALL, = 5 | MAX_BIT - "Not Ready", // RETURN_NOT_READY = 6 | MAX_BIT - "Device Error", // RETURN_DEVICE_ERROR = 7 | MAX_BIT - "Write Protected", // RETURN_WRITE_PROTECTED = 8 | MAX_BIT - "Out of Resources", // RETURN_OUT_OF_RESOURCES = 9 | MAX_BIT - "Volume Corrupt", // RETURN_VOLUME_CORRUPTED = 10 | MAX_BIT - "Volume Full", // RETURN_VOLUME_FULL = 11 | MAX_BIT - "No Media", // RETURN_NO_MEDIA = 12 | MAX_BIT - "Media changed", // RETURN_MEDIA_CHANGED = 13 | MAX_BIT - "Not Found", // RETURN_NOT_FOUND = 14 | MAX_BIT - "Access Denied", // RETURN_ACCESS_DENIED = 15 | MAX_BIT - "No Response", // RETURN_NO_RESPONSE = 16 | MAX_BIT - "No mapping", // RETURN_NO_MAPPING = 17 | MAX_BIT - "Time out", // RETURN_TIMEOUT = 18 | MAX_BIT - "Not started", // RETURN_NOT_STARTED = 19 | MAX_BIT - "Already started", // RETURN_ALREADY_STARTED = 20 | MAX_BIT - "Aborted", // RETURN_ABORTED = 21 | MAX_BIT - "ICMP Error", // RETURN_ICMP_ERROR = 22 | MAX_BIT - "TFTP Error", // RETURN_TFTP_ERROR = 23 | MAX_BIT - "Protocol Error", // RETURN_PROTOCOL_ERROR = 24 | MAX_BIT - "Incompatible Version", // RETURN_INCOMPATIBLE_VERSION = 25 | MAX_BIT - "Security Violation", // RETURN_SECURITY_VIOLATION = 26 | MAX_BIT - "CRC Error", // RETURN_CRC_ERROR = 27 | MAX_BIT - "End of Media", // RETURN_END_OF_MEDIA = 28 | MAX_BIT - "Reserved (29)", // RESERVED = 29 | MAX_BIT - "Reserved (30)", // RESERVED = 30 | MAX_BIT - "End of File", // RETURN_END_OF_FILE = 31 | MAX_BIT - "Invalid Language", // RETURN_INVALID_LANGUAGE = 32 | MAX_BIT - "Compromised Data" // RETURN_COMPROMISED_DATA = 33 | MAX_BIT -}; +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mErrorString = \ + "Load Error\0" /* RETURN_LOAD_ERROR = 1 | MAX_BIT */ \ + "Invalid Parameter\0" /* RETURN_INVALID_PARAMETER = 2 | MAX_BIT */ \ + "Unsupported\0" /* RETURN_UNSUPPORTED = 3 | MAX_BIT */ \ + "Bad Buffer Size\0" /* RETURN_BAD_BUFFER_SIZE = 4 | MAX_BIT */ \ + "Buffer Too Small\0" /* RETURN_BUFFER_TOO_SMALL, = 5 | MAX_BIT */ \ + "Not Ready\0" /* RETURN_NOT_READY = 6 | MAX_BIT */ \ + "Device Error\0" /* RETURN_DEVICE_ERROR = 7 | MAX_BIT */ \ + "Write Protected\0" /* RETURN_WRITE_PROTECTED = 8 | MAX_BIT */ \ + "Out of Resources\0" /* RETURN_OUT_OF_RESOURCES = 9 | MAX_BIT */ \ + "Volume Corrupt\0" /* RETURN_VOLUME_CORRUPTED = 10 | MAX_BIT */ \ + "Volume Full\0" /* RETURN_VOLUME_FULL = 11 | MAX_BIT */ \ + "No Media\0" /* RETURN_NO_MEDIA = 12 | MAX_BIT */ \ + "Media changed\0" /* RETURN_MEDIA_CHANGED = 13 | MAX_BIT */ \ + "Not Found\0" /* RETURN_NOT_FOUND = 14 | MAX_BIT */ \ + "Access Denied\0" /* RETURN_ACCESS_DENIED = 15 | MAX_BIT */ \ + "No Response\0" /* RETURN_NO_RESPONSE = 16 | MAX_BIT */ \ + "No mapping\0" /* RETURN_NO_MAPPING = 17 | MAX_BIT */ \ + "Time out\0" /* RETURN_TIMEOUT = 18 | MAX_BIT */ \ + "Not started\0" /* RETURN_NOT_STARTED = 19 | MAX_BIT */ \ + "Already started\0" /* RETURN_ALREADY_STARTED = 20 | MAX_BIT */ \ + "Aborted\0" /* RETURN_ABORTED = 21 | MAX_BIT */ \ + "ICMP Error\0" /* RETURN_ICMP_ERROR = 22 | MAX_BIT */ \ + "TFTP Error\0" /* RETURN_TFTP_ERROR = 23 | MAX_BIT */ \ + "Protocol Error\0" /* RETURN_PROTOCOL_ERROR = 24 | MAX_BIT */ \ + "Incompatible Version\0" /* RETURN_INCOMPATIBLE_VERSION = 25 | MAX_BIT */ \ + "Security Violation\0" /* RETURN_SECURITY_VIOLATION = 26 | MAX_BIT */ \ + "CRC Error\0" /* RETURN_CRC_ERROR = 27 | MAX_BIT */ \ + "End of Media\0" /* RETURN_END_OF_MEDIA = 28 | MAX_BIT */ \ + "Reserved (29)\0" /* RESERVED = 29 | MAX_BIT */ \ + "Reserved (30)\0" /* RESERVED = 30 | MAX_BIT */ \ + "End of File\0" /* RETURN_END_OF_FILE = 31 | MAX_BIT */ \ + "Invalid Language\0" /* RETURN_INVALID_LANGUAGE = 32 | MAX_BIT */ \ + "Compromised Data"; /* RETURN_COMPROMISED_DATA = 33 | MAX_BIT */ + + +CONST CHAR8 * +FindNthStr ( + IN CONST CHAR8 *Start, + IN UINTN Index + ) +{ + CONST CHAR8 *str; + + for (str = Start; Index > 0; str++) { + if (*str == '\0') { + Index--; + if (Index == 0) { + str++; + break; + } + } + } + + return str; +} /** @@ -1005,12 +1025,12 @@ BasePrintLibSPrintMarker ( // Index = Status & ~MAX_BIT; if (Index > 0 && Index <= ERROR_STATUS_NUMBER) { - ArgumentString = mErrorString [Index - 1]; + ArgumentString = FindNthStr (mErrorString, Index - 1); } } else { Index = Status; if (Index <= WARNING_STATUS_NUMBER) { - ArgumentString = mWarningString [Index]; + ArgumentString = FindNthStr (mWarningString, Index); } } if (ArgumentString == ValueBuffer) { Thanks, Andrew Fish > On Jun 15, 2020, at 6:57 PM, Liming Gao wrote: > > This data is great. The change is good. Reviewed-by: Liming Gao > > > From: Liu, Zhiguang > > Sent: 2020年6月15日 15:34 > To: Ard Biesheuvel >; Gao, Liming >; devel@edk2.groups.io > Cc: Kinney, Michael D > > Subject: RE: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute addresses for error strings > > Hi Ard, > > I also collected the image size of OVMFX64 size building with VS2015x86. > > Debug > Release > > before > after > After-before > before > after > after-before > sec > 27664 > 27409 > -255 > 13968 > 13968 > 0 > pei > 223016 > 221000 > -2016 > 107208 > 106984 > -224 > dxe > 4507000 > 4481336 > -25664 > 2987064 > 2979384 > -7680 > compact > 1179776 > 1172528 > -7248 > 922664 > 920304 > -2360 > > It can reduce the image size in X64. > Reviewed-by: Zhiguang Liu > > > > -----Original Message----- > > From: Ard Biesheuvel > > > Sent: Friday, June 12, 2020 11:11 PM > > To: Gao, Liming >; devel@edk2.groups.io > > Cc: Kinney, Michael D >; Liu, Zhiguang > > > > > Subject: Re: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute > > addresses for error strings > > > > On 6/12/20 4:33 PM, Gao, Liming wrote: > > > Ard: > > > I will collect the image size on OVMF X64 platform with this patch. > > > > > > > Building OvmfPkgX64.dsc in RELEASE mode using GCC5 profile gives me > > > > > > Before: > > SECFV [5%Full] 212992 total, 11760 used, 201232 free PEIFV [9%Full] 917504 > > total, 89384 used, 828120 free DXEFV [22%Full] 12582912 total, 2806320 used, > > 9776592 free FVMAIN_COMPACT [26%Full] 3440640 total, 918760 used, > > 2521880 free > > > > After: > > SECFV [5%Full] 212992 total, 11760 used, 201232 free PEIFV [9%Full] 917504 > > total, 89192 used, 828312 free DXEFV [22%Full] 12582912 total, 2802928 used, > > 9779984 free FVMAIN_COMPACT [26%Full] 3440640 total, 916784 used, > > 2523856 free > > > > So no change for SECFV, -192 bytes for PEIFV, -3392 bytes for DXEFV and > > -1976 bytes for the resulting outer FV image. > > > > > > > > >> -----Original Message----- > > >> From: devel@edk2.groups.io > On Behalf Of Ard > > Biesheuvel > > >> Sent: Friday, June 12, 2020 6:35 AM > > >> To: devel@edk2.groups.io > > >> Cc: Ard Biesheuvel >; Kinney, Michael D > > >; Gao, Liming > > > >> Subject: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute > > addresses for error strings > > >> > > >> The mStatusString[] array is constructed as an array of pointer-to-char, > > >> which means that on X64 or AARCH64, it is emitted as a single linear list > > >> of 64-bit quantities, each containing the absolute address of one of the > > >> string literals in memory. > > >> > > >> This means that each string takes up 8 bytes of additional space, along > > >> with 2 bytes of relocation data. It also means that extra work needs to > > >> be done at runtime to process these relocations, every time a module is > > >> loaded that incorporates this library. > > >> > > >> So fix both issues, by splitting mStatusString into two arrays of char > > >> arrays. The memory footprint decreases from 955 to 843 bytes, and given > > >> that in the latter case, the overhead consists of 278 NUL characters rather > > >> than 390 bytes worth of absolute addresses and relocation records, the > > size > > >> of a compressed image is reduced even further. For example, when > > building > > >> ArmVirtQemu.dsc in RELEASE mode for AARCH64 with the GCC5 profile, I > > get: > > >> > > >> Before > > >> > > >> FV Space Information > > >> FVMAIN [100%Full] 5329920 total, 5329920 used, 0 free > > >> FVMAIN_COMPACT [38%Full] 2093056 total, 811840 used, 1281216 free > > >> > > >> After > > >> > > >> FV Space Information > > >> FVMAIN [100%Full] 5321728 total, 5321728 used, 0 free > > >> FVMAIN_COMPACT [38%Full] 2093056 total, 809696 used, 1283360 free > > >> > > >> So the uncompressed contents of the compressed image are 8 KB smaller, > > >> whereas the resulting flash image (consisting of the compressed image > > >> along with SEC, PEI_CORE and a set of PEIMs that execute in place) is > > >> 2 KB smaller. > > >> > > >> Cc: "Kinney, Michael D" > > > >> Cc: "Gao, Liming" > > > >> Signed-off-by: Ard Biesheuvel > > > >> --- > > >> v3: > > >> - add code comments to explain what the inner dimension of each array is > > >> based on > > >> > > >> v2: > > >> - split off this patch from the StandaloneMmPkg series, since they are not > > >> interdependent anyway, and so they can be discussed separately > > >> - remove mention of StandaloneMmPkg from the commit log - the space > > savings > > >> by themselves are sufficient justification > > >> - update the before/after numbers with the current results > > >> - split the warnings and errors into a separate array, so that the latter > > >> can use smaller entries > > >> - clarify the commit log to explain the effect on compressed as well as > > >> XIP images (which both get smaller) > > >> > > >> MdePkg/Library/BasePrintLib/PrintLibInternal.c | 15 ++++++++++++--- > > >> 1 file changed, 12 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > >> index b6ec5ac4fbb9..50c6e8559c43 100644 > > >> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > >> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > >> @@ -27,13 +27,22 @@ > > >> > > >> > > >> GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] = > > {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'}; > > >> > > >> > > >> > > >> -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 * CONST > > mStatusString[] = { > > >> > > >> +// > > >> > > >> +// Longest string: RETURN_WARN_BUFFER_TOO_SMALL => 24 > > characters plus NUL byte > > >> > > >> +// > > >> > > >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 > > mWarningString[][24+1] = { > > >> > > >> "Success", // RETURN_SUCCESS = 0 > > >> > > >> "Warning Unknown Glyph", // RETURN_WARN_UNKNOWN_GLYPH > > = 1 > > >> > > >> "Warning Delete Failure", // RETURN_WARN_DELETE_FAILURE = 2 > > >> > > >> "Warning Write Failure", // RETURN_WARN_WRITE_FAILURE = 3 > > >> > > >> "Warning Buffer Too Small", // > > RETURN_WARN_BUFFER_TOO_SMALL = 4 > > >> > > >> "Warning Stale Data", // RETURN_WARN_STALE_DATA = 5 > > >> > > >> +}; > > >> > > >> + > > >> > > >> +// > > >> > > >> +// Longest string: RETURN_INCOMPATIBLE_VERSION => 20 characters > > plus NUL byte > > >> > > >> +// > > >> > > >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 > > mErrorString[][20+1] = { > > >> > > >> "Load Error", // RETURN_LOAD_ERROR = 1 | MAX_BIT > > >> > > >> "Invalid Parameter", // RETURN_INVALID_PARAMETER = 2 | > > MAX_BIT > > >> > > >> "Unsupported", // RETURN_UNSUPPORTED = 3 | > > MAX_BIT > > >> > > >> @@ -996,12 +1005,12 @@ BasePrintLibSPrintMarker ( > > >> // > > >> > > >> Index = Status & ~MAX_BIT; > > >> > > >> if (Index > 0 && Index <= ERROR_STATUS_NUMBER) { > > >> > > >> - ArgumentString = mStatusString [Index + > > WARNING_STATUS_NUMBER]; > > >> > > >> + ArgumentString = mErrorString [Index - 1]; > > >> > > >> } > > >> > > >> } else { > > >> > > >> Index = Status; > > >> > > >> if (Index <= WARNING_STATUS_NUMBER) { > > >> > > >> - ArgumentString = mStatusString [Index]; > > >> > > >> + ArgumentString = mWarningString [Index]; > > >> > > >> } > > >> > > >> } > > >> > > >> if (ArgumentString == ValueBuffer) { > > >> > > >> -- > > >> 2.26.2 > > >> > > >> > > >> -=-=-=-=-=-= > > >> Groups.io Links: You receive all messages sent to this group. > > >> > > >> View/Reply Online (#61170): > > https://edk2.groups.io/g/devel/message/61170 > > >> Mute This Topic: https://groups.io/mt/74829004/1759384 > > >> Group Owner: devel+owner@edk2.groups.io > > >> Unsubscribe: https://edk2.groups.io/g/devel/unsub > > [liming.gao@intel.com ] > > >> -=-=-=-=-=-= > > > > >