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

Initial memop syscall implementation + tests #554

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Conversation

hudson-ayers
Copy link
Contributor

@hudson-ayers hudson-ayers commented Jul 22, 2024

This PR adds memop syscalls corresponding to memop 0, 1, 2, 10, and 11 to libtock-rs. Any instances where the underlying raw system calls were being used to call memop are migrated to the safe higher level syscall APIs as possible. This PR also adds a function to libtock-runtime for retrieving the value of _heap_start as set by the linker.

This PR also builds out some initial infrastructure for unit tests to include the memop syscall, and some tests of the system call implementation itself. This was definitely the trickier part of this PR. Because many of these memop APIs need to accept pointers for argument0, and because the unit tests include Miri's strict provenance tests, I modified the expected syscall type and the syscall log type to use the "Register" type for memop argument0, enabling tests to pass pointers to actual buffers without Miri complaining or without tests failing on 64 bit systems. Another tricky aspect of getting these tests right is that memop is the only system call which uses two different underlying raw system calls (syscall1 and syscall2 in this case). That makes the system call logging etc. a little weird -- I chose to still use a single type, and just follow a convention where I always pass 0 as the second argument when testing any memop calls that will use syscall1 under the hood, but I am open to suggestions for improvement there as well.

This PR has not yet been tested on hardware, but I wanted to go ahead and open this PR so I could get feedback on the unit test infrastructure. I will test on hardware shortly when I am back in the bay area.

@hudson-ayers
Copy link
Contributor Author

I tested this on nrf52840dk and the console example runs fine and the fault output looks right if I fault it using the process console:

 ╔═══════════╤══════════════════════════════════════════╗
 ║  Address  │ Region Name    Used | Allocated (bytes)  ║
 ╚0x20011000═╪══════════════════════════════════════════╝
             │ Grant Ptrs      120
             │ Upcalls         320
             │ Process         668
  0x20010BAC ┼───────────────────────────────────────────
             │ ▼ Grant          76
  0x20010B60 ┼───────────────────────────────────────────
             │ Unused
  0x20010100 ┼───────────────────────────────────────────
             │ ▲ Heap            0 |   2656               S
  0x20010100 ┼─────────────────────────────────────────── R
             │ Data              0 |      0               A
  0x20010100 ┼─────────────────────────────────────────── M
             │ ▼ Stack         184 |    256
  0x20010048 ┼───────────────────────────────────────────
             │ Unused
  0x20010000 ┴───────────────────────────────────────────
             .....
  0x00040800 ┬─────────────────────────────────────────── F
             │ App Flash      1920                        L
  0x00040080 ┼─────────────────────────────────────────── A
             │ Protected       128                        S
  0x00040000 ┴─────────────────────────────────────────── H

alistair23
alistair23 previously approved these changes Jul 24, 2024
Copy link
Collaborator

@alistair23 alistair23 left a comment

Choose a reason for hiding this comment

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

Awesome!

Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

Given the safety requirements for memop_brk and sbrk, is there an argument to be made we should also have memop_pbrk (I don't know if p is good, but "positive" brk)? I know it's not conventional, but the existing conventions weren't designed for Rust. I'm not sure how brk is going to be used in libtock-rs, but we aren't linking with newlib, and it seems like we typically allocate and not deallocate.

Or could pbrk potentially be unsafe too?

Comment on lines +92 to +93
let _ = TockSyscalls::memop_debug_stack_start(rt_header.stack_top as *const u8);
let _ = TockSyscalls::memop_debug_heap_start(rt_header.initial_break as *const u8);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let _ = TockSyscalls::memop_debug_stack_start(rt_header.stack_top as *const u8);
let _ = TockSyscalls::memop_debug_heap_start(rt_header.initial_break as *const u8);
let _ = TockSyscalls::memop_debug_stack_start(rt_header.stack_top.into());
let _ = TockSyscalls::memop_debug_heap_start(rt_header.initial_break.into());

Does it not work to keep those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error[E0277]: the trait bound `*const u8: From<*mut ()>` is not satisfied
  --> runtime/src/startup/mod.rs:92:75
   |
92 |         let _ = TockSyscalls::memop_debug_stack_start(rt_header.stack_top.into());
   |                                                                           ^^^^ the trait `From<*mut ()>` is not implemented for `*const u8`
   |
   = help: the following other types implement trait `From<T>`:
             <u8 as From<bool>>
             <u8 as From<NonZeroU8>>
   = note: required for `*mut ()` to implement `Into<*const u8>`

It does not work because of the intermediate type that was introduced by my helper function

@hudson-ayers
Copy link
Contributor Author

Given the safety requirements for memop_brk and sbrk, is there an argument to be made we should also have memop_pbrk (I don't know if p is good, but "positive" brk)? I know it's not conventional, but the existing conventions weren't designed for Rust. I'm not sure how brk is going to be used in libtock-rs, but we aren't linking with newlib, and it seems like we typically allocate and not deallocate.

Or could pbrk potentially be unsafe too?

I believe pbrk would be safe -- we had a similar function in libtock-rs 1.0. I think that is probably a good idea to add, actually, though maybe with a different name. I will get to that soon

@hudson-ayers
Copy link
Contributor Author

Added a safe memop_increment_brk() and one test for it.

@alevy alevy added this pull request to the merge queue Aug 5, 2024
Merged via the queue into master with commit 1dc6133 Aug 5, 2024
3 checks passed
@alevy alevy deleted the hudson.ayers/alloc branch August 5, 2024 21:08
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.

4 participants