Is this still UB when using raw pointers?

Context

I am currently writing some hardware abstraction for a micro controller which supports gpio.
On this particular piece of hardware the gpio is organized as follows:

  • the board can have multiple "gpio ports"
  • each "gpio port" is mapped into a separate memory region
  • each "gpio port" supports up to 32 "gpio pins"
  • each (implemented) "gpio pin" corresponds to a specific physical pin on the board

My Idea

So basically I wanted to have a single (unsafe) function that returns a list of GpioHandle<GpioPort, GpioPin> where GpioPort and GpioPin are some const generic parameters that describe the physical available resources. So far this works :slight_smile:
In order to actually write to the "gpio port" I implemented a struct that represents the registers of the memory mapped region.
Actual example of what this looks like for the interested:

pub struct GpioInstanceRegisterBlock {
    pub out: ValueWithOffset<Register<{RegisterMode::RW}>, { 1284 - 0 }>,
    pub outset: ValueWithOffset<Register<{RegisterMode::RW}>, { 1288 - (1284 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
    pub outclr: ValueWithOffset<Register<{RegisterMode::RW}>, { 1292 - (1288 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
    pub in: ValueWithOffset<Register<{RegisterMode::R}>, { 1296 - (1292 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
    pub dir: ValueWithOffset<Register<{RegisterMode::RW}>, { 1300 - (1296 + core::mem::size_of::<Register<{ RegisterMode::R }>>()) }>,
    pub dirset: ValueWithOffset<Register<{RegisterMode::RW}>, { 1304 - (1300 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
    pub dirclr: ValueWithOffset<Register<{RegisterMode::RW}>, { 1308 - (1304 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
    pub latch: ValueWithOffset<Register<{RegisterMode::RW}>, { 1312 - (1308 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
    pub detectmode: ValueWithOffset<Register<{RegisterMode::RW}>, { 1316 - (1312 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
    pub pin_cnf: ValueWithOffset<[Register<{RegisterMode::RW}>; 32], { 1792 - (1316 + core::mem::size_of::<Register<{ RegisterMode::RW }>>()) }>,
}

which looks a bit weird, because it is autogenerated by a macro_rules macro into which I just paste the register description from the documentation.

The problem

Conceptually my model allows me to (mutably) share around different "gpio pins" on the same "gpio port" even though I need to write to the same physical addresses when writing/reading the pins. In my implementation I have something like:

impl <const PIN: GpioPin> Gpio<PIN> {
    pub fn set(&mut self){
        let registerBlock = self.getRegisterBlockMut();
        // write something to registerBlock
    }
    
    fn getRegisterBlockMut(&mut self) -> &mut GpioInstanceRegisterBlock {
        //compute the memory address using information from PIN and return a mutable reference to it
    }
}

From my understanding this code has UB if set is called for multiple Gpio<GpioPin> instances where the GpioPin's share the same "gpio port" because the aliasing rules for the memory region of the "gpio port" would be broken (two mutable references to the same memory region).

My Question

Can I solve this problem simply by returning a *mut GpioInstanceRegisterBlock instead of a &mut GpioInstanceRegisterBlock? The potentially different threads writing/reading to the memory location itself should not conflict as those use volatile reads/writes and I would assume the hardware should always detect all reads/writes independt of how close they are to each other (from a time perspective).
Also all read/write operations are basically atomic.

1 Like

If the operations are atomic, use AtomicUxx type. Alternatively, wrap the destination in UnsafeCell, which will warn the compiler that you're breaking usual aliasing rules, and then use volatile writes.

1 Like

But would be using AtomicUxx sufficient? My understanding is that the aliasing rules would be broken already if I have to two mutable references to the same memory address: independent of the fact that the struct only contains AtomicUxx types?

Edit: I guess I would not need mutable references anymore if all values I want to write to are atomics, as those can also be written to through a non mutable reference

I already use the volatile writes, I will have a look at the UnsafeCell abstraction and how I can apply it to my case

In your case I'd return a MutexGuard<'_, GpioInstanceRegisterBlock>, by having the global / static GpioInstanceRegisterBlock be Mutex-wrapped, so as to have that function body be guaranteed exclusive access to it:

impl <const PIN: GpioPin> Gpio<PIN> {
    pub fn set(&mut self){
        let mut registerBlock = self.getRegisterBlockMut(); /* it's a guard that `DerefMut`s to a GpioInstanceRegisterBlock`
        // write something to registerBlock
    } // <- the guard is release, letting a concurrent (e.g., parallel) call take over.

If you know there is no multi-threading involved for some reason, only re-entrant concurrency, then you could loosen it to a RefCell (or to Cell fields…).


The above would take care of the aliasing issues, by using shared mutability, as you have here. But then there is also, and mainly,

The problem of volatile operations

regarding which, Cell, for instance, is not enough, and more generally, any true Rust reference is doomed not to have the right semantics (Rust references can trigger "spurious" reads/writes by being allowed to move those around if the compiler can notice that the "code wouldn't notice" (since it assumes such reads / writes are side-effect-free, by virtue of being operations on a Rust reference instead of volatile operations).

This makes manipulating volatile stuff quite tricky to get right. There is only one crate out there which does the things totally right (by abstracting around the pointer rather than the pointee!), which is:

I believe this would already take care of the shared mutability as well, but I'll cc @Lokathor so that they confirm or deny this.

3 Likes

My idea was to explicitly not require any runtime checking since there is only a single function that can return those handles (which is marked as unsafe and should only be called once). The handles are do not implement Sync so the actual physical resources (Pins on the board) can always only have a single owner. The problem is that those physical resources share a few registers; but those shouldn't be a problem as the writes are all volatile.

The crate you linked to looks interesting and is actually similar to what I was doing before writing the GpioInstanceRegisterBlock, however I find that approach to be a bit more cumbersome (for reasons I am not perfectly sure of).

Maybe I will have to redesign my approach so that GpioInstanceRegisterBlock becomes a ZST, taking the actual address of the instance as a const generic parameter and my register abstraction would then also be a ZST having the address as an const generic aswell. That I think would actually solve both my problem and would also be more like the approach taken by voladdress.

Edit: after rewriting a bit I would post my new design here and would be happy to receive feedback for it :slight_smile:

1 Like

@raidwas, what you're trying to do looks a lot like the code generated by svd2rust. Have you seen that project? If you have or could create an SVD for your chip, it would auto-generate register interfaces for all of your peripherals.

In general, you might want to look through the existing embedded Rust ecosystem, to see how they handle these problems, if you haven't already.

Also, I'd like to point out another recent thread about microcontroller GPIO.

I'll add a bit of detail. svd2rust does indeed use ZSTs as marker types to define the capabilities of various registers. And it uses closures combined with a VolatileCell type to ensure that multiple statements, all modifying bits of the same register, are combined into a single instruction.

When accessing MMIO you should:

  1. never make a reference to the actual MMIO address
  2. apply any synchronization yourself (if necessary)

So what I personally do for my single-core device is just let the pointers get copied around and don't worry about exclusive access.

If there's threads then something like a mutex over a zero sized type would be appropriate. Then you have to lock the mutex and get your token and then call methods on the token to do whatever.