Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libutils: util.h: fix the GENMASK_32(h, l) macro for been used in assembly file #6798

Merged
merged 1 commit into from May 8, 2024
Merged

libutils: util.h: fix the GENMASK_32(h, l) macro for been used in assembly file #6798

merged 1 commit into from May 8, 2024

Conversation

TonyHan11
Copy link
Contributor

The macro has a problem when it is used in an assembly file: .e.g ".word GENMASK_32(15, 8)" will be compiled to ".word 0xffffff00"

The issue is caused by the compiler shift right a 64-bit ~0 instead of a 32-bit ~0. Fix it by using shift left.

Maybe it's a compiler issue and just do a workaround here.
The compiler I tested is gcc-arm-8.2-2019.01-x86_64-arm-linux-gnueabi.

Target: arm-linux-gnueabi
Configured with: /tmp/dgboter/bbs/rhev-vm2--rhe6x86_64/buildbot/rhe6x86_64--arm-linux-gnueabi/build/src/gcc/configure --target=arm-linux-gnueabi --prefix= --with-sysroot=/arm-linux-gnueabi/libc --with-build-sysroot=/tmp/dgboter/bbs/rhev-vm2--rhe6x86_64/buildbot/rhe6x86_64--arm-linux-gnueabi/build/build-arm-linux-gnueabi/install//arm-linux-gnueabi/libc --with-bugurl=https://bugs.linaro.org/ --enable-gnu-indirect-function --enable-shared --disable-libssp --disable-libmudflap --enable-checking=release --enable-languages=c,c++,fortran --with-gmp=/tmp/dgboter/bbs/rhev-vm2--rhe6x86_64/buildbot/rhe6x86_64--arm-linux-gnueabi/build/build-arm-linux-gnueabi/host-tools --with-mpfr=/tmp/dgboter/bbs/rhev-vm2--rhe6x86_64/buildbot/rhe6x86_64--arm-linux-gnueabi/build/build-arm-linux-gnueabi/host-tools --with-mpc=/tmp/dgboter/bbs/rhev-vm2--rhe6x86_64/buildbot/rhe6x86_64--arm-linux-gnueabi/build/build-arm-linux-gnueabi/host-tools --with-isl=/tmp/dgboter/bbs/rhev-vm2--rhe6x86_64/buildbot/rhe6x86_64--arm-linux-gnueabi/build/build-arm-linux-gnueabi/host-tools --with-arch=armv7-a --with-pkgversion='GNU Toolchain for the A-profile Architecture 8.2-2019.01 (arm-rel-8.28)'
Thread model: posix
gcc version 8.2.1 20180802 (GNU Toolchain for the A-profile Architecture 8.2-2019.01 (arm-rel-8.28))

etienne-lms
etienne-lms reviewed Apr 23, 2024
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why the ASM implementation fails.
That said, this change looks ok to me. If it helps:
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>.

TonyHan11 added a commit to TonyHan11/optee_os that referenced this pull request May 8, 2024
@TonyHan11
core: arm: test if 'GENMASK_32' could be used in assembly properly
d5f18c7 
workaround for the issue: OP-TEE#6798

Signed-off-by: Tony Han <tony.han@microchip.com>
@TonyHan11
Copy link
Contributor Author

Here is a test on branch test_genmask I did with adding GENMASK_32 to asm file.

  • commit d5f18c7: add orr r0, r0, #GENMASK_32(20, 16) to entry_a32.S (line 220)
    From the CI log we can see make (multi-platform) fails due to the macro be treated as 0xffffffffffff0000 instead of 0x01ff0000: core/arch/arm/kernel/entry_a32.S:220: Error: invalid constant (ffffffffffff0000) after fixup
    genmask_fail_in_asm
  • commit 1d9687f: cherry-picked the patch. According to CI log make (multi-platform) is OK.

jenswi-linaro
jenswi-linaro reviewed May 8, 2024
View reviewed changes
lib/libutils/ext/include/util.h Outdated
@@ -135,7 +135,7 @@
* GENMASK_64(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
#define GENMASK_32(h, l) \
(((~UINT32_C(0)) << (l)) & (~UINT32_C(0) >> (32 - 1 - (h))))
(((~UINT32_C(0)) << (l)) & ~((~UINT32_C(0) << 1) << (h)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you shifting in two steps? Why not:

(((~UINT32_C(0)) << (l)) & ~(~UINT32_C(0) << (1 + (h))))

However, the root cause is that ~UINT32_C(0) doesn't give 0xffffffff as expected.
Wouldn't it make more sense to write ~UINT32_C(0) as UINT32_C(0xffffffff)?
It should give the expected result for both C code and assembly code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason of shifting in two steps is to avoid error: left shift count >= width of type [-Werror=shift-count-overflow].

Good suggestion! I'll update the macro to ((UINT32_C(0xffffffff) << (l)) & (UINT32_C(0xffffffff) >> (32 - 1 - (h)))). Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and rebased on the latest master.

@TonyHan11 TonyHan11 force-pushed the fix_genmask branch 2 times, most recently from 258e223 to e03abec Compare May 8, 2024 07:04
@jenswi-linaro
Copy link
Contributor

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

@TonyHan11
libutils: util.h: fix the GENMASK_32(h, l) macro
6c20491 
The macro has a problem when it is used in an assembly file:
.e.g ".word GENMASK_32(15, 8)" will be compiled to ".word 0xffffff00"

The issue is caused by the compiler always treating ~0 as a 64-bit
value. Fix it by replacing '~UINT32_C(0)' with 'UINT32_C(0xffffffff)'.

Signed-off-by: Tony Han <tony.han@microchip.com>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@TonyHan11
Copy link
Contributor Author

Added the review tag.

@jforissier jforissier merged commit e716d49 into OP-TEE:master May 8, 2024
7 checks passed
@TonyHan11 TonyHan11 deleted the fix_genmask branch May 9, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenswi-linaro jenswi-linaro jenswi-linaro left review comments

@etienne-lms etienne-lms etienne-lms left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@TonyHan11 @jenswi-linaro @etienne-lms @jforissier