Details

Reviewers

emaste
jhb
andrew
imp

Commits

rS295041: Welcome the RISC-V 64-bit kernel.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

br updated this revision to Diff 12422.Jan 18 2016, 3:58 PM

br retitled this revision from to RISC-V kernel.

br updated this object.

br edited the test plan for this revision. (Show Details)

br added reviewers: andrew, emaste.

emaste added inline comments.Jan 18 2016, 4:08 PM

sys/sys/elf_common.h
900–902 ↗	(On Diff #12422)	Please put these in the right spot in alpha order, and you could commit this file independently.

UART changes removed (they looks wrong and we don't need them)
ELF relocation types committed

First pass, I haven't looked over pmap yet.

sys/dev/fdt/fdt_riscv.c
51 ↗	(On Diff #12422)	We should limit the code that uses this to existing architectures. I should have done it with arm64, but adding riscv would also be a good time to do it. As far as I can tell only arm and powerpc use it, and the arm one is broken if we were to ever get a GENERIC kernel.
sys/riscv/conf/GENERIC
102 ↗	(On Diff #12422)	I've purposely disallowed this from arm64 so we can have a GENERIC kernel, I would strongly suggest you do so with riscv if at all possible before 11 is branched, e.g. with a simple loader shim.
sys/riscv/htif/htif.c
108 ↗	(On Diff #12422)	Why `0xFF`?
141 ↗	(On Diff #12422)	Do you need `M_NOWAIT` here? the malloc may fail with it set. If you can sleep you cna change it to `M_WAITOK`.
144 ↗	(On Diff #12422)	And here.
167 ↗	(On Diff #12422)	What are these magic numbers?
sys/riscv/htif/htif_block.c
103 ↗	(On Diff #12422)	Why does this need to be packed?
sys/riscv/include/riscvreg.h
54–61 ↗	(On Diff #12422)	You could just commit this sort of thing.
sys/riscv/riscv/copyinout.S
58 ↗	(On Diff #12422)	You might want to look at the changes @kib made to the arm64 version of these functions to check if the address was valid, i.e. is it below `VM_MAXUSER_ADDRESS`.
sys/riscv/riscv/copystr.c
58 ↗	(On Diff #12422)	This should return `error` (it seems I made this mistake in arm64).
sys/riscv/riscv/cpufunc_asm.S
71 ↗	(On Diff #12422)	`XXXRISCV: Implement`?
sys/riscv/riscv/elf_machdep.c
89 ↗	(On Diff #12422)	This seems to be missing `.sv_trap = NULL` added in rS293613
sys/riscv/riscv/machdep.c
575–581 ↗	(On Diff #12422)	Is this needed?
682 ↗	(On Diff #12422)	I strongly suggest using a loader to do this for you.
sys/riscv/riscv/nexus.c
367 ↗	(On Diff #12422)	Are you expecting to support non-FDT? If not this can be merged into the above nexus driver.
422 ↗	(On Diff #12422)	Are you expecting to need to support ACPI?
sys/riscv/riscv/support.S
78 ↗	(On Diff #12422)	You should return 0 on success in `casueword*`
sys/riscv/riscv/swtch.S
247–252 ↗	(On Diff #12422)	On arm64 we rely on eret loading a the program state register with interrupts enabled, can you do something similar here?

kib added inline comments.Jan 19 2016, 3:26 AM

sys/riscv/riscv/elf_machdep.c
90 ↗	(On Diff #12428)	This would be only formal.
sys/riscv/riscv/exception.S
161 ↗	(On Diff #12428)	Indeed disabling interrupts while checking for new ASTs is important to not return to usermode with asts pending. The later is very undesirable.
sys/riscv/riscv/machdep.c
397 ↗	(On Diff #12428)	Why do you need this local variable ?
448 ↗	(On Diff #12428)	Why is this comment there ? It should be done. Is the #if 0 block below about the stated action ? It is rather critical issue.
457 ↗	(On Diff #12428)	Signature of set_mcontext() assumes that error is returned from the function. Why the error is not checked (I think it is worth keep it right despite current set_mcontext() does not return any error) ?
518 ↗	(On Diff #12428)	Really ?
554 ↗	(On Diff #12428)	Do not use PS_STRINGS, use p->p_sysent->sv_psstrings.
sys/riscv/riscv/mem.c
114 ↗	(On Diff #12428)	If anything was copyied, returned error must be zero. Look at the very end of amd64 memrw().

br updated this revision to Diff 12520.Jan 20 2016, 3:07 PM

br edited edge metadata.

br marked 22 inline comments as done.

br marked an inline comment as done.Jan 20 2016, 3:17 PM

br added inline comments.

sys/dev/fdt/fdt_riscv.c
52 ↗	(On Diff #12428)	what we need to do with that? ofw code uses this
sys/riscv/conf/GENERIC
103 ↗	(On Diff #12520)	sure! but we don't have loader yet :-(
sys/riscv/riscv/machdep.c
683 ↗	(On Diff #12520)	we don't have loader yet :-(
sys/riscv/riscv/swtch.S
248–253 ↗	(On Diff #12520)	Yes, so we now disabling interrupts in early start of fork_trampoline. Interrupts will be enabled because we have SSTATUS_PIE set in cpu_fork(). (IE will be set to PIE on eret instruction)

emaste added inline comments.Jan 20 2016, 4:18 PM

sys/riscv/conf/GENERIC
103 ↗	(On Diff #12520)	I think it's fine initially, but please add a `RISCVTODO` and a note that this needs to be done via loader instead when it's available.
sys/riscv/riscv/machdep.c
683 ↗	(On Diff #12520)	Same comment as earlier, it would be useful to have a `RISCVTODO: remove once we have loader` or similar

I do not have more comments. OTOH, I do not know the arch.

andrew added inline comments.Jan 21 2016, 12:36 PM

sys/riscv/htif/htif.c
139 ↗	(On Diff #12520)	It feels like this should be `struct htif_dev_ivars`, then implement `bus_read_ivar` to read the data in the child driver.
145 ↗	(On Diff #12520)	Will `id` always point to a string at least `HTIF_ID_LEN` bytes long?
sys/riscv/htif/htif_console.c
141 ↗	(On Diff #12520)	What is it preventing from optimising? i.e. what does the following do?
sys/riscv/riscv/bus_machdep.c
147 ↗	(On Diff #12520)	Is this needed?
sys/riscv/riscv/nexus.c
364 ↗	(On Diff #12520)	You should move this abovr `nexus_attach`. There is also no need to check for `OF_peer(0) == 0` as you only have a single nexus attachment.
378 ↗	(On Diff #12520)	You could move this into `nexus_attach` and remove this function.
sys/riscv/riscv/timer.c
183 ↗	(On Diff #12520)	Are these `#ifdef FDT` checks needed given we expect to only use fdt?

br updated this revision to Diff 12555.Jan 21 2016, 3:27 PM

br updated this revision to Diff 12557.

br marked 14 inline comments as done.

br added inline comments.Jan 21 2016, 3:31 PM

sys/riscv/htif/htif.c
145 ↗	(On Diff #12520)	yes, we declare it like that: char id[HTIF_ID_LEN] __aligned(HTIF_ALIGN);
sys/riscv/htif/htif_console.c
141 ↗	(On Diff #12520)	oops. It looks we don't need that. So we just clearing "interrupt received" bit, then pushing a char, and waiting for an "interrupt received" bit.

fdt_fixup_table removed

br marked an inline comment as done.Jan 21 2016, 5:07 PM

emaste added inline comments.Jan 21 2016, 5:11 PM

sys/conf/Makefile.riscv
9–10 ↗	(On Diff #12563)	Is this part true?

br added inline comments.Jan 21 2016, 5:20 PM

sys/conf/Makefile.riscv
9–10 ↗	(On Diff #12563)	No, this makefile is copy pasted from arm64

br added a reviewer: jhb.Jan 25 2016, 11:03 AM

br added a reviewer: imp.Jan 25 2016, 5:12 PM

Share first level of pagetables instead of splitting sptbr (supervisor page table base register)

jhb added inline comments.Jan 28 2016, 3:33 PM

sys/riscv/riscv/nexus.c
387 ↗	(On Diff #12793)	Probably there will be ACPI, yes. It is not yet settled, but some folks are pushing for that over FDT (HP already has a port of UEFI to RISC-V).
sys/riscv/riscv/pmap.c
3097 ↗	(On Diff #12793)	So does this mean you have a single top-level page table and you change an entry in it to refer to a new user l1 page table on each context switch? That's fine for now for UP, but for SMP you will need to do something different. One option would be to have per-CPU l0 tables. On other architectures we have a per-process l0 table that all point to the same shared l1 tables.

br marked an inline comment as done.Jan 28 2016, 3:59 PM

br added inline comments.

sys/riscv/riscv/pmap.c
3097 ↗	(On Diff #12793)	Yes, we just change an entry in L0 on each context switch. Ok, I plan to work on SMP very soon, so will see. thanks!

Mark TODO

br marked 6 inline comments as done.Jan 29 2016, 10:59 AM

Closed by commit rS295041: Welcome the RISC-V 64-bit kernel. (authored by br). · Explain WhyJan 29 2016, 3:13 PM

This revision was automatically updated to reflect the committed changes.

imp added inline comments.Jan 30 2016, 3:22 AM

head/sys/riscv/conf/DEFAULTS
9–13	These likely belong in GENERIC. They are bogusly in all the other system's DEFAULTS however :(
head/sys/riscv/conf/GENERIC
88	Is this really needed? It keeps GENERIC from being generic...
101–104	Things like this prevent GENERIC from being generic.

jhb added inline comments.Jan 30 2016, 3:51 AM

head/sys/riscv/conf/GENERIC
101–104	There isn't a /boot/loader yet I believe (hence the comment). Probably the ROOTDEVNAME option should just move down here for now. Alternatively, we could have a "GENERIC" config that is generic, and then have a SPIKE config that includes GENERIC and adds the options needed to boot a kernel under spike. (We have done this in the past with a SIMICS config for Alpha and a SKI config for ia64 which were both for use with simulators.)

RISC-V kernelClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 12823

head/sys/boot/fdt/dts/riscv/spike.dts

head/sys/boot/ficl/riscv/sysdep.h

head/sys/boot/ficl/riscv/sysdep.c

head/sys/cddl/compat/opensolaris/sys/atomic.h

head/sys/cddl/contrib/opensolaris/uts/common/sys/isa_defs.h

head/sys/conf/Makefile.riscv

head/sys/conf/files.riscv

head/sys/conf/kern.mk

head/sys/conf/kern.pre.mk

head/sys/conf/ldscript.riscv

head/sys/conf/options.riscv

head/sys/dev/hwpmc/hwpmc_riscv.h

head/sys/riscv/conf/DEFAULTS

head/sys/riscv/conf/GENERIC

head/sys/riscv/htif/htif.h

head/sys/riscv/htif/htif.c

head/sys/riscv/htif/htif_block.c

head/sys/riscv/htif/htif_console.c

head/sys/riscv/riscv/autoconf.c

head/sys/riscv/riscv/bcopy.c

head/sys/riscv/riscv/bus_machdep.c

head/sys/riscv/riscv/busdma_machdep.c

head/sys/riscv/riscv/clock.c

head/sys/riscv/riscv/copyinout.S

head/sys/riscv/riscv/copystr.c

head/sys/riscv/riscv/cpufunc_asm.S

head/sys/riscv/riscv/devmap.c

head/sys/riscv/riscv/dump_machdep.c

head/sys/riscv/riscv/elf_machdep.c

head/sys/riscv/riscv/exception.S

head/sys/riscv/riscv/genassym.c

head/sys/riscv/riscv/identcpu.c

head/sys/riscv/riscv/in_cksum.c

head/sys/riscv/riscv/intr_machdep.c

head/sys/riscv/riscv/locore.S

head/sys/riscv/riscv/machdep.c

head/sys/riscv/riscv/mem.c

head/sys/riscv/riscv/minidump_machdep.c

head/sys/riscv/riscv/nexus.c

head/sys/riscv/riscv/pmap.c

head/sys/riscv/riscv/support.S

head/sys/riscv/riscv/swtch.S

head/sys/riscv/riscv/sys_machdep.c

head/sys/riscv/riscv/timer.c

head/sys/riscv/riscv/trap.c

head/sys/riscv/riscv/uio_machdep.c

head/sys/riscv/riscv/uma_machdep.c

head/sys/riscv/riscv/vm_machdep.c

head/sys/sys/cdefs.h

head/sys/sys/kerneldump.h

RISC-V kernel
ClosedPublic
Actions

Revision Contents
Changeset List