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 userspace IPC support #556

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Add userspace IPC support #556

wants to merge 10 commits into from

Conversation

pqcfox
Copy link

@pqcfox pqcfox commented Aug 13, 2024

Pull Request Overview

This pull request adds the following:

  • A userspace driver for IPC, exposing the same API that libtock-c does
  • A mock IPC SyscallDriver which allows mocking IPC API calls from different processes
  • Tests for both the mock IPC driver as well as the userspace API

Testing Strategy

For the actual SyscallDriver IPC mock, tests were added to ensure that the exists, discover, notify service, and notify client system calls were all functional, and returned errors when appropriate.

For the userspace API, test cases were added for testing that the driver is present, that IPC services can be discovered, that both service and client callbacks can be both registered and notified, and that a R/W buffer can be shared between IPC services and clients.

Documentation Updated

Documentation was added to both the mock IPC SyscallDriver as well as the userspace API in accordance with the existing mock drivers and userspace APIs respectively.

TODO or Help Wanted

Preliminary reviews are welcome at this point. I'm currently waiting on a nRF52840 dongle to arrive via mail (due by next Monday, 2024-08-19) in order to properly test the provided example, but make test completes successfully at the moment, and I'd love any feedback available on this PR as I wait for the dongle to arrive.

(EDIT 2024-08-13: I've noticed that the examples do need a quick couple of fixes re: how the IPC shared buffers work--will push those tomorrow, it may be wise to delay reviewing the examples themselves until then.)

@alistair23
Copy link
Collaborator

alistair23 commented Aug 13, 2024

Just a heads up there is already a draft PR for this, you can see it here: #535

This approach might be better (I haven't looked yet), but just wanted to point it out

@pqcfox
Copy link
Author

pqcfox commented Aug 13, 2024

Hi Alistair! Thank you so much, that was the one detail I was forgetting to mention last night! This PR was indeed based off the excellent work in #535, and uses the exact same approach illustrated there.

I had a chat over the past couple weeks with Amit regarding design decisions for this PR, and we both agreed that the dedicated subscribe_ipc function as in #535 which elides scoped handles in favor of static lifetimes indeed makes the most sense.

If it'd be useful, I'd be more than happy to rebase onto the development branch for #535--I initially pushed to a new branch because I was experimenting with various approaches, but think rebasing now could certainly make sense. My apologies for omitting this detail in the original PR!

@pqcfox
Copy link
Author

pqcfox commented Aug 13, 2024

One additional detail regarding the static lifetime approach--while it dramatically simplifies the lifetime management involved, it does necessitate static mutable buffers for IPC shared buffers, which naively require using unsafe to access.

I experimented with the ergonomics of a few other approaches, but ultimately decided the simplest was a single unsafe access by the client to pass a mutable reference to ipc_share--would love any thoughts on this decision, as it's the one I wanted feedback the most on.

Here are some approaches I tried for gating access to the underlying buffer shared between IPC processes:

  • spin::Mutex: adds code size overhead, external dependency as well
  • lazy_static!: requires spin when used as no_std, same issue
  • #[thread_local]: unstable feature, allows no_std but still requires unsafe for accessing thread-local statics, see rust-lang/rust #17954
  • tock_cells::take_cell::TakeCell: removes the need for unsafe when accessing the shared buffer, but is !Sync so can't be static
  • Box::leak(): works quite well, but adds a requirement on alloc (this seems like the most reasonable tradeoff--would love feedback if we think this is reasonable)

Would love any suggestions/thoughts on this! I understand that it's not quite ergonomically perfect to necessitate unsafe in Tock app code whenever IPC is present, so any suggested approaches are more than welcome.

/// wraps should be an actual function instead of a short-lived closure.
pub struct IpcListener<F: Fn(IpcCallData)>(pub F);

impl<F: Fn(IpcCallData)> Upcall<subscribe::AnyId> for IpcListener<F> {
Copy link

Choose a reason for hiding this comment

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

Any reason the callback Fn argument is a struct rather than two arguments? (e.g.: Fn(caller_id: u32, buffer: &Option<&mut [u8]>)

Copy link
Author

@pqcfox pqcfox Aug 15, 2024

Choose a reason for hiding this comment

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

I'd only put it in a struct to parallel the structure of some of the other drivers that register listeners, but in retrospect I agree having it just as Fn(caller_id: u32, buffer: &Option<&mut [u8]>) makes the most sense. Will add a commit to edit that :)

#[derive(Debug, Eq, PartialEq)]
pub struct IpcCallData<'a> {
pub caller_id: u32,
pub buffer: Option<&'a mut [u8]>,
Copy link

Choose a reason for hiding this comment

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

Should we consider the lifetime of this slice to be 'static?

My thoughts are that when one process shares a buffer with another via Ipc::share, it's effectively handing ownership over to the target processes. The sharing process needs some way of knowing when the target is done with the buffer and is handing it back.

Example:
A shares buf with B.
A notifies B.
B upcall is invoked with (A::id, buf). B now owns buf.
B performs some long running calculation on buf.
B notifies A.
A upcall is invoked with (B::id, None). A now owns buf again.

Copy link
Author

@pqcfox pqcfox Aug 15, 2024

Choose a reason for hiding this comment

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

Ooh, I think that's not a bad idea. I only instantiate it as IpcCallData<'static> in the userspace driver itself, but I think for extra clarity removing the generic lifetime and replacing it with 'static makes sense. Will also add this change with the previous one.

@alevy
Copy link
Member

alevy commented Aug 15, 2024

I'm currently waiting on a nRF52840 dongle to arrive via mail (due by next Monday, 2024-08-19

@lschuermann can we get @pqcfox access to (pre-alpha but probably functional enough) treadmill in the meantime?

@pqcfox FYI the nrf52840 dongle is really cool, but not very fun for actually programming since it doesn't have an on-board swd debugger, so you'll need a separate JLink jtag device, plus adapters, plus a tag connect cable. The development kit is much much more friendly for development.

@lschuermann
Copy link
Member

@lschuermann can we get @pqcfox access to (pre-alpha but probably functional enough) treadmill in the meantime?

Yes, I will reach out via Matrix in the coming days.

@pqcfox
Copy link
Author

pqcfox commented Aug 17, 2024

Thank you so much @alevy and @lschuermann! I'll order the development board as well in that case, and excited to try out treadmill :)

@HMiyaziwala
Copy link

HMiyaziwala commented Aug 26, 2024

Happy to share that this PR works well as is on our x86 port of Tock and Libtock-rs. Thank you for putting this together.
I am thinking about how to ergonomically gate access to the shared IPC buffer and am interested in any developments you find regarding that.

@pqcfox
Copy link
Author

pqcfox commented Aug 27, 2024

I'm so glad to hear! And of course--glad I can help out!

I just received access to treadmill last week, so I'll find a window in the next week or two to knock out fixing the example and testing on an actual board, at which point I'll move this from a draft PR to an actual PR.

In the meantime though, let me know about any thoughts you have on improving ergonomics--I've not had too much time to ponder it since, so I'd love any input.

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.

6 participants