[edk2-devel] [PATCH 1/5] MdePkg/BasePrintLib: avoid absolute addresses for error strings
Michael D Kinney
michael.d.kinney at intel.com
Wed Jun 10 15:09:21 UTC 2020
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.
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.
Mike
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On
> Behalf Of Ard Biesheuvel
> Sent: Wednesday, June 10, 2020 1:37 AM
> To: devel at edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney at intel.com>; Gao,
> Liming <liming.gao at intel.com>; Yao, Jiewen
> <jiewen.yao at intel.com>; Sami Mujawar
> <sami.mujawar at arm.com>; Ilias Apalodimas
> <ilias.apalodimas at 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 at 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
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61081): https://edk2.groups.io/g/devel/message/61081
Mute This Topic: https://groups.io/mt/74792288/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list