Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

["Guomin Jiang" <guomin.jiang@intel.com>]

I have compare the Image different, the are two image size will be affect before and after move _fltused to BaseLib.

One is TcgPei, another is SecurityStubDxe, the detail as below table
                               TcgPei                    | SecurityStubDxe
Before move:       22688(decimal)   |   30624(decimal)
After move           22656(decimal)   |   30592(decimal)

The size of reducation is 32 or 0x20 bytes
Those component is located the boundary, so it will occupy more size to meet the alignment.

Another question is that some component will fail in CI result when move _fltused to BaseLib.
I am checking it.
 
> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Thursday, April 23, 2020 1:49 PM
> To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> ard.biesheuvel@linaro.org
> Cc: lersek@redhat.com; macarl@microsoft.com
> Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> 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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Guomin
> > Jiang
> > Sent: Thursday, April 23, 2020 12:04 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Kinney, Michael
> > D <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > Cc: lersek@redhat.com; macarl@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@intel.com>
> > > Sent: Thursday, April 23, 2020 11:32 AM
> > > To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io;
> > > Kinney, Michael D <michael.d.kinney@intel.com>;
> > > ard.biesheuvel@linaro.org
> > > Cc: lersek@redhat.com; macarl@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@intel.com>
> > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>;
> > > > Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org
> > > > Cc: lersek@redhat.com; macarl@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > Guomin Jiang
> > > > > Sent: Tuesday, April 14, 2020 3:01 PM
> > > > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney,
> > > > > Michael D <michael.d.kinney@intel.com>;
> > > > > ard.biesheuvel@linaro.org
> > > > > Cc: lersek@redhat.com; macarl@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@linaro.org>
> > > > > Approve: Laszlo Ersek lersek@redhat.com
> > > > > Netual: Michael D Kinney <michael.d.kinney@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@intel.com
> > > > > Oppose: Sean sean.brogan=microsoft.com@groups.io , Ard
> > > > > Biesheuvel <ard.biesheuvel@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > Ni,
> > > > > Ray
> > > > > > Sent: Tuesday, April 14, 2020 1:03 PM
> > > > > > To: devel@edk2.groups.io; Kinney, Michael D
> > > > > > <michael.d.kinney@intel.com>; ard.biesheuvel@linaro.org;
> > > > > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > Cc: lersek@redhat.com; macarl@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@edk2.groups.io <devel@edk2.groups.io> On Behalf
> > > > > > > Of
> > > > > > Michael
> > > > > > > D Kinney
> > > > > > > Sent: Thursday, April 2, 2020 12:38 AM
> > > > > > > To: devel@edk2.groups.io; ard.biesheuvel@linaro.org; Kinney,
> > > > > > > Michael D <michael.d.kinney@intel.com>
> > > > > > > Cc: lersek@redhat.com; macarl@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@edk2.groups.io <devel@edk2.groups.io> On
> > > > > > > > Behalf Of
> > > > > Ard
> > > > > > > > Biesheuvel
> > > > > > > > Sent: Tuesday, March 31, 2020 11:43 PM
> > > > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > > > Cc: devel@edk2.groups.io; lersek@redhat.com;
> > > > > > > > macarl@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@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.
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> >
> >