cpuset: Byte swap cpuset for compat32 on big endian architectures

[jhibbits]

Summary

BITSET uses long as its basic underlying type, which is dependent on the
compile type, meaning on 32-bit builds the basic type is 32 bits, but on
64-bit builds it's 64 bits. On little endian architectures this doesn't
matter, because the LSB is always at the low bit, so the words get
effectively concatenated moving between 32-bit and 64-bit, but on
big-endian architectures it throws a wrench in, as setting bit 0 in
32-bit mode is equivalent to setting bit 32 in 64-bit mode. To
demonstrate:

32-bit mode:

BIT_SET(foo, 0): 0x00000001

64-bit sees: 0x0000000100000000

cpuset is the only system interface that uses bitsets, so solve this
by swapping the integer sub-components at the copyin/copyout points.

Sponsored by: Juniper Networks, Inc.

[kib]

sys/kern/kern_cpuset.c

1750

Style: there should be a blank line after the locals declaration block.

1754

I belive this code should go somewhere in sys/compat/freebsd32/freebsd32_misc.c.
Then you would define a structure with two pointers to functions: one for copyin, another for copyout, and pass this structure to corresponding kern_ functions.

This way compat32 cpuset syscalls would be contained in compat/freebsd32.

Most exemplar case following this pattern is umtx_op(2), but for some reason compat32 bits are present in kern_umtx.c.

[jhibbits]

sys/kern/kern_cpuset.c

1754

Good idea. I had thought of that, but scrapped the idea before implementing thinking it would be too intrusive. But, on actually implementing, it is cleaner.

[jhibbits]

Address @kib's comments. Looks nicer now.

[kib]

sys/compat/freebsd32/freebsd32_misc.c

3328

I would use multi-line comment there.

/*
 * Loop through swapping words.
 * `size' is in bytes, we need bits.
 */

3335

IMO return (0)'; there is better. You do this for copyout.

sys/sys/cpuset.h

162

Why do you need 'struct thread *' argument? Do you have plans to support e.g. both big- and little-endian compat32 on same host?

[jhibbits]

sys/sys/cpuset.h

162

It was remnant from the previous patch version, and can be removed. Supporting BE and LE on one host would be interesting and fun, but probably little practicality.

[jhibbits]

Further address @kib's feedback.

[kib]

sys/compat/freebsd32/freebsd32_misc.c

3322

Extra blank line.

[kib]

sys/compat/freebsd32/freebsd32_misc.c

3356

suword returns -1 on error, which is practically converted to ERESTART there. You need to return EFAULT below, instead of rv.

[jhibbits]

Fix return on suword() failure.

[kib]

sys/compat/freebsd32/freebsd32_misc.c

3366

Can this be static const struct cpuset_copy_cb ... ? You would need to constify some function arguments for this to work.

sys/kern/kern_cpuset.c

1746

const there as well?

[markj]

Seems ok to me, just some nitpicking. I agree with kib's comments about qualifying cpuset_copy_cb pointers with const.

sys/compat/freebsd32/freebsd32_misc.c

3328

3328

Or perhaps just explain that sizeof(__bits[0]) == 8 for LP64 and 4 for ILP32.

3352

Maybe then it's clear enough without the comment.