[edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
Liming Gao
liming.gao at intel.com
Thu Apr 23 05:49:08 UTC 2020
Guomin:
You can count the size of all generated EFI images for IA32 and X64, then compare them between two builds. This change should impact EFI image only. Based on this data, we can know the image size impact.
Thanks
Liming
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Guomin Jiang
> Sent: Thursday, April 23, 2020 12:04 PM
> To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io; Kinney, Michael D <michael.d.kinney at intel.com>; ard.biesheuvel at linaro.org
> Cc: lersek at redhat.com; macarl at microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
>
> I guess the key point is /GL option. IntrinsicLib omit the /GL option to avoid the build error, but it abandon the optimization
> meanwhile.
> I will do a test: create a simplest application and just depend IntrinsicLib and add /GL option to compare the different with and
> without /GL.
>
> > -----Original Message-----
> > From: Ni, Ray <ray.ni at intel.com>
> > Sent: Thursday, April 23, 2020 11:32 AM
> > To: Jiang, Guomin <guomin.jiang at intel.com>; devel at edk2.groups.io; Kinney,
> > Michael D <michael.d.kinney at intel.com>; ard.biesheuvel at linaro.org
> > Cc: lersek at redhat.com; macarl at microsoft.com
> > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> > float.
> >
> > Guomin,
> > Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib to
> > MdePkg/BaseLib saves size?
> >
> > Thanks,
> > Ray
> >
> > > -----Original Message-----
> > > From: Jiang, Guomin <guomin.jiang at intel.com>
> > > Sent: Thursday, April 23, 2020 9:34 AM
> > > To: devel at edk2.groups.io; Jiang, Guomin <guomin.jiang at intel.com>; Ni,
> > > Ray <ray.ni at intel.com>; Kinney, Michael D
> > > <michael.d.kinney at intel.com>; ard.biesheuvel at linaro.org
> > > Cc: lersek at redhat.com; macarl at microsoft.com
> > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib
> > for float.
> > >
> > > The size comparation between with _fltused and without _fltused, use
> > OvmfPkgX64 as build target
> > > OvmfX64 EFI_FV_TAKEN_SIZE | Without _fltused | With _fltused
> > > DXEFV | 0x46bbe0 | 0x46bbc0
> > > FVMAIN_COMPACT | 0x12c8a0 | 0x12c7f0
> > > PEIFV | 0x4cae8 | 0x4ca68
> > > From the result, it will occupy less size rather than more size.
> > >
> > > Code Change as below and build command is ```build -p
> > > OvmfPkg/OvmfPkgX64 -b DEBUG -t VS2019 -a X64 - DTPM_ENABLE=TRUE```
> > >
> > > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > index 94fe341bec..c4136916d0 100644
> > > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > > @@ -2,7 +2,7 @@
> > > Intrinsic Memory Routines Wrapper Implementation for OpenSSL-based
> > > Cryptographic Library.
> > >
> > > -Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > **/
> > > @@ -13,16 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > typedef UINTN size_t;
> > >
> > > -#if defined(__GNUC__) || defined(__clang__)
> > > - #define GLOBAL_USED __attribute__((used)) -#else
> > > - #define GLOBAL_USED
> > > -#endif
> > > -
> > > -/* OpenSSL will use floating point support, and C compiler produces the
> > _fltused
> > > - symbol by default. Simply define this symbol here to satisfy the linker. */
> > > -int GLOBAL_USED _fltused = 1;
> > > -
> > > /* Sets buffers to a specified character */ void * memset (void
> > > *dest, int ch, size_t count) { diff --git
> > > a/MdePkg/Library/BaseLib/BaseLib.inf
> > > b/MdePkg/Library/BaseLib/BaseLib.inf
> > > index 3586beb0ab..bab31c07c7 100644
> > > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > > @@ -1,7 +1,7 @@
> > > ## @file
> > > # Base Library implementation.
> > > #
> > > -# Copyright (c) 2007 - 2019, Intel Corporation. All rights
> > > reserved.<BR>
> > > +# Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > > # Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > > reserved.<BR> # Portions copyright (c) 2011 - 2013, ARM Ltd. All
> > > rights reserved.<BR> # @@ -60,6 +60,7 @@
> > > String.c
> > > FilePaths.c
> > > BaseLibInternals.h
> > > + FltUsed.c | MSFT
> > >
> > > [Sources.Ia32]
> > > Ia32/WriteTr.nasm
> > > diff --git a/MdePkg/Library/BaseLib/FltUsed.c
> > > b/MdePkg/Library/BaseLib/FltUsed.c
> > > new file mode 100644
> > > index 0000000000..c065594266
> > > --- /dev/null
> > > +++ b/MdePkg/Library/BaseLib/FltUsed.c
> > > @@ -0,0 +1,14 @@
> > > +/** @file
> > > + Declare _fltused symbol for MSVC
> > > +
> > > + MSVC need this symbol for float, andd it here to feed MSVC. it may
> > > + remove if MSVC not need it any more.
> > > +
> > > + Copyright (c) 2020 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > > + SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > > +
> > > +//
> > > +// Just for MSVC float
> > > +//
> > > +int _fltused = 0x1;
> > >
> > > > -----Original Message-----
> > > > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > > Guomin Jiang
> > > > Sent: Tuesday, April 14, 2020 3:01 PM
> > > > To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Kinney,
> > > > Michael D <michael.d.kinney at intel.com>; ard.biesheuvel at linaro.org
> > > > Cc: lersek at redhat.com; macarl at microsoft.com
> > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > FltUsedLib for float.
> > > >
> > > > Summarize current status:
> > > >
> > > > Problem Statement:
> > > > Openssl require _fltused to be defined as a constant anywhere
> > > > floating point is used.
> > > > It may use float out of edk2 tree and need _fltused, for example,
> > > > Microsoft’s OnScreenKeyboard and UiToolKit.
> > > >
> > > > Current Proposal as below:
> > > >
> > > > Proposal 1: Add FltUsed.c into exist library
> > > > Detail: Add FltUsed.c into EntryPointLib(PEIM, DXE).
> > > > Recommend: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> > > > Approve: Laszlo Ersek lersek at redhat.com
> > > > Netual: Michael D Kinney <michael.d.kinney at intel.com>
> > > > Benefit: Doesn’t need modify every .dsc description file.
> > > > Defection: I test that it will fail because of /GL option, the error
> > > > show fatal error LNK1237: during code generation, compiler
> > > > introduced reference to symbol '_fltused' defined in module
> > > > 'UefiApplicationEntryPoint.lib(FltUsed.obj)' compiled with /GL
> > > >
> > > > Proposal 2: Define NULL library
> > > > Recommend: Michael D Kinney michael.d.kinney at intel.com
> > > > Oppose: Sean sean.brogan=microsoft.com at groups.io , Ard Biesheuvel
> > > > <ard.biesheuvel at linaro.org>
> > > > Benefit: I test it and prove that it is executable.
> > > > Defection: It is required that modify every description file.
> > > > Defection: It need be very careful that it should only apply some
> > > > module type(PEIM, DXE_DRIVER, UEFI_APPLICATION) rather than all.
> > > > Defection: Build break up.
> > > > Action Required: Need evaluate the affection on size.
> > > > Consideration: Add PCD to control the feature
> > > >
> > > > Proposal 3: Define FltUsedLib
> > > > Detail: Define FltUsedLib and add dependence
> > > > Oppose: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> > > > Benefit: Doesn’t need care that which module will use it, we will
> > > > explicitly point it in component file.
> > > > Defection: More complex, It is required that modify every
> > > > description file and modify component meanwhile.
> > > > Defection: Build breakup
> > > >
> > > > Thanks
> > > > guomin
> > > >
> > > > > -----Original Message-----
> > > > > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni,
> > > > Ray
> > > > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > > > To: devel at edk2.groups.io; Kinney, Michael D
> > > > > <michael.d.kinney at intel.com>; ard.biesheuvel at linaro.org; Kinney,
> > > > > Michael D <michael.d.kinney at intel.com>
> > > > > Cc: lersek at redhat.com; macarl at microsoft.com
> > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > FltUsedLib for float.
> > > > >
> > > > > UEFI spec allows using float operation so asking module developers
> > > > > avoid using float may not make sense. Even
> > > > > UefiCpuPkg\Library\BaseUefiCpuLib\
> > > > > provides routine to initialize float support for X86.
> > > > >
> > > > > Given ARM already uses NULL library class mechanism to supply the
> > > > > compiler stub, I prefer X86 aligns to the ARM style.
> > > > >
> > > > > The only unsure thing is the size impact when a module is not
> > > > > using float. We expect there is no size impact when a module is not
> > using float.
> > > > >
> > > > > Thanks,
> > > > > Ray
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > > > Michael
> > > > > > D Kinney
> > > > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > > > To: devel at edk2.groups.io; ard.biesheuvel at linaro.org; Kinney,
> > > > > > Michael D <michael.d.kinney at intel.com>
> > > > > > Cc: lersek at redhat.com; macarl at microsoft.com
> > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > FltUsedLib for float.
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > I think adding a dependency in the module entry point libs is
> > > > > > also good way to guarantee an intrinsic lib is available. I
> > > > > > agree that it can provide module type and arch specific
> > > > > > mappings. The NULL lib instance can do the same if the NULL
> > > > > > instance is listed in the module/arch specific library classes
> > > > > > sections. a few example section names.
> > > > > >
> > > > > > [LibraryClasses.ARM.PEIM, LibraryClasses.AARCH64.PEIM]
> > > > > >
> > > > > > [LibraryClasses.IA32.UEFI_DRIVER]
> > > > > >
> > > > > > [LibraryClasses.X64.DXE_DRIVER]
> > > > > >
> > > > > > So either way, the DSC files need to provide the library mapping.
> > > > > > The only difference between these
> > > > > > 2 approaches is that adding a dependency to the module entry
> > > > > > point libs uses a formally defined library class name for the
> > > > > > intrinsic functions vs.
> > > > > > the un-named NULL library instance. A formally defined library
> > > > > > class name is better supported for things like unit tests from
> > > > > > the UnitTestFrameworkPkg.
> > > > > >
> > > > > > The fltused issue would not go away of we removed the float
> > > > > > usage for OpenSSL. One or more other libs or the module could
> > > > > > use float/double types and this issue would pop up again. I do
> > > > > > agree it would be cleaner if we could use OpenSSL without floating
> > point.
> > > > > >
> > > > > > I think adding an intrinsic lib for IA32/X64 for VS20xx
> > > > > > generation of intrinsic functions would address fltused and
> > > > > > other common C implementation styles that generate intrinsic
> > > > > > functions (e.g. 64-bit int math on 32-bit, structure variable
> > > > > > assignments, and loops that fill a buffer with a const value) by
> > > > > > VS20xx compilers. An intrinsic lib for GCC would also help with
> > > > > > these same common C implementation styles that generate intrinsic
> > functions.
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > > Ard
> > > > > > > Biesheuvel
> > > > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > > > To: Kinney, Michael D <michael.d.kinney at intel.com>
> > > > > > > Cc: devel at edk2.groups.io; lersek at redhat.com;
> > > > > > > macarl at microsoft.com
> > > > > > > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> > > > > > > Add FltUsedLib for float.
> > > > > > >
> > > > > > > On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
> > > > > > > <michael.d.kinney at intel.com> wrote:
> > > > > > > >
> > > > > > > > ARM and AARCH64 have a compiler intrinsic lib that is
> > > > > > > linked against all modules.
> > > > > > > >
> > > > > > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > > > > > > > #
> > > > > > > > # It is not possible to prevent ARM compiler calls to
> > > > > > > generic intrinsic functions.
> > > > > > > > # This library provides the instrinsic functions
> > > > > > > generated by a given compiler.
> > > > > > > > # [LibraryClasses.ARM] and NULL mean link this
> > > > > > > library into all ARM images.
> > > > > > > > #
> > > > > > > >
> > > > > > > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrins
> > > > > > > icsLib.inf
> > > > > > > >
> > > > > > > > Can we use this same technique for IA32/X64 VS builds?
> > > > > > > >
> > > > > > >
> > > > > > > In my opinion, having these intrinsics libraries added via
> > > > > > > NULL library class resolution was a mistake to begin with.
> > > > > > >
> > > > > > > Every component that we build incorporates some kind of
> > > > > > > startup code library that defines the _ModuleEntryPoint
> > > > > > > symbol, and it would be much better to make those libraries
> > > > > > > include an IntrinsicsLibrary dependency that can be satisfied
> > > > > > > by arch specific versions that also encapsulate the toolchain
> > > > > > > dependencies (such as this _fltused symbol, or memcpy/memset
> > on ARM/GCC).
> > > > > > >
> > > > > > > On another note, it is still deeply disappointing that we need
> > > > > > > to jump through all of these hoops because the *only* single
> > > > > > > use of floating point in OpenSSL is the entropy estimate of an
> > > > > > > RNG, which is in the range of 0..1023 to begin with [IIRC).
> > > > > > > Remember that we also have a softfloat submodule for 32-bit
> > > > > > > ARM, for the same stupid reason. If we could stop pulling in
> > > > > > > that part of the code (or replace it with an
> > > > > > > UINT16 when building for the UEFI target), the whole floating
> > > > > > > point issue would mostly go away AFAICT.
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#57910): https://edk2.groups.io/g/devel/message/57910
Mute This Topic: https://groups.io/mt/72648022/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