Page MenuHomeFreeBSD

arm64 lib32: prepare arm64 headers to redirect to arm
ClosedPublic

Authored by karels on Jul 8 2023, 10:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 8:19 PM
Unknown Object (File)
Tue, Nov 12, 2:37 PM
Unknown Object (File)
Tue, Nov 12, 2:37 PM
Unknown Object (File)
Sun, Nov 10, 1:40 PM
Unknown Object (File)
Fri, Nov 8, 6:37 AM
Unknown Object (File)
Fri, Nov 8, 4:14 AM
Unknown Object (File)
Thu, Nov 7, 6:48 PM
Unknown Object (File)
Thu, Nov 7, 6:48 PM

Details

Summary

In order to compile lib32 libraries and other 32-bit code on arm64,
<machine/foo.h> needs to be redirected to an arm header rather
than arm64 when building with -m32. Ifdef the arm64 headers that
are installed in /usr/include/machine and used by user-level software
(including references from /usr/include/*.h) so that if arm is
defined when including the arm64 version, <arm/foo.h> is included
rather than using the rest of the file's contents. Some arm headers
had no arm64 equivalent; headers were added just to do the redirection.
These files use #error if arm is not defined to guard against
confusion. Also add an include/arm Makefile, and modify Makefiles
as needed to install everything, including the arm files in
/usr/include/arm.

The new arm64 headers are:

acle-compat.h
atomic-v6.h
cpu-v6.h
cpuinfo.h
pmap-v6.h
pte-v6.h
swi.h
sysreg.h

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
include/arm/Makefile
50

Yeah, procstat/zfs is a total mess that really needs to be redone... (https://github.com/CTSRD-CHERI/cheribsd/commit/68cc84debb0c275a364b065dab0aa06843602895 for some fun we had downstream)

54

Not sure if that one's such an XXX

61

I might hoist some of this into some common ../Makefile.libcompat post-commit, but this inter-file duplication is fine to commit for now IMO, feels less bad than intra-file

lib/msun/Makefile
163–164 ↗(On Diff #124512)

Best to avoid TARGET_ARCH checks outside top-level files

165 ↗(On Diff #124512)

Hm, this is because arm and aarch64 have different fenv.h, whilst x86 has a common one? The math.h one is going to be installed into arm/ and unused; this should be doing INCSDIR_fenv.h=.

lib/msun/aarch64/fenv.h
28

Missing blank line

sys/arm64/include/pmap-v6.h
6 ↗(On Diff #124512)

?

sys/arm64/include/pte-v6.h
6 ↗(On Diff #124512)

?

sys/arm64/include/reloc.h
5

#else?

6

?

sys/arm64/include/swi.h
6 ↗(On Diff #124512)

?

sys/arm64/include/sysreg.h
7

?

I'd like to review this, but won't have time in the next 1-2 weeks.

I'd like to review this, but won't have time in the next 1-2 weeks.

@andrew, Are you OK with the general scheme of doing this in the necessary arm64/include headers:

#ifdef arm
#include <arm/foo.h>
#else
/* rest of file */
#endif

If so, we can make second-order corrections later.

include/arm/Makefile
6

Sorry, that's a left-over from approach 1.

54

pte-v6.h is dead, right?

lib/msun/Makefile
165 ↗(On Diff #124512)

Correct. math.h needed to be in arm/ in approach 1. I guess math.h will be installed twice in the same place, but that's OK.

sys/arm64/include/reloc.h
5

This was a pre-existing empty file. Including it didn't cause an error in the past, so still should not.

The -v6.h is due to an incomplete removal of the v4/5 code that should have inlined these, but didn't.

sys/arm64/include/pmap-v6.h
6 ↗(On Diff #124512)

Yea, arm/pmap.h includes arm/pmap-v6.h

sys/arm64/include/pte-v6.h
6 ↗(On Diff #124512)

arm/pmap_var.h includes this, along with cpu-v6.h

lib/msun/Makefile
165 ↗(On Diff #124512)

Yeah, same as x86 and powerpc. Best to avoid confusion and not install unused headers.

sys/arm64/include/reloc.h
5

Ack, didn't notice that when skimming. I'd suggest:

#else
/* empty */
#endif

or similar (could ditch the error, but then might look like a mistake?) perhaps?

include/arm/Makefile
54

v7 uses the same PTE format as v6 (as opposed to the v4 and v5 which shared the -v4 pmap bits). The only thing "dead" about it is that it doesn't need to be versioned any more.

sys/arm64/include/pmap-v6.h
6 ↗(On Diff #124512)

The ?'s here are for the blank lines specifically, not the files themselves

include/arm/Makefile
54

Yea, one of the 'todo' items that never got done was inlining them back into the original files now that the need for them is gone.

fix cosmetic problems, add fenv.h to include/arm/Makefile, move
include subdirectory creation into a loop to save a set of ifdefs

karels marked 6 inline comments as done.

missed some blank lines, need /* empty */ comments

add fenv.h to include/arm/Makefile

This is odd, what's that for?

include/Makefile
16

You can just SUBDIR+=${INCUDE_SUBDIRS} outside the ifs rather than duplicate each directory name. Though why not use the same variable name as Makefile.inc1?

add fenv.h to include/arm/Makefile

This is odd, what's that for?

fenv.h wasn't getting installed in /usr/include/arm, just ${WORLDTMP}/usr/include/arm. lib/msun/Makefile doesn't get invoked with a LIBCOMPAT run during installworld.

include/Makefile
16

I didn't want to assume that the source directory and the install directory had the same name. I didn't use the same name for INCLUDE_SUBDIRS because this is not necessarily related to LIBCOMPAT.

add fenv.h to include/arm/Makefile

This is odd, what's that for?

fenv.h wasn't getting installed in /usr/include/arm, just ${WORLDTMP}/usr/include/arm. lib/msun/Makefile doesn't get invoked with a LIBCOMPAT run during installworld.

That doesn't sound right. install${libcompat} recurses on _lc_install which then recurses into lib, which should in turn recurse into lib/msun.

add fenv.h to include/arm/Makefile

This is odd, what's that for?

fenv.h wasn't getting installed in /usr/include/arm, just ${WORLDTMP}/usr/include/arm. lib/msun/Makefile doesn't get invoked with a LIBCOMPAT run during installworld.

That doesn't sound right. install${libcompat} recurses on _lc_install which then recurses into lib, which should in turn recurse into lib/msun.

I don't know, but the lib32 section of msun install output has only this:

===> msun (install)
install  -C -o root -g wheel -m 444   libm.a /scratch/arm64/usr/lib32/
install  -s -o root -g wheel -m 444   -S  libm.so.5 /scratch/arm64/usr/lib32/
install  -o root -g wheel -m 444    libm.so.5.debug /scratch/arm64/usr/lib/debug
/usr/lib32/
install -l rs -o root -g wheel -m 755 -S   libm.so.5 /scratch/arm64/usr/lib32/li
bm.so
===> libsqlite3 (install)

The non-compat install also installs headers (aarch64 for fenv.h), man pages, etc.

Oh, LIB${LIBCOMPAT}IMAKE has MK_INCLUDES=no, hmm, this is more of a mess than I thought :(

My instinct is MK_INCLUDES=no should be in LIB${LIBCOMPAT}WMAKE not ...IMAKE so that we get a consistent set of headers for ${WORLDTMP} and the installed world, but who knows if that breaks something... I'll go look at that with a tinderbox later

The lib32/arm build still seems to work with this change, assuming the fenv.h change to include/arm/Makefile.

I think the review comments are all addressed now. Anyone have any remaining comments? On this or the other reviews in the series?

Where do we stand on this review? Any more comments or approvals? Do we need to wait for @andrew ?

Where do we stand on this review? Any more comments or approvals? Do we need to wait for @andrew ?

I'll note that we're down to just over 2 weeks until 14.0 slush. I'd really like to get this into a build in time to fix it before slush, but that time is approximately now. Several people approved the approach in email; should I commit this on that basis?

sys/arm64/include/atomic-v6.h
1 ↗(On Diff #124610)

Can we merge the arm atomic-v6.h and atomic.h?

sys/arm64/include/cpu-v6.h
1 ↗(On Diff #124610)

Do we need this file? On arm it's only usable by the kernel.

sys/arm64/include/cpuinfo.h
2

Do we need this file? It looks like it's kernel specific.

sys/arm64/include/pte-v6.h
1 ↗(On Diff #124610)

Is this file used by userspace?

sys/arm64/include/swi.h
1 ↗(On Diff #124610)

Is this file used by userspace? If not I don't think it should exist on arm.

sys/arm64/include/sysreg.h
2

Do we need this file? It looks like it's kernel specific.

I'll double-check on the individual files you asked about in an hour or so, but in summary: I didn't add headers unless they were included, directly or indirectly, via files in <sys/>, or were included by something in the build. I wasn't really considering any merges, etc, as part of the same change.

sys/arm64/include/atomic-v6.h
1 ↗(On Diff #124610)

In theory this can be done. I wouldn't want to do it in this commit, as it is a fairly large change (or at least, a lot of code moves).

sys/arm64/include/cpu-v6.h
1 ↗(On Diff #124610)

It's included by <machine/cpu.h>, which in turn is included by <sys/proc.h>.

sys/arm64/include/cpuinfo.h
2

Included by cpu-v6.h.

sys/arm64/include/pte-v6.h
1 ↗(On Diff #124610)

yes, it is directly included from libkvm.

sys/arm64/include/swi.h
1 ↗(On Diff #124610)

It's used by SYS.h for arm libc.

sys/arm64/include/sysreg.h
2

It is included (#ifndef _KERNEL) by arm's atomic.h.

I think that post-v4/v5 removal cleanups should be done separately and not required as a blocker for this change (e.g. merging pmap.h and pmap-v6.h)

See D41133 to D41140 for arm header cleanups.

For anyone who hasn't looked at these, they eliminate or combine some of the headers like atomic-v6.h. This raises the question of ordering of the two sets of commits. As much as I'd like to close out the lib32 changes, it makes more sense to have @andrew's go in first so new headers don't get created and them removed immediately.

See D41133 to D41140 for arm header cleanups.

For anyone who hasn't looked at these, they eliminate or combine some of the headers like atomic-v6.h. This raises the question of ordering of the two sets of commits. As much as I'd like to close out the lib32 changes, it makes more sense to have @andrew's go in first so new headers don't get created and them removed immediately.

Agreed on the ordering, but it also seems like Andrew's has landed now.

Now that Andrew's changes have landed this is good

This revision is now accepted and ready to land.Jul 24 2023, 9:23 PM

Integrate with andrew's changes, remove added headers no longer needed.

This revision now requires review to proceed.Jul 24 2023, 11:47 PM

The summary's "new arm64 headers are" list is out of date and should not be blindly copied into commit notes.

The summary's "new arm64 headers are" list is out of date and should not be blindly copied into commit notes.

The list is correct in git, but Phabricator doesn't update it. The list is:

acle-compat.h
cpuinfo.h
sysreg.h

Andrew reduced the use of sysreg.h, but cpu.h includes it, and libprocstat/zfs uses cpu.h.

Any comments? I'd like to get this into this week's build. The only changes are to delete a number of the new wrapper files, and add one new redirection (pte.h instead of pte-v6.h).

Thinking about this more, would the dummy headers not be better off in include/arm/?

jrtc27 requested changes to this revision.Jul 25 2023, 10:31 PM
jrtc27 added inline comments.
include/arm/Makefile
60–61
lib/msun/Makefile
163 ↗(On Diff #125107)

This code should now be dead; Makefile.libcompat now does all builds with MK_INCLUDES=no

This revision now requires changes to proceed.Jul 25 2023, 10:31 PM

Thinking about this more, would the dummy headers not be better off in include/arm/?

You mean the headers that are just wrappers? They are included as <machine/foo.h>, so need to be in include/machine.

Thinking about this more, would the dummy headers not be better off in include/arm/?

You mean the headers that are just wrappers? They are included as <machine/foo.h>, so need to be in include/machine.

Uh, right, yes, disregard that then

Oops, missed one nit, but can be fixed at commit time. Otherwise looks ok to me.

sys/arm64/include/acle-compat.h
6–7
This revision is now accepted and ready to land.Jul 25 2023, 11:45 PM