Page MenuHomeFreeBSD
Differential D47846

sys/intr.h: Make it safe to include from assembler
ClosedPublic
Actions

Authored by imp on Fri, Nov 29, 9:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 10:38 AM
Unknown Object (File)
Sun, Dec 8, 9:58 AM
Unknown Object (File)
Fri, Dec 6, 7:27 PM
Unknown Object (File)
Thu, Dec 5, 6:54 PM
Unknown Object (File)
Wed, Dec 4, 6:11 AM
Unknown Object (File)
Tue, Dec 3, 1:38 PM
Unknown Object (File)
Mon, Dec 2, 1:01 PM
Subscribers
ehem_freebsd_m5p.com
emaste
jhb
riscv

Details

Reviewers
kevans
mmel
andrew
manu
Commits
rG7b16a4a75dc1: sys/intr.h: Make it safe to include from assembler
Summary

Sometimes we need defines from this file in assembler code. Today we do
the heavyweight approach of using genassym for that. However, they are
just #defines, so in the future we want to include sys/intr.h to pick up
the needed constants in exception.S.

Sponsored by: Netflix

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60885
Build 57769: arc lint + arc unit

Event Timeline

imp created this revision.Fri, Nov 29, 9:21 PM
Herald added a reviewer: andrew. · View Herald TranscriptFri, Nov 29, 9:21 PM
Herald added a reviewer: andrew. · View Herald Transcript
Herald added a reviewer: manu. · View Herald Transcript
Herald added subscribers: riscv, emaste. · View Herald Transcript
imp requested review of this revision.Fri, Nov 29, 9:21 PM
Harbormaster completed remote builds in B60885: Diff 147177.Fri, Nov 29, 9:22 PM
imp added a child revision: D47847: arm: Use constants from sys/intr.h, not genassym.Fri, Nov 29, 9:22 PM
andrew accepted this revision.Fri, Nov 29, 9:27 PM
This revision is now accepted and ready to land.Fri, Nov 29, 9:27 PM
imp added a comment.Fri, Nov 29, 9:30 PM

Another way to do this would be to move INTR_IRQ_INVALID and INTR_ROOT_IRQ into machine/intr.h and then include that later in the patch series.

ehem_freebsd_m5p.com added a subscriber: ehem_freebsd_m5p.com.Fri, Nov 29, 10:08 PM
ehem_freebsd_m5p.com added inline comments.
sys/arm/include/intr.h
50–52

Why expose NIRQ to assembly-language? Perhaps if a future commit needed it, but right now why not keep it out. If nothing else, this serves to document the value isn't touched by assembly-language and hinting going back isn't too difficult.

sys/arm64/include/intr.h
39–49

Similar to above, the only values touched by assembly-language are the INTR_ROOT_* values.

sys/riscv/include/intr.h
57

Unneeded. RISC-V doesn't use any of this in assembly, so why make the header assembly-safe?

sys/sys/intr.h
42

I'm against having sys/intr.h be assembly-safe. This is an architecture detail and shouldn't dictate what this common header looks like.

ehem_freebsd_m5p.com added a comment.Fri, Nov 29, 10:12 PM

Unless there is something I'm unaware of, INTR_IRQ_INVALID isn't ever used by assembly-language.

mmel accepted this revision.Sat, Nov 30, 12:23 PM
jhb added a subscriber: jhb.Sat, Nov 30, 5:14 PM
jhb added inline comments.
sys/arm/include/intr.h
42

It would be nice if we could be somewhat consistent. Currently LOCORE is the most prominent thing we test for (and in particular this is what we use in x86 arches) and the one documented as such in sys/conf/kern.pre.mk:

# XXX LOCORE means "don't declare C stuff" not "for locore.s".
ASM_CFLAGS= -x assembler-with-cpp -DLOCORE ${CFLAGS} ${ASM_CFLAGS.${.IMPSRC:T}}

Next is ASSEMBLER and last is __ASSEMBLER__.

50–52

In general if we are going to make a header assembly-safe, we should default to exposing all the assembly-safe things.

sys/riscv/include/intr.h
57

Presumably the goal is to make <machine/intr.h> universally safe across all architectures that define one?

imp added a comment.Sat, Nov 30, 6:37 PM

yea, the only question I have is if we move all the assembly values into machine/intr.h, duplicating one or two across architectures and include machine/intr.h for the assembler, or if I leave it as I have it inthe review. @jhb

sys/arm/include/intr.h
42

Somehow I had it in my head that ASSEMBLER was the preferred way, since toolchains define that now and LOCORE was legacy... I'm not entirely sure how I got there, but I'm happy to change it to LOCORE... There's a few places we test for both though (well, 2, both in elf_common.h)

sys/sys/elf_common.h:#if !defined(LOCORE) && !defined(ASSEMBLER)

should se change that?

50–52

yea, NIRQ is assembly safe

sys/riscv/include/intr.h
57

Yes.

jhb added a comment.Sat, Nov 30, 7:41 PM
In D47846#1090926, @imp wrote:

yea, the only question I have is if we move all the assembly values into machine/intr.h, duplicating one or two across architectures and include machine/intr.h for the assembler, or if I leave it as I have it inthe review. @jhb

I don't see a compelling reason to duplicate values across architectures. That would seem to defeat the point of the shared header file.

jhb added inline comments.Sat, Nov 30, 7:42 PM
sys/arm/include/intr.h
42

It may be that we should adopt __ASSEMBLER__ if that is what modern toolchains always define, but we should do a principled adoption of that if so. For now I would stick with LOCORE and maybe we can do a global sweep to switch to __ASSEMBLER__ (and note it in style.9) as a separate change.

ehem_freebsd_m5p.com added a comment.Sat, Nov 30, 8:32 PM
In D47846#1090953, @jhb wrote:

I don't see a compelling reason to duplicate values across architectures. That would seem to defeat the point of the shared header file.

The values in question actually belong to the architecture. An architecture can arbitrarily replace them without problems. The issue is having all these associated values in 1 place makes keeping them synchronized (non-conflicting) easier. For another the issue is having a default which is overridden by 2 of 3 architectures doesn't make sense either.

sys/arm/include/intr.h
50–52

Certainly assembly-safe, but doesn't seem assembly-necessary. If there was a desire to convert back in the future, all of these would need checking and not exposing extra values would make this easier. x86/include/intr_machdep.h appears to no longer be used in assembly-language yet no action has been taken yet (I've got a patch/commit, but it is extremely low priority).

sys/riscv/include/intr.h
57

Shouldn't the goal be D47848? This is one way to achieve this, but there are other ways.

andrew added inline comments.Sat, Nov 30, 10:00 PM
sys/arm/include/intr.h
42

I added __ASSEMBLER__ to elf_common.h as I wanted to include it from userspace asm where LOCORE may not be defined. I probably kept LOCORE in that file as I'm not sure if all compilers that we care about define it when building asm files.

imp updated this revision to Diff 147258.Sat, Nov 30, 11:43 PM

Use LOCORE

This revision now requires review to proceed.Sat, Nov 30, 11:43 PM
Harbormaster completed remote builds in B60897: Diff 147258.Sat, Nov 30, 11:43 PM
ehem_freebsd_m5p.com added a comment.Mon, Dec 2, 11:30 PM
In D47846#1090926, @imp wrote:

yea, the only question I have is if we move all the assembly values into machine/intr.h, duplicating one or two across architectures and include machine/intr.h for the assembler, or if I leave it as I have it inthe review. @jhb

In the words of mhorne. This case duplication seems worthwhile. Important part is the architecture is completely free to use other names/values.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Dec 6, 5:55 PM
Closed by commit rG7b16a4a75dc1: sys/intr.h: Make it safe to include from assembler (authored by imp). · Explain Why
This revision was automatically updated to reflect the committed changes.
imp added a commit: rG7b16a4a75dc1: sys/intr.h: Make it safe to include from assembler.

Revision Contents
Changeset List

PathSize
sys/
arm/
include/
6 lines
arm64/
include/
10 lines
riscv/
include/
2 lines
sys/
6 lines

Diff 147177

View Options

sys/arm/include/intr.h

Loading...
View Options

sys/arm64/include/intr.h

Loading...
View Options

sys/riscv/include/intr.h

Loading...
View Options

sys/sys/intr.h

Loading...