Allow muxing I2C userspace driver and I2C Devices #3867

alistair23 · 2024-02-19T05:38:39Z

Pull Request Overview

To allow an I2C bus to support devices created by the board and still be exposed to userspace let's convert the I2CMasterDriver to use an I2CDevice instead of the physical I2C hardware.

This allows us to use the Mux with userspace applications and kernel uses.

Testing Strategy

Communicating with a I2C device from userspace while still supporting kernel devices.

TODO or Help Wanted

Documentation Updated

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

Formatting

Ran make prepush.

lschuermann

This makes sense to me, in particular also the part about I2CDevices being able to change their address at runtime (e.g., to support peripherals where the address is flashed into the device on initial configuration). I'm perhaps slightly concerned that this will make it easier to create address conflicts on a given bus (as the kernel can't know what userspace is doing on this bus), but presumably the kernel has some amount of trust towards the app when it gives it access to a shared I2C bus, and later changes could introduce some optional filtering infrastructure.

bradjc

I feel like this is digging us a deeper hole. I2CDevice is explicitly supposed to restrict a user to a single I2C device (or really a single I2C address).

I2C is an example of why virtualizing with a different type is problematic.

I think we need to add a different virtualization layer.

boards/components/src/i2c.rs

alistair23 · 2024-02-19T22:04:06Z

I feel like this is digging us a deeper hole. I2CDevice is explicitly supposed to restrict a user to a single I2C device (or really a single I2C address).

This doesn't change that.

If a board only creates an I2CDevice then that is all that is exposed to userspace. Userspace still can't change the address.

If a board designer wants to expose an I2CDevice and the I2C bus, this allows them to expose both. It also still allows only exposing the bus and no I2CDevice. It just allows the board configuration to be more configurable.

I don't see how a different virtuliser would help here. What do you think it should virtulise?

bradjc · 2024-02-20T19:00:38Z

kernel/src/hil/i2c.rs

@@ -230,6 +230,7 @@ pub trait I2CDevice {
 fn write(&self, data: &'static mut [u8], len: usize) -> Result<(), (Error, &'static mut [u8])>;
 fn read(&self, buffer: &'static mut [u8], len: usize)
 -> Result<(), (Error, &'static mut [u8])>;
+ fn set_address(&self, addr: u8);


We can't add this API. The address is fixed, and grants the holder of an I2CDevice access to only the specified device (address).

/// It gives an interface for communicating with a specific I2C
/// device.

What is the device address changes though? It's not uncommon to have programmable I2C addresses

I think either a capsule could use the I2CMaster interface, or a shim layer that tracks the address changes below the I2CDevice would work.

I agree with @bradjc, for this abstraction, anyway, the whole point of the I2CDevice trait is that it isolates drivers talking to different peripherals over the same I2C bus. If a client of I2CDevice can modify the address arbitrarily, it completely negates the point of this abstraction.

There could be a different interface. For example, an extension trait might work, but I'd really want to understand the use case. Is the idea that some application/driver would change the I2C device dynamically after boot? What's a specific case for this?

The goal is to be able to share an I2C bus with multiple kernel drivers and with userspace. Something like:

App1 App2 =====syscall===== Sensor Screen I2CMasterDriver --------- --------- ----------------- I2CDevice I2CDevice I2CMaster --------------------------------------------- Virtual I2C --------------------------------------------- I2C bus

alistair23 · 2024-02-20T23:48:36Z

I don't think there is a better way to do this.

The other approach is to modify the virtual_i2c. We could have MuxI2C implement i2c::I2CMaster so then the i2c_master::I2CMasterDriver can just use MuxI2C directly.

That would involve adding something like i2c_bus: OptionalCell<&'a I2CMaster<'a, I>> to MuxI2C and tracking operations and addresses from the i2c::I2CMaster implementation. Then do_next_op() would call the I2C hardware based on pending operations.

In the long run though, that seems even worse. Then there is no way to filter or prevent userspace from accessing other I2C devices. This current implementation will allow us to restrict access to I2C devices from userspace as required. We can also check for conflicts in set_address(), Something which I think will be useful.

bradjc · 2024-02-21T02:38:24Z

Can you outline what you are trying to achieve?

It seems like something like this might work? Feels like we need multiple virtualization layers to support both kernel and userspace and multiple userspace apps.

  I2CDevice
  --------
   I2CMux                 Userspace
-----------              -----------
Virtual_I2C              Virtual_I2C
------------------------------------
               I2CMux
------------------------------------
             I2CMaster

alistair23 · 2024-02-21T04:26:26Z

I'm trying to use a single I2C bus for some I2CDevices and also expose the bus to userspace.

Another virtuliser layer to virtulise i2c::I2CMaster directly has too much overhead. We end up with 2 layers of virutlisation, the second only virtulising two users (MuxI2C and I2CMasterDriver)

bradjc · 2024-02-21T22:33:42Z

has too much overhead

In terms of latency? Or memory use?

It sounds like you need a new capsule, once which uses the I2CMaster interface, but then allows the kernel to create I2CDevices for its own use and exposes the userspace interface you want.

alistair23 · 2024-02-22T02:43:18Z

In terms of latency? Or memory use?

Memory usage

It sounds like you need a new capsule, once which uses the I2CMaster interface, but then allows the kernel to create I2CDevices for its own use and exposes the userspace interface you want.

I don't think that's a great idea though, see #3867 (comment)

lschuermann · 2024-02-22T21:34:40Z

How about a second instance of the system call driver? AFAIK they were always assumed to be duplicable and can be instantiated at a second syscall driver number. This would solve many such problems across subsystems, is something that downstream users are likely to do anyways, and may motivate userspace APIs that can be supplied an alternative driver number.

bradjc · 2024-02-22T21:40:08Z

How about a second instance of the system call driver? AFAIK they were always assumed to be duplicable and can be instantiated at a second syscall driver number. This would solve many such problems across subsystems, is something that downstream users are likely to do anyways, and may motivate userspace APIs that can be supplied an alternative driver number.

Yes that would be no problem, but userspace would only have access to a single I2C device (address) without a different virtualizer.

bradjc · 2024-03-12T17:49:57Z

What should we do with this PR? Is it helpful to serve as a placeholder? Or close it?

I think the intent is completely fine, but we want to preserve the current I2CDevice abstraction.

alistair23 · 2024-03-13T00:53:58Z

What should we do with this PR? Is it helpful to serve as a placeholder? Or close it?

I think this is something that needs to be supported. Otherwise we loose the entire I2C bus when one I2CDevice is created.

Maybe some else can have a go doing it in a way you approve of?

bradjc · 2024-03-13T01:47:53Z

A capsule that exposes I2CDevice to userspace would work.

alistair23 · 2024-03-13T02:10:17Z

A capsule that exposes I2CDevice to userspace would work.

How does that fix the issue?

bradjc · 2024-03-13T02:14:32Z

The existing virtual_i2c layer could be used to provide I2CDevices for both kernel drivers and userspace.

alistair23 · 2024-03-13T02:25:51Z

I don't see how that helps the original problem though. How is the I2C bus then exposed to userspace?

bradjc · 2024-03-13T02:29:45Z

I don't think it makes sense to provide an I2CDevice to a kernel driver, and then also provide access to the same device (address) via a low level bus interface to userspace. I mean it's possible using another virtualizer, but I don't think it fits the Tock ethos of isolation to allow userspace to interfere with the kernel.

alistair23 · 2024-03-25T01:03:30Z

What is the specific use case? What is the happy end-goal?

The end goal is what the PR description says

To allow an I2C bus to support devices created by the board and still be exposed to userspace let's convert the I2CMasterDriver to use an I2CDevice instead of the physical I2C hardware.

This allows us to use the Mux with userspace applications and kernel uses.

If you have an I2C bus on a board, with multiple I2C devices connected to a physical bus. Some of those devices are supported by kernel capsules (say a https://github.com/tock/tock/blob/master/capsules/extra/src/bmp280.rs) and some aren't (such as a GNSS device). Currently you have to set up Tock such that every single physical I2C device has a capsule written for it. Otherwise you can't access it from user space (unless you use no kernel capsules).

This PR changes the virtulisation such that a I2C bus can have both kernel supported capsules AND still expose the other devices to userspace.

What does a user-space process get to do with an I2C system call driver. Currently, it gets to send/receive I2C to arbitrary I2C devices on one bus. There is an invariant that no other kernel component is using any device on the same bus (because multiplexing only happens through virtual I2CDevices). Multiple processes can interfere with each other (this is already a problem, and realistically, if this is ever used in practice, it should probably be restricted to a single process).

Yes. This PR does not address the existing issues with the I2C syscall drivers. The drivers are already used and enabled by default on a range of Tock boards, so I'm not sure why this PR is the issue. If there are multiple apps running on any Tock board using an I2C driver they can conflict with each other. They can also conflict with the kernel as well.

The nRF as well as pretty much every board already has this problem. This PR doesn't change that in anyway.

One thing that this PR does do, is move in a direction where we can in the future add filtering to the I2C accesses. That would then allow us to ensure no two users are communicating with the same address. This PR does NOT do that, it just makes that easier in the future.

Just to be clear, this PR doesn't fix or worsen the existing power of the I2C driver to access the entire bus. It just allows a board design to also use I2CDevices on the bus.

2. What's an example scenario that needs this (what specific problem does this solve)? If the problem is "I want to be able to talk to a raw I2C bus from userspace, e.g. for development or debugging" this is already possible. If the problem is "I want to be able to talk to the same I2C device concurrently from both the kernel and userspace" I'm skeptical there is a path for supporting that use case upstream (it really goes against much of kind of isolation upstream is trying to achieve). If the problem is "I want to be able to talk to devices from processes which are on the same bus as other devices which are used in kernel components, but are distinct devices" this PR doesn't actually enforce or accomplish that. @bradjc's suggestion of an driver that "owned" a bunch of (virtual) I2CDevices and allowed processes to talk specifically to those would work (though I'm empathetic, at least in principle, to the discoverability concerns). Maybe there is another mechanism.

"I want to be able to talk to devices from processes which are on the same bus as other devices which are used in kernel components, but are distinct devices" is exactly what this PR allows. Before #3881 I was using this PR to get NMEA sentences from a I2C device while also allowing the kernel to communicate with other I2C devices.

Why do you think this PR doesn't accomplish that?

Basically the idea is that Tock can be flashed to a device. I can plug in an I2C device and I don't have to re-flash the kernel. Instead I can install an app that can communicate with that device via I2C.

Considering how hard it is to support new I2C devices (see #3881) I think this is a reasonable approach.

I think @bradjc's suggestion has a few issues, as I pointed out in #3867 (comment). I might be misunderstanding the suggestion, but I haven't heard a response explaining what I'm missing.

bradjc · 2024-03-25T14:03:24Z

Yes. This PR does not address the existing issues with the I2C syscall drivers. The drivers are already used and enabled by default on a range of Tock boards, so I'm not sure why this PR is the issue.

Currently we support two different I2C setups, if you will:

I2C devices are supported in the kernel, and userspace only has access through dedicated drivers. Semantically, the kernel owns the bus. This matches the Tock ethos of isolating the resource with the kernel.
The I2C bus is entirely exposed to userspace. Semantically, userspace owns the I2C bus. This provides no guarantees on isolation. In practice, this is for debugging and development boards. I can't see how a production kernel would ever want to expose this level of control userspace.

This PR merges the two, and we need to compare to setup #1 as the baseline (not #2). To maintain isolation guarantees for the kernel, we need to do this very carefully. That the "debugging" I2C setup (setup #2) allows for resource conflict does not mean we can compromise setup #1. We need to ensure that the kernel can maintain strong I2C isolation guarantees as that is what existing I2C-in-kernel users expect.

bradjc · 2024-03-25T14:14:47Z

How can userspace know which devices have been exposed? Is userspace supposed to just brute force syscall numbers? How does userspace know which syscall corresponds to which device? How do we keep that consistent across boards?

I don't know how userspace would know the syscall driver numbers, but I assume it would work similarly to how userspace would know what I2C addresses are available and what those addresses correspond to. This is a fundamental issue with exposing any low-level/generic interface.

Also this greatly increases the complexity of boards. If a device wants to communicate with an off-board I2C device we need to create a custom board and then re-compile and re-flash the kernel. That would be a large scale up of #3873 to support a range of I2C device combinations.

I don't think we can have everything. The ideal solution might support these features:

Userspace can access an I2C bus directly, for accessing arbitrary devices added to the bus.
The kernel can use the I2C bus for well-known devices on the board.
Tock provides robust isolation and guarantees userspace cannot interfere with an I2C device the kernel owns.
Minimal memory overhead for virtualization.

If feature 2 is enabled, then feature 3 is also a hard requirement for upstream code. Out-of-tree code of course does not need to uphold 3.

Set (1, 2, 3) is possible by adding a second virtualization layer.
Set (2, 3, 4) is possible by instantiating I2CDevices for each I2C device.
Set (1, 2) is possible with this PR, but it doesn't enable feature 3.

alistair23 · 2024-03-26T04:31:23Z

I2C devices are supported in the kernel, and userspace only has access through dedicated drivers. Semantically, the kernel owns the bus. This matches the Tock ethos of isolating the resource with the kernel.

Agreed. This PR doesn't change this setup

2. The I2C bus is entirely exposed to userspace. Semantically, userspace owns the I2C bus. This provides no guarantees on isolation. In practice, this is for debugging and development boards. I can't see how a production kernel would ever want to expose this level of control userspace.

Yep. This is what a lot of Tock boards do today.

I see it as this PR changes this option to be a little more flexible. It allows exposing the I2C bus to user space (as most boards do), while also having dedicated drivers on the same bus.

A production kernel probably does not want to do this. Although this PR does move towards address filtering which might change that.

If the issue is specifically the set_address() it could be taken out of the HIL. Or there could be a bool set at I2CDevice creation to allow dynamic addresses.

I don't think we can have everything. The ideal solution might support these features:

1. Userspace can access an I2C bus directly, for accessing arbitrary devices added to the bus.

2. The kernel can use the I2C bus for well-known devices on the board.

3. Tock provides robust isolation and guarantees userspace cannot interfere with an I2C device the kernel owns.

4. Minimal memory overhead for virtualization.

Wouldn't a simple filter on top of this get 1, 2, 3 and 4? We could just check if a device is already in use before allowing set_address() to succeed

bradjc · 2024-03-26T12:27:44Z

If the issue is specifically the set_address() it could be taken out of the HIL.

This is the issue. We absolutely cannot add this API to I2CDevice HIL.

Wouldn't a simple filter on top of this get 1, 2, 3 and 4? We could just check if a device is already in use before allowing set_address() to succeed

Maybe. I'm concerned it would be fragile and easy to get wrong. Unless there is a way to use the type system to ensure that all I2CDevices are accurately represented in the filter.

alistair23 · 2024-03-26T21:38:42Z

This is the issue. We absolutely cannot add this API to I2CDevice HIL.

Would a public function not in the HIL work?

Maybe. I'm concerned it would be fragile and easy to get wrong. Unless there is a way to use the type system to ensure that all I2CDevices are accurately represented in the filter.

If there is a general consensus that this can be accepted with filtering I could add it. I would rather no add it if this isn't going to be accepted though.

It should be as simple as (untested)

fn set_address_check(&self, new_addr: u8) -> bool {
   self
        .i2c_devices
        .iter()
        .find(|node| node.addr.get() == new_addr).is_none()
}

fn set_address(&self, addr: u8) -> Result<> {
    if self.mux.set_address_check(addr) {
        self.addr.set(addr);
        Ok(())
    } else {
        Err(())
    }
}

That should avoid conflicts with I2CDevices

bradjc · 2024-03-29T17:14:41Z

I think a design where the I2C mux exposes the "rest" of the I2C bus as a separate object would be workable. I don't think it should be called I2CDevice as that implies a single device. But the virtualizer could support a separate interface for all I2C addresses that are not used by I2CDevices.

I think that would balance the competing concerns.

Ensure there is a minimum clock_period_nanos of 1, otherwise depending on the CPU frequency we can divide by zero. Signed-off-by: Alistair Francis <[email protected]>

Support changing the address of an I2CDevice. This is the first step towards allowing an I2C bus to have kernel devices created by the board and still be exposed to userspace. This also allows supporting devices with runtime configurable addresses. Signed-off-by: Alistair Francis <[email protected]>

To allow an I2C bus to have devices created by the board and still be exposed to userspace let's convert the I2CMasterDriver to use an I2CDevice instead of a I2C hardware. This allows muxing an I2C bus with both userspace and devices in the kernel. This commit also adds a I2CMasterDriverComponent to help in creating the I2CMasterDriver. Signed-off-by: Alistair Francis <[email protected]>

Signed-off-by: Alistair Francis <[email protected]>

alistair23 · 2024-04-02T10:21:49Z

I think a design where the I2C mux exposes the "rest" of the I2C bus as a separate object would be workable. I don't think it should be called I2CDevice as that implies a single device. But the virtualizer could support a separate interface for all I2C addresses that are not used by I2CDevices.

I think that would balance the competing concerns.

I pushed an update that I think addresses this. It builds but is untested, hence the draft status. Let me know if it is in the right direction and I'll test it

bradjc

I've been thinking more about this and I think this idea is very workable. I do think we want a clear distinction between an I2CDevice and the "remaining" I2C bus.

capsules/core/src/virtualizers/virtual_i2c.rs

bradjc · 2024-04-02T18:10:56Z

capsules/core/src/virtualizers/virtual_i2c.rs

@@ -307,9 +398,17 @@ impl<'a, I: i2c::I2CMaster<'a>> i2c::I2CDevice for I2CDevice<'a, I> {
 }
 }

+impl<'a, I: i2c::I2CMaster<'a>> i2c::I2CMultiDevice for I2CDevice<'a, I, true, NoSMBus> {


Why not just use I2CMaster?

I feel the layering is a little weird using I2CMaster. As the I2CDevice layers on top of I2CMaster it feels a little cleaner to then also layer the "rest of the bus" on I2CMaster as well.

To me it also makes it clearer that we only have access to some of the bus, instead of full access that I2CMaster has.

By using I2CMultiDevice we can piggy back on the existing I2CDevice support. Plus then we need to check the address less often, only when setting it instead of on every operation

bradjc · 2024-04-02T18:11:46Z

capsules/core/src/i2c_master.rs

@@ -37,14 +37,14 @@ struct Transaction {
 read_len: OptionalCell<usize>,
 }

-pub struct I2CMasterDriver<'a, I: i2c::I2CMaster<'a>> {
+pub struct I2CMasterDriver<'a, I: i2c::I2CMultiDevice> {


Then we wouldn't need to change this.

Signed-off-by: Alistair Francis <[email protected]>

alistair23 · 2024-04-04T07:43:53Z

I've been thinking more about this and I think this idea is very workable. I do think we want a clear distinction between an I2CDevice and the "remaining" I2C bus.

Ready to go then

alistair23 · 2024-04-16T04:09:19Z

Ping!

alistair23 · 2024-04-22T00:17:06Z

Ping!

alistair23 · 2024-05-01T05:48:50Z

Ping!

alistair23 · 2024-05-12T23:59:00Z

Ping!

alistair23 · 2024-06-04T21:35:56Z

Ping!

github-actions bot added HIL This affects a Tock HIL interface. WG-OpenTitan In the purview of the OpenTitan working group. component labels Feb 19, 2024

alistair23 force-pushed the alistair/i2c-mux branch 2 times, most recently from 9f1436a to 747aaa4 Compare February 19, 2024 10:19

alistair23 changed the title ~~Alistair/i2c mux~~ Allow muxing I2C userspace driver and I2C Devices Feb 19, 2024

alistair23 force-pushed the alistair/i2c-mux branch from 7255ae8 to 536e2b0 Compare February 19, 2024 12:19

lschuermann previously approved these changes Feb 19, 2024

View reviewed changes

bradjc requested changes Feb 19, 2024

View reviewed changes

bradjc reviewed Feb 19, 2024

View reviewed changes

boards/components/src/i2c.rs Show resolved Hide resolved

alistair23 dismissed lschuermann’s stale review via 42ea7b1 February 19, 2024 23:38

alistair23 force-pushed the alistair/i2c-mux branch from 536e2b0 to 42ea7b1 Compare February 19, 2024 23:38

bradjc reviewed Feb 20, 2024

View reviewed changes

brghena mentioned this pull request Feb 22, 2024

libtock: Rewrite and standardize API tock/libtock-c#370

Merged

alistair23 mentioned this pull request Mar 11, 2024

Add support for reading NMEA sentences from I2C #3881

Open

2 tasks

alevy added the waiting-on-author label Mar 23, 2024

alistair23 added 4 commits April 2, 2024 14:15

chips/lowrisc: i2c: Ensure we don't divide by zero

ec487ff

Ensure there is a minimum clock_period_nanos of 1, otherwise depending on the CPU frequency we can divide by zero. Signed-off-by: Alistair Francis <[email protected]>

capsules: i2c: Only enable the device when being used

3388356

Signed-off-by: Alistair Francis <[email protected]>

alistair23 force-pushed the alistair/i2c-mux branch from 42ea7b1 to cb67b50 Compare April 2, 2024 10:20

alistair23 marked this pull request as draft April 2, 2024 10:20

bradjc reviewed Apr 2, 2024

View reviewed changes

alistair23 force-pushed the alistair/i2c-mux branch from cb67b50 to af7c51c Compare April 3, 2024 00:58

Split out I2CDevice set_address() from trait

4b1c55f

Signed-off-by: Alistair Francis <[email protected]>

alistair23 force-pushed the alistair/i2c-mux branch from af7c51c to 4b1c55f Compare April 4, 2024 07:16

alistair23 marked this pull request as ready for review April 4, 2024 07:17

alistair23 requested review from bradjc, alevy and lschuermann April 16, 2024 04:09

bradjc removed the waiting-on-author label May 17, 2024

Allow muxing I2C userspace driver and I2C Devices #3867

Are you sure you want to change the base?

Allow muxing I2C userspace driver and I2C Devices #3867

Conversation

alistair23 commented Feb 19, 2024

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

lschuermann left a comment

Choose a reason for hiding this comment

bradjc left a comment

Choose a reason for hiding this comment

alistair23 commented Feb 19, 2024

bradjc Feb 20, 2024

Choose a reason for hiding this comment

alistair23 Feb 20, 2024

Choose a reason for hiding this comment

bradjc Feb 20, 2024

Choose a reason for hiding this comment

alevy Feb 22, 2024

Choose a reason for hiding this comment

bradjc Feb 22, 2024 • edited

Choose a reason for hiding this comment

alistair23 commented Feb 20, 2024

bradjc commented Feb 21, 2024

alistair23 commented Feb 21, 2024

bradjc commented Feb 21, 2024

alistair23 commented Feb 22, 2024

lschuermann commented Feb 22, 2024

bradjc commented Feb 22, 2024

bradjc commented Mar 12, 2024

alistair23 commented Mar 13, 2024

bradjc commented Mar 13, 2024

alistair23 commented Mar 13, 2024

bradjc commented Mar 13, 2024

alistair23 commented Mar 13, 2024

bradjc commented Mar 13, 2024

alistair23 commented Mar 25, 2024

bradjc commented Mar 25, 2024

bradjc commented Mar 25, 2024

alistair23 commented Mar 26, 2024

bradjc commented Mar 26, 2024

alistair23 commented Mar 26, 2024

bradjc commented Mar 29, 2024

alistair23 commented Apr 2, 2024

bradjc left a comment

Choose a reason for hiding this comment

bradjc Apr 2, 2024

Choose a reason for hiding this comment

alistair23 Apr 3, 2024

Choose a reason for hiding this comment

bradjc Apr 2, 2024

Choose a reason for hiding this comment

alistair23 commented Apr 4, 2024

alistair23 commented Apr 16, 2024

alistair23 commented Apr 22, 2024

alistair23 commented May 1, 2024

alistair23 commented May 12, 2024

alistair23 commented Jun 4, 2024

bradjc Feb 22, 2024 •

edited