Page MenuHomeFreeBSD
Differential D20250

Build arm64 kernel with inline ffs, fls
ClosedPublic
Actions

Authored by dougm on May 12 2019, 9:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 12, 3:28 AM
Unknown Object (File)
Mon, Apr 7, 4:38 PM
Unknown Object (File)
Sun, Mar 16, 5:46 PM
Unknown Object (File)
Mar 2 2025, 5:04 AM
Unknown Object (File)
Feb 17 2025, 1:52 AM
Unknown Object (File)
Feb 10 2025, 1:33 AM
Unknown Object (File)
Feb 3 2025, 10:11 AM
Unknown Object (File)
Jan 28 2025, 2:38 AM
View All Files
Subscribers
andrew
emaste
imp
manu
pho
val_packett.cool

Details

Reviewers
kib
markj
alc
manu
Commits
rS347927: Implement the ffs and fls functions, and their longer counterparts, in
Summary

Implement the ffs and fls instructions, and their longer counterparts, in cpufunc, in terms of gcc extensions like __builtin_ffs, for arm64 architectures, and use those, rather than simple libkern implementations, in building arm64 kernels.

Tested by: greg_unrelenting.technology

Test Plan

I can complete an arm64 build, and I can examine an object file like pmap.o or subr_blist.o for signs that the inlining has happened, as instruction sequences like

244: dac001c9 rbit x9, x14
248: dac01129 clz x9, x9

appear there to indicate a fast implementation of ffs. My reference on smart bit ops for arm64 and how to recognize them is https://fgiesen.wordpress.com/2013/10/18/bit-scanning-equivalencies/.

Without the patch, the arm64 build does not have these sequences in these object files and has lines like this instead:

af34:       94000000        bl      0 <ffsl>

which do not appear with the patch in place.

None of this suggests that this patch is correct, but only that it does something. I don't have the means to test an arm64 kernel at this time. If Peter Holm can help, that would be great. If not, I'll have to wait for the freebsd arm64 test boxes to come back online.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

dougm created this revision.May 12 2019, 9:16 PM
Herald added subscribers: emaste, andrew. · View Herald TranscriptMay 12 2019, 9:16 PM
markj added subscribers: val_packett.cool, manu.May 12 2019, 11:56 PM

Perhaps @andrew, @manu, @greg_unrelenting.technology or one of the other ARM folks would be willing to test?

arm64/include/cpufunc.h
56

The indentation in this function and the one above looks off.

dougm updated this revision to Diff 57349.May 13 2019, 1:14 AM

Address indentation issues.

kib added a comment.May 13 2019, 8:44 AM

Can somebody explain to me why don't we use __builtin_ffs() and other functions everywhere ?

arm64/include/cpufunc.h
47

Why do you need the cast of mask to u_int ? Prototype in the gcc reference shows
int __builtin_ctz(unsigned).

I also find it strange to write mask == 0 ? mask : ..., I would certainly express it as mask == 0 ? 0 : ....

85

Why casting the result of __builtin_clzl() to int ? It is tautology.

dougm updated this revision to Diff 57360.May 13 2019, 2:25 PM
dougm marked 3 inline comments as done.

Reduce strangeness.

arm64/include/cpufunc.h
47

In imitating the code in amd64/include/cpufunc.h, I unnecessarily copied a cast-to-int. I will remove it. I also copied the use of 'mask' that seems strange. I will remove it too.

85

Agreed.

dougm added a comment.May 13 2019, 8:14 PM
In D20250#436408, @kib wrote:

Can somebody explain to me why don't we use __builtin_ffs() and other functions everywhere ?

In case you are asking why I wrote code using builtin_clz and not builtin_ffs, it is because I believe that many call to ffs and fls always subtract one from the result, and always know that the argument is nonzero, and I think that the way I wrote things might let the compiler optimize out the zero-check and/or the plus-one sometimes, and maybe with __builtin_ffs that wouldn't happen. But that is speculation.

I case you are asking about the whole freebsd codebase in general, I can't say.

val_packett.cool added a comment.May 14 2019, 9:54 AM

Kernel with this patch boots fine on Amazon EC2 (which btw you can use too, freebsd-snapshots posts have aarch64 AMIs)

dougm edited the summary of this revision. (Show Details)May 14 2019, 4:27 PM
dougm added a reviewer: alc.May 16 2019, 7:41 AM
dougm updated this revision to Diff 57437.May 16 2019, 7:46 AM

The only difference in the pmap.o code on arm64 between using builtin_ffsl and _builtin_ctzl (with a zero-test and an add) is that the version with builtin_ffsl has an 'orr' instruction where the other version has an 'add'.

So this version uses builtin_ffs and builtin_ffsl.

alc added inline comments.May 16 2019, 5:24 PM
arm64/include/cpufunc.h
94

This assumes that long and long long will always be the same size. If a tool chain came along that defined long long as 128 bits, this would compile without an error or warning and produce broken results. I would either add a CTASSERT() that long and long long are the same size or implement this using __builtin_clzll.

dougm updated this revision to Diff 57452.May 16 2019, 5:35 PM

Handle the possibility that sizeof(long long) != sizeof(long).

alc accepted this revision.May 17 2019, 4:36 AM
This revision is now accepted and ready to land.May 17 2019, 4:36 AM
kib accepted this revision.May 17 2019, 7:58 AM
Closed by commit rS347927: Implement the ffs and fls functions, and their longer counterparts, in (authored by dougm). · Explain WhyMay 17 2019, 3:52 PM
This revision was automatically updated to reflect the committed changes.
Herald added a reviewer: manu. · View Herald TranscriptMay 17 2019, 3:52 PM
Herald added a subscriber: imp. · View Herald Transcript
dougm mentioned this in D40594: Build riscv kernel with inline ffs, fls.Jun 18 2023, 4:11 AM

Revision Contents
Changeset List

PathSize
arm64/
include/
56 lines
conf/
6 lines

Diff 57437

View Options

arm64/include/cpufunc.h

Show First 20 Lines • Show All 302 Lines • ▼ Show 20 Lines
void void
exec_sysvec_init(void *param) exec_sysvec_init(void *param)
{ {
struct sysentvec *sv; struct sysentvec *sv;
#ifdef RANDOM_FENESTRASX #ifdef RANDOM_FENESTRASX
ptrdiff_t base; ptrdiff_t base;
#endif #endif
u_int flags;
sv = (struct sysentvec *)param; sv = (struct sysentvec *)param;
if ((sv->sv_flags & SV_SHP) == 0) flags = sv->sv_flags;
if ((flags & SV_SHP) == 0)
kibUnsubmitted
Not Done
Inline Actions

I think that this assert (sv_shared_page_base != 0) should be only done if SV_SHP flag is set.

kib: I think that this assert (sv_shared_page_base != 0) should be only done if SV_SHP flag is set.
return; return;
MPASS(sv->sv_shared_page_obj == NULL);
MPASS(sv->sv_shared_page_base != 0);
sv->sv_shared_page_obj = shared_page_obj; sv->sv_shared_page_obj = shared_page_obj;
if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD) {
sv->sv_sigcode_base = sv->sv_shared_page_base + sv->sv_sigcode_base = sv->sv_shared_page_base +
shared_page_fill(*(sv->sv_szsigcode), 16, sv->sv_sigcode); shared_page_fill(*(sv->sv_szsigcode), 16, sv->sv_sigcode);
if ((sv->sv_flags & SV_ABI_MASK) != SV_ABI_FREEBSD) }
return; if ((flags & SV_TIMEKEEP) != 0) {
kibUnsubmitted
Not Done
Inline Actions

Add {} around then statement, it is multi-line.

kib: Add {} around then statement, it is multi-line.
if ((sv->sv_flags & SV_TIMEKEEP) != 0) {
#ifdef COMPAT_FREEBSD32 #ifdef COMPAT_FREEBSD32
if ((sv->sv_flags & SV_ILP32) != 0) { if ((flags & SV_ILP32) != 0) {
if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD) {
KASSERT(compat32_svtk == NULL, KASSERT(compat32_svtk == NULL,
("Compat32 already registered")); ("Compat32 already registered"));
compat32_svtk = alloc_sv_tk_compat32(); compat32_svtk = alloc_sv_tk_compat32();
} else {
KASSERT(compat32_svtk != NULL,
("Compat32 not registered"));
}
kibUnsubmitted
Not Done
Inline Actions

Why do you need to check this condition? The assert on the previous line verified that the condition is true.

kib: Why do you need to check this condition? The assert on the previous line verified that the…
sv->sv_timekeep_base = sv->sv_shared_page_base + sv->sv_timekeep_base = sv->sv_shared_page_base +
compat32_svtk->sv_timekeep_off; compat32_svtk->sv_timekeep_off;
} else { } else {
#endif #endif
KASSERT(host_svtk == NULL, ("Host already registered")); if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD) {
KASSERT(host_svtk == NULL,
("Host already registered"));
host_svtk = alloc_sv_tk(); host_svtk = alloc_sv_tk();
} else {
KASSERT(host_svtk != NULL,
("Host not registered"));
}
sv->sv_timekeep_base = sv->sv_shared_page_base + sv->sv_timekeep_base = sv->sv_shared_page_base +
host_svtk->sv_timekeep_off; host_svtk->sv_timekeep_off;
#ifdef COMPAT_FREEBSD32 #ifdef COMPAT_FREEBSD32
} }
#endif #endif
} }
#ifdef RANDOM_FENESTRASX #ifdef RANDOM_FENESTRASX
if ((sv->sv_flags & SV_RNG_SEED_VER) != 0) { if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD &&
(flags & SV_RNG_SEED_VER) != 0) {
kibUnsubmitted
Not Done
Inline Actions

This one should be conditional on ABI_FREEBSD as well, perhaps?

kib: This one should be conditional on ABI_FREEBSD as well, perhaps?
kibUnsubmitted
Not Done
Inline Actions

This can be written as a single check

((flags  & (SV_ABI_MASK | SV_RNG_SEED_VER) ) ==  (SV_ABI_FREEBSD | SV_RNG_SEED_VER))

but it is up to you.

kib: This can be written as a single check ``` ((flags & (SV_ABI_MASK | SV_RNG_SEED_VER) ) ==…
/* /*
* Only allocate a single VDSO entry for multiple sysentvecs, * Only allocate a single VDSO entry for multiple sysentvecs,
* i.e., native and COMPAT32. * i.e., native and COMPAT32.
*/ */
if (fxrng_shpage_mapping == NULL) if (fxrng_shpage_mapping == NULL)
alloc_sv_fxrng_generation(); alloc_sv_fxrng_generation();
base = (char *)fxrng_shpage_mapping - shared_page_mapping; base = (char *)fxrng_shpage_mapping - shared_page_mapping;
sv->sv_fxrng_gen_base = sv->sv_shared_page_base + base; sv->sv_fxrng_gen_base = sv->sv_shared_page_base + base;
Show All 18 Linesexec_sysvec_init_secondary(struct sysentvec *sv, struct sysentvec *sv2)
if ((sv2->sv_flags & SV_TIMEKEEP) != 0) { if ((sv2->sv_flags & SV_TIMEKEEP) != 0) {
sv2->sv_timekeep_base = sv2->sv_shared_page_base + sv2->sv_timekeep_base = sv2->sv_shared_page_base +
(sv->sv_timekeep_base - sv->sv_shared_page_base); (sv->sv_timekeep_base - sv->sv_shared_page_base);
} }
if ((sv2->sv_flags & SV_RNG_SEED_VER) != 0) { if ((sv2->sv_flags & SV_RNG_SEED_VER) != 0) {
sv2->sv_fxrng_gen_base = sv2->sv_shared_page_base + sv2->sv_fxrng_gen_base = sv2->sv_shared_page_base +
(sv->sv_fxrng_gen_base - sv->sv_shared_page_base); (sv->sv_fxrng_gen_base - sv->sv_shared_page_base);
} }
} }
kibUnsubmitted
Not Done
Inline Actions

Now, with the changes to exec_sysvec_init(), what is the reason to have exec_sysvec_init_abi()? It only asserts some state.

kib: Now, with the changes to exec_sysvec_init(), what is the reason to have exec_sysvec_init_abi()?
kibUnsubmitted
Not Done
Inline Actions

Should sv->sv_shared_page_obj == 0 (NULL BTW?) and sv->sv_shared_page_base != 0 asserts moved to exec_sysvec_init() ?

kib: Should `sv->sv_shared_page_obj == 0` (NULL BTW?) and `sv->sv_shared_page_base != 0` asserts…
View Options

conf/files.arm64

Show First 20 Lines • Show All 302 Lines • ▼ Show 20 Lines
void void
exec_sysvec_init(void *param) exec_sysvec_init(void *param)
{ {
struct sysentvec *sv; struct sysentvec *sv;
#ifdef RANDOM_FENESTRASX #ifdef RANDOM_FENESTRASX
ptrdiff_t base; ptrdiff_t base;
#endif #endif
u_int flags;
sv = (struct sysentvec *)param; sv = (struct sysentvec *)param;
if ((sv->sv_flags & SV_SHP) == 0) flags = sv->sv_flags;
if ((flags & SV_SHP) == 0)
kibUnsubmitted
Not Done
Inline Actions

I think that this assert (sv_shared_page_base != 0) should be only done if SV_SHP flag is set.

kib: I think that this assert (sv_shared_page_base != 0) should be only done if SV_SHP flag is set.
return; return;
MPASS(sv->sv_shared_page_obj == NULL);
MPASS(sv->sv_shared_page_base != 0);
sv->sv_shared_page_obj = shared_page_obj; sv->sv_shared_page_obj = shared_page_obj;
if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD) {
sv->sv_sigcode_base = sv->sv_shared_page_base + sv->sv_sigcode_base = sv->sv_shared_page_base +
shared_page_fill(*(sv->sv_szsigcode), 16, sv->sv_sigcode); shared_page_fill(*(sv->sv_szsigcode), 16, sv->sv_sigcode);
if ((sv->sv_flags & SV_ABI_MASK) != SV_ABI_FREEBSD) }
return; if ((flags & SV_TIMEKEEP) != 0) {
kibUnsubmitted
Not Done
Inline Actions

Add {} around then statement, it is multi-line.

kib: Add {} around then statement, it is multi-line.
if ((sv->sv_flags & SV_TIMEKEEP) != 0) {
#ifdef COMPAT_FREEBSD32 #ifdef COMPAT_FREEBSD32
if ((sv->sv_flags & SV_ILP32) != 0) { if ((flags & SV_ILP32) != 0) {
if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD) {
KASSERT(compat32_svtk == NULL, KASSERT(compat32_svtk == NULL,
("Compat32 already registered")); ("Compat32 already registered"));
compat32_svtk = alloc_sv_tk_compat32(); compat32_svtk = alloc_sv_tk_compat32();
} else {
KASSERT(compat32_svtk != NULL,
("Compat32 not registered"));
}
kibUnsubmitted
Not Done
Inline Actions

Why do you need to check this condition? The assert on the previous line verified that the condition is true.

kib: Why do you need to check this condition? The assert on the previous line verified that the…
sv->sv_timekeep_base = sv->sv_shared_page_base + sv->sv_timekeep_base = sv->sv_shared_page_base +
compat32_svtk->sv_timekeep_off; compat32_svtk->sv_timekeep_off;
} else { } else {
#endif #endif
KASSERT(host_svtk == NULL, ("Host already registered")); if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD) {
KASSERT(host_svtk == NULL,
("Host already registered"));
host_svtk = alloc_sv_tk(); host_svtk = alloc_sv_tk();
} else {
KASSERT(host_svtk != NULL,
("Host not registered"));
}
sv->sv_timekeep_base = sv->sv_shared_page_base + sv->sv_timekeep_base = sv->sv_shared_page_base +
host_svtk->sv_timekeep_off; host_svtk->sv_timekeep_off;
#ifdef COMPAT_FREEBSD32 #ifdef COMPAT_FREEBSD32
} }
#endif #endif
} }
#ifdef RANDOM_FENESTRASX #ifdef RANDOM_FENESTRASX
if ((sv->sv_flags & SV_RNG_SEED_VER) != 0) { if ((flags & SV_ABI_MASK) == SV_ABI_FREEBSD &&
(flags & SV_RNG_SEED_VER) != 0) {
kibUnsubmitted
Not Done
Inline Actions

This one should be conditional on ABI_FREEBSD as well, perhaps?

kib: This one should be conditional on ABI_FREEBSD as well, perhaps?
kibUnsubmitted
Not Done
Inline Actions

This can be written as a single check

((flags  & (SV_ABI_MASK | SV_RNG_SEED_VER) ) ==  (SV_ABI_FREEBSD | SV_RNG_SEED_VER))

but it is up to you.

kib: This can be written as a single check ``` ((flags & (SV_ABI_MASK | SV_RNG_SEED_VER) ) ==…
/* /*
* Only allocate a single VDSO entry for multiple sysentvecs, * Only allocate a single VDSO entry for multiple sysentvecs,
* i.e., native and COMPAT32. * i.e., native and COMPAT32.
*/ */
if (fxrng_shpage_mapping == NULL) if (fxrng_shpage_mapping == NULL)
alloc_sv_fxrng_generation(); alloc_sv_fxrng_generation();
base = (char *)fxrng_shpage_mapping - shared_page_mapping; base = (char *)fxrng_shpage_mapping - shared_page_mapping;
sv->sv_fxrng_gen_base = sv->sv_shared_page_base + base; sv->sv_fxrng_gen_base = sv->sv_shared_page_base + base;
Show All 18 Linesexec_sysvec_init_secondary(struct sysentvec *sv, struct sysentvec *sv2)
if ((sv2->sv_flags & SV_TIMEKEEP) != 0) { if ((sv2->sv_flags & SV_TIMEKEEP) != 0) {
sv2->sv_timekeep_base = sv2->sv_shared_page_base + sv2->sv_timekeep_base = sv2->sv_shared_page_base +
(sv->sv_timekeep_base - sv->sv_shared_page_base); (sv->sv_timekeep_base - sv->sv_shared_page_base);
} }
if ((sv2->sv_flags & SV_RNG_SEED_VER) != 0) { if ((sv2->sv_flags & SV_RNG_SEED_VER) != 0) {
sv2->sv_fxrng_gen_base = sv2->sv_shared_page_base + sv2->sv_fxrng_gen_base = sv2->sv_shared_page_base +
(sv->sv_fxrng_gen_base - sv->sv_shared_page_base); (sv->sv_fxrng_gen_base - sv->sv_shared_page_base);
} }
} }
kibUnsubmitted
Not Done
Inline Actions

Now, with the changes to exec_sysvec_init(), what is the reason to have exec_sysvec_init_abi()? It only asserts some state.

kib: Now, with the changes to exec_sysvec_init(), what is the reason to have exec_sysvec_init_abi()?
kibUnsubmitted
Not Done
Inline Actions

Should sv->sv_shared_page_obj == 0 (NULL BTW?) and sv->sv_shared_page_base != 0 asserts moved to exec_sysvec_init() ?

kib: Should `sv->sv_shared_page_obj == 0` (NULL BTW?) and `sv->sv_shared_page_base != 0` asserts…