Page MenuHomeFreeBSD

sys/arm64/include
ClosedPublic

Authored by andrew on Mar 13 2015, 2:57 AM.

Details

Reviewers
emaste
imp
Summary

Include files for arm64

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

emaste updated this revision to Diff 4204.Mar 13 2015, 2:57 AM
emaste retitled this revision from to sys/arm64/include.
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added reviewers: imp, andrew.

There are a number of 4-clause licensed files copied from elsewhere that will need to be replaced.

sys/arm64/include/in_cksum.h
13–16 ↗(On Diff #4204)

UCB clause 3 needs to be removed

andrew added inline comments.Mar 13 2015, 10:12 AM
sys/arm64/include/endian.h
47

I think this file may need a little clean up

sys/arm64/include/psci.h
1 ↗(On Diff #4204)

I think we can exclude this file, I'm planning on moving the driver to sys/dev/psci as it will also be useful on armv6.

sys/arm64/include/ucontext.h
36

I would like to reorder this to be the same as struct reg as it will allow us to memcpy them in a few places in the kernel. The order of this has been fixed, but not been copied over as struct reg is part of the ABI.

The trapframe may also need to be reordered, but this is less of a concern as it is only used within the kernel.

sys/arm64/include/vmparam.h
155

This comment is most likely wrong, it was left here after I copied the file.

162

As is this one

imp edited edge metadata.Mar 13 2015, 10:07 PM

Lots of crazy copying going on here :(

I've just done a first quick pass.

sys/arm64/include/_inttypes.h
214

We have two versions of this in the tree, one for ILP32 and one for LP64. Maybe we should just have two versions rather than multiple copies.

sys/arm64/include/_limits.h
86

Ditto this file, with the possible exception of MINSIGSTKSZ

sys/arm64/include/_stdint.h
159

Ditto this one.

sys/arm64/include/_types.h
115

Ditto. I'm going to stop dittoing

sys/arm64/include/bus.h
479 ↗(On Diff #4204)

We have several versions of the table of pointers. Perhaps that cna be cleaned up.

sys/arm64/include/endian.h
136

This is close to an LP64 / ILP32 divide thing.

sys/arm64/include/exec.h
33

I'm not sure this file qualifies for copyright protection. And if it did, you've changed all the lines that aren't scenes a faire...

sys/arm64/include/float.h
99

Looks like a case of having a IEEE file vs non IEEE file.

sys/arm64/include/param.h
114–119

why both types of macros here? arm64 seems redundant.

sys/arm64/include/proc.h
35

We're inconsistent. Sometimes it is ARM64_INCLUDE_foo others it is MACHINE_foo

sys/arm64/include/ptrace.h
35 ↗(On Diff #4204)

Empty files don't need a copyright notice :(

sys/arm64/include/trap.h
31 ↗(On Diff #4204)

ditto, though this one is less insane since it is your copyright.

andrew added inline comments.Mar 16 2015, 4:46 PM
sys/arm64/include/fdt.h
1 ↗(On Diff #4204)

Nathan Whitehorn has said this file should not be used.

andrew added inline comments.Mar 17 2015, 4:59 PM
sys/arm64/include/_types.h
115

The wchar_t bits of this are arm64 specific.

sys/arm64/include/exec.h
33

The only place I can find that uses __LDPGSZ is in sys/imgact_aout.h. As such I'm planning on removing this define as we won't support a.out on arm64.

sys/arm64/include/param.h
114–119

It came from sparc64. We have both there so it was copied over when I was getting the kernel building.

emaste added inline comments.Mar 18 2015, 4:06 AM
sys/arm64/include/_align.h
42

FWIW NetBSD #defines __ALIGNBYTES in sys/arch/<arch>/include/cdefs.h

They also avoid the arch name in the header guard, which seems reasonable to me (e.g. `_MACHINE_CDFES_H_ for this define) - it would make finding duplicates easier.

sys/arm64/include/_inttypes.h
214

I think we should work to unify these cases, but I don't want that to be a dependency of this project. My general take is that if we already have n copies, n >= 2, then the (n+1)th copy is tolerable.

sys/arm64/include/armreg.h
45–46 ↗(On Diff #4204)

A note, Phabricator renders tabs strangely.

These lines are OK - they're #define<tab>CPACR_FPEN_MASK and #define<tab><space>CPACR_FPEN_TRAP_ALL1

sys/arm64/include/param.h
114–119

We also have for amd64:

#define atop(x)         ((unsigned long)(x) >> PAGE_SHIFT)
#define ptoa(x)         ((unsigned long)(x) << PAGE_SHIFT)

#define amd64_btop(x)   ((unsigned long)(x) >> PAGE_SHIFT)
#define amd64_ptob(x)   ((unsigned long)(x) << PAGE_SHIFT)
sys/arm64/include/pmap.h
34–38

Looks like this comment should go (possibly replacing it with a 1 sentence explaining the history to arrive at the arm64 version).

sys/arm64/include/proc.h
35

See comments above - I suspect it's inconsistent across the tree, but I think we should be using MACHINE_ in all new cases.

sys/arm64/include/psci.h
1 ↗(On Diff #4204)

Sounds good.

sys/arm64/include/pte.h
8–20

This should have a new 2-clause license

sys/arm64/include/ptrace.h
35 ↗(On Diff #4204)

And for that matter, the include guards don't provide much value for empty files. Probably just /* $FreeBSD$ */?

imp added a comment.Mar 18 2015, 7:06 PM

More nit-picky comments.

I'm quite worried about our plethera of identical file copies, but issues related to this won't gate this is.

sys/arm64/include/_inttypes.h
214

If we have more than two copies, we're clearly doing something very wrong. Especially for files of this length.

Tolerable, perhaps, for this commit, but not tolerable in the long term.

sys/arm64/include/bus.h
45–51 ↗(On Diff #4204)

We might want to look at 'redoing' our import of these files since the license has changed upstream.

sys/arm64/include/clock.h
32 ↗(On Diff #4204)

There's no copyrightable material in this file.

sys/arm64/include/cpu.h
7–8

Jolitz did arm stuff? I'd be surprised... But that may be an issue with where this was copied from.

38

Is there anything that's really common with the i386 version? Or is this a copy of the amd64 version with the same question...

sys/arm64/include/db_machdep.h
123 ↗(On Diff #4204)

This looks mostly new and no longer derived from the mach fore-fathers.

sys/arm64/include/frame.h
2 ↗(On Diff #4204)

It looks like this file has been mostly rewritten, as such shouldn't Andrew's or the FF's copyright be on it.

sys/arm64/include/hypervisor.h
2 ↗(On Diff #4204)

We prefer - to , in the tree.

sys/arm64/include/param.h
114–119

They are almost unused on amd64. They are defined for btop in libkvm, but then never referenced (only sparc64 appears to use them)...

sys/arm64/include/pte.h
8–20

This is from Brinicombe, not NetBSD. Did he relicense?

sys/arm64/include/setjmp.h
79

This file is also a candidate for the 'rewritten enough to get its own copyright' since it shares almost nothing from its ancestor files with the possible exception of the jmp_buf flavors at the end.

sys/arm64/include/ucontext.h
36

Reordering should be fine. This file also looks like it might be a complete re-write.

emaste added inline comments.Mar 18 2015, 7:34 PM
sys/arm64/include/_inttypes.h
214

Agreed. It's been a while since we've brought in a brand-new architecture, and I think the careful review here demonstrates some of the shortcuts that were taken with previous new archs.

What I want to do is consider this a bit of a wake-up call: continue the copying in the short term, but subsequently go back and eliminate the redundant copies across all architectures.

I'm not sure quite what the final layout should look like, but maybe something along the lines of:

sys/arch/ilp32/include
sys/arch/lp64/include

with appropriate .PATH settings in a Makefile.inc to pick the right one per arch.

sys/arm64/include/bus.h
45–51 ↗(On Diff #4204)

It has for some files, but I think NetBSD still has some 4-clause files left.

sys/arm64/include/clock.h
32 ↗(On Diff #4204)

Indeed, it should be removed and we can add an appropriate copyright when content is added.

32 ↗(On Diff #4204)

Right. We should remove the copyright block for now, and add an appropriate one if/when content is added.

imp added inline comments.Mar 19 2015, 4:23 PM
sys/arm64/include/_inttypes.h
214

that's an interesting idea, but relying on .PATH settings isn't going to work out too well. We need to refer to the files in place, as well as from userland after they are installed. And then there's things like mips and powerpc where both 32 and 64 bit stuff is installed at the same time.

What would work out better, imho, is if we had something like the following:

sys/gen/_inttypes.h would have

#ifdef LP64
#include <sys/lp64/_inttypes.h>
#else
#include <sys/ilp32/_inttypes.h>
#endif

And nearly all the port's _inttypes.h files (so sys/*/include/_inttypes.h) would be the two liner:
/* This file is in the public domain. $FreeBSD$ */
#include <sys/gen/_inttypes.h>

which would work in place, with linked installed and with copy install of the kernel config files.

plus sys/arch is a bit of a lightening rod name... :(

andrew commandeered this revision.Mar 19 2015, 4:26 PM
andrew edited reviewers, added: emaste; removed: andrew.
andrew updated this revision to Diff 4290.Mar 19 2015, 4:27 PM
andrew edited edge metadata.

Clean up headers, and only import the headers needed for toolchain

andrew updated this revision to Diff 4291.Mar 19 2015, 4:46 PM

Update the license on rewritten headers

imp accepted this revision.Mar 20 2015, 4:49 PM
imp edited edge metadata.

I think this is ready, subject to some vague commitment in the future that we (the project) will clean up the excessive copying.

This revision is now accepted and ready to land.Mar 20 2015, 4:49 PM
andrew closed this revision.Mar 23 2015, 1:43 PM

Committed in r280364