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

[arichardson]

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.

[cem]

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.

[arichardson]

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.

[cem]

In D28395#635155, @arichardson wrote:

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

58

The extra blank line isn't needed.

69–74

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

131

We don't need a second blank line here.

[arichardson]

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

[cem]

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.

[arichardson]

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.

[arichardson]

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()?

[cem]

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]

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

[cem]

tests/sys/kern/Makefile

60

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

53

size_t is canonical here, not a big deal.

63–70

sizeof(buffer) -> length for all of these

148–150

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

[arichardson]

tests/sys/kern/Makefile

60

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

63–70

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

[cem]

tests/sys/kern/Makefile

60

Ok, seems reasonable to me then.

tests/sys/kern/libkern_crc32.c

63–70

No worries

[arichardson]

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

[arichardson]

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

[cem]

In D28395#636443, @arichardson wrote:

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

148–150

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

[cem]

Looks great to me!

tests/sys/kern/Makefile

60

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

tests/sys/kern/libkern_crc32.c

55–56

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