Details

Reviewers

cem
kib
markj

Commits

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

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

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36578
Build 33467: arc lint + arc unit

Event Timeline

arichardson requested review of this revision.Jan 28 2021, 10:05 AM

arichardson created this revision.

Harbormaster completed remote builds in B36568: Diff 83033.Jan 28 2021, 10:05 AM

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:

Move the CPU feature test to ATF_TP_ADD_TCS instead of repeating it in each test case.
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:

Move the CPU feature test to ATF_TP_ADD_TCS instead of repeating it in each test case.

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.

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
62	The extra blank line isn't needed.
73–78	Could just move the ifdef inside the function body and drop the second definition.
136–137	We don't need a second blank line here.

This revision is now accepted and ready to land.Jan 28 2021, 2:31 PM

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

Herald added a subscriber: imp. · View Herald TranscriptJan 28 2021, 4:09 PM

Harbormaster completed remote builds in B36578: Diff 83051.Jan 28 2021, 4:09 PM

arichardson edited the summary of this revision. (Show Details)Jan 28 2021, 4:10 PM

cem requested changes to this revision.Jan 28 2021, 4:29 PM

cem added inline comments.

tests/sys/kern/libkern_crc32.c
36–38	This construction is unusual. Why do we need to declare this function? And why include the other CU instead of linking it?
100–125	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 planned changes to this revision.Jan 28 2021, 4:31 PM

arichardson added inline comments.

tests/sys/kern/libkern_crc32.c
36–38	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 added inline comments.Jan 28 2021, 5:12 PM

tests/sys/kern/libkern_crc32.c
36–38	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 added inline comments.Jan 29 2021, 4:41 PM

tests/sys/kern/libkern_crc32.c
36–38	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.

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

Harbormaster completed remote builds in B36646: Diff 83209.Feb 1 2021, 11:53 AM

arichardson edited the summary of this revision. (Show Details)Feb 1 2021, 11:54 AM

arichardson added reviewers: kib, markj.

cem added inline comments.Feb 1 2021, 3:14 PM

tests/sys/kern/Makefile
60 ↗	(On Diff #83209)	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
189–191	I'm seeing 0x139C729D from other implementations, so I'm not sure what is correct here.

arichardson added inline comments.Feb 1 2021, 3:18 PM

tests/sys/kern/Makefile

60 ↗

(On Diff #83209)

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.

arichardson mentioned this in D9342: calculate_crc32c: Add SSE4.2 implementation on x86.Feb 1 2021, 3:21 PM

cem added inline comments.Feb 1 2021, 3:22 PM

tests/sys/kern/Makefile
60 ↗	(On Diff #83209)	Ok, seems reasonable to me then.
tests/sys/kern/libkern_crc32.c
63–70	No worries

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

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

Harbormaster completed remote builds in B36653: Diff 83230.Feb 1 2021, 3:27 PM

arichardson edited the summary of this revision. (Show Details)Feb 1 2021, 3:27 PM

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
189–191	(To explain 0x139C729D: it is just ~0xec638d62.)

Looks great to me!

tests/sys/kern/Makefile
60 ↗	(On Diff #83209)	(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.