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

Introduce umode binary and map it in salus user space address. #178

Merged
merged 4 commits into from
Dec 13, 2022

Conversation

glg-rv
Copy link
Contributor

@glg-rv glg-rv commented Dec 9, 2022

This PR includes a umode and libuser library (taken from Dylan's branch). These are just stubs at the moment, but they do create a risc-v elf binary that can be used as a basis for future userspace work.

It also introduces riscv-elf. A super simple ELF library tailored to our needs, with no memory allocation and focused on loading elf. Care has been taken in checking the validity of file offset before accessing them.

It then proceed to extend HypMap to allow a new type of region to be loaded. Specifically, it's a region that needs to be allocated and populated with existing data.

With this patch, the ELF is loaded in userspace, and there's a copy of the whole elf for each CPU. Current abstraction can be changed though to share .text and .rodata across CPUs. Although the big batch of memory (.data and .stack) will always be per-cpu.

lds/umode.lds Outdated Show resolved Hide resolved
lds/umode.lds Outdated

.stack : ALIGN(4096) {
PROVIDE(_stack_start = .);
. += 4096;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 4kB going to be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No but can be changed quite easily. This is just for the stub.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, can we put some space between the bss and the stack so that we get a page fault when we inevitably hit that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per inevitably, once we get an actual binary doing something the linking would've been changed.

Updated. Got rid of MEMORY command as it influences the way ld finds the next available address when assigning a section to a memory region.

Now there's an empty 4k section between the end of BSS and stack.

riscv-elf/src/lib.rs Show resolved Hide resolved
Comment on lines 31 to 34
fn as_usize(&self) -> Option<usize> {
self.inner.try_into().ok()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're assuming 64-bit everywhere else in the module, so I don't really see a problem assuming usize == u64 here too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I went for paranoia in this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. as_usize() returns usize. (with a comment).

src/hyp_map.rs Outdated
}

impl HypMap {
/// Create a new hypervisor map from a hardware memory mem map.
pub fn new(mem_map: HwMemMap) -> HypMap {
let regions = mem_map
pub fn new(mem_map: HwMemMap, elf_map: ElfMap) -> HypMap {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/elf_map/user_map (or umode_map)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I have to chose (and not chose elf_map, i'll go for user_map. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to user_map.

src/hyp_map.rs Outdated
Comment on lines 108 to 109
// Data to be populated in the VA area
data: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this extra memcpy() from the ELF to this temporary buffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're describing my Thursday night. Originally I had kept the reference from 'elf to here. It is sane because afterall the elf is 'static. But turns out that at least I couldn't understand how to let Rust understand in a non convoluted way that there were to lifetimes here, one of the underlying data structure and the other of the user_map.

In general the structure needs to have a life time, the code becomes much much harder to follow and should essentially hold the ElfMap in it to keep the reference lifetime valid. Just over complication for a language that clearly doesn't like these sort of things.

This would also remove generality to the whole concept of a populated region.

In general, much more complicated code, to save a few kbs of RAM.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you had your lifetimes backwards in ElfMap::segments(). With that fixed, this becomes pretty straightforward, see https://github.com/abrestic-rivos/salus/tree/for-glg/user_mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whops. That's a few hours of my life I'll never get back. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to your code. Thanks again.

@@ -459,8 +449,34 @@ extern "C" fn kernel_init(hart_id: u64, fdt_addr: u64) {
let guest_phys_size = mem_map.regions().last().unwrap().end().bits()
- mem_map.regions().next().unwrap().base().bits();

// Parse the user-mode ELF containing the user-mode task.
let user_elf = include_bytes!("../target/riscv64gc-unknown-none-elf/release/umode");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this is gross but I don't immediately have a better suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is indefensible but also the best thing we can do now. Double-checked with @dgreid about this as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's just a hack until we figure out a better build system. I'm hoping to let zach and tom figure out blaze for hubris tasks then copy that.

@glg-rv
Copy link
Contributor Author

glg-rv commented Dec 10, 2022

Simplified HypMapPopulatedRegion. As segments are aligned to 4k we can eliminate the offset. This in turns makes everything simpler.

Copy link
Collaborator

@abrestic-rivos abrestic-rivos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Comment on lines 19 to 24
la t1, task_main
la ra, 1f
jr t1

j 1f
1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call task_main is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch. Had a note somewhere when I saw your patches to change this too. Thanks!

Fixed.

lds/umode.lds Outdated

.stack : ALIGN(4096) {
PROVIDE(_stack_start = .);
. += 4096;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, can we put some space between the bss and the stack so that we get a page fault when we inevitably hit that case?

This patch introduces stub userspace binary and library for salus.

Currently the code is compiled but unused.

Original code by Dylan, with minor changes from me.

Signed-off-by: Dylan Reid <[email protected]>
Signed-off-by: Gianluca Guida <[email protected]>
@glg-rv glg-rv force-pushed the topic/user_mode branch 2 times, most recently from ca212f7 to e8a264c Compare December 12, 2022 20:58
@glg-rv
Copy link
Contributor Author

glg-rv commented Dec 12, 2022

Added a fix to load Program Headers that have no data in file.

riscv-elf is a library to parse an elf file in memory in Rust. It is
esplicitly tailored to salus and riscv. Work has been done (mainly
through fuzzing) to check that the library can handle malformed ELF
functions.

It is a simple library focused on loading an elf. Its main use is to
return an `ElfSegment` iterator that mirrors the Program Header
section and its data content.

Signed-off-by: Gianluca Guida <[email protected]>
This patch allows to map the `umode` binary into salus address
space. More work might be needed for a better build system and
integration of the umode binary into salus.

Signed-off-by: Gianluca Guida <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants