Re: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute addresses for error strings

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 6/16/20 3:57 AM, Gao, Liming wrote:
> This data is great. The change is good. Reviewed-by: Liming Gao 
> <liming.gao@intel.com>
> 

Submitted as #699 and merged,

Thanks all

> *From:*Liu, Zhiguang <zhiguang.liu@intel.com>
> *Sent:* 2020年6月15日15:34
> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>; Gao, Liming 
> <liming.gao@intel.com>; devel@edk2.groups.io
> *Cc:* Kinney, Michael D <michael.d.kinney@intel.com>
> *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 <zhiguang.liu@intel.com 
> <mailto:zhiguang.liu@intel.com>>
> 
>> -----Original Message-----
> 
>> From: Ard Biesheuvel <ard.biesheuvel@arm.com <mailto:ard.biesheuvel@arm.com>>
> 
>> Sent: Friday, June 12, 2020 11:11 PM
> 
>> To: Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> 
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Liu, 
> Zhiguang
> 
>> <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>
> 
>> 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>> On Behalf Of Ard
> 
>> Biesheuvel
> 
>> >> Sent: Friday, June 12, 2020 6:35 AM
> 
>> >> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> 
>> >> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com <mailto:ard.biesheuvel@arm.com>>; Kinney, Michael D
> 
>> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Gao, 
> Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>
> 
>> >> 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" <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> 
>> >> Cc: "Gao, Liming" <liming.gao@intel.com <mailto:liming.gao@intel.com>>
> 
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com <mailto:ard.biesheuvel@arm.com>>
> 
>> >> ---
> 
>> >> 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 <mailto:devel+owner@edk2.groups.io>
> 
>> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> 
>> [liming.gao@intel.com]
> 
>> >> -=-=-=-=-=-=
> 
>> >
>