üApp: picar-s: refactoring i2c-bcm2708 driver to isolate i2c I/O space accesses

This task will refactor the kernel driver i2c-bcm2708, the low-level i2c driver on the raspberry pi 3.

The refactoring is aimed at isolating the I2C I/O address space accesses (both reads and writes) into a set of functions that have no Linux kernel dependency. These will eventually be moved into a uobject and accessed via hypercalls to protect the I2C I/O range access from the guest OS.

The following was the identified flow for the i2c-dev read call that we had discussed previously in this post: üApp: picar-s: add HMAC-SHA256 message authentication code between app and i2c driver

This might be a good place to start when attempting to begin the refactoring.

The eventual idea is that the picar-s should still follow the line-following with the refactoring that will be accomplished by this task.

We should disable any interrupts in the i2c-bcm2708 device driver (if enabled) and use polling.

Linked PRs:

Merges:

After examining the code I see that the two functions of interest in i2c-bcm2708.c are: bcm2708_rd() and bcm2708_wr().
They have a single line of readl() and writel() low level function calls written in assembly as part of this file:
arch/alpha/include/asm/io.h

Great. This would then be our strawman solution. Can you post what the readl() and writel() low-level functions look like?

If we can come up with a local version of readl() and writel() without any use of Linux kernel support functions that would be great.

We can then take bcm2708_rd(), bcm2708_wr() and the local definitions of readl() and writel() and move them into a uobject. Then replace calls to bcm2708_rd() and bcm2708_wr() with equivalent hypercalls.

The only caveat is that the above strawman solution might suffer from performance issues if the rd() and wr() functions are invoked from other top-level functions in this module within loops. We can cross that bridge when we get there…

If we look into the writing function there is the following sequence of calls that are relevant:

extern inline void writel(u32 b, volatile void __iomem *addr)
{
__raw_writel(b, addr);
mb();
}

extern inline void __raw_writel(u32 b, volatile void __iomem *addr)
{
IO_CONCAT(__IO_PREFIX,writel)(b, addr);
}

#define IO_CONCAT(a,b) _IO_CONCAT(a,b)
#define _IO_CONCAT(a,b) a ## _ ## b

#define __IO_PREFIX generic

#define mb() asm volatile(“mb”: : :“memory”)

readl() sequence is very similar.

Great. So we can have our own implementation of readl() and writel() within the uobject ecosystem. Can you go ahead and construct local self-contained implementations of the above within the module itself?

Going forward, we should probably start collecting such global self-contained Linux driver function calls within a separate library so that we can link uobjects with it for other drivers in the future :slight_smile:

Yes, I can create functions in the i2c-bcm2708.c, but the code above is not the code called.
The indexing shows me a file, which is from a different family of CPUs.
I will find out first the code and then figure out what it is.

1 Like

For this CPU the implementation is in this file : arch/arm/include/asm/io.h
We have the following for the writel() function:

#define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
#define __iowmb() wmb()
#define wmb() asm volatile(“wmb”: : :“memory”)

#define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c)

#define __raw_writel __raw_writel
static inline void __raw_writel(u32 val, volatile void __iomem addr)
{
asm volatile(“str %1, %0”
: : “Qo” (
(volatile u32 __force *)addr), “r” (val));
}

__raw_writel() has some assembly in it.
I can try putting this in the i2c-bcm2708.c driver and see if it compiles.

Sounds good. Let us try that first and see if the functionality is preserved. Same with the reads too.

Thanks!

The following snippet shows how I moved the functions and renamed them with u_function()
and the functionality is preserved.
This is the first step.

// Local u_readl() and u_writel() functions

#define __u_raw_writel __u_raw_writel
static inline void __u_raw_writel(u32 val, volatile void __iomem addr)
{
asm volatile(“str %1, %0”
: : “Qo” (
(volatile u32 __force *)addr), “r” (val));
}

#define u_writel_relaxed(v,c) __u_raw_writel((__force u32) cpu_to_le32(v),c)
#define u_writel(v,c) ({ __iowmb(); u_writel_relaxed(v,c); })

#define __u_raw_readl __u_raw_readl
static inline u32 __u_raw_readl(const volatile void __iomem addr)
{
u32 val;
asm volatile(“ldr %0, %1”
: “=r” (val)
: “Qo” (
(volatile u32 __force *)addr));
return val;
}
#define u_readl_relaxed© ({ u32 __r = le32_to_cpu((__force __le32)
__u_raw_readl©); __r; })
#define u_readl© ({ u32 __v = u_readl_relaxed©; __iormb(); __v; })

static inline u32 bcm2708_rd(struct bcm2708_i2c *bi, unsigned reg)
{
return u_readl(bi->base + reg);
}

static inline void bcm2708_wr(struct bcm2708_i2c *bi, unsigned reg, u32 val)
{
u_writel(val, bi->base + reg);
}

The functions as shown above reside in i2c-bcm2708.c and the car still follows the line with the modified driver.

Great stuff! One question:

  1. How does i2c-bcm2708 transfer bytes on I2C reads? Does it currently employ interrupts or is it polling the I/O space?

There is irq code, but I am not sure if it is used for the reads and writes that we do.

I believe the driver is using interrupts/IRQ to read and write. If you look at bcm2708_i2c_master_xfer (https://github.com/uberspark/uobjcoll-raspberrypi-linux-i2c-bcm2835/blob/uobjcoll-4.4.y/drivers/i2c/busses/i2c-bcm2708.c) which is the function that is invoked on i2c reads and writes from upper-level i2c-dev driver, you can see the following:

	ret = wait_for_completion_timeout(&bi->done, adap->timeout);
	if (ret == 0) {
		dev_err(&adap->dev, "transfer timed out\n");
		goto error_timeout;
	}

This is the bottom-half completion routine that is set by the interrupt handler once the xfer (read or write) is complete. The interrupt handler itself is bcm2708_i2c_interrupt which performs the required read and write for num bytes and then sets the I/O completion signal via complete(&bi->done);

We will need to have i2c-bcm2708 do I2C reads and writes via polling instead of interrupts so we can eventually move it into an uobject (since uobjects currently don’t have interrupt support).

One way of doing this is to refactor the logic of bcm2708_i2c_master_xfer so that it directly calls a polled read/wrte function (which we will write as mentioned next) without the wait for I/O completion.

This polled read/write function will then incorporate logic of bcm2708_i2c_interrupt without the I/O completion calls. We could bring this in as a #ifdef POLLING build option for i2c-bcm2708.

Let me know your thoughts. Thanks!

I have seen this code and also in bcm2708_bsc_setup() we have some indication that interrupts are used:
if (bi->msg->flags & I2C_M_RD)
c |= BSC_C_INTR | BSC_C_READ;
else
c |= BSC_C_INTT;

Some drivers have the ability to work in either polling or interrupt mode.
This one does not seem to be one of them.
This would mean pretty much rewriting some portions of the driver.

Yep, I agree, one option to a rewrite is by using and adapting existing code as above. What do you think?

We have to consider that this is the established driver that everybody uses and I could not find threads where anybody needed to change it. The discussions are mostly on how to use it.
In order to successfully modify the driver one needs to become very familiar with the registers of the chip and do a lot of experimentation with the possibility of breaking things for some time.
I am also wondering if the overhead of the OS will change significantly once we move from interrupts to polling and what would be the effect on our i2c transactions and eventual line following?

All good questions imo :slight_smile:
But hopefully we can seek answers to these with our probing.

It also brings in a higher-level question: what a driver architecture should look like when we are considering security properties such as device data read/write integrity. I think these are the kinds of answers that might lead us to re-invent the ways device drivers can be re-architected for verifiable security on commodity platforms.

As far as performance goes sure polling probably is not ideal, but given that supporting interrupts within uobject is a work-in-progess, we can definitely use that as our strawman solution for now.

I would vote for learning from this one device driver before we can generalize requirements for other devices/drivers and developers in general.

More concretely for our task, below are a set of possible high-level steps I can think of towards getting a polled mode read/write, having read through the driver’s code:

  1. copy the logic of the bcm2708_i2c_interrupt function into a new bcm2708_i2c_polling function.

  2. Remove the spin_locks related to IRQs within bcm2708_i2c_polling

  3. remove the complete calls. within bcm2708_i2c_polling

  4. invoke bcm2708_i2c_polling function from within bcm2708_i2c_master_xfer in place of the wait_for_completeion routine.

Let me know if you want me to take a shot at fleshing this out first…

Oh and just for fun and closure :), here is the datasheet of the Broadcom I2C Bus controller. Look at Page-28 onwards for register descriptions:

Oh and in case you don’t want to go from the interrupt logic to polling logic as described before, I found the following two functions that might be of interest:

  1. bcm2708_bsc_fifo_fill --> for writes
  2. bcm2708_bsc_fifo_drain --> for reads

Coulpled with the FIFO and status register descriptions in the above referenced manual, we should be able to come up with our own polled mode master_xfer routine implementation :slight_smile:

Sounds good. Let me take a closer look at the driver and the functions mentioned in the last several posts.

1 Like