[PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2

[PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2

[Tuan Phan]

The RiscV toolchain doesn't provide __ctzdi2 implementation when
compiled with -nostdlib that needed by openssl library.
So adding the implementation of __ctzdi2.

Forcing to use CopyMem of EDK2 as memcpy buildin disabled for RiscV
with -fno-builtin-memcpy flag.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4103
Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
Acked-by: Sunil V L <sunilvl@ventanamicro.com>
---
V2:
- Add license header.
- Add REF to the bugzilla.

 .../Library/IntrinsicLib/CompilerHelper.c     | 42 +++++++++++++++++++
 .../Library/IntrinsicLib/IntrinsicLib.inf     |  6 ++-
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 CryptoPkg/Library/IntrinsicLib/CompilerHelper.c

diff --git a/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c b/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
new file mode 100644
index 000000000000..3844fd14ae66
--- /dev/null
+++ b/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
@@ -0,0 +1,42 @@
+/** @file
+  Implement functions that not available when compiled with -nostdlib.
+
+  Copyright (c) 2022, Ventana Micro Systems Inc. All Rights Reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+unsigned int
+__ctzdi2 (unsigned long long x)
+{
+  unsigned int ret = 0;
+
+  if (!x) {
+    return 64;
+  }
+  if (!(x & 0xffffffff)) {
+    x >>= 32;
+    ret |= 32;
+  }
+  if (!(x & 0xffff)) {
+    x >>= 16;
+    ret |= 16;
+  }
+  if (!(x & 0xff)) {
+    x >>= 8;
+    ret |= 8;
+  }
+  if (!(x & 0xf)) {
+    x >>= 4;
+    ret |= 4;
+  }
+  if (!(x & 0x3)) {
+    x >>= 2;
+    ret |= 2;
+  }
+  if (!(x & 0x1)) {
+    x >>= 1;
+    ret |= 1;
+  }
+  return ret;
+}
diff --git a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
index 86e74b57b109..6796b39b07cf 100644
--- a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
+++ b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
@@ -18,7 +18,7 @@
 #
 # The following information is for reference only and not required by the build tools.
 #
-#  VALID_ARCHITECTURES           = IA32 X64
+#  VALID_ARCHITECTURES           = IA32 X64 RISCV64
 #
 
 [Sources]
@@ -43,6 +43,10 @@
 [Sources.X64]
   CopyMem.c
 
+[Sources.RISCV64]
+  CopyMem.c
+  CompilerHelper.c
+
 [Packages]
   MdePkg/MdePkg.dec
 
-- 
2.25.1

Re: [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2

[Michael D Kinney]

If that intrinsic is specific to RISCV, then should CompilerHelper.c go into a RiscV64 subdir?

Mike

> -----Original Message-----
> From: Tuan Phan <tphan@ventanamicro.com>
> Sent: Friday, December 16, 2022 10:48 AM
> Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Jiang, Guomin
> <guomin.jiang@intel.com>; Tuan Phan <tphan@ventanamicro.com>
> Subject: [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2
> 
> The RiscV toolchain doesn't provide __ctzdi2 implementation when
> compiled with -nostdlib that needed by openssl library.
> So adding the implementation of __ctzdi2.
> 
> Forcing to use CopyMem of EDK2 as memcpy buildin disabled for RiscV
> with -fno-builtin-memcpy flag.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4103
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> Acked-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
> V2:
> - Add license header.
> - Add REF to the bugzilla.
> 
>  .../Library/IntrinsicLib/CompilerHelper.c     | 42 +++++++++++++++++++
>  .../Library/IntrinsicLib/IntrinsicLib.inf     |  6 ++-
>  2 files changed, 47 insertions(+), 1 deletion(-)
>  create mode 100644 CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
> 
> diff --git a/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c b/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
> new file mode 100644
> index 000000000000..3844fd14ae66
> --- /dev/null
> +++ b/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
> @@ -0,0 +1,42 @@
> +/** @file
> +  Implement functions that not available when compiled with -nostdlib.
> +
> +  Copyright (c) 2022, Ventana Micro Systems Inc. All Rights Reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +unsigned int
> +__ctzdi2 (unsigned long long x)
> +{
> +  unsigned int ret = 0;
> +
> +  if (!x) {
> +    return 64;
> +  }
> +  if (!(x & 0xffffffff)) {
> +    x >>= 32;
> +    ret |= 32;
> +  }
> +  if (!(x & 0xffff)) {
> +    x >>= 16;
> +    ret |= 16;
> +  }
> +  if (!(x & 0xff)) {
> +    x >>= 8;
> +    ret |= 8;
> +  }
> +  if (!(x & 0xf)) {
> +    x >>= 4;
> +    ret |= 4;
> +  }
> +  if (!(x & 0x3)) {
> +    x >>= 2;
> +    ret |= 2;
> +  }
> +  if (!(x & 0x1)) {
> +    x >>= 1;
> +    ret |= 1;
> +  }
> +  return ret;
> +}
> diff --git a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> index 86e74b57b109..6796b39b07cf 100644
> --- a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> +++ b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -18,7 +18,7 @@
>  #
> 
>  # The following information is for reference only and not required by the build tools.
> 
>  #
> 
> -#  VALID_ARCHITECTURES           = IA32 X64
> 
> +#  VALID_ARCHITECTURES           = IA32 X64 RISCV64
> 
>  #
> 
> 
> 
>  [Sources]
> 
> @@ -43,6 +43,10 @@
>  [Sources.X64]
> 
>    CopyMem.c
> 
> 
> 
> +[Sources.RISCV64]
> 
> +  CopyMem.c
> 
> +  CompilerHelper.c
> 
> +
> 
>  [Packages]
> 
>    MdePkg/MdePkg.dec
> 
> 
> 
> --
> 2.25.1

Re: [edk2-devel] [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

On Sat, Dec 17, 2022 at 12:06 AM Michael D Kinney <
michael.d.kinney@intel.com> wrote:

> If that intrinsic is specific to RISCV, then should CompilerHelper.c go
> into a RiscV64 subdir?


Mike and Tuan,

Two comments:
1) __ctzdi2 is not riscv specific and is a part of the standard functions
provided by libgcc/compiler-rt (read: the compiler can call it as it
pleases)

> Forcing to use CopyMem of EDK2 as memcpy buildin disabled for RiscV
> > with -fno-builtin-memcpy flag.
>

2) Using -fno-builtin-memcpy doesn't stop GCC/clang from trying to call
memcpy (see https://godbolt.org/z/xYsYEq6En for an example). In fact,
fno-builtin-memcpy will call memcpy more,
as GCC stops assuming normal behavior from memcpy and generates calls
instead of e.g inlining memcpy code.
GCC and clang require memcpy, memmove, memset and memcmp (with standard C
library behavior) to compile code. AFAIK no option can change this.
GCC and clang also require libgcc/compiler-rt (although getting around this
is saner and way more reliable, done in e.g operating system kernels such
as linux itself).

The only way to reliably generate code with GCC/clang is to have the
CompilerIntrinsicsLib as a dependency for every platform compiled with GCC
(or have BaseTools inject that).

I actually ran into this issue a few weeks back when trying to add security
features (https://github.com/heatd/edk2/commits/toolchain-fixes). Can we
properly fix this?

Pedro

[-- Attachment #2: Type: text/html, Size: 2227 bytes --]

Re: [edk2-devel] [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2

[Tuan Phan]

[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

On Fri, Dec 16, 2022 at 5:59 PM Pedro Falcato <pedro.falcato@gmail.com>
wrote:

> On Sat, Dec 17, 2022 at 12:06 AM Michael D Kinney <
> michael.d.kinney@intel.com> wrote:
>
>> If that intrinsic is specific to RISCV, then should CompilerHelper.c go
>> into a RiscV64 subdir?
>
>
> Mike and Tuan,
>
> Two comments:
> 1) __ctzdi2 is not riscv specific and is a part of the standard functions
> provided by libgcc/compiler-rt (read: the compiler can call it as it
> pleases)
>
> > Forcing to use CopyMem of EDK2 as memcpy buildin disabled for RiscV
>> > with -fno-builtin-memcpy flag.
>>
>
> 2) Using -fno-builtin-memcpy doesn't stop GCC/clang from trying to call
> memcpy (see https://godbolt.org/z/xYsYEq6En for an example). In fact,
> fno-builtin-memcpy will call memcpy more,
> as GCC stops assuming normal behavior from memcpy and generates calls
> instead of e.g inlining memcpy code.
> GCC and clang require memcpy, memmove, memset and memcmp (with standard C
> library behavior) to compile code. AFAIK no option can change this.
> GCC and clang also require libgcc/compiler-rt (although getting around
> this is saner and way more reliable, done in e.g operating system kernels
> such as linux itself).
>
> The only way to reliably generate code with GCC/clang is to have the
> CompilerIntrinsicsLib as a dependency for every platform compiled with GCC
> (or have BaseTools inject that).
>
> I actually ran into this issue a few weeks back when trying to add
> security features (https://github.com/heatd/edk2/commits/toolchain-fixes).
> Can we properly fix this?
>
> It would not be a problem if libgcc/compiler-rt can be linked during the
build process. With the -nostdlib flag enforced for most targets, each
module needs to explicitly link libgcc in INF if want to use the
compiler-rt functions. I am not sure what is the best way to do it unless
we remove -nostdlib or enforce linking libgcc with BaseTools.

Tuan

> Pedro
>

[-- Attachment #2: Type: text/html, Size: 3192 bytes --]

Re: [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2

[Tuan Phan]

[-- Attachment #1: Type: text/plain, Size: 3856 bytes --]

On Fri, Dec 16, 2022 at 4:06 PM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> If that intrinsic is specific to RISCV, then should CompilerHelper.c go
> into a RiscV64 subdir?
>
> Mike
>
>
Hi Mike,
While this intrinsic is not specific to RISCV,  it is needed due to the GCC
for RISCV64 does not generate internal implementation, requiring external
link with libgcc/compile-rt. It can happen to other platforms or future GCC
toolchain if it decides to move __ctzdi2 to libgcc.
If you think it is better to put it in the RiscV64 subdir for now, I am
good with it.

Tuan,

> -----Original Message-----
> > From: Tuan Phan <tphan@ventanamicro.com>
> > Sent: Friday, December 16, 2022 10:48 AM
> > Cc: devel@edk2.groups.io; sunilvl@ventanamicro.com; Kinney, Michael D <
> michael.d.kinney@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Lu,
> Xiaoyu1 <xiaoyu1.lu@intel.com>; Jiang, Guomin
> > <guomin.jiang@intel.com>; Tuan Phan <tphan@ventanamicro.com>
> > Subject: [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide
> implementation of memcpy and __ctzdi2
> >
> > The RiscV toolchain doesn't provide __ctzdi2 implementation when
> > compiled with -nostdlib that needed by openssl library.
> > So adding the implementation of __ctzdi2.
> >
> > Forcing to use CopyMem of EDK2 as memcpy buildin disabled for RiscV
> > with -fno-builtin-memcpy flag.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4103
> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> > Acked-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> > V2:
> > - Add license header.
> > - Add REF to the bugzilla.
> >
> >  .../Library/IntrinsicLib/CompilerHelper.c     | 42 +++++++++++++++++++
> >  .../Library/IntrinsicLib/IntrinsicLib.inf     |  6 ++-
> >  2 files changed, 47 insertions(+), 1 deletion(-)
> >  create mode 100644 CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
> >
> > diff --git a/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
> b/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
> > new file mode 100644
> > index 000000000000..3844fd14ae66
> > --- /dev/null
> > +++ b/CryptoPkg/Library/IntrinsicLib/CompilerHelper.c
> > @@ -0,0 +1,42 @@
> > +/** @file
> > +  Implement functions that not available when compiled with -nostdlib.
> > +
> > +  Copyright (c) 2022, Ventana Micro Systems Inc. All Rights
> Reserved.<BR>
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +unsigned int
> > +__ctzdi2 (unsigned long long x)
> > +{
> > +  unsigned int ret = 0;
> > +
> > +  if (!x) {
> > +    return 64;
> > +  }
> > +  if (!(x & 0xffffffff)) {
> > +    x >>= 32;
> > +    ret |= 32;
> > +  }
> > +  if (!(x & 0xffff)) {
> > +    x >>= 16;
> > +    ret |= 16;
> > +  }
> > +  if (!(x & 0xff)) {
> > +    x >>= 8;
> > +    ret |= 8;
> > +  }
> > +  if (!(x & 0xf)) {
> > +    x >>= 4;
> > +    ret |= 4;
> > +  }
> > +  if (!(x & 0x3)) {
> > +    x >>= 2;
> > +    ret |= 2;
> > +  }
> > +  if (!(x & 0x1)) {
> > +    x >>= 1;
> > +    ret |= 1;
> > +  }
> > +  return ret;
> > +}
> > diff --git a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> > index 86e74b57b109..6796b39b07cf 100644
> > --- a/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> > +++ b/CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> > @@ -18,7 +18,7 @@
> >  #
> >
> >  # The following information is for reference only and not required by
> the build tools.
> >
> >  #
> >
> > -#  VALID_ARCHITECTURES           = IA32 X64
> >
> > +#  VALID_ARCHITECTURES           = IA32 X64 RISCV64
> >
> >  #
> >
> >
> >
> >  [Sources]
> >
> > @@ -43,6 +43,10 @@
> >  [Sources.X64]
> >
> >    CopyMem.c
> >
> >
> >
> > +[Sources.RISCV64]
> >
> > +  CopyMem.c
> >
> > +  CompilerHelper.c
> >
> > +
> >
> >  [Packages]
> >
> >    MdePkg/MdePkg.dec
> >
> >
> >
> > --
> > 2.25.1
>
>

[-- Attachment #2: Type: text/html, Size: 6103 bytes --]

Re: [edk2-devel] [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]

On Sat, Dec 17, 2022 at 2:16 AM Tuan Phan <tphan@ventanamicro.com> wrote:

>
>
> On Fri, Dec 16, 2022 at 5:59 PM Pedro Falcato <pedro.falcato@gmail.com>
> wrote:
>
>> On Sat, Dec 17, 2022 at 12:06 AM Michael D Kinney <
>> michael.d.kinney@intel.com> wrote:
>>
>>> If that intrinsic is specific to RISCV, then should CompilerHelper.c go
>>> into a RiscV64 subdir?
>>
>>
>> Mike and Tuan,
>>
>> Two comments:
>> 1) __ctzdi2 is not riscv specific and is a part of the standard functions
>> provided by libgcc/compiler-rt (read: the compiler can call it as it
>> pleases)
>>
>> > Forcing to use CopyMem of EDK2 as memcpy buildin disabled for RiscV
>>> > with -fno-builtin-memcpy flag.
>>>
>>
>> 2) Using -fno-builtin-memcpy doesn't stop GCC/clang from trying to call
>> memcpy (see https://godbolt.org/z/xYsYEq6En for an example). In fact,
>> fno-builtin-memcpy will call memcpy more,
>> as GCC stops assuming normal behavior from memcpy and generates calls
>> instead of e.g inlining memcpy code.
>> GCC and clang require memcpy, memmove, memset and memcmp (with standard C
>> library behavior) to compile code. AFAIK no option can change this.
>> GCC and clang also require libgcc/compiler-rt (although getting around
>> this is saner and way more reliable, done in e.g operating system kernels
>> such as linux itself).
>>
>> The only way to reliably generate code with GCC/clang is to have the
>> CompilerIntrinsicsLib as a dependency for every platform compiled with GCC
>> (or have BaseTools inject that).
>>
>> I actually ran into this issue a few weeks back when trying to add
>> security features (https://github.com/heatd/edk2/commits/toolchain-fixes).
>> Can we properly fix this?
>>
>> It would not be a problem if libgcc/compiler-rt can be linked during the
> build process. With the -nostdlib flag enforced for most targets, each
> module needs to explicitly link libgcc in INF if want to use the
> compiler-rt functions. I am not sure what is the best way to do it unless
> we remove -nostdlib or enforce linking libgcc with BaseTools.
>
We can't link with the standard libgcc/compiler-rt that comes with the
toolchain due to various reasons:
1) Your toolchain may not even have one (most clang toolchains out there
support cross-compilation to RISCV but don't come with pre-compiled
runtimes due to size constraints)
2) Your architecture may need to pass special compile options for safe
usage of runtime libraries (like x86_64's -mno-red-zone on SysV ABI)
3) Your architecture may straight up refuse to link with object files
compiled in other modes - this is the case in RISCV, as e.g you can't link
softfloat with hardfloat object files

So we need to carry a libgcc in EDK2. I know there have been efforts in the
past to consolidate all these little compiler intrinsic implementations
into a specific spot, I don't know what came of that.
But we definitely should make BaseTools enforce compiler runtime libs when
building - it's the only safe and backwards compatible choice, and if we
add more runtime libraries (think UBSAN or ASAN) those can be silently
added to BaseTools as well.


Pedro

[-- Attachment #2: Type: text/html, Size: 4490 bytes --]