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.web10.6697.1592298903749207963 for ; Tue, 16 Jun 2020 02:15:04 -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 6FD561FB; Tue, 16 Jun 2020 02:15:02 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.36]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0D53F3F71F; Tue, 16 Jun 2020 02:15:00 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute addresses for error strings To: "Gao, Liming" , "Liu, Zhiguang" , "devel@edk2.groups.io" Cc: "Kinney, Michael D" References: <20200611223509.344290-1-ard.biesheuvel@arm.com> From: "Ard Biesheuvel" Message-ID: Date: Tue, 16 Jun 2020 11:14:56 +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: quoted-printable On 6/16/20 3:57 AM, Gao, Liming wrote: > This data is great. The change is good. Reviewed-by: Liming Gao=20 > >=20 Submitted as #699 and merged, Thanks all > *From:*Liu, Zhiguang > *Sent:* 2020=E5=B9=B46=E6=9C=8815=E6=97=A515:34 > *To:* Ard Biesheuvel ; Gao, Liming=20 > ; devel@edk2.groups.io > *Cc:* Kinney, Michael D > *Subject:* RE: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid=20 > absolute addresses for error strings >=20 > Hi Ard, >=20 > I also collected the image size of OVMFX64 size building with VS2015x86= . >=20 > =09 >=20 > Debug >=20 > =09 >=20 > Release >=20 > =09 >=20 > before >=20 > =09 >=20 > after >=20 > =09 >=20 > After-before >=20 > =09 >=20 > before >=20 > =09 >=20 > after >=20 > =09 >=20 > after-before >=20 > sec >=20 > =09 >=20 > 27664 >=20 > =09 >=20 > 27409 >=20 > =09 >=20 > -255 >=20 > =09 >=20 > 13968 >=20 > =09 >=20 > 13968 >=20 > =09 >=20 > 0 >=20 > pei >=20 > =09 >=20 > 223016 >=20 > =09 >=20 > 221000 >=20 > =09 >=20 > -2016 >=20 > =09 >=20 > 107208 >=20 > =09 >=20 > 106984 >=20 > =09 >=20 > -224 >=20 > dxe >=20 > =09 >=20 > 4507000 >=20 > =09 >=20 > 4481336 >=20 > =09 >=20 > -25664 >=20 > =09 >=20 > 2987064 >=20 > =09 >=20 > 2979384 >=20 > =09 >=20 > -7680 >=20 > compact >=20 > =09 >=20 > 1179776 >=20 > =09 >=20 > 1172528 >=20 > =09 >=20 > -7248 >=20 > =09 >=20 > 922664 >=20 > =09 >=20 > 920304 >=20 > =09 >=20 > -2360 >=20 > It can reduce the image size in X64. >=20 > Reviewed-by: Zhiguang Liu > >=20 >> -----Original Message----- >=20 >> From: Ard Biesheuvel > >=20 >> Sent: Friday, June 12, 2020 11:11 PM >=20 >> To: Gao, Liming >;=20 > devel@edk2.groups.io >=20 >> Cc: Kinney, Michael D >; Liu,=20 > Zhiguang >=20 >> > >=20 >> Subject: Re: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolu= te >=20 >> addresses for error strings >=20 >>=20 >=20 >> On 6/12/20 4:33 PM, Gao, Liming wrote: >=20 >> > Ard: >=20 >> >=C2=A0=C2=A0=C2=A0 I will collect the image size on OVMF X64 platform= with this patch. >=20 >> > >=20 >>=20 >=20 >> Building OvmfPkgX64.dsc in RELEASE mode using GCC5 profile gives me >=20 >>=20 >=20 >>=20 >=20 >> Before: >=20 >> SECFV [5%Full] 212992 total, 11760 used, 201232 free PEIFV [9%Full] 91= 7504 >=20 >> total, 89384 used, 828120 free DXEFV [22%Full] 12582912 total, 2806320= used, >=20 >> 9776592 free FVMAIN_COMPACT [26%Full] 3440640 total, 918760 used, >=20 >> 2521880 free >=20 >>=20 >=20 >> After: >=20 >> SECFV [5%Full] 212992 total, 11760 used, 201232 free PEIFV [9%Full] 91= 7504 >=20 >> total, 89192 used, 828312 free DXEFV [22%Full] 12582912 total, 2802928= used, >=20 >> 9779984 free FVMAIN_COMPACT [26%Full] 3440640 total, 916784 used, >=20 >> 2523856 free >=20 >>=20 >=20 >> So no change for SECFV, -192 bytes for PEIFV, -3392 bytes for DXEFV an= d >=20 >> -1976 bytes for the resulting outer FV image. >=20 >>=20 >=20 >>=20 >=20 >>=20 >=20 >> >> -----Original Message----- >=20 >> >> From: devel@edk2.groups.io > On Behalf Of Ard >=20 >> Biesheuvel >=20 >> >> Sent: Friday, June 12, 2020 6:35 AM >=20 >> >> To: devel@edk2.groups.io >=20 >> >> Cc: Ard Biesheuvel >; Kinney, Michael D >=20 >> >; Gao,= =20 > Liming > >=20 >> >> Subject: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolut= e >=20 >> addresses for error strings >=20 >> >> >=20 >> >> The mStatusString[] array is constructed as an array of pointer-to-= char, >=20 >> >> which means that on X64 or AARCH64, it is emitted as a single linea= r list >=20 >> >> of 64-bit quantities, each containing the absolute address of one o= f the >=20 >> >> string literals in memory. >=20 >> >> >=20 >> >> This means that each string takes up 8 bytes of additional space, a= long >=20 >> >> with 2 bytes of relocation data. It also means that extra work need= s to >=20 >> >> be done at runtime to process these relocations, every time a modul= e is >=20 >> >> loaded that incorporates this library. >=20 >> >> >=20 >> >> So fix both issues, by splitting mStatusString into two arrays of c= har >=20 >> >> arrays. The memory footprint decreases from 955 to 843 bytes, and g= iven >=20 >> >> that in the latter case, the overhead consists of 278 NUL character= s rather >=20 >> >> than 390 bytes worth of absolute addresses and relocation records, = the >=20 >> size >=20 >> >> of a compressed image is reduced even further. For example, when >=20 >> building >=20 >> >> ArmVirtQemu.dsc in RELEASE mode for AARCH64 with the GCC5 profile, = I >=20 >> get: >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0 Before >=20 >> >> >=20 >> >>=C2=A0 =C2=A0=C2=A0FV Space Information >=20 >> >>=C2=A0=C2=A0=C2=A0 FVMAIN [100%Full] 5329920 total, 5329920 used, 0 = free >=20 >> >>=C2=A0=C2=A0=C2=A0 FVMAIN_COMPACT [38%Full] 2093056 total, 811840 us= ed, 1281216 free >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0 After >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0 FV Space Information >=20 >> >>=C2=A0=C2=A0=C2=A0 FVMAIN [100%Full] 5321728 total, 5321728 used, 0 = free >=20 >> >>=C2=A0=C2=A0=C2=A0 FVMAIN_COMPACT [38%Full] 2093056 total, 809696 us= ed, 1283360 free >=20 >> >> >=20 >> >> So the uncompressed contents of the compressed image are 8 KB small= er, >=20 >> >> whereas the resulting flash image (consisting of the compressed ima= ge >=20 >> >> along with SEC, PEI_CORE and a set of PEIMs that execute in place) = is >=20 >> >> 2 KB smaller. >=20 >> >> >=20 >> >> Cc: "Kinney, Michael D" > >=20 >> >> Cc: "Gao, Liming" > >=20 >> >> Signed-off-by: Ard Biesheuvel > >=20 >> >> --- >=20 >> >> v3: >=20 >> >> - add code comments to explain what the inner dimension of each arr= ay is >=20 >> >>=C2=A0=C2=A0=C2=A0 based on >=20 >> >> >=20 >> >> v2: >=20 >> >> - split off this patch from the StandaloneMmPkg series, since they = are not >=20 >> >>=C2=A0=C2=A0=C2=A0 interdependent anyway, and so they can be discuss= ed separately >=20 >> >> - remove mention of StandaloneMmPkg from the commit log - the space >=20 >> savings >=20 >> >>=C2=A0=C2=A0=C2=A0 by themselves are sufficient justification >=20 >> >> - update the before/after numbers with the current results >=20 >> >> - split the warnings and errors into a separate array, so that the = latter >=20 >> >>=C2=A0=C2=A0=C2=A0 can use smaller entries >=20 >> >> - clarify the commit log to explain the effect on compressed as wel= l as >=20 >> >>=C2=A0=C2=A0=C2=A0 XIP images (which both get smaller) >=20 >> >> >=20 >> >>=C2=A0=C2=A0 MdePkg/Library/BasePrintLib/PrintLibInternal.c | 15 +++= +++++++++--- >=20 >> >>=C2=A0=C2=A0 1 file changed, 12 insertions(+), 3 deletions(-) >=20 >> >> >=20 >> >> diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c >=20 >> b/MdePkg/Library/BasePrintLib/PrintLibInternal.c >=20 >> >> index b6ec5ac4fbb9..50c6e8559c43 100644 >=20 >> >> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c >=20 >> >> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c >=20 >> >> @@ -27,13 +27,22 @@ >=20 >> >> >=20 >> >> >=20 >> >>=C2=A0=C2=A0 GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] =3D >=20 >> {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'}; >=20 >> >> >=20 >> >> >=20 >> >> >=20 >> >> -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 * CONST >=20 >> mStatusString[] =3D { >=20 >> >> >=20 >> >> +// >=20 >> >> >=20 >> >> +// Longest string: RETURN_WARN_BUFFER_TOO_SMALL =3D> 24 >=20 >> characters plus NUL byte >=20 >> >> >=20 >> >> +// >=20 >> >> >=20 >> >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 >=20 >> mWarningString[][24+1] =3D { >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Success",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 //=C2=A0 RETURN_SUCCESS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D 0 >=20 >> >> >=20 >> >>=C2=A0 =C2=A0=C2=A0=C2=A0"Warning Unknown Glyph",=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 //=C2=A0 RETURN_WARN_UNKNOWN_GLYPH >=20 >> =3D 1 >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Warning Delete Failure",=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 //=C2=A0 RETURN_WARN_DELETE_FAILURE=C2=A0=C2=A0=C2=A0 = =3D 2 >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Warning Write Failure",=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 //=C2=A0 RETURN_WARN_WRITE_FAILURE=C2=A0=C2=A0=C2=A0= =C2=A0 =3D 3 >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Warning Buffer Too Small",=C2=A0=C2=A0=C2=A0= =C2=A0 // >=20 >> RETURN_WARN_BUFFER_TOO_SMALL=C2=A0 =3D 4 >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Warning Stale Data",=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 //=C2=A0 RETURN_WARN_STALE_DATA=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D 5 >=20 >> >> >=20 >> >> +}; >=20 >> >> >=20 >> >> + >=20 >> >> >=20 >> >> +// >=20 >> >> >=20 >> >> +// Longest string: RETURN_INCOMPATIBLE_VERSION =3D> 20 characters >=20 >> plus NUL byte >=20 >> >> >=20 >> >> +// >=20 >> >> >=20 >> >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 >=20 >> mErrorString[][20+1] =3D { >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Load Error",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 //=C2=A0 RETURN_LOAD_ERROR=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D 1=C2=A0 | MAX_BIT >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Invalid Parameter",=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 //=C2=A0 RETURN_INVALID_PARAME= TER=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D 2=C2=A0 | >=20 >> MAX_BIT >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0 "Unsupported",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = //=C2=A0 RETURN_UNSUPPORTED=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 =3D 3=C2=A0 | >=20 >> MAX_BIT >=20 >> >> >=20 >> >> @@ -996,12 +1005,12 @@ BasePrintLibSPrintMarker ( >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 // >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 Index =3D Status & ~MAX_BIT; >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 if (Index > 0 && Index <=3D ERROR_STATUS_NUMBER) { >=20 >> >> >=20 >> >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= ArgumentString =3D mStatusString [Index + >=20 >> WARNING_STATUS_NUMBER]; >=20 >> >> >=20 >> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= ArgumentString =3D mErrorString [Index - 1]; >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 } >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else = { >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 Index =3D Status; >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 if (Index <=3D WARNING_STATUS_NUMBER) { >=20 >> >> >=20 >> >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= ArgumentString =3D mStatusString [Index]; >=20 >> >> >=20 >> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= ArgumentString =3D mWarningString [Index]; >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 } >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >=20 >> >> >=20 >> >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (Arg= umentString =3D=3D ValueBuffer) { >=20 >> >> >=20 >> >> -- >=20 >> >> 2.26.2 >=20 >> >> >=20 >> >> >=20 >> >> -=3D-=3D-=3D-=3D-=3D-=3D >=20 >> >> Groups.io Links: You receive all messages sent to this group. >=20 >> >> >=20 >> >> View/Reply Online (#61170): >=20 >> https://edk2.groups.io/g/devel/message/61170 >=20 >> >> Mute This Topic: https://groups.io/mt/74829004/1759384 >=20 >> >> Group Owner: devel+owner@edk2.groups.io >=20 >> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub >=20 >> [liming.gao@intel.com] >=20 >> >> -=3D-=3D-=3D-=3D-=3D-=3D >=20 >> > >=20