From: "Ni, Ray" <ray.ni@intel.com>
To: "Jiang, Guomin" <guomin.jiang@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>
Cc: "lersek@redhat.com" <lersek@redhat.com>,
"macarl@microsoft.com" <macarl@microsoft.com>
Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
Date: Thu, 23 Apr 2020 03:31:38 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C5099F3@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <DM6PR11MB2955B47CA5E5A7490BB98DE09DD30@DM6PR11MB2955.namprd11.prod.outlook.com>
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.
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
next prev parent reply other threads:[~2020-04-23 3:38 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 8:52 [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float Guomin Jiang
2020-03-30 9:02 ` [edk2-devel] " Ard Biesheuvel
2020-03-30 19:04 ` Laszlo Ersek
2020-03-30 21:27 ` Matthew Carlson
2020-03-30 21:41 ` Ard Biesheuvel
2020-03-31 12:42 ` Laszlo Ersek
2020-03-31 14:36 ` Michael D Kinney
2020-03-31 22:29 ` Laszlo Ersek
2020-03-31 22:57 ` Sean
2020-03-31 23:36 ` Michael D Kinney
2020-04-01 6:42 ` Ard Biesheuvel
2020-04-01 16:38 ` Michael D Kinney
2020-04-14 5:02 ` Ni, Ray
2020-04-14 7:01 ` Guomin Jiang
2020-04-17 8:15 ` Ard Biesheuvel
2020-04-23 2:36 ` Guomin Jiang
[not found] ` <16059D94172527B2.17445@groups.io>
2020-04-23 1:33 ` Guomin Jiang
2020-04-23 3:31 ` Ni, Ray [this message]
2020-04-23 4:04 ` Guomin Jiang
2020-04-23 5:49 ` Liming Gao
2020-04-24 5:07 ` Guomin Jiang
2020-04-26 15:32 ` Liming Gao
2020-04-27 2:32 ` Ni, Ray
2020-03-31 1:40 ` Guomin Jiang
2020-03-31 7:13 ` Ard Biesheuvel
2020-03-31 12:32 ` Laszlo Ersek
2020-03-30 16:55 ` [EXTERNAL] " Bret Barkelew
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=734D49CCEBEEF84792F5B80ED585239D5C5099F3@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox