@ia0 this structure for a board/chip-specific driver is exactly right!

Because this requires a change to the kernel crate, and uses it in a way that is contrary to the typical hierarchies of layers (typically chip crates aren't really "aware" of applications), I'm going to think through whether this can be done differently.

My initial intuition is not without a more substantial change, in which case I think this design is good, especially if we're willing to mark this new kernel function as experimental.

Will respond again soon (and please feel free to ping if soon turns into not so soon)

Because this requires a change to the kernel crate, and uses it in a way that is contrary to the typical hierarchies of layers (typically chip crates aren't really "aware" of applications)

I see. I agree this is weird for chips to know about applications. I didn't realize it.

Here are some possible alternatives:

• The chip-specific driver does not check if the application is attempting to write outside the flash regions that the application is allowed to write. The check would be done by libtock-rs using memop syscalls to know the writeable flash regions. This is very unsafe and relies on the application using the abstractions built in libtock-rs.
• Have a writeable flash allow syscall, similar to the possible read-only allow syscall of [RFC] Create a read-only-allow #1274. The syscall would check that the slice is indeed in the writeable flash region of the application. This solution looks fine but probably requires to revamp the allow syscall interface.

I'll also continue thinking about it on my side and post any other solutions that would come to mind here.

It seems like it would be good to have the Driver implementation in a capsule so that other chips/boards could use it. Also, why the flexibility to implement a driver in a chip crate is nice from an interface design point of view, it starts to make the code base much more difficult to understand, particularly for people new to Tock.

AFAIU, having the Driver in a capsule would require a HIL. For that, we have at least 3 choices:

1. Create a chip-specific HIL. Not sure it's fine with Tock policies.
2. Extend the existing flash HIL (see RFC: Flash HIL improvements #1459). I think this is too early to take such decision. We need more flash experience.
3. Add an advanced flash HIL (as you suggest here). I guess it would fit to the "chip-agnostic" drivers (2nd point in doc/Design.md: Add 'Some in-kernel design principles` section #1466 under "Role of HILs"), although it would not be supported by all chips. I can write such PR if you think it makes sense.

Regarding my previous comment about alternatives for this PR, I think there would be a 3rd alternative. If we want chips to define syscall drivers but not expose the application, then we need a new trait. That trait would implement kernel::Driver and possibly expose a restricted set of application-related functions. For example:

trait ChipDriver {
    fn subscribe(&self, minor: usize, callback: Option<Callback>, app: RestrictedAppId) -> ReturnCode;
    fn command(&self, minor: usize, r2: usize, r3: usize, app: RestrictedAppId) -> ReturnCode;
    fn allow(...);
}
impl<T: ChipDriver> Driver for T { ... }
impl RestrictedAppId {
    fn in_writeable_flash_region(...) -> bool;
}

So the 3 alternatives to have chip-specific drivers would be:

1. Do not provide AppId to chip drivers. Some of those would then be unsafe.
2. Add support for allow types: ReadOnly (the one from Add USB register mapping (for nRF52840-DK) to chips/nrf52. #1427), ReadWriteMemory (the current one), ReadWriteFlash (the one from this issue). Each would check that the application has correct permission. For ReadWriteFlash it would be writeable flash region and writing to such slice should be done with a flash driver (instead of writing directly to the slice).
3. Restrict the "app" argument of chip-specific syscalls to only provide functions that make sense for chips (not sure if in_writeable_flash_region would be one of them).

Hi @alevy,

Do you have any updates on how to fix the fact that chip drivers should not be aware of applications?

As I see it, a new trait is necessary to hide the AppId argument of kernel::Driver. Then, to provide safety regarding slices to writeable flash regions, it seems that this could be a new concept similar to the read-only slices (see #1274). Slices from users would be partitioned depending on whether they are in flash or memory, and whether they are expected to be read or written.

I believe this is blocked on the outcome of the discussion on #1459

This PR is only blocked by a design question raised by @alevy : How to implement chip drivers that would like to ensure an allow slice is in the writeable flash regions of the application? See #1467 (comment) for the original concern.

This PR is not blocked by #1459. It's actually the opposite, this PR will provide data to be able to take a decision on that issue.

Hi @alevy,

Did you have time to look into how chip-specific drivers can require a slice to be in writable flash region without breaking the typical hierarchies of layers?

I think where this is at is:

• We aren't quite comfortable allowing chips to implement Driver. That would be a pretty significant change in Tock's layers.
• The flash HIL is relatively high level, and restricts very low level access to the hardware.
• Having a capsule that allows for apps that know what they are doing to edit flash directly is a good idea.

Thanks @bradjc for the feedback!

The flash HIL is relatively high level, and restricts very low level access to the hardware.

Yes, #1459 is an effort to either extend the current flash HIL with low-level operations or add a new low-level flash HIL.

Having a capsule that allows for apps that know what they are doing to edit flash directly is a good idea.

That sounds great! How do we proceed? As far as I understand we would need some HIL at some point, which seems to require that #1459 gets fixed. Or do you see an alternative way? (like maybe having a HIL for a family of flash controllers like in #1457)

Hi @alevy,

I updated the pull request to not check whether the syscall applies to the writable flash regions of the application, because this can now be done in the platform using filter_syscall (see #1350).

The only usage of appid from the driver is to access the grant. Is that still an issue regarding the hierarchy of layers? Or would the PR in the current state satisfy Tock design principles?

Thanks!

I don't quite follow how this actually going to be used in conjuction with board-specific policies. In particular, it's important both that the AppSlice passed into this driver's allow is able to reside in flash (which is the kernel will refuse, by default) and that it's not in RAM (since attempts to use the flash controller to write to memory will probably fail?).

There's probably a misunderstanding on how the driver works:

• The allowed slice is only used to read from. It contains the source data that needs to be copied from.
• The destination slice is provided as location in flash (address + length) in the command. This is where the allowed slice is copied to.
• The platform checks that the flash location is writable by the application.
• The allowed slice is checked to be readable by the application using usual Tock checks (AFAIU should be in application RAM).

Note that due to #1274 it is not possible to copy from flash to flash.

Since you've tested this, I trust you have a mechanism for this, I just don't totally follow it.

I would need to test again now that I merged with master, because we currently use an older version of Tock.

I could test that this PR works for our use-case (OpenSK) on a nRF52840 dev-kit board.

I think we want to keep Driver implementations in /capsules. However, I think this functionality can proceed in light of #2248 where a new (or additional) HIL will allow capsules to provide a low-level flash interface to userspace if so desired.

Thanks for the update. I'll follow #2248 to see if it can fit our use-case.