Page MenuHomeFreeBSD

RISC-V kernel
ClosedPublic

Authored by br on Jan 18 2016, 3:58 PM.
Tags
None
Referenced Files
F93674779: D4982.diff
Wed, Sep 11, 1:47 PM
Unknown Object (File)
Wed, Sep 11, 8:21 AM
Unknown Object (File)
Sun, Sep 8, 11:21 PM
Unknown Object (File)
Sun, Sep 8, 6:36 PM
Unknown Object (File)
Sun, Sep 8, 5:57 PM
Unknown Object (File)
Sun, Sep 8, 2:58 PM
Unknown Object (File)
Sat, Sep 7, 2:15 PM
Unknown Object (File)
Thu, Sep 5, 4:26 AM
Subscribers

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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.
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
andrew added a subscriber: kib.

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
103

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
109

Why 0xFF?

142

Do you need M_NOWAIT here? the malloc may fail with it set. If you can sleep you cna change it to M_WAITOK.

145

And here.

168

What are these magic numbers?

sys/riscv/htif/htif_block.c
104

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
59

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
59

This should return error (it seems I made this mistake in arm64).

sys/riscv/riscv/cpufunc_asm.S
72

XXXRISCV: Implement?

sys/riscv/riscv/elf_machdep.c
90

This seems to be missing .sv_trap = NULL added in rS293613

sys/riscv/riscv/machdep.c
576–582

Is this needed?

683

I strongly suggest using a loader to do this for you.

sys/riscv/riscv/nexus.c
368

Are you expecting to support non-FDT? If not this can be merged into the above nexus driver.

423

Are you expecting to need to support ACPI?

sys/riscv/riscv/support.S
79

You should return 0 on success in casueword*

sys/riscv/riscv/swtch.S
248–253

On arm64 we rely on eret loading a the program state register with interrupts enabled, can you do something similar here?

sys/riscv/riscv/elf_machdep.c
91

This would be only formal.

sys/riscv/riscv/exception.S
162

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
398

Why do you need this local variable ?

449

Why is this comment there ? It should be done. Is the #if 0 block below about the stated action ? It is rather critical issue.

458

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) ?

519

Really ?

555

Do not use PS_STRINGS, use p->p_sysent->sv_psstrings.

sys/riscv/riscv/mem.c
115

If anything was copyied, returned error must be zero. Look at the very end of amd64 memrw().

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
104

sure! but we don't have loader yet :-(

sys/riscv/riscv/machdep.c
684

we don't have loader yet :-(

sys/riscv/riscv/swtch.S
249–254

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)

sys/riscv/conf/GENERIC
104

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
684

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.

sys/riscv/htif/htif.c
140

It feels like this should be struct htif_dev_ivars, then implement bus_read_ivar to read the data in the child driver.

146

Will id always point to a string at least HTIF_ID_LEN bytes long?

sys/riscv/htif/htif_console.c
142

What is it preventing from optimising? i.e. what does the following do?

sys/riscv/riscv/bus_machdep.c
148

Is this needed?

sys/riscv/riscv/nexus.c
365

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.

379

You could move this into nexus_attach and remove this function.

sys/riscv/riscv/timer.c
184

Are these #ifdef FDT checks needed given we expect to only use fdt?

br updated this revision to Diff 12557.
br marked 14 inline comments as done.
sys/riscv/htif/htif.c
146

yes, we declare it like that:
char id[HTIF_ID_LEN] __aligned(HTIF_ALIGN);

sys/riscv/htif/htif_console.c
142

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
sys/conf/Makefile.riscv
9–10

Is this part true?

sys/conf/Makefile.riscv
9–10

No, this makefile is copy pasted from arm64

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

sys/riscv/riscv/nexus.c
388

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
3098

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
3098

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!

br marked an inline comment as done.

Mark TODO

br marked 6 inline comments as done.Jan 29 2016, 10:59 AM
This revision was automatically updated to reflect the committed changes.
head/sys/riscv/conf/DEFAULTS
9–13 ↗(On Diff #12823)

These likely belong in GENERIC. They are bogusly in all the other system's DEFAULTS however :(

head/sys/riscv/conf/GENERIC
88 ↗(On Diff #12823)

Is this really needed? It keeps GENERIC from being generic...

101–104 ↗(On Diff #12823)

Things like this prevent GENERIC from being generic.

head/sys/riscv/conf/GENERIC
101–104 ↗(On Diff #12823)

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.)