I may miss some discussions.
I remember the original discussion output was to create a compiler stub for x86.
Why now we trend to move to BaseLib?
> -----Original Message-----
> From: Gao, Liming <liming....@intel.com>
> Sent: Sunday, April 26, 2020 11:33 PM
> To: Jiang, Guomin <guomin.ji...@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
> ard.biesheu...@linaro.org
> Cc: ler...@redhat.com; mac...@microsoft.com
> Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
>
> Guomin:
> The size impact is small. This solution that moves _fltused to BaseLib is
> OK to
> me.
>
> Thanks
> Liming
> > -----Original Message-----
> > From: Jiang, Guomin <guomin.ji...@intel.com>
> > Sent: Friday, April 24, 2020 1:08 PM
> > To: Gao, Liming <liming....@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray...@intel.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; ard.biesheu...@linaro.org
> > Cc: ler...@redhat.com; mac...@microsoft.com
> > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> >
> > 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....@intel.com>
> > > Sent: Thursday, April 23, 2020 1:49 PM
> > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.ji...@intel.com>; Ni,
> Ray
> > > <ray...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
> > > ard.biesheu...@linaro.org
> > > Cc: ler...@redhat.com; mac...@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...@intel.com>; devel@edk2.groups.io; Kinney, Michael
> > > > D <michael.d.kin...@intel.com>; ard.biesheu...@linaro.org
> > > > Cc: ler...@redhat.com; mac...@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...@intel.com>
> > > > > Sent: Thursday, April 23, 2020 11:32 AM
> > > > > To: Jiang, Guomin <guomin.ji...@intel.com>; devel@edk2.groups.io;
> > > > > Kinney, Michael D <michael.d.kin...@intel.com>;
> > > > > ard.biesheu...@linaro.org
> > > > > Cc: ler...@redhat.com; mac...@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.ji...@intel.com>
> > > > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > > > To: devel@edk2.groups.io; Jiang, Guomin <guomin.ji...@intel.com>;
> > > > > > Ni, Ray <ray...@intel.com>; Kinney, Michael D
> > > > > > <michael.d.kin...@intel.com>; ard.biesheu...@linaro.org
> > > > > > Cc: ler...@redhat.com; mac...@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...@intel.com>; Kinney,
> > > > > > > Michael D <michael.d.kin...@intel.com>;
> > > > > > > ard.biesheu...@linaro.org
> > > > > > > Cc: ler...@redhat.com; mac...@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.biesheu...@linaro.org>
> > > > > > > Approve: Laszlo Ersek ler...@redhat.com
> > > > > > > Netual: Michael D Kinney <michael.d.kin...@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.kin...@intel.com
> > > > > > > Oppose: Sean sean.brogan=microsoft....@groups.io , Ard
> > > > > > > Biesheuvel <ard.biesheu...@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.biesheu...@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.kin...@intel.com>; ard.biesheu...@linaro.org;
> > > > > > > > Kinney, Michael D <michael.d.kin...@intel.com>
> > > > > > > > Cc: ler...@redhat.com; mac...@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.biesheu...@linaro.org; Kinney,
> > > > > > > > > Michael D <michael.d.kin...@intel.com>
> > > > > > > > > Cc: ler...@redhat.com; mac...@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.kin...@intel.com>
> > > > > > > > > > Cc: devel@edk2.groups.io; ler...@redhat.com;
> > > > > > > > > > mac...@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.kin...@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 (#58159): https://edk2.groups.io/g/devel/message/58159
Mute This Topic: https://groups.io/mt/72648022/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
