Rewrite crate to operate on reference values

[phil-opp]

This PR rewrites the crate completely. Instead of wrapping the value directly, we now wrap a reference. This way, we can implement volatile operations on slices, which is for example important for operating on pixel-based framebuffers since writing each pixel individually would be to slow (the compiler is not allowed to combine multiple volatile writes).

Since some methods require unstable features and thus only work on nightly, this PR also introduces a cargo feature named unstable. This way, users can opt-in to the unstable functionality if they like, but otherwise this crate works fine on stable (without the slice methods).

[Freax13]

This makes wrapping struct fields in Volatile a lot more complicated doesn't it? Previously one could write something like this:

#[repr(C)]
struct HardwareMemoryMappedFoo {
    bar: Volatile<u32>,
    baz: Volatile<u16>,
}

ig it's still possible to just take references to each field individually, but that seems needlesly complicated.

i still like the idea of taking a reference though. perhaps this could be solved by making two different Volatile types

[phil-opp]

Good point! Two different types sound like a good solution.

[phil-opp]

After thinking a bit more about this, I think a better solution is to add a map method that allows to turn a Volatile<&mut SomeStruct> into a Volatile<&mut FieldOfSomeStruct>. This way, you can wrap a reference to the whole HardwareMemoryMappedFoo struct into Volatile and then use map and then read/write some of its fields. The advantage of this over making each field Volatile is that it is now also possible to read/write the complete struct in a single volatile operation, which can be better optimized than multiple smaller volatile operations (each field individually).

The drawback is that the map method gives you access to the raw &mut/& reference, so that you can write the value without volatile. This isn't unsafe (memory safety violation is not possible), but it is still something you normally don't want to do. I think it should be ok if we document this clearly.

[phil-opp]

I pushed a new version with an implementation of the map/map_mut. I published a test version on crates.io as version 0.4.0-alpha.00. The rendered documentation is available on docs.rs.

[phil-opp]

I decided to merge this for now. The new implementation is already much more powerful than the old and I think the struct access problem is also resolved with the new map/map_mut methods.

[cdecompilador]

Maybe it would be better to have a Volatile wrapper for values and another wrapper like VolatileRef for slices, this change breaks some code, for example your OS tutorial, when I tried to use newer versions of the libraries.

[phil-opp]

See #15 for some discussion about this. I'm aware that this was a breaking change and requires restructuring your code. However, I don't think that there are common use cases that only work with the old API, so I decided against adding an additional value-based type.

Since the version number was increased to 0.4.x for this change, cargo recognizes that this version is not semver-compatible with versions 0.3.x, so no code should be broken unless you manually update the specfied version in your Cargo.toml.

[zyklotomic]

Is there an easy way to do the following

#[repr(C)]
struct myStruct {
  rwValue: Volatile<u8>,
  rValue: Volatile<u8, ReadOnly>
}

where you have a mix of different access controls for the struct members? I'm aware of the map method but this would not work if you directly used Volatile::new() on a struct right? The new() (or new_read_only, etc) method will create a Volatile struct with all the inner values having the same second generic parameter A used in PhantomData correct*?

It definitely is not a big deal, just a question on ergonomics. I can always fall back to just using RW as a default.

Edit: *that is isomorphic to a struct with all inner values having the same second generic parameter to be pedantic, just in case.

@zyklotomic I just created to https://github.com/rust-osdev/volatile/pull/19 to add methods to restrict access. With this you could do something like struct_instance.map(|s| &s.rValue).read_only(). You could then add a MyStructAccess trait with some getter/setter methods and implement this for Volatile<&mut myStruct>:

trait MyStructAccess {
    fn rValue(&self) -> Volatile<&u8, volatile::access::ReadOnly>;
}

impl MyStructAccess for Volatile<&mut myStruct> {
    fn rValue(&self) -> Volatile<&u8, volatile::access::ReadOnly> {
        self.map(|s| &s.rValue).read_only()
    }
}

Or you could add a custom ReadOnlyU8 wrapper type around u8 and implement some custom access trait for it.

Would this work for you?

[zyklotomic]

Yup that works! I also came to the conclusion that I would have to move the access abstraction to a secondary getter/setter interface and expose methods accordingly, but I like the way your PR handles it even more since it removes that layer of indirection by letting you use the same existing methods in the crate.

The second option sounds good too, time will tell if people like using that too, but the PR one feels cleaner imo.

I also wanted to say thanks for all your dedication to this crate (as well as to blog_os). I really appreciate it as your resources have been invaluable to me as a student.

[phil-opp]

Great to hear that! :)

[bjorn3]

I am pretty sure this PR is incorrect. References are marked by rustc as dereferencable which tells LLVM that it is fine to insert spurious (non-volatile) reads.

https://llvm.org/docs/LangRef.html#parameter-attributes

dereferenceable(<n>) This indicates that the parameter or return pointer is dereferenceable. This attribute may only be applied to pointer typed parameters. A pointer that is dereferenceable can be loaded from speculatively without a risk of trapping. The number of bytes known to be dereferenceable must be provided in parentheses. It is legal for the number of bytes to be less than the size of the pointee type. The nonnull attribute does not imply dereferenceability (consider a pointer to one element past the end of an array), however dereferenceable() does imply nonnull in addrspace(0) (which is the default address space), except if the null_pointer_is_valid function attribute is present. n should be a positive number. The pointer should be well defined, otherwise it is undefined behavior. This means dereferenceable() implies noundef.

In addition for immutable references passed as argument the noalias metadata tells LLVM that the value will never change for the duration of the function call, which makes the volatile reads kind of useless.

I think you can solve this problem by requiring an UnsafeCell wrapper around the value and avoiding references in favor of raw pointers within the implementation of this crate.

[phil-opp]

@bjorn3 Thanks a lot for voicing your concerns! I think you're right that we should avoid using reference when operating on volatile values. I'm not sure how LLVM prioritizes between volatile operations and noalias/defereferencable attributes (e.g. does it still keep all volatile loads/stores even for noalias values?), so it's probably best to not risk triggering undefined behavior.

I started to create an UnsafeCell-based Volatile type in https://github.com/rust-osdev/volatile/pull/22 as a potential solution. It would be great if you could take a look!