New design based on `*mut T`

[phil-opp]

TODO:

• [x] update tests
• [ ] update docs

cc https://github.com/rust-osdev/volatile/pull/13#issuecomment-842455552

[bjorn3]

@RalfJung volatile accesses to &UnsafeCell<T> are fine, right?

[RalfJung]

volatile accesses to &UnsafeCell are fine, right?

I mean, the access itself is fine, but if you are doing MMIO then you don't want to use a reference. References are considered "dereferencable" and the compiler may insert spurious reads. See e.g. here for more details.

[phil-opp]

@RalfJung Thanks for taking a look! I pushed a new commit that switches from &UnsafeCell<T> to *mut T as the base type. Thus it should no longer be considered as dereferencable, right?

See e.g. here for more details.

The problem with pushing the volatile to the lowest level (e.g. struct fields) is that it doesn't work well for slices/arrays. For example, we don't want to write each byte of a [u8] slice through a separate volatile instruction since this would harm performance.

What I'm trying with this crate is to allow putting the volatile wrapper around on the outer level and then providing methods to narrow it down to specific slice ranges (through Index/IndexMut impls) or struct fields (through the addr_of_mut! macro). I'm also trying to provide volatile versions of slice fill/copy methods using intrinsics such as volatile_copy_nonoverlapping_memory and volatile_set_memory.

[RalfJung]

I pushed a new commit that switches from &UnsafeCell to *mut T as the base type. Thus it should no longer be considered as dereferencable, right?

Yes. Basically, "use raw pointers and only raw pointers" is currently the only option for spec-compliant MMIO in Rust.

But I'm really not the expert here, it'd be great if @Lokathor could take a look. :)

[Lokathor]

I will have a look later today if I can get the chance, but if you want a comparison point you can look at yourself the voladdress crate is currently what I use on the GBA. It provides abstraction types for single addresses, compact block of addresses, and strided series of addresses.

Edit: oh, you've probably already read my crate though XD

[josephlr]

It seems like you could simplify the marker traits/structs to avoid duplication (and making things like ReadWrite just be an alias for a more generic type):

pub struct NoAccess;
pub struct UnsafeAccess;
pub struct SafeAccess;

pub trait Unsafe {}
pub trait Safe: Unsafe {}

impl Unsafe for UnsafeAccess {}
impl Unsafe for SafeAccess {}
impl Safe for SafeAccess {}

pub struct Access<R, W> {
    pub read: R,
    pub write: W,
}

pub type ReadOnly = Access<SafeAccess, NoAccess>;
pub type WriteOnly = Access<NoAccess, SafeAccess>;
pub type ReadWrite = Access<SafeAccess, SafeAccess>;

Then the implementation would be fairly straightforward:

impl<T: ?Sized> Volatile<T> {
    pub const unsafe fn new<A>(pointer: *mut T) -> Volatile<T, A> {
        Volatile { pointer, access: PhantomData }
    }
    pub const unsafe fn from_ptr(pointer: *const T) -> Volatile<T, ReadOnly> {
        Volatile::new(pointer as *mut T)
    }
    pub const unsafe fn from_mut_ptr(pointer: *mut T) -> Volatile<T, ReadWrite> {
        Volatile::new(pointer)
    }
}

impl<T, Read, Write> Volatile<T, Access<Read, Write>> {
    pub unsafe fn read_unsafe(&self) -> T
    where
        Read: Unsafe,
    {
        ptr::read_volatile(self.pointer)
    }

    pub fn read(&self) -> T
    where
        Read: Safe,
    {
        unsafe { self.read_unsafe() }
    }

    pub unsafe fn write_unsafe(&self, val: T)
    where
        Write: Unsafe,
    {
        ptr::write_volatile(self.pointer, val)
    }

    pub fn write(&self, val: T)
    where
        Write: Safe,
    {
        unsafe { self.write_unsafe(val) }
    }

    pub unsafe fn update_unsafe(&mut self, f: impl FnOnce(&mut T))
    where
        Read: Unsafe,
        Write: Unsafe,
    {
        let mut value = self.read_unsafe();
        f(&mut value);
        self.write_unsafe(value);
    }

    pub fn update(&mut self, f: impl FnOnce(&mut T))
    where
        Read: Safe,
        Write: Safe,
    {
        unsafe { self.update_unsafe(f) }
    }
}

[phil-opp]

@josephlr

I thought about removing the ReadWrite etc types too, but unfortunately, type aliases cannot be used as type constructors. But methods like the following could work:

pub fn read_only() -> Access<SafeAccess, NoAccess> {
    Access {
        read: SafeAccess,
        write: NoAccess,
    }
}

[Lokathor]

unfortunately, type aliases cannot be used as type constructors

I'm not sure what this means. Do you mean if you use an alias like

pub type MyPhantom = PhantomData<u8>;

then you can't use MyPhantom to make the value? That's easy enough to fix, just add a const.

#[allow(non_upper_case_globals)]
pub const MyPhantom: MyPhantom = PhantomData;

[josephlr]

That's easy enough to fix, just add a const.

#[allow(non_upper_case_globals)]
pub const MyPhantom: MyPhantom = PhantomData;

I think we should either:

• Remove new_with_access (and thus the need to even have Access values)
  • Then Access can just be pub struct Access<R, W>(R, W);
• Define associated constants for Access instead of const fn methods.

  impl Access<SafeAccess, NoAccess> {
      pub const READ_ONLY: ReadOnly = Access {
          read: SafeAccess,
          write: NoAccess,
      };
      pub const WRITE_ONLY: WriteOnly = Access {
          read: NoAccess,
          write: SafeAccess,
      };
      pub const READ_WRITE: ReadWrite = Access {
          read: SafeAccess,
          write: SafeAccess,
      };
  }

• Define a single Access::new() method:

  pub struct Access<R, W>(PhantomData<(R, W)>);
  
  impl<R, W> Access<R, W> {
      const fn new() -> Self {
          Self(PhantomData)
      }
  }

  • Then you just can use ReadOnly::new() when you need a value.

[Freax13]

Is it UB to use two VolatilePtr's with ReadWrite access pointing to the same address (assuming single threaded execution)?

[Lokathor]

Nope.

In fact in some cases one could argue that's "the point".

[Freax13]

That also implies that VolatilePtr<T, ReadWrite> can never be Send regardless of T.

[Lokathor]

It's possible for there to be device specific rules, but generally you can send and use the pointer from the other thread. Access is not synchronized on its own, you have to rig that up yourself if you're going to use it in both threads at once.

Having it not be send by default and allowing people to override the default is maybe the safe move there.

[josephlr]

So would that mean that VolatilePtr<T> is Send and Sync iff T is Sync? (as VolatilePtr behaves like a mutable reference).

[Freax13]

VolatilePtr doesn't behave like a mutable reference because it's not UB to have two VolatilePtrs pointing to the same address. This also means it can't be Send: If it were one could create two VolatilePtrs pointing to the same address, send one to another thread and create a race condiditon eg. by writing using both pointers from different threads at the same time. This only applies to ReadWrite access though.

I think it would be sound to implement Sync if T is Sync.

[Lokathor]

Precise wording note: Rust allows race conditions (which are not UB), just not data races (which are UB).

[mkroening]

Is there anything I can do to help this move forward? :)

[josephlr]

Is there anything I can do to help this move forward? :)

It's mostly a question of if we can get this PR small enough so that I, @Lokathor, or someone else can review the API.

We would probably also want to prototype some use-cases with this API to make sure things are ergonomic.

[josephlr]

VolatilePtr doesn't behave like a mutable reference because it's not UB to have two VolatilePtrs pointing to the same address. This also means it can't be Send: If it were one could create two VolatilePtrs pointing to the same address, send one to another thread and create a race condiditon eg. by writing using both pointers from different threads at the same time. This only applies to ReadWrite access though.

I think it would be sound to implement Sync if T is Sync.

I think in the interest of getting this review though, we should postpone discussions of Send/Sync for a followup issue/PR. For the initial implementation, we should NonNull's lead. The type should be unconditionally !Send and !Sync. We can always relax the constraints later.

[Lokathor]

This PR is rather old and since it was opened I've experimentally added a "volatile region" type to voladdress: https://github.com/rust-console/voladdress/blob/main/src/volregion.rs

So, that's just how I thought things could work. Maybe it can help as a point of reference. I set it up so that it's never a dynamically sized type, because it's a pain in the butt and adds essentially zero value for my purposes.

The type should be unconditionally !Send and !Sync. We can always relax the constraints later.

This is generally the right call, because (unlike Copy) if someone "knows what they're doing" and wants to override your decision they can just go ahead and do that.

[Freax13]

Is there a good way to make splitting mutable borrows work? We could maybe achieve this by implementing destructuring in a macro and getting access to the fields that way though it's not quite the same a splitting borrows.

[Lokathor]

Short answer: Not with just a volatile type.

Fundamentally, rust considers a mutable borrow exclusive, and volatile is used on things that you never have exclusive control over.

So if you want to build some sort of access control layer to the API using the borrow checker, you should build it "above" this base layer of abstraction.

[Freax13]

Fundamentally, rust considers a mutable borrow exclusive, and volatile is used on things that you never have exclusive control over.

As of right now, we never assert that we have exclusive control of the memory, but we require mutable references to a VolatilePtr to allow writing to the memory location. What I want is a bit different: I'd like to be able to get two VolatilePtrs to two different fields of a struct pointed to by a VolatilePtr. Or put even more concretly: I want this code to work somehow:

[Lokathor]

but we require mutable references to a VolatilePtr to allow writing to the memory location.

I simply would not write that code to begin with. These pointers should be Copy and they should take self in methods.

Or put even more concretly: I want this code to work somehow

I completely agree, that code should work.

[Freax13]

I simply would not write that code to begin with. These pointers should be Copy and they should take self in methods.

I explored writing such an api in #28 though @phil-opp correctly pointed out in https://github.com/rust-osdev/volatile/pull/28#issuecomment-1146810733 that that would require VolatilePtr<T> to be !Sync irregardless of T which we should probably avoid if possible.

[Lokathor]

Well according to LLVM it's up to the platform how sync a volatile access is or not, so.... [thumbs up sign while doing a nervous smile] I guess good luck with that if your platform is multi-core and doesn't let them be sync. That sounds terrible to me.

[Freax13]

Well according to LLVM it's up to the platform how sync a volatile access is or not, so.... [thumbs up sign while doing a nervous smile] I guess good luck with that if your platform is multi-core and doesn't let them be sync. That sounds terrible to me.

According to https://doc.rust-lang.org/core/ptr/fn.read_volatile.html#safety data races caused by volatile reads and writes are UB:

Just like in C, whether an operation is volatile has no bearing whatsoever on questions involving concurrent access from multiple threads. Volatile accesses behave exactly like non-atomic accesses in that regard. __In particular, a race between a read_volatile and any write operation to the same location is undefined behavior.__

But yeah, I totally get what you're saying though, for some applications (especially single threaded ones) having a volatile pointer be !Send + !Sync would be completely fine, but for some applications that's not what you want to do. This would likely also be a problem if we wanted to introduce this into the language: We might need to two different volatile pointer types, one that's Send + Sync and one that isn't. Not sure if there's a way to unify both of those into one type.

[Lokathor]

Hmmmmmm, well LLVM has a rather more flexible definition:

Given that definition, R_byte is defined as follows:

• If R is volatile, the result is target-dependent. (Volatile is supposed to give guarantees which can support sig_atomic_t in C/C++, and may be used for accesses to addresses that do not behave like normal memory. It does not generally provide cross-thread synchronization.)
• Otherwise, if there is no write to the same byte that happens before R_byte, R_byte returns undef for that byte.
• Otherwise, if R_byte may see exactly one write, R_byte returns the value written by that write.
• Otherwise, if R is atomic, and all the writes R_byte may see are atomic, it chooses one of the values written. See the Atomic Memory Ordering Constraints section for additional constraints on how the choice is made.
• Otherwise R_byte returns undef.

I think the stdlib docs are actually just in the wrong here. Particularly because it makes interrupt handlers and signal handlers literally impossible to write if the target doesn't have atomics.

[Freax13]

I fully agree that making Volatile a Copy type makes it easier to use. My main concern with this design is whether we can still make the read/write methods safe without causing undefined behavior. If not, there would not be a big advantage in using Volatile instead of using core::ptr::read_volatile directly.

(from https://github.com/rust-osdev/volatile/pull/28#issuecomment-1385588597)

With the current implementation the read and write methods are sound. However, the soundness depends on the VolatilePtr being !Send + !Sync.

[phil-opp]

After reading through this discussion again, I think there are two possible designs. We can either implement Copy or Send, but not both. Implementing Copy makes the type easier to work with in single-thread environments. Implementing Send makes it possible to store the type in a static, behind a Mutex.

Both approaches seem valid to me, so I implemented them both in #29, as a base for further discussion. The question is now: Which type do we prefer? Or does it make sense to provide them both? Please let me know what you think in #29, so that we can finally replace the current unsound implementation.