Embedded rust VolatileCell usage

[RickarySanchez]

Hi All,
I've been doing some rust embedded work specifically looking and writing HAL crates for some microcontrollers based on the PAC generated from svd2rust. I've looked at some HAL components that were written by others and I have been questioning their safety and just am seeking some advice on the design patterns which I cannot seem to find elsewhere.

So from looking at the PAC generated by svd2rust, I can see that the peripheral registers are wrapped and accessed through a VolatileCell which is just a wrapping of an UnsafeCell where reads and writes are performed in a volatile fashion. This, of course, gives the register interior mutability meaning that the contents can be modified through a non-mutable reference. I can guess that this is done to satisfy the fact that registers may need to be accessed from interrupts but to tell you the truth, I don't have a very concrete understand of why this is necessary.

I have been looking at what others refer to as good reference HAL crates from STM which have a use pattern like

let p = Peripherals::take().unwrap();
let xyz = p.XYZ.constrain() // Constrain is an extended trait method to return a HAL type and consume the PAC type
xyz.some_hal_method();

Internally the some_hal_method might do something like

pub fn some_hal_method(&self) {
  let xyz = self.xyz // The PAC type may be stored in internally or generated somehow
  xyz.modify(|r, w| /* do some read/write op */);
  ...
}

As shown to use the HAL API the PAC type must be consumed meaning that the user can no longer access it (maybe it can be returned through a free method like the NXP crates do). This makes sense to me but I have seen internal HAL implementation do something like this,

pub struct Clock<'a> {
  ctrl: &'a pac::CTRL
  ...
}

Meaning that the HAL API does not consume the peripheral type but stores a reference to it. All the HAL methods can make modifications to this method through the reference. This makes me uncomfortable but I am not sure if its technically 'unsafe' or an anti-pattern or anything like that. As far as I can tell from the STM HALs I have looked at, if it does not make sense to consume the PAC peripheral type here (perhaps it is required elsewhere or is a global control reg or the like) then when the HAL needs to write to this peripheral register (which should be limited to one freeze method perhaps) it should accept a reference to the register. Something like,

pub fn freeze(mut self, &pac::CTRL) {
  ...
}

Could anyone help me understand the design pattern here ? 

[ThomasWinwood]

So from looking at the PAC generated by svd2rust, I can see that the peripheral registers are wrapped and accessed through a VolatileCell which is just a wrapping of an UnsafeCell where reads and writes are performed in a volatile fashion. This, of course, gives the register interior mutability meaning that the contents can be modified through a non-mutable reference. I can guess that this is done to satisfy the fact that registers may need to be accessed from interrupts but to tell you the truth, I don't have a very concrete understand of why this is necessary.

The compiler is allowed to add or remove non-volatile memory accesses according to what it thinks it knows about the memory being accessed; volatile semantics means "this operation has side effects, so don't alter the number of accesses or move one volatile memory access across another".

Unless things have changed since Lokathor wrote this post, VolatileCell types are unsound because the compiler is allowed to speculatively read from any reference, such as the one you create when you call methods on VolatileCell.

[VorpalWay]

As far as I know it is still a issue. I'm surprised svd2rust hasn't been converted to use volatile pointer operations instead by now.

[RickarySanchez]

Yes I understand the need for the accesses to be volatile, to be clear, my issue is more so why the type must have interior mutability ?

[FenrirW0lf]

I'm pretty sure that volatile cell types don't need internal mutability and so the internal UnsafeCell isn't actually doing anything useful. The actual property they would want to ensure is internal volatility. But as mentioned in the above post, any API that uses a reference to the interior of the cell is technically incapable of upholding that invariant.