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.

Thank you everyone for the great comments. I did not yet have the time to read through all the linked articles/blogs but will surely do so :slight_smile:

I will definitely have a look how the generated code looks like, but I am specifically tinkering around with this to learn for myself how this can be modeled, not exactly what I can do with the micro controller in the first place (so I am not looking for the easiest crate to use the microcontroller if that makes sense ^^)

For now what I came up with which works decently ok is the following.
For starters we have a few typed wrappers around the idea of a "register" (some u32 memory location):

Register Mode
#[derive(PartialEq, Eq)]
pub enum RegisterMode {
    R,
    W,
    RW,
}
Register Trait
pub trait Register {
    const ADDRESS: usize;
}
pub trait RegisterR: Register {
    unsafe fn read() -> u32 {
        read_volatile(Self::ADDRESS as *const u32)
    }
}

pub trait RegisterW: Register {
    unsafe fn write(value: u32) {
        write_volatile(Self::ADDRESS as *mut u32, value);
    }
}
Register Implementation
#[derive(Default)]
pub struct RegisterAtAddress<const RM: RegisterMode, const ADDRESS: usize> {}

impl<const RM: RegisterMode, const ADDRESS: usize> Register for RegisterAtAddress<RM, ADDRESS> {
    const ADDRESS: usize = ADDRESS;
}

impl<const ADDRESS: usize> RegisterR for RegisterAtAddress<{ RegisterMode::R }, ADDRESS> {}
impl<const ADDRESS: usize> RegisterW for RegisterAtAddress<{ RegisterMode::W }, ADDRESS> {}
impl<const ADDRESS: usize> RegisterR for RegisterAtAddress<{ RegisterMode::RW }, ADDRESS> {}
impl<const ADDRESS: usize> RegisterW for RegisterAtAddress<{ RegisterMode::RW }, ADDRESS> {}

And something very similar but for "blocks" (technically they do not have to be contiguous, but I consider that a plus):

Register Block
pub trait RegisterBlockIndex {
    fn index(&self) -> usize;
}

pub trait RegisterBlock {
    const ADDRESS: usize;
    type Index: RegisterBlockIndex;
    fn indexed_address(index: &Self::Index) -> usize {
        Self::ADDRESS + 0x4 * index.index()
    }
}
pub trait RegisterBlockR: RegisterBlock {
    unsafe fn read(index: &Self::Index) -> u32 {
        read_volatile(Self::indexed_address(index) as *const u32)
    }
}

pub trait RegisterBlockW: RegisterBlock {
    unsafe fn write(index: &Self::Index, value: u32) {
        write_volatile(Self::indexed_address(index) as *mut u32, value);
    }
}

pub struct RegisterBlockAtAddress<IndexType, const RM: RegisterMode, const BASE_ADDRESS: usize> {
    _phantom: PhantomData<IndexType>,
}

impl<IndexType, const RM: RegisterMode, const BASE_ADDRESS: usize> Default
    for RegisterBlockAtAddress<IndexType, RM, BASE_ADDRESS>
{
    fn default() -> Self {
        Self {
            _phantom: Default::default(),
        }
    }
}

impl<IndexType, const RM: RegisterMode, const BASE_ADDRESS: usize> RegisterBlock
    for RegisterBlockAtAddress<IndexType, RM, BASE_ADDRESS>
where
    IndexType: RegisterBlockIndex,
{
    const ADDRESS: usize = BASE_ADDRESS;
    type Index = IndexType;
}

impl<IndexType, const BASE_ADDRESS: usize> RegisterBlockR
    for RegisterBlockAtAddress<IndexType, { RegisterMode::R }, BASE_ADDRESS>
where
    IndexType: RegisterBlockIndex,
{
}
impl<IndexType, const BASE_ADDRESS: usize> RegisterBlockW
    for RegisterBlockAtAddress<IndexType, { RegisterMode::W }, BASE_ADDRESS>
where
    IndexType: RegisterBlockIndex,
{
}
impl<IndexType, const BASE_ADDRESS: usize> RegisterBlockR
    for RegisterBlockAtAddress<IndexType, { RegisterMode::RW }, BASE_ADDRESS>
where
    IndexType: RegisterBlockIndex,
{
}
impl<IndexType, const BASE_ADDRESS: usize> RegisterBlockW
    for RegisterBlockAtAddress<IndexType, { RegisterMode::RW }, BASE_ADDRESS>
where
    IndexType: RegisterBlockIndex,
{
}

What took me some time here is to abandon the Idea of having a seperate type that represents a register with a base address and an offset. This resulted in annoying where bounds like where [(); BaseAddress + Offset]: , that needed to be propagated all the way up; not nice.

Having this abstraction I can write the struct that describes the memory layout as follows (after some thought this is I think very similar to this, which I found in the thread @bradleyharden mentioned).

We start with a trait that defines what type of registers the instance has:

pub trait GpioInstance {
    type Out: RegisterR + RegisterW;
    type OutSet: RegisterR + RegisterW;
    type OutClr: RegisterR + RegisterW;
    type In: RegisterR;
    type Dir: RegisterR + RegisterW;
    type DirSet: RegisterR + RegisterW;
    type DirClr: RegisterR + RegisterW;
    type Latch: RegisterR + RegisterW;
    type DetectMode: RegisterR + RegisterW;
    type PinCnf: RegisterBlockR<Index = GpioPort> + RegisterBlockW<Index = GpioPort>;
}

Then, using a macro, we can easily create the instances that exist:

macro_rules! generate_gpio_instance_impl {
    ($struct_name:ident, $base_address:literal) => {
        pub struct $struct_name;
        impl GpioInstance for $struct_name {
            type Out = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x504 }>;
            type OutSet = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x508 }>;
            type OutClr = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x50C }>;
            type In = RegisterAtAddress<{ RegisterMode::R }, { $base_address + 0x510 }>;
            type Dir = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x514 }>;
            type DirSet = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x518 }>;
            type DirClr = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x51C }>;
            type Latch = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x520 }>;
            type DetectMode = RegisterAtAddress<{ RegisterMode::RW }, { $base_address + 0x524 }>;
            type PinCnf =
                RegisterBlockAtAddress<GpioPort, { RegisterMode::RW }, { $base_address + 0x700 }>;
        }
    };
}

generate_gpio_instance_impl!(GpioInstanceI0, 0x5000_0000);
generate_gpio_instance_impl!(GpioInstanceI1, 0x5000_0300);

GpioPort is a simple enum with hardcoded discriminants:

#[derive(PartialEq, Eq, Copy, Clone)]
pub enum GpioPort {
    P00 = 00,
    P01 = 01,
    P02 = 02,
    P03 = 03,
    P04 = 04,
    P05 = 05,
    P06 = 06,
    P07 = 07,
    P08 = 08,
    P09 = 09,
    P10 = 10,
    P11 = 11,
    P12 = 12,
    P13 = 13,
    P14 = 14,
    P15 = 15,
    P16 = 16,
    P17 = 17,
    P18 = 18,
    P19 = 19,
    P20 = 20,
    P21 = 21,
    P22 = 22,
    P23 = 23,
    P24 = 24,
    P25 = 25,
    P26 = 26,
    P27 = 27,
    P28 = 28,
    P29 = 29,
    P30 = 30,
    P31 = 31,
}

impl RegisterBlockIndex for GpioPort {
    fn index(&self) -> usize {
        (*self) as usize
    }
}

After all this we can finally define the Type that will be used as a handle throughout the system:

pub struct GpioPin<Instance, const PORT: GpioPort> {
    _private: (),
    _phantom: PhantomData<Instance>,
}

as well as the trait that defines what AnyGpioPin actually can be used for:

pub unsafe trait AnyGpioPin {
    type Instance: GpioInstance;
    const PORT: GpioPort;

    const PORT_MASK: u32 = Self::PORT.get_port_mask();

    unsafe fn cfg_pin(&mut self, config: &GpioPinConfig) {
        <<Self as AnyGpioPin>::Instance as GpioInstance>::PinCnf::write(
            &Self::PORT,
            config.build_value(),
        );
    }

    fn is_low(&self) -> bool {
        let value = unsafe { <<Self as AnyGpioPin>::Instance as GpioInstance>::In::read() };
        value & Self::PORT_MASK == 0
    }

    fn out_set(&mut self) {
        unsafe {
            <<Self as AnyGpioPin>::Instance as GpioInstance>::OutSet::write(Self::PORT_MASK);
        }
    }

    fn out_clr(&mut self) {
        unsafe {
            <<Self as AnyGpioPin>::Instance as GpioInstance>::OutClr::write(Self::PORT_MASK);
        }
    }

    fn is_set(&self) -> bool {
        let value = unsafe { <<Self as AnyGpioPin>::Instance as GpioInstance>::Out::read() };
        value & Self::PORT_MASK != 0
    }
}

(This is still a bit ugly due to all the fully qualified trait syntax, but I'm not sure yet how to improve this)

Last but not least we implement the trait for the handles:

unsafe impl<Instance: GpioInstance, const PORT: GpioPort> AnyGpioPin for GpioPin<Instance, PORT> {
    type Instance = Instance;
    const PORT: GpioPort = PORT;
}

And then it seems to just work :smiley: and it could even be UB free, at least no more weird reference <-> raw pointer conversions :slight_smile:

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.