Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial RISC-V support + HiFive board #1317

Merged
merged 3 commits into from Jun 11, 2019

Conversation

@bradjc
Copy link
Contributor

commented Jun 3, 2019

Pull Request Overview

Support for RISC-V has been progressing on the side for a while, and I've been realizing that trying to make a pull request that includes three different boards and support for a new architecture is not only going to be very difficult to review, but also very difficult to create. So instead, this is an attempt to start the process with some initial code that supports the HiFive1 rev A board.

This code is very similar to the riscv branch that has been around for a while. Some things (like better OpenOCD support) have improved in the meantime, and it includes some learnings from using a different chip (we cannot assume RISC-V cores have a PLIC, for example), but otherwise it doesn't include any new functionality from what was on that branch from a while ago. It also does not include any of the new context switching code. With this PR established or merged it will be a lot easier to see what "hidden" changes we had to make to get context switching to work.

Since the HiFive1a does not support User mode I removed all of the code related to supporting applications in main.rs.

One nice thing about this PR is that only one file actually changed, most of this patch is just adding code. The changed file is the kernel linker script which requires a few additions.

Testing Strategy

This pull request was tested by running the kernel on a HiFive1 board and verifying that it still prints the debug!() message in main.rs.

TODO or Help Wanted

A big question is how to integrate the improvements from others related to the HiFive1/E310 code. My preference would be to merge this PR and then use it as base for other improvements, but if other code is ready to go it might make sense to extend this PR or create a series of active PRs.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

I updated several READMEs, but I'm still not sure that I didn't miss anything.

Formatting

  • Ran make formatall.
Add initial RISC-V support
This commit is the first towards adding RISC-V support to Tock. It
includes enough code that a RISC-V core will boot and can handle
interrupts. It also includes basic support for the HiFive1 (a) board,
and some SiFive peripherals.

The HiFive1(a) board does not support User mode, so there is no support
for userland (yet). This simply allows kernel code to boot and run.

The SiFive peripherals are organized into their own folder because they
are shared among multiple SiFive cores.
@ppannuto
Copy link
Member

left a comment

This is super exciting!

In general, I think I agree with the path forward of merging this as a "base" RiscV support and then adding more manageable PRs along the way

arch/rv32i/src/lib.rs Show resolved Hide resolved
pub unsafe fn atomic<F, R>(f: F) -> R
where
F: FnOnce() -> R,
{

This comment has been minimized.

Copy link
@ppannuto

ppannuto Jun 3, 2019

Member

You can make the body of this unimplemented!(), which the compiler is aware of and can then warn folks if this gets used

This comment has been minimized.

Copy link
@bradjc

bradjc Jun 4, 2019

Author Contributor

Well it is called, but just doesn't really protect against anything.

boards/hifive1/Makefile Outdated Show resolved Hide resolved
* shouldn't have any effect.
*/
KEEP(*(.riscv.start));
KEEP(*(.riscv.trap));

This comment has been minimized.

Copy link
@ppannuto

ppannuto Jun 3, 2019

Member

This looks good -- perhaps we should consider renaming the vectors and irqs sections to arm.*

Not something that needs fixing in this PR though -- wish GH had a 'this is really a comment not something that needs addressing' option :/

This comment has been minimized.

Copy link
@phil-levis

phil-levis Jun 7, 2019

Collaborator

Agree with Pat, but more generally does it make sense to separate ARM kernel_layout and RISC-V kernel_layout?

@@ -261,6 +292,13 @@ SECTIONS
. = ALIGN(MPU_MIN_ALIGN);
*(.app_memory)
} > ram

/* Discard RISC-V relevant .eh_frame, we are not doing unwind on panic

This comment has been minimized.

Copy link
@ppannuto

ppannuto Jun 3, 2019

Member

I'm guessing this proved necessary due to size? This isn't something you necessarily want to do as it was will make a debugger less useful; unwind info isn't just used for runtime panics

This comment has been minimized.

Copy link
@bradjc

bradjc Jun 4, 2019

Author Contributor

I have no idea where that came from, probably rust-riscv I started from.

chips/e310x/src/chip.rs Outdated Show resolved Hide resolved
hifive1 updates
- use tock-rt0 functions
- remove old commented out code
- add more to readme.md
@bradjc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Updated after Pat's comments.

@bradjc bradjc referenced this pull request Jun 4, 2019
3 of 18 tasks complete
@phil-levis
Copy link
Collaborator

left a comment

I left a few comments. The only one that's substantive is the question about inline assembly.

// frame pointer (needed for closures to work in start_rust) and the global
// pointer. Then it calls _start_rust.
#[cfg(any(target_arch = "riscv32", target_arch = "riscv64"))]
global_asm!(

This comment has been minimized.

Copy link
@phil-levis

phil-levis Jun 7, 2019

Collaborator

A while ago we talked about migrating away from inline assembly and instead just relying on assembly, linked as C functions. IIRC this was because of the optimization tricks the compiler does such that the generated assembly may (unless you get all of the annotations right) not be what you wrote.

This comment has been minimized.

Copy link
@bradjc

bradjc Jun 8, 2019

Author Contributor

Ok, yes, this will be fixed in the next PR. It's fixed in the riscv2 branch.

@@ -0,0 +1,10 @@
MEMORY

This comment has been minimized.

Copy link
@phil-levis

phil-levis Jun 7, 2019

Collaborator

Can you reference where these values come from? I'm trying to understand why prog is 507M.

* shouldn't have any effect.
*/
KEEP(*(.riscv.start));
KEEP(*(.riscv.trap));

This comment has been minimized.

Copy link
@phil-levis

phil-levis Jun 7, 2019

Collaborator

Agree with Pat, but more generally does it make sense to separate ARM kernel_layout and RISC-V kernel_layout?

@@ -218,6 +230,25 @@ SECTIONS
{
. = ALIGN(4);
_srelocate = .;

/* The Global Pointer is used by the RISC-V architecture to provide

This comment has been minimized.

Copy link
@phil-levis

phil-levis Jun 7, 2019

Collaborator

Data past 4kB has to do something else (that's less efficient)? It seems like there are certain kernel structures we'll want to make sure are in this 4kB region, then.

}
}

impl hil::uart::Transmit<'a> for Uart<'a> {

This comment has been minimized.

Copy link
@phil-levis

phil-levis Jun 7, 2019

Collaborator

Any thoughts on the new UART HIL?

This comment has been minimized.

Copy link
@bradjc

bradjc Jun 10, 2019

Author Contributor

You know this was just a pretty mechanical translation from the old to the new, so I didn't really interact with the new HIL API. I did like it is more explicit that receive has not been implemented.

@bradjc bradjc merged commit 6942503 into master Jun 11, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@bors bors bot deleted the hifive1 branch Jun 11, 2019

@bradjc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

If there are any other comments I'm happy to address them, but the consensus was to merge riscv code aggressively so here we go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.