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

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

2020-04-26 Thread Ni, Ray

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 
> Sent: Sunday, April 26, 2020 11:33 PM
> To: Jiang, Guomin ; devel@edk2.groups.io; Ni, Ray
> ; Kinney, Michael D ;
> 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 
> > Sent: Friday, April 24, 2020 1:08 PM
> > To: Gao, Liming ; devel@edk2.groups.io; Ni, Ray
> ; Kinney, Michael D
> > ; 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 
> > > Sent: Thursday, April 23, 2020 1:49 PM
> > > To: devel@edk2.groups.io; Jiang, Guomin ; Ni,
> Ray
> > > ; Kinney, Michael D ;
> > > 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  On Behalf Of
> > > Guomin
> > > > Jiang
> > > > Sent: Thursday, April 23, 2020 12:04 PM
> > > > To: Ni, Ray ; devel@edk2.groups.io; Kinney, Michael
> > > > D ; 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 
> > > > > Sent: Thursday, April 23, 2020 11:32 AM
> > > > > To: Jiang, Guomin ; devel@edk2.groups.io;
> > > > > Kinney, Michael D ;
> > > > > 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 
> > > > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > > > To: devel@edk2.groups.io; Jiang, Guomin ;
> > > > > > Ni, Ray ; Kinney, Michael D
> > > > > > ; ard.biesheu...@linaro.org
> > > > > > Cc: ler...@redhat.com; mac...@microsoft.com
> > > > > > Subject: RE: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add
> > > > > > Flt

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

2020-04-26 Thread Liming Gao

Guomin:
  The size impact is small. This solution that moves _fltused to BaseLib is OK 
to me. 

Thanks
Liming
> -Original Message-
> From: Jiang, Guomin 
> Sent: Friday, April 24, 2020 1:08 PM
> To: Gao, Liming ; devel@edk2.groups.io; Ni, Ray 
> ; Kinney, Michael D
> ; 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 
> > Sent: Thursday, April 23, 2020 1:49 PM
> > To: devel@edk2.groups.io; Jiang, Guomin ; Ni, Ray
> > ; Kinney, Michael D ;
> > 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  On Behalf Of
> > Guomin
> > > Jiang
> > > Sent: Thursday, April 23, 2020 12:04 PM
> > > To: Ni, Ray ; devel@edk2.groups.io; Kinney, Michael
> > > D ; 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 
> > > > Sent: Thursday, April 23, 2020 11:32 AM
> > > > To: Jiang, Guomin ; devel@edk2.groups.io;
> > > > Kinney, Michael D ;
> > > > 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 
> > > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > > To: devel@edk2.groups.io; Jiang, Guomin ;
> > > > > Ni, Ray ; Kinney, Michael D
> > > > > ; 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 -

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

2020-04-23 Thread Guomin Jiang

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 
> Sent: Thursday, April 23, 2020 1:49 PM
> To: devel@edk2.groups.io; Jiang, Guomin ; Ni, Ray
> ; Kinney, Michael D ;
> 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  On Behalf Of
> Guomin
> > Jiang
> > Sent: Thursday, April 23, 2020 12:04 PM
> > To: Ni, Ray ; devel@edk2.groups.io; Kinney, Michael
> > D ; 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 
> > > Sent: Thursday, April 23, 2020 11:32 AM
> > > To: Jiang, Guomin ; devel@edk2.groups.io;
> > > Kinney, Michael D ;
> > > 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 
> > > > Sent: Thursday, April 23, 2020 9:34 AM
> > > > To: devel@edk2.groups.io; Jiang, Guomin ;
> > > > Ni, Ray ; Kinney, Michael D
> > > > ; 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.
> > > > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > > > +reserved.
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >

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

2020-04-22 Thread Liming Gao

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  On Behalf Of Guomin Jiang
> Sent: Thursday, April 23, 2020 12:04 PM
> To: Ni, Ray ; devel@edk2.groups.io; Kinney, Michael D 
> ; 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 
> > Sent: Thursday, April 23, 2020 11:32 AM
> > To: Jiang, Guomin ; devel@edk2.groups.io; Kinney,
> > Michael D ; 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 
> > > Sent: Thursday, April 23, 2020 9:34 AM
> > > To: devel@edk2.groups.io; Jiang, Guomin ; Ni,
> > > Ray ; Kinney, Michael D
> > > ; 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.
> > > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > > +reserved.
> > >  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.
> > > +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > > +reserved.
> > >  #  Portions copyright (

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

2020-04-22 Thread Guomin Jiang

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 
> Sent: Thursday, April 23, 2020 11:32 AM
> To: Jiang, Guomin ; devel@edk2.groups.io; Kinney,
> Michael D ; 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 
> > Sent: Thursday, April 23, 2020 9:34 AM
> > To: devel@edk2.groups.io; Jiang, Guomin ; Ni,
> > Ray ; Kinney, Michael D
> > ; 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.
> > +Copyright (c) 2010 - 2020, Intel Corporation. All rights
> > +reserved.
> >  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.
> > +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > +reserved.
> >  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights
> > reserved.  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All
> > rights reserved.  # @@ -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 00..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.
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > +
> > +//
> > +// Just for MSVC float
> > +//
> >

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

2020-04-22 Thread Ni, Ray

Guomin,
Can you investigate why moving _fltused from CryptoPkg/IntrinsicLib to 
MdePkg/BaseLib saves size?

Thanks,
Ray

> -Original Message-
> From: Jiang, Guomin 
> Sent: Thursday, April 23, 2020 9:34 AM
> To: devel@edk2.groups.io; Jiang, Guomin ; Ni, Ray 
> ; Kinney, Michael D
> ; 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.
> +Copyright (c) 2010 - 2020, Intel Corporation. All rights reserved.
>  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.
> +#  Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.
>  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
>  #  Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.
>  #
> @@ -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 00..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.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +//
> +// Just for MSVC float
> +//
> +int _fltused = 0x1;
> 
> > -Original Message-----
> > From: devel@edk2.groups.io  On Behalf Of Guomin
> > Jiang
> > Sent: Tuesday, April 14, 2020 3:01 PM
> > To: devel@edk2.groups.io; Ni, Ray ; Kinney, Michael D
> > ; 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 
> > Approve: Laszlo Ersek ler...@redhat.com
> > Netual: Michael D Kinney 
> > Benefit: Doesn’t need modify every .dsc description file.

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

2020-04-22 Thread Guomin Jiang

Hi Ard,

it explain the reason at 
https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk1237?view=vs-2019.
It can use ```MSFT:*_*_*_DLINK_FLAGS = /include:_fltused``` to resolve the 
error, but it is complex.

Best Regards
Guomin

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Ard
> Biesheuvel
> Sent: Friday, April 17, 2020 4:15 PM
> To: Jiang, Guomin ; Ard Biesheuvel
> 
> Cc: devel@edk2.groups.io; Ni, Ray ; Kinney, Michael D
> ; ler...@redhat.com; mac...@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> On Tue, 14 Apr 2020 at 09:01, Jiang, Guomin 
> wrote:
> >
> > 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 
> > Approve: Laszlo Ersek ler...@redhat.com
> > Netual: Michael D Kinney 
> > 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
> >
> 
> Can you elaborate on this issue? What does /GL do, and why is it throwing
> this error?
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57894): https://edk2.groups.io/g/devel/message/57894
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-04-22 Thread Guomin Jiang

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.
+Copyright (c) 2010 - 2020, Intel Corporation. All rights reserved.
 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.
+#  Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.
 #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
 #  Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.
 #
@@ -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 00..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.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+//
+// Just for MSVC float
+//
+int _fltused = 0x1;

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Guomin
> Jiang
> Sent: Tuesday, April 14, 2020 3:01 PM
> To: devel@edk2.groups.io; Ni, Ray ; Kinney, Michael D
> ; 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 
> Approve: Laszlo Ersek ler...@redhat.com
> Netual: Michael D Kinney 
> 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
> 
> 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 
> Benefit: Doesn’t need care that which module will use it, we will explicitly
> point it i

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

2020-04-17 Thread Ard Biesheuvel

On Tue, 14 Apr 2020 at 09:01, Jiang, Guomin  wrote:
>
> 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 
> Approve: Laszlo Ersek ler...@redhat.com
> Netual: Michael D Kinney 
> 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
>

Can you elaborate on this issue? What does /GL do, and why is it
throwing this error?

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57501): https://edk2.groups.io/g/devel/message/57501
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-04-14 Thread Guomin Jiang

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 
Approve: Laszlo Ersek ler...@redhat.com
Netual: Michael D Kinney 
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 

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 
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  On Behalf Of Ni, Ray
> Sent: Tuesday, April 14, 2020 1:03 PM
> To: devel@edk2.groups.io; Kinney, Michael D ;
> ard.biesheu...@linaro.org; Kinney, Michael D 
> 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  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
> > 
> > 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
> >
> > >

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

2020-04-01 Thread Michael D Kinney

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  On
> Behalf Of Ard Biesheuvel
> Sent: Tuesday, March 31, 2020 11:43 PM
> To: Kinney, Michael D 
> 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
>  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 (#56860): https://edk2.groups.io/g/devel/message/56860
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-04-01 Thread Ard Biesheuvel

On Tue, 31 Mar 2020 at 16:36, Kinney, Michael D
 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/CompilerIntrinsicsLib.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 (#56817): https://edk2.groups.io/g/devel/message/56817
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-31 Thread Michael D Kinney

Hi Sean,

This lib defines a global variable that is referenced when a compiler detects 
use of float/double types.  If the global is not referenced, then it should be 
optimized away, so the size impact should be zero.  That can be verified as 
part of the review of this feature.

Mike



From: devel@edk2.groups.io  On Behalf Of Sean via 
Groups.Io
Sent: Tuesday, March 31, 2020 3:58 PM
To: Laszlo Ersek ; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for 
float.

Does anyone know off hand if defining this and enabling floating point has any 
negative side effects if you don't need it?  Size? Optimization? Other?   That 
is my only concern for enabling in all modules which is why the initial 
proposal was for a new library.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56799): https://edk2.groups.io/g/devel/message/56799
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-31 Thread Sean via Groups.Io

Does anyone know off hand if defining this and enabling floating point has any 
negative side effects if you don't need it?  Size? Optimization? Other?   That 
is my only concern for enabling in all modules which is why the initial 
proposal was for a new library.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56797): https://edk2.groups.io/g/devel/message/56797
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-31 Thread Laszlo Ersek

On 03/31/20 16:36, Kinney, Michael D 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/CompilerIntrinsicsLib.inf
> 
> Can we use this same technique for IA32/X64

It's not a problem for in-tree platforms (I do hope the edk2 patch
series will include the patch for OvmfPkg), but it will require all
out-of-tree platforms to be updated.

> VS builds?

Yes; I think the library instance should consist of one totally empty C
file, and an |MSFT specific C file providing the external definition of
_fltused. When building for GCC, the lib instance should compile to an
empty object (archive) file.

In my opinion, of course.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56785): https://edk2.groups.io/g/devel/message/56785
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-31 Thread Michael D Kinney

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/CompilerIntrinsicsLib.inf

Can we use this same technique for IA32/X64 VS builds?

Mike


> -Original Message-
> From: devel@edk2.groups.io  On
> Behalf Of Laszlo Ersek
> Sent: Tuesday, March 31, 2020 5:42 AM
> To: Ard Biesheuvel ; edk2-
> devel-groups-io ;
> mac...@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
> Add FltUsedLib for float.
> 
> On 03/30/20 23:41, Ard Biesheuvel wrote:
> > On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via
> Groups.Io
> >  wrote:
> >>
> >> So it's not required by OpenSSL, it's required by the
> compiler whenever floating point is used, which can be in
> multiple places. For example, this is used in mu_plus
> (the Microsoft UEFI value add to EDK2) by the
> OnScreenKeyboard driver as well as the UiToolKit driver.
> 
> OK, so this is the part that was not obvious to me. This
> is basically
> code that exists on top of edk2 (consumes edk2) *AND*
> uses floating
> point *internally*.
> 
> > If you happen to use BaseCryptLib or the intrinsic in a
> driver that happens to need crypto as well, it can break
> due to multiple.
> >>
> >> I do agree, this only applies to MSVC and requiring
> every platform to add a line in their DSC would be a
> situation I would prefer to avoid if possible. Is there a
> way to specify a library dependency only if a toolchain
> is being used?
> >
> > Yes, so this either belongs in one of the
> IntrinsicsLibs we have, or
> > in the various Entrypoint libraries we have for PEIMs,
> DXEs, etc.
> >
> > However, given that we are talking about static
> libraries here, adding
> > a source file that *only* defines __fltused (and
> nothing else) to any
> > library should also work, as the resulting object file
> will only be
> > incorporated by the linker if it is needed to satisfy a
> symbol
> > dependency, and so it can never cause a conflict if it
> is the only
> > symbol in the object.
> 
> IMO: if edk2 intends to support out-of-tree modules that
> themselves
> utilize floating point, *with or without* a dependency on
> OpensslLib,
> then we should add Ard's suggestion:
> 
>  [Sources]
> +  FloatUsed.c | MSFT
> 
> to some of the "most core" edk2 library instances, i.e.
> those that *all*
> modules of the given module type inevitably depend on.
> The entry point
> libs look like a great idea to me.
> 
> That should link the _fltused external definition exactly
> once into all
> modules built with MSVC, regardless of whether each such
> module uses
> floating point or not.
> 
> Thanks
> Laszlo
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56768): https://edk2.groups.io/g/devel/message/56768
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-31 Thread Laszlo Ersek

On 03/30/20 23:41, Ard Biesheuvel wrote:
> On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via Groups.Io
>  wrote:
>>
>> So it's not required by OpenSSL, it's required by the compiler whenever 
>> floating point is used, which can be in multiple places. For example, this 
>> is used in mu_plus (the Microsoft UEFI value add to EDK2) by the 
>> OnScreenKeyboard driver as well as the UiToolKit driver.

OK, so this is the part that was not obvious to me. This is basically
code that exists on top of edk2 (consumes edk2) *AND* uses floating
point *internally*.

> If you happen to use BaseCryptLib or the intrinsic in a driver that happens 
> to need crypto as well, it can break due to multiple.
>>
>> I do agree, this only applies to MSVC and requiring every platform to add a 
>> line in their DSC would be a situation I would prefer to avoid if possible. 
>> Is there a way to specify a library dependency only if a toolchain is being 
>> used?
> 
> Yes, so this either belongs in one of the IntrinsicsLibs we have, or
> in the various Entrypoint libraries we have for PEIMs, DXEs, etc.
> 
> However, given that we are talking about static libraries here, adding
> a source file that *only* defines __fltused (and nothing else) to any
> library should also work, as the resulting object file will only be
> incorporated by the linker if it is needed to satisfy a symbol
> dependency, and so it can never cause a conflict if it is the only
> symbol in the object.

IMO: if edk2 intends to support out-of-tree modules that themselves
utilize floating point, *with or without* a dependency on OpensslLib,
then we should add Ard's suggestion:

 [Sources]
+  FloatUsed.c | MSFT

to some of the "most core" edk2 library instances, i.e. those that *all*
modules of the given module type inevitably depend on. The entry point
libs look like a great idea to me.

That should link the _fltused external definition exactly once into all
modules built with MSVC, regardless of whether each such module uses
floating point or not.

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56763): https://edk2.groups.io/g/devel/message/56763
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-31 Thread Laszlo Ersek

On 03/31/20 09:13, Ard Biesheuvel wrote:
> On Tue, 31 Mar 2020 at 03:40, Jiang, Guomin  wrote:
>>
>> Hi Laszlo,
>>
>> Thanks for you spending time review the changes.
>>
>> And I just want to present how to reproduce the build error.
>>
>> When build OvmfPkgX64, you can encounter this issue with your local change. 
>> The error as below:
>> TcgPei.lib(AutoGen.obj) : error LNK2001: unresolved external symbol _fltused
>> c:\sourcecodes\tiano\Build\OvmfX64\DEBUG_VS2019\X64\SecurityPkg\Tcg\TcgPei\TcgPei\DEBUG\TcgPei.dll
>>  : fatal error LNK1120: 1 unresolved externals
>>
>> The build command: build -p OvmfPkg\OvmfPkgX64.dsc -b DEBUG -t VS2019 -a X64 
>> -D TPM_ENABLE
>> The code changes for reproducing this symptom:
>> -  int  GLOBAL_USED _fltused = 1;
>> + //int  GLOBAL_USED _fltused = 1;
>> The machine: WIN10
>> The branch: edk2 master
>>
> 
> Doesn't the build error go away as well if you simply add FloatUsed.c
> to all BaseCryptLib flavours if Visual Studio is being used?
> 
> Something like
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> index 1bbe4f435aec..f40bf18e7f5d 100644
> --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -27,6 +27,7 @@ [Defines]
>  #
> 
>  [Sources]
> +  FloatUsed.c | MSFT
>InternalCryptLib.h
>Hash/CryptMd4.c
>Hash/CryptMd5.c
> 

This is *exactly* what I've had in mind!

(With the additional (optional) tweak that IMO "FloatUsed.c" belongs
with the openssl INF files, as those are where the floating point usage
originates from. The cryptlib instances themselves have no floating
point code, AIUI.)

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56761): https://edk2.groups.io/g/devel/message/56761
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-31 Thread Ard Biesheuvel

On Tue, 31 Mar 2020 at 03:40, Jiang, Guomin  wrote:
>
> Hi Laszlo,
>
> Thanks for you spending time review the changes.
>
> And I just want to present how to reproduce the build error.
>
> When build OvmfPkgX64, you can encounter this issue with your local change. 
> The error as below:
> TcgPei.lib(AutoGen.obj) : error LNK2001: unresolved external symbol _fltused
> c:\sourcecodes\tiano\Build\OvmfX64\DEBUG_VS2019\X64\SecurityPkg\Tcg\TcgPei\TcgPei\DEBUG\TcgPei.dll
>  : fatal error LNK1120: 1 unresolved externals
>
> The build command: build -p OvmfPkg\OvmfPkgX64.dsc -b DEBUG -t VS2019 -a X64 
> -D TPM_ENABLE
> The code changes for reproducing this symptom:
> -  int  GLOBAL_USED _fltused = 1;
> + //int  GLOBAL_USED _fltused = 1;
> The machine: WIN10
> The branch: edk2 master
>

Doesn't the build error go away as well if you simply add FloatUsed.c
to all BaseCryptLib flavours if Visual Studio is being used?

Something like

diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 1bbe4f435aec..f40bf18e7f5d 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -27,6 +27,7 @@ [Defines]
 #

 [Sources]
+  FloatUsed.c | MSFT
   InternalCryptLib.h
   Hash/CryptMd4.c
   Hash/CryptMd5.c

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56725): https://edk2.groups.io/g/devel/message/56725
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-30 Thread Guomin Jiang

Hi Laszlo,

Thanks for you spending time review the changes.

And I just want to present how to reproduce the build error.

When build OvmfPkgX64, you can encounter this issue with your local change. The 
error as below:
TcgPei.lib(AutoGen.obj) : error LNK2001: unresolved external symbol _fltused
c:\sourcecodes\tiano\Build\OvmfX64\DEBUG_VS2019\X64\SecurityPkg\Tcg\TcgPei\TcgPei\DEBUG\TcgPei.dll
 : fatal error LNK1120: 1 unresolved externals

The build command: build -p OvmfPkg\OvmfPkgX64.dsc -b DEBUG -t VS2019 -a X64 -D 
TPM_ENABLE
The code changes for reproducing this symptom:
-  int  GLOBAL_USED _fltused = 1;
+ //int  GLOBAL_USED _fltused = 1;
The machine: WIN10
The branch: edk2 master

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, March 31, 2020 3:05 AM
> To: devel@edk2.groups.io; ard.biesheu...@linaro.org; Jiang, Guomin
> 
> Cc: Wang, Jian J ; Lu, XiaoyuX
> ; Yao, Jiewen ; Sean Brogan
> ; mac...@microsoft.com
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for
> float.
> 
> On 03/30/20 11:02, Ard Biesheuvel wrote:
> > On Mon, 30 Mar 2020 at 10:52, Guomin Jiang 
> > wrote:
> >>
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2596
> >>
> >> OpenSSL requires _fltused to be defined as a constant anywhere
> >> floating point is used.
> >> This is to satisfy the linker, however, it is possible to compile a
> >> module with multiple definitions of fltused. This causes the MSVC
> >> compiler to fail the build.
> >> To solve this problem, the FltUsedLib was created that is one spot
> >> that the global static can exist.
> >>
> >> Cc: Jian J Wang 
> >> Cc: Xiaoyu Lu 
> >> Signed-off-by: Guomin Jiang 
> >
> > Doesn't this affect *every* platform? Isn't there a better way to do
> > this? E.g., using weak linkage?
> 
> We already have manually added files under
> "CryptoPkg/Library/OpensslLib":
> 
> - ossl_store.c
> - rand_pool.c
> - rand_pool_noise.c
> - rand_pool_noise_tsc.c
> 
> These files are then referenced in both OpensslLib.inf and
> OpensslLibCrypto.inf, outside of the "Autogenerated files list".
> 
> In particular "ossl_store.c" looks like a good example -- it does nothing, 
> just
> provides a needed symbol.
> 
> The comment at <https://bugzilla.tianocore.org/show_bug.cgi?id=2596#c0>
> states that _fltused is an OpenSSL requirement -- so why not move _fltused
> into the edk2 openssl library instances, and even then, only when building
> with MSVC?
> 
> BTW I don't think I understand the actual problem, from the bug report.
> Matthew wrote, "it is possible to compile a module with multiple definitions
> of fltused" -- I don't see how (and no example is provided), assuming the
> module in question already uses IntrinsicLib.
> 
> In <https://bugzilla.tianocore.org/show_bug.cgi?id=2596#c5>, Sean writes,
> "the reason we moved to a library to define this symbol was to deal with two
> libraries within the same module.  If both libs defined it then there were
> problems". -- And I don't understand why *either* of those libraries defined
> _fltused at all; I think they should have only dependend on InstrinsicLib,
> which already ensures there's exactly one external definition of _fltused.
> 
> I just applied the following patch locally:
> 
> > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > index 94fe341bec9d..6ae4c4c82ecf 100644
> > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > @@ -21,7 +21,9 @@ typedef UINTN  size_t;
> >
> >  /* OpenSSL will use floating point support, and C compiler produces the
> _fltused
> > symbol by default. Simply define this symbol here to satisfy the
> > linker. */
> > +#if 0
> >  int  GLOBAL_USED _fltused = 1;
> > +#endif
> >
> >  /* Sets buffers to a specified character */  void * memset (void
> > *dest, int ch, size_t count)
> 
> and witnessed no build failures in my environment.
> 
> This external definition of "_fltused" comes from historical commit
> 97f98500c1d4 ("Add CryptoPkg (from UDK2010.UP3)", 2010-11-01), and has
> been updated (tweaked) once since, in commit 933681b20844 ("CryptoPkg
> IntrinsicLib: Make _fltused always be used", 2019-10-24), for
> TianoCore#1603.
> 
> To me, even the initial addition (from 2010) seems incorrect.
> 
> Summary:
> 
> - I don't understand the problem. Please state it clearly, including build

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

2020-03-30 Thread Ard Biesheuvel

On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via Groups.Io
 wrote:
>
> So it's not required by OpenSSL, it's required by the compiler whenever 
> floating point is used, which can be in multiple places. For example, this is 
> used in mu_plus (the Microsoft UEFI value add to EDK2) by the 
> OnScreenKeyboard driver as well as the UiToolKit driver. If you happen to use 
> BaseCryptLib or the intrinsic in a driver that happens to need crypto as 
> well, it can break due to multiple.
>
> I do agree, this only applies to MSVC and requiring every platform to add a 
> line in their DSC would be a situation I would prefer to avoid if possible. 
> Is there a way to specify a library dependency only if a toolchain is being 
> used?

Yes, so this either belongs in one of the IntrinsicsLibs we have, or
in the various Entrypoint libraries we have for PEIMs, DXEs, etc.

However, given that we are talking about static libraries here, adding
a source file that *only* defines __fltused (and nothing else) to any
library should also work, as the resulting object file will only be
incorporated by the linker if it is needed to satisfy a symbol
dependency, and so it can never cause a conflict if it is the only
symbol in the object.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56691): https://edk2.groups.io/g/devel/message/56691
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-30 Thread Matthew Carlson via Groups.Io

So it's not required by OpenSSL, it's required by the compiler whenever 
floating point is used, which can be in multiple places. For example, this is 
used in mu_plus (the Microsoft UEFI value add to EDK2) by the OnScreenKeyboard 
driver as well as the UiToolKit driver. If you happen to use BaseCryptLib or 
the intrinsic in a driver that happens to need crypto as well, it can break due 
to multiple.

I do agree, this only applies to MSVC and requiring every platform to add a 
line in their DSC would be a situation I would prefer to avoid if possible. Is 
there a way to specify a library dependency only if a toolchain is being used?
--
- Matthew Carlson

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56689): https://edk2.groups.io/g/devel/message/56689
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-30 Thread Laszlo Ersek

On 03/30/20 11:02, Ard Biesheuvel wrote:
> On Mon, 30 Mar 2020 at 10:52, Guomin Jiang 
> wrote:
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2596
>>
>> OpenSSL requires _fltused to be defined as a constant anywhere
>> floating point is used.
>> This is to satisfy the linker, however, it is possible to compile a
>> module with multiple definitions of fltused. This causes the
>> MSVC compiler to fail the build.
>> To solve this problem, the FltUsedLib was created that is one spot
>> that the global static can exist.
>>
>> Cc: Jian J Wang 
>> Cc: Xiaoyu Lu 
>> Signed-off-by: Guomin Jiang 
>
> Doesn't this affect *every* platform? Isn't there a better way to do
> this? E.g., using weak linkage?

We already have manually added files under
"CryptoPkg/Library/OpensslLib":

- ossl_store.c
- rand_pool.c
- rand_pool_noise.c
- rand_pool_noise_tsc.c

These files are then referenced in both OpensslLib.inf and
OpensslLibCrypto.inf, outside of the "Autogenerated files list".

In particular "ossl_store.c" looks like a good example -- it does
nothing, just provides a needed symbol.

The comment at 
states that _fltused is an OpenSSL requirement -- so why not move
_fltused into the edk2 openssl library instances, and even then, only
when building with MSVC?

BTW I don't think I understand the actual problem, from the bug report.
Matthew wrote, "it is possible to compile a module with multiple
definitions of fltused" -- I don't see how (and no example is provided),
assuming the module in question already uses IntrinsicLib.

In , Sean
writes, "the reason we moved to a library to define this symbol was to
deal with two libraries within the same module.  If both libs defined it
then there were problems". -- And I don't understand why *either* of
those libraries defined _fltused at all; I think they should have only
dependend on InstrinsicLib, which already ensures there's exactly one
external definition of _fltused.

I just applied the following patch locally:

> diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c 
> b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> index 94fe341bec9d..6ae4c4c82ecf 100644
> --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> @@ -21,7 +21,9 @@ typedef UINTN  size_t;
>
>  /* OpenSSL will use floating point support, and C compiler produces the 
> _fltused
> symbol by default. Simply define this symbol here to satisfy the linker. 
> */
> +#if 0
>  int  GLOBAL_USED _fltused = 1;
> +#endif
>
>  /* Sets buffers to a specified character */
>  void * memset (void *dest, int ch, size_t count)

and witnessed no build failures in my environment.

This external definition of "_fltused" comes from historical commit
97f98500c1d4 ("Add CryptoPkg (from UDK2010.UP3)", 2010-11-01), and has
been updated (tweaked) once since, in commit 933681b20844 ("CryptoPkg
IntrinsicLib: Make _fltused always be used", 2019-10-24), for
TianoCore#1603.

To me, even the initial addition (from 2010) seems incorrect.

Summary:

- I don't understand the problem. Please state it clearly, including
build platform, target (firmware) platform, toolchain, modules,
libraries, and so on.

- Assuming the _fltused external definition is needed in fact, I think
it should be moved into a C source file referenced by the OpenSSL INF
files. This will solve problems where some module depends on the
OpensslLib class, but not the IntrinsicLib class.

- And, this reference in the OpensslLib INF files should be toolchain
specific.

Adding a new lib *class* dependency to the CryptLib instances will break
*every* platform out there, which is especially incomprehensible given
that some platforms don't need _fltused *at all*. Please let's not make
this 9+ years old hack worse.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56679): https://edk2.groups.io/g/devel/message/56679
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-03-30 Thread Ard Biesheuvel

On Mon, 30 Mar 2020 at 10:52, Guomin Jiang  wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2596
>
> OpenSSL requires _fltused to be defined as a constant anywhere floating
> point is used.
> This is to satisfy the linker, however, it is possible to compile a
> module with multiple definitions of fltused. This causes the
> MSVC compiler to fail the build.
> To solve this problem, the FltUsedLib was created that is one spot
> that the global static can exist.
>
> Cc: Jian J Wang 
> Cc: Xiaoyu Lu 
> Signed-off-by: Guomin Jiang 

Doesn't this affect *every* platform? Isn't there a better way to do
this? E.g., using weak linkage?

> ---
>  CryptoPkg/CryptoPkg.dsc   |  2 ++
>  .../Library/BaseCryptLib/BaseCryptLib.inf |  1 +
>  .../Library/BaseCryptLib/PeiCryptLib.inf  |  1 +
>  .../Library/BaseCryptLib/RuntimeCryptLib.inf  |  1 +
>  .../Library/BaseCryptLib/SmmCryptLib.inf  |  1 +
>  CryptoPkg/Library/FltUsedLib/FltUsedLib.c | 14 
>  CryptoPkg/Library/FltUsedLib/FltUsedLib.inf   | 33 +++
>  CryptoPkg/Library/TlsLib/TlsLib.inf   |  1 +
>  8 files changed, 54 insertions(+)
>  create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.c
>  create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
>
> diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
> index 4cb37b1349..e9854087b5 100644
> --- a/CryptoPkg/CryptoPkg.dsc
> +++ b/CryptoPkg/CryptoPkg.dsc
> @@ -97,6 +97,7 @@
>IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf 
>  #???
>OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
>IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +  FltUsedLib|CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
>SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>
>  [LibraryClasses.ARM]
> @@ -232,6 +233,7 @@
>CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
>CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +  CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
>CryptoPkg/Library/TlsLib/TlsLib.inf
>CryptoPkg/Library/TlsLibNull/TlsLibNull.inf
>CryptoPkg/Library/OpensslLib/OpensslLib.inf
> diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf 
> b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> index 1bbe4f435a..c82e703aa0 100644
> --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -84,6 +84,7 @@
>DebugLib
>OpensslLib
>IntrinsicLib
> +  FltUsedLib
>PrintLib
>
>  #
> diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf 
> b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> index c836c257f8..342aad008c 100644
> --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> @@ -78,6 +78,7 @@
>DebugLib
>OpensslLib
>IntrinsicLib
> +  FltUsedLib
>
>  #
>  # Remove these [BuildOptions] after this library is cleaned up
> diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf 
> b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> index bff308a4f5..dcf209d7f5 100644
> --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> @@ -89,6 +89,7 @@
>DebugLib
>OpensslLib
>IntrinsicLib
> +  FltUsedLib
>PrintLib
>
>  #
> diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf 
> b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> index cc0b65fd25..7fdaaa48a6 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> @@ -88,6 +88,7 @@
>MemoryAllocationLib
>OpensslLib
>IntrinsicLib
> +  FltUsedLib
>PrintLib
>
>  #
> diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.c 
> b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
> new file mode 100644
> index 00..8f2487ea13
> --- /dev/null
> +++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
> @@ -0,0 +1,14 @@
> +/** @file
> +  Need include this when use floats.
> +
> +  Copyright (C) Microsoft Corporation. All rights reserved.
> +  Copyright (c) 2020, Intel Corporation. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +//
> +// You need to include this to let the compiler know we are going to use
> +// floating point
> +//
> +int _fltused = 0x9875;
> diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf 
> b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
> new file mode 100644
> index 00..8d67eadfa5
> --- /dev/null
> +++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
> @@ -0,0 +1,33 @@
> +## @file
> +# It is depent on when using floats
> +#
> +# Copyright (C) Microsoft Corporation. All rights reserved.
> +# Copyright (c) 2020, Intel Corporation. All rights reserved.
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION= 0x00010005

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

24 matches

Site Navigation

Mail list logo

Footer information