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

RISC-V: Add context switching #1323

Merged
merged 9 commits into from Aug 2, 2019

Conversation

@bradjc
Copy link
Contributor

commented Jun 11, 2019

Pull Request Overview

This pull request implements the UserlandKernelBoundary trait for RISC-V platforms.

This PR is towards tracking issue #1135.

This depends on #1318 so those commits have been included here. That PR should be merged first, and then I can update this one.

While I pulled things together for the PR, @sv2bb did much of this.

Testing Strategy

This pull request was tested by running blink and c_hello on the the arty-e21 FPGA based board.

TODO or Help Wanted

See inline comments.

Documentation Updated

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

Formatting

  • Ran make formatall.
lw x30, 31*4(sp)
lw x31, 32*4(sp)
addi sp, sp, 33*4 // Reset kernel stack pointer

This comment has been minimized.

Copy link
@bradjc

bradjc Jun 11, 2019

Author Contributor

There is a lot of assembly here. At this point, however, with the kernel registers restored, we should be able to switch back to rust. The reason it is all in assembly is that the assembly needs the reference to state, but once I pass the &mut state to asm!() Rust complains whenever I try to use it again. If someone knows how to remedy this we can probably remove some of this assembly.

@@ -85,6 +85,7 @@ SECTIONS
* shouldn't have any effect.
*/
KEEP(*(.riscv.start));
. = ALIGN(64);
KEEP(*(.riscv.trap));

This comment has been minimized.

Copy link
@bradjc

bradjc Jun 11, 2019

Author Contributor

The trap handler must be aligned to a multiple of 64 bytes. With global_asm!() this could be inserted in the assembly for the start trap function. However, using functions with asm!() I think the only place to add this is here. Does anyone know a better way?

This comment has been minimized.

Copy link
@ppannuto

ppannuto Jul 23, 2019

Member

To remove the padding on boards without this, this works:

        . = DEFINED(_start_trap) ? ALIGN(64) : ALIGN(1);

Though whether it's worth the worst-case 63 bytes, I'm not sure I care as much

@bradjc bradjc referenced this pull request Jun 11, 2019
3 of 18 tasks complete
@niklasad1

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@bradjc Maybe co-author this PR then?

bradjc and others added 4 commits Jun 11, 2019
sifive: add clock speed as argument to uart
Obviously setting the baud rate for UART depends on the clock speed.
Make this a parameter because different boards use different settings.
We probably want a more formal way to do this, but this is simple and
works for now.
risc-v: add e21 core and context switching
Co-authored-by: Samyukta Venkat <sv2bb@virginia.edu>

@bradjc bradjc force-pushed the riscv-pr branch from cc36f23 to 5414ac4 Jul 22, 2019

@bradjc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

OK anyone want to take a look at this? I think we should merge quickly so we can work on #1324 more easily, and get this in for 1.4 release testing.

@alevy

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Maybe @DieracDelta can take a look?

@bradjc can you maybe add a link in the PR to whatever RISC-V reference is appropriate for this?

@alevy

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

All that said, I agree we should merge this reasonably promptly, since we decided we're allowing RISC-V to move fast for now as long as it's changes that don't interfere with other platforms.

@ppannuto
Copy link
Member

left a comment

+1 to generally staying out of the way of self-contained RISC-V stuff for now, mostly looks good to me, a few comments but nothing I would hard-block on.

Final thought, maybe a README or something in the cores directory explaining what this beast is:

boards/arty-e21/core/sifive_coreip_E21_FPGA_Evaluation_v19_02_rc0.mcs

?

arch/rv32i/src/clic.rs Outdated Show resolved Hide resolved
@@ -85,6 +85,7 @@ SECTIONS
* shouldn't have any effect.
*/
KEEP(*(.riscv.start));
. = ALIGN(64);
KEEP(*(.riscv.trap));

This comment has been minimized.

Copy link
@ppannuto

ppannuto Jul 23, 2019

Member

To remove the padding on boards without this, this works:

        . = DEFINED(_start_trap) ? ALIGN(64) : ALIGN(1);

Though whether it's worth the worst-case 63 bytes, I'm not sure I care as much

arch/rv32i/src/clic.rs Outdated Show resolved Hide resolved
arch/rv32i/src/clic.rs Outdated Show resolved Hide resolved
arch/rv32i/src/clic.rs Outdated Show resolved Hide resolved
arch/rv32i/src/clic.rs Outdated Show resolved Hide resolved
arch/rv32i/src/clic.rs Outdated Show resolved Hide resolved
arch/rv32i/src/clic.rs Outdated Show resolved Hide resolved
bradjc and others added 2 commits Jul 25, 2019
riscv: Apply suggestions from code review
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@bradjc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Any other comments?

}
}
1 => kernel::syscall::ContextSwitchReason::Interrupted,
2 => kernel::syscall::ContextSwitchReason::Fault,

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Jul 31, 2019

Member

this seems redundant, would be caught by wildcard

pub struct SysCall();

impl SysCall {
pub const unsafe fn new() -> SysCall {

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Jul 31, 2019

Member

unnecessary unsafe, alternatively add some docs which explains why it is marked as unsafe

This comment has been minimized.

Copy link
@alevy

alevy Jul 31, 2019

Member

Agree that this is only meaningful if SysCall cannot otherwise be easily instatiated by anyone (which I believe this unsafe is meant to protect)

This comment has been minimized.

Copy link
@bradjc

bradjc Aug 1, 2019

Author Contributor

I think this is either a copy-and-paste artifact, or the unsafe was needed at some point during the development, and the compiler only tells you when it is necessary to add it.

}

/// Implementation of the `UserspaceKernelBoundary` for the RISC-V architecture.
pub struct SysCall();

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Jul 31, 2019

Member

I would prefer pub struct SysCall; over empty unit struct but it is ok.

This comment has been minimized.

Copy link
@alevy

alevy Jul 31, 2019

Member

Actually, I think none of the above (including what’s in the code) is what we want. All of these options allow anyone to construct a SysCall since all of it’s fields (of which there are none) are public.

SysCall needs to be a singleton (I think?), and should only be insatiable through the unsafe fn new() function.

The declaration should, therefore, be:

pub struct SysCall( () ); // Can elide spaces

Which would result in an error if someone outside of this module tries to create one. Unlike:

pub struct SysCall(pub ());

Which can be instantiated by anyone.

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Jul 31, 2019

Member

Right, if it needs to be a singleton then it makes sense.

This comment has been minimized.

Copy link
@bradjc

bradjc Aug 1, 2019

Author Contributor

I think I updated this correctly, let me know if not.

boards/arty-e21/src/io.rs Outdated Show resolved Hide resolved
arch/rv32i/src/syscall.rs Outdated Show resolved Hide resolved
arch/rv32i/src/syscall.rs Outdated Show resolved Hide resolved
}

impl ArtyExx {
pub unsafe fn new() -> ArtyExx {

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Jul 31, 2019

Member

unsafe block could be removed if you agree with my comment in SysCall impl

@niklasad1
Copy link
Member

left a comment

This is cool, LGTM overall but I haven't read the assembly stuff...

Added some inline comments mainly rust style stuff

@bradjc bradjc dismissed stale reviews from niklasad1 and alevy via 518015e Jul 31, 2019

pub struct SysCall(());

impl SysCall {
pub const fn new() -> SysCall {

This comment has been minimized.

Copy link
@alevy

alevy Aug 1, 2019

Member

Is it, or is it not safe to have multiple instances of SysCall? If it is, than this is fine, but the definition of SysCall doesn't need to have any extra parens or unit types, just a plain pub struct SysCall; is good enough.

If it does, then this has to be marked unsafe.

This comment has been minimized.

Copy link
@alevy

alevy Aug 1, 2019

Member

This is a discrepancy we have in the cortex-m variants as well (where this was likely copied from). Let's decide here and fix in the cortex-m in a different PR.

This comment has been minimized.

Copy link
@bradjc

bradjc Aug 1, 2019

Author Contributor

It certainly doesn't make a lot of sense to have multiple SysCall objects, but I'm not sure if it is strictly safe or not. Since everything is handled on the stack it might be safe.

This comment has been minimized.

Copy link
@alevy

alevy Aug 1, 2019

Member

We typically think of it as potentially unsafe when we have might end up with two instances that use the same set of hardware registers, so that they are not actually isolated instances...

I think in this particular case, I think switch_to_process is what should motivate us to ensure SysCall is a singleton and keep new unsafe: It's operating on a shared hardware variables, such as mscratch, mepc etc...

This comment has been minimized.

Copy link
@bradjc

bradjc Aug 1, 2019

Author Contributor

I agree.

@alevy
alevy approved these changes Aug 2, 2019
@bradjc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

bors r+

bors bot added a commit that referenced this pull request Aug 2, 2019
Merge #1323
1323: RISC-V: Add context switching r=bradjc a=bradjc

### Pull Request Overview

This pull request implements the `UserlandKernelBoundary` trait for RISC-V platforms.

This PR is towards tracking issue #1135.

This depends on #1318 so those commits have been included here. That PR should be merged first, and then I can update this one.

While I pulled things together for the PR, @sv2bb did much of this.

### Testing Strategy

This pull request was tested by running blink and c_hello on the the arty-e21 FPGA based board.


### TODO or Help Wanted

See inline comments.


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@bors bors bot merged commit 25e64fc into master Aug 2, 2019

4 checks passed

bors Build succeeded
Details
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 riscv-pr branch Aug 2, 2019

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