Tock 2.0 IPC

[alevy]

Pull Request Overview

This pull request ports the IPC driver to the Tock 2.0 driver interface.

This is a mostly fairly straightforward port. However, the old driver API relied on allow to return a discovered IPC endpoint upon success, which is no longer possible in the 2.0 driver interface (allow cannot return a value).

Recall that IPC works as follows:

1. A client "discovers" an IPC service by supplying the kernel with the service's name (taken from the process's TBF name header)
2. If such a service exists, the kernel supplies the client process with a 32-bit non-zero descriptor to reference the service (implemented as the AppId + 1, but that's not guaranteed).
3. The client can use this descriptor to share memory with the service (using allow) and to notify it (using command).

For discovery, allow(IPC, 0) had been used to both share a buffer containing the service name and request that the kernel perform discovery, returning a descriptor on success.

This is no longer possible, and it also not canonical driver semantics.

With this PR, discovery is split into an allow_readonly (which is nice anyway so
package names can be hardcoded in ROM rather than copied to
memory) followed by a command(IPC, 1) to actually perform the discovery and return a descriptor.

To make this cleaner, I also rejiggerd the arguments in command, which now has to mean more than just notify.

Previously, the arguments were:

1. Descriptor (as opposed to the normal command number)
2. Service-or-client-notify (which is actually kind of like a command number)
3. Unused

Now, the arguments are:

1. Command number (can be "discover," "service notify," or "client notify")
2. Target descriptor
3. Unused

There are a number of other changes I believe could make sense to make to IPC before releasing 2.0, but I think they should be decoupled from this pull request, which merely adapts the interface very little to get it working. These are just ideas, and we should discuss all these separately:

• Allow both read-only and read-write buffer sharing between client/service
• Use borrow semantics for memory ownership
• Let client subscribe to be notified when a service is available
• Probably more...

Testing Strategy

Tested using the IPC tutorial in libtock-c (examples/tutorial/05-ipc) after applying changes from tock/libtock-c#176

TODO or Help Wanted

Still need to adapt libtock-c userspace to properly test. (Done in tock/libtock-c#176)

Still need to update documentation

Please review not only the implementation but the change in interface described above. As I said, there are a variety of changes that might make sense for IPC that go beyond these minimal changes. I ask that we get something in that reliably reproduces the existing high-level semantics for IPC and after (or maybe concurrently) decide whether other changes should happen (either before or after 2.0).

Documentation Updated

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

Formatting

• Ran make prepush.

[alevy]

Please do not merge yet, I have not tested properly and have not updated all the documentation. However, I believe it's more or less complete, so worth reviewing.

I've separated into two commits, one that changes the IPC driver and one (with a one-line change in a ton of files) that updates references to IPC in all boards using it. I suggest looking primarily at the first commit, which isn't actually that big.

[hudson-ayers]

This increases code size by enough that the kernel no longer fits into flash on Imix (hence the CI failure). Looking at hail, it increased size by 1264 bytes.

[alevy]

1264 bytes

I'll look into it, but that's got to be an alignment thing (e.g., it maybe increased by enough of a margin to just push it over the alignment boundary or something), right?

[hudson-ayers]

Looks mostly good to me, I have a few comments.

[hudson-ayers]

1264 bytes

I'll look into it, but that's got to be an alignment thing (e.g., it maybe increased by enough of a margin to just push it over the alignment boundary or something), right?

Using Phil's size tool, the code size required for functions in the kernel::ipc::IPC::* namespace went from using 1006 bytes before this PR to using 2392 bytes afterwards. I think the size tool counts code space wasted due to alignment separately. Unfortunately the tool does not provide any resolution on where the size is going within individual methods on the struct. The one place in the changes where it is clear size might have changed is command() -- before, the code for service notify and client notify was almost entirely shared, and now it is duplicated across the two match arms. Still it makes no sense how that could over double the code size used..

[phil-levis]

* Use borrow semantics for memory ownership

This is interesting; fundamentally the idea of this shared memory goes against Rust's ownership model, so does need to be protected by some sort of lock. If we do decide to follow the borrow semantics, though, this becomes message passing (except if it's read-only on both sides)?

[hudson-ayers]

This is interesting; fundamentally the idea of this shared memory goes against Rust's ownership model, so does need to be protected by some sort of lock. If we do decide to follow the borrow semantics, though, this becomes message passing (except if it's read-only on both sides)?

I suspect that the way we use shared memory now would be difficult to use without undefined behavior for a rust userspace? I realize libtock-rs does not currently support IPC.

I am not sure what exactly defines "message passing", but it seems that the ability to

• take a lock
• modify 1 byte of the shared memory
• release the lock

is lower overhead than having to pass the entire shared memory region back as a message. Maybe this is more of a semantic difference

[phil-levis]

This increases code size by enough that the kernel no longer fits into flash on Imix (hence the CI failure). Looking at hail, it increased size by 1264 bytes.

I've been banging up on this a bit with imix. We should do a spring cleaning and see where we could reclaim some space. ADC for example is 6.1k!

[alevy]

@phil-levis:

I've been banging up on this a bit with imix. We should do a spring cleaning and see where we could reclaim some space. ADC for example is 6.1k!

I totally agree, but also I'm not sure how reliable those numbers are at that granularity (the tool is great! don't get me wrong). For example, removing the nrf51 capsule from Hail adds ~1kB to the kernel, and the scheduler module occupies ~10kB on Hail, but only ~5kB on the Nano BLE, and ~8kB on the nrf52840DKs

The code changes really shouldn't require adding much code space at all, so I'll debug this enough to figure out what's going on (though again, removing things sometimes increases the size, so it's pretty brittle) and get imix inline.

But agree that we should do a deeper more comprehensive dive into this. These modules are all far bigger than they need to be.

[phil-levis]

the scheduler module occupies ~10kB on Hail, but only ~5kB on the Nano BLE, and ~8kB on the nrf52840DKs

I think this is inlining: modules higher in the call tree can take on code from lower ones. Happens less in the opposite direction.

[bradjc]

We should probably look at #1993 since we are here. What happens if add_mpu_region fails?

[alevy]

Please still don't merge, I'm currently testing with libtock-c and still need to update documentation

[bradjc]

I was playing around with IPC today and I found some issues. #1993 Might not be relevant for this PR but wanted to make a note of it.

[alevy]

I pushed a commit (which I will squash before we merge) that fixes a bug I had introduced in schedule_callback. Because the bug was subtle and hard to catch, I also updated the names of the arguments. While after testing I'm fairly certain that function is now correct, it would be very useful for someone to quickly review my changes (just in that last commit should be fine) and verify that what's going on in that function kind of actually makes sense.

[alevy]

Testing complete. Just going to update documentation, then I think we should be good (modulo squashing the last commit with the first one)

[alevy]

Documentation updated

[hudson-ayers]

The most recent commit looks correct to me, it seems to match the behavior from when expose_to() was used, and the updated names are helpful

[alevy]

Squashed

[alevy]

bors r+

[bors]

Build succeeded:

• ci-build (macos-latest)
• ci-build (ubuntu-latest)
• ci-format (macos-latest)
• ci-format (ubuntu-latest)
• ci-qemu
• ci-tests (macos-latest)
• ci-tests (ubuntu-latest)

[phil-levis]

Hurray!