Page MenuHomeFreeBSD

tests/sys/kern/crc32: Check for SSE4.2 before using it
ClosedPublic

Authored by arichardson on Jan 28 2021, 10:05 AM.

Details

Summary

This avoids a SIGILL when running these tests on QEMU (which
defaults to a basic amd64 CPU without SSE4.2).

This commit also tests the table-based implementations in addition to
testing the hw-accelerated crc32 versions.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

arichardson created this revision.
cem requested changes to this revision.Jan 28 2021, 1:51 PM

I think the idea here is that CRC32 is part of SSE4.2, which is part of the base feature set of amd64. If you're hitting SIGILL in QEMU in amd64 mode, that suggests QEMU's amd64 emulation is somewhat invalid. However, it's certainly optional on at least some early models of i386.

I would suggest:

  1. Move the CPU feature test to ATF_TP_ADD_TCS instead of repeating it in each test case.
  2. Figure out how to not run these tests on (unaccelerated?) QEMU.
This revision now requires changes to proceed.Jan 28 2021, 1:51 PM
In D28395#635150, @cem wrote:

I think the idea here is that CRC32 is part of SSE4.2, which is part of the base feature set of amd64. If you're hitting SIGILL in QEMU in amd64 mode, that suggests QEMU's amd64 emulation is somewhat invalid. However, it's certainly optional on at least some early models of i386.

I would suggest:

  1. Move the CPU feature test to ATF_TP_ADD_TCS instead of repeating it in each test case.
  2. Figure out how to not run these tests on (unaccelerated?) QEMU.

I think QEMU provides a basic amd64 CPU by default which does not include any newer features (wikipedia suggests SSE4.2 was introduced ~2008). There are probably some flags to enable newer features.

I'll try rewriting the test to check the non-accelerated functions and the accelerated ones only if available.

I think QEMU provides a basic amd64 CPU by default which does not include any newer features (wikipedia suggests SSE4.2 was introduced ~2008).

Oh, right, amd64 dates to ~2000. I guess only SSE and SSE2 are core amd64 features. Mea culpa.

I'll try rewriting the test to check the non-accelerated functions and the accelerated ones only if available.

These tests are intended for the accelerated ones. If you want to add non-accelerated tests, cool, but I think they should be distinct and unconditional.

Your existing change mostly LGTM, though please fix the style nits before committing.

tests/sys/kern/libkern_crc32.c
57

The extra blank line isn't needed.

68–73

Could just move the ifdef inside the function body and drop the second definition.

131

We don't need a second blank line here.

This revision is now accepted and ready to land.Jan 28 2021, 2:31 PM
arichardson marked 3 inline comments as done.

Test the table-based crc32 in addition to the hw-accelerated ones.

This revision now requires review to proceed.Jan 28 2021, 4:09 PM
cem requested changes to this revision.Jan 28 2021, 4:29 PM
cem added inline comments.
tests/sys/kern/libkern_crc32.c
36–49

This construction is unusual. Why do we need to declare this function? And why include the other CU instead of linking it?

119–121

The loop body is probably worth extracting into a verify_one_crc32() or something at this point, right? It's repeated verbatim in all 3 loops.

This revision now requires changes to proceed.Jan 28 2021, 4:29 PM
arichardson added inline comments.
tests/sys/kern/libkern_crc32.c
36–49

The .c file fails -Werror builds due to a missing declaration of calculate_crc32c() if _KERNEL is not defined. I'll try fixing that file instead of including it.

tests/sys/kern/libkern_crc32.c
36–49

Ah I remember the real reason for this now: the two crc32 implementations are static functions and the only exposed function is one that dispatches to the different implementations based on size/cpu features.

I guess I could drop the singletable+multitable checks and just use calculate_crc32c()?

tests/sys/kern/libkern_crc32.c
36–49

I'd prefer making them conditionally non-static #ifdef TESTING (or something like that) with a similar conditional declaration in the header, and testing them individually as you've done here. This is similar to a pattern we've used before (conditional definitions #if _KERNEL || TESTING) for testing kernel mode functions in userspace tests.

arichardson marked 2 inline comments as done.

Apply review suggestions, noticed that SSE4.2 appears broken for the trailing bytes case.

tests/sys/kern/Makefile
59

Isn't this one already included in SRCS via ATF_TESTS_C? I think you only need the other additions.

tests/sys/kern/libkern_crc32.c
52

size_t is canonical here, not a big deal.

62–69

sizeof(buffer) -> length for all of these

147–149

I'm seeing 0x139C729D from other implementations, so I'm not sure what is correct here.

tests/sys/kern/Makefile
59

I don't quite understand why it's needed, but if I don't add it explicitly I get the following:

/usr/lib/llvm-10/bin/clang -fuse-ld=lld -Qunused-arguments -target x86_64-unknown-freebsd14.0 --sysroot=/local/scratch/alr48/cheri/build/freebsd-amd64-build/local/scratch/alr48/cheri/freebsd/amd64.amd64/tmp -B/local/scratch/alr48/cheri/build/freebsd-amd64-build/local/scratch/alr48/cheri/freebsd/amd64.amd64/tmp/usr/bin -O2 -pipe -fno-common -DTESTING -g -std=gnu99 -Wno-format-zero-length -nobuiltininc -idirafter /usr/lib/llvm-10/lib/clang/10.0.0/include -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Qunused-arguments   -fuse-ld=/usr/lib/llvm-10/bin/ld.lld -o libkern_crc32 gsb_crc32.o /local/scratch/alr48/cheri/freebsd/sys/libkern/x86/crc32_sse42.o  -lprivateatf-c
ld.lld: error: undefined symbol: main
tests/sys/kern/libkern_crc32.c
62–69

Sorry, uploaded the wrong version without my latest fixup commit.

tests/sys/kern/Makefile
59

Ok, seems reasonable to me then.

tests/sys/kern/libkern_crc32.c
62–69

No worries

Turns out the bug was that I messed up the size being passed to the SSE4.2 version. It does work correctly.

arichardson marked 5 inline comments as done.

Make sure SSE4.2 and non-SSE4.2 lengths match...

Turns out the bug was that I messed up the size being passed to the SSE4.2 version. It does work correctly.

Oh, ok! That's a pleasant outcome.

tests/sys/kern/libkern_crc32.c
147–149

(To explain 0x139C729D: it is just ~0xec638d62.)

Looks great to me!

tests/sys/kern/Makefile
59

(That might be why we were abusing LDADD earlier, I guess.)

tests/sys/kern/libkern_crc32.c
54–55

One last style(9) nit: I'd suggest adding a blank line after the variable declaration.

This revision is now accepted and ready to land.Mon, Feb 1, 3:55 PM
This revision was automatically updated to reflect the committed changes.
arichardson marked 5 inline comments as done.