* [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 @ 2022-12-16 18:47 Tuan Phan 2022-12-17 0:06 ` Michael D Kinney 0 siblings, 1 reply; 6+ messages in thread From: Tuan Phan @ 2022-12-16 18:47 UTC (permalink / raw) Cc: devel, sunilvl, michael.d.kinney, jiewen.yao, jian.j.wang, xiaoyu1.lu, guomin.jiang, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 2022-12-16 18:47 [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 Tuan Phan @ 2022-12-17 0:06 ` Michael D Kinney 2022-12-17 1:59 ` [edk2-devel] " Pedro Falcato 2022-12-19 18:26 ` Tuan Phan 0 siblings, 2 replies; 6+ messages in thread From: Michael D Kinney @ 2022-12-17 0:06 UTC (permalink / raw) To: Tuan Phan, Kinney, Michael D Cc: devel@edk2.groups.io, sunilvl@ventanamicro.com, Yao, Jiewen, Wang, Jian J, Lu, Xiaoyu1, Jiang, Guomin, Kinney, Michael D 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 2022-12-17 0:06 ` Michael D Kinney @ 2022-12-17 1:59 ` Pedro Falcato 2022-12-17 2:15 ` Tuan Phan 2022-12-19 18:26 ` Tuan Phan 1 sibling, 1 reply; 6+ messages in thread From: Pedro Falcato @ 2022-12-17 1:59 UTC (permalink / raw) To: devel, michael.d.kinney Cc: Tuan Phan, sunilvl@ventanamicro.com, Yao, Jiewen, Wang, Jian J, Lu, Xiaoyu1, Jiang, Guomin [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 2022-12-17 1:59 ` [edk2-devel] " Pedro Falcato @ 2022-12-17 2:15 ` Tuan Phan 2022-12-21 2:56 ` Pedro Falcato 0 siblings, 1 reply; 6+ messages in thread From: Tuan Phan @ 2022-12-17 2:15 UTC (permalink / raw) To: Pedro Falcato Cc: devel, michael.d.kinney, sunilvl@ventanamicro.com, Yao, Jiewen, Wang, Jian J, Lu, Xiaoyu1, Jiang, Guomin [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 2022-12-17 2:15 ` Tuan Phan @ 2022-12-21 2:56 ` Pedro Falcato 0 siblings, 0 replies; 6+ messages in thread From: Pedro Falcato @ 2022-12-21 2:56 UTC (permalink / raw) To: Tuan Phan Cc: devel, michael.d.kinney, sunilvl@ventanamicro.com, Yao, Jiewen, Wang, Jian J, Lu, Xiaoyu1, Jiang, Guomin [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 2022-12-17 0:06 ` Michael D Kinney 2022-12-17 1:59 ` [edk2-devel] " Pedro Falcato @ 2022-12-19 18:26 ` Tuan Phan 1 sibling, 0 replies; 6+ messages in thread From: Tuan Phan @ 2022-12-19 18:26 UTC (permalink / raw) To: Kinney, Michael D Cc: devel@edk2.groups.io, sunilvl@ventanamicro.com, Yao, Jiewen, Wang, Jian J, Lu, Xiaoyu1, Jiang, Guomin [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-21 2:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-16 18:47 [PATCH v2] CryptoPkg/IntrinsicLib: RiscV: Provide implementation of memcpy and __ctzdi2 Tuan Phan 2022-12-17 0:06 ` Michael D Kinney 2022-12-17 1:59 ` [edk2-devel] " Pedro Falcato 2022-12-17 2:15 ` Tuan Phan 2022-12-21 2:56 ` Pedro Falcato 2022-12-19 18:26 ` Tuan Phan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox