From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.509.1591807185861175483 for ; Wed, 10 Jun 2020 09:39:46 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8223A1FB; Wed, 10 Jun 2020 09:39:44 -0700 (PDT) Received: from [192.168.1.69] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5F23A3F6CF; Wed, 10 Jun 2020 09:39:43 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: "Gao, Liming" , "Yao, Jiewen" , Sami Mujawar , Ilias Apalodimas References: <20200610081740.54581-1-ard.biesheuvel@arm.com> <20200610081740.54581-2-ard.biesheuvel@arm.com> <4d970dd2-4b28-046b-40e7-685b6b680c61@arm.com> From: "Ard Biesheuvel" Message-ID: <764cb700-3d89-144e-b6aa-f7aca2e262ec@arm.com> Date: Wed, 10 Jun 2020 18:39:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 6/10/20 5:09 PM, Michael D Kinney via groups.io wrote: > Hi Ard, > > The size reduction is very interesting. Would be good to > see uncompressed size differences as well because this lib > is also used in pre-memory XIP modules that are fixed up > by the build tools that handle the relocation fixups. > As I said in my reply-to-self, this results in a net decrease in both cases: >> FV Space Information >> >> FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free >> FVMAIN_COMPACT [37%Full] 2093056 total, 792984 used, >> 1300072 free >> >> to >> >> FV Space Information >> FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free >> FVMAIN_COMPACT [37%Full] 2093056 total, 791072 used, >> 1301984 free >> FVMAIN is the uncompressed primary FV FVMAIN_COMPACT contains FVMAIN in compressed form, along with the PEI and SEC modules. So the XIP case is a win as well. > The more general concern I have is if there are standard > C coding practices (such as a pre-initialized array of > pointers to strings) that are not compatible with > StandaloneMmPkg use cases. > In general, yes. In this case, it is only about being able to make it to the PE/COFF relocation routines and back, which happens very early (in a AARCH64 specific StMM core entrypoint library, mind you). So using statically initialized pointers is fine in general, just not in the early startup code that runs before relocation. >> -----Original Message----- >> From: devel@edk2.groups.io On >> Behalf Of Ard Biesheuvel >> Sent: Wednesday, June 10, 2020 1:37 AM >> To: devel@edk2.groups.io >> Cc: Kinney, Michael D ; Gao, >> Liming ; Yao, Jiewen >> ; Sami Mujawar >> ; Ilias Apalodimas >> >> Subject: Re: [edk2-devel] [PATCH 1/5] >> MdePkg/BasePrintLib: avoid absolute addresses for error >> strings >> >> On 6/10/20 10:17 AM, Ard Biesheuvel wrote: >>> The mStatusString[] 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 >> the array cannot be >>> used until PE/COFF relocation has completed, and so >> the following >>> invocation >>> >>> Status = PeCoffLoaderRelocateImage >> (&ImageContext); >>> ASSERT_EFI_ERROR (Status); >>> >>> that we will be introducing into StandaloneMmCore >> entrypoint for AARCH64 >>> to relocate the executable on the fly is guaranteed to >> return bogus output >>> or crash, which is less than helpful. >>> >>> So fix both issues, by emitting mStatusString as an >> array of char arrays >>> instead. The memory footprint increases from 955 to >> 975 bytes, but given >>> that in the latter case, the overhead consists of 410 >> NUL characters >>> rather than 390 bytes worth of absolute addresses and >> relocation records, >>> the impact on a compressed image is actually positive. >> For example, when >>> building ArmVirtQemu.dsc in RELEASE mode for AARCH64 >> with the GCC5 profile, >>> I get: >>> >>> Before >>> >>> FV Space Information >>> FVMAIN [99%Full] 4784768 total, 4784720 used, 48 >> free >>> FVMAIN_COMPACT [38%Full] 2093056 total, 811560 >> used, 1281496 free >>> >>> After >>> >>> FV Space Information >>> FVMAIN [99%Full] 4780672 total, 4780624 used, 48 >> free >>> FVMAIN_COMPACT [38%Full] 2093056 total, 813488 >> used, 1279568 free >>> >>> So the compressed image is 4 KB smaller, whereas the >> entire image is >>> < 2 KB larger, which is in the order of 0.2 % >>> >>> Signed-off-by: Ard Biesheuvel >> >> Actually, I found a slightly better way of doing this, >> by applying the >> following delta on top, i.e., split the errors and >> warnings, and use an >> array of 21 byte character arrays for the former. >> >> My build went from >> >> FV Space Information >> >> FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free >> FVMAIN_COMPACT [37%Full] 2093056 total, 792984 used, >> 1300072 free >> >> to >> >> FV Space Information >> FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free >> FVMAIN_COMPACT [37%Full] 2093056 total, 791072 used, >> 1301984 free >> >> >> in this case, i.e., both compressed and non-compressed >> images are >> slightly smaller. >> >> >> >> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c >> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c >> @@ -27,13 +27,16 @@ >> >> 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 >> mStatusString[][25] = { >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 >> mWarningString[][25] = { >> "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 >> mErrorString[][21] = { >> "Load Error", // >> RETURN_LOAD_ERROR = >> 1 | MAX_BIT >> "Invalid Parameter", // >> RETURN_INVALID_PARAMETER = >> 2 | MAX_BIT >> "Unsupported", // >> RETURN_UNSUPPORTED = >> 3 | MAX_BIT >> @@ -996,12 +999,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) { >> >> >>> --- >>> MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2 >> +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git >> a/MdePkg/Library/BasePrintLib/PrintLibInternal.c >> b/MdePkg/Library/BasePrintLib/PrintLibInternal.c >>> index b6ec5ac4fbb9..c8b932c7e07a 100644 >>> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c >>> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c >>> @@ -27,7 +27,7 @@ >>> >>> 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[] = { >>> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 >> mStatusString[][25] = { >>> "Success", // RETURN_SUCCESS >> = 0 >>> "Warning Unknown Glyph", // >> RETURN_WARN_UNKNOWN_GLYPH = 1 >>> "Warning Delete Failure", // >> RETURN_WARN_DELETE_FAILURE = 2 >>> >> >> >> > > > >