From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
To: devel@edk2.groups.io, michael.d.kinney@intel.com
Cc: "Gao, Liming" <liming.gao@intel.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>,
Sami Mujawar <sami.mujawar@arm.com>,
Ilias Apalodimas <ilias.apalodimas@linaro.org>
Subject: Re: [edk2-devel] [PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings
Date: Wed, 10 Jun 2020 18:39:41 +0200 [thread overview]
Message-ID: <764cb700-3d89-144e-b6aa-f7aca2e262ec@arm.com> (raw)
In-Reply-To: <MN2PR11MB4461748940674B297ADDEC18D2830@MN2PR11MB4461.namprd11.prod.outlook.com>
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 <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 <michael.d.kinney@intel.com>; Gao,
>> Liming <liming.gao@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Sami Mujawar
>> <sami.mujawar@arm.com>; Ilias Apalodimas
>> <ilias.apalodimas@linaro.org>
>> 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 <ard.biesheuvel@arm.com>
>>
>> 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
>>>
>>
>>
>>
>
>
>
>
next prev parent reply other threads:[~2020-06-10 16:39 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-10 8:17 [PATCH 0/5] StandaloneMmPkg: make StMM core relocatable Ard Biesheuvel
2020-06-10 8:17 ` [PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings Ard Biesheuvel
2020-06-10 8:37 ` Ard Biesheuvel
2020-06-10 15:09 ` [edk2-devel] " Michael D Kinney
2020-06-10 16:39 ` Ard Biesheuvel [this message]
2020-06-10 8:17 ` [PATCH 2/5] StandaloneMmPkg/Core: fix bogus FV pointer in DEBUG string Ard Biesheuvel
2020-06-14 12:35 ` [edk2-devel] " Yao, Jiewen
2020-06-15 12:42 ` Sami Mujawar
2020-06-10 8:17 ` [PATCH 3/5] StandaloneMmPkg/Core: add missing GUID reference Ard Biesheuvel
2020-06-14 12:36 ` Yao, Jiewen
2020-06-15 12:49 ` [edk2-devel] " Sami Mujawar
2020-06-10 8:17 ` [PATCH 4/5] StandaloneMmPkg: generate position independent code for StMM core Ard Biesheuvel
2020-06-10 18:21 ` [edk2-devel] " Sean
2020-06-10 18:33 ` Ard Biesheuvel
2020-06-14 12:38 ` Yao, Jiewen
2020-06-10 8:17 ` [PATCH 5/5] StandaloneMmPkg/StandaloneMmCoreEntryPoint: relocate StMM core on the fly Ard Biesheuvel
2020-06-14 12:37 ` [edk2-devel] " Yao, Jiewen
2020-06-15 13:59 ` Sami Mujawar
2020-06-15 14:12 ` Ard Biesheuvel
2020-06-15 14:40 ` Sami Mujawar
2020-06-10 10:21 ` [PATCH 0/5] StandaloneMmPkg: make StMM core relocatable Ilias Apalodimas
2020-06-12 9:58 ` Ard Biesheuvel
2020-06-16 16:16 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=764cb700-3d89-144e-b6aa-f7aca2e262ec@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox