Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we have VolatileCell #411

Open
chorman0773 opened this issue Jun 6, 2023 · 50 comments
Open

Can we have VolatileCell #411

chorman0773 opened this issue Jun 6, 2023 · 50 comments
Labels
A-dereferenceable Topic: when exactly does a reference need to point to regular dereferenceable memory?

Comments

@chorman0773
Copy link
Contributor

This question is intended to replace #33 and #265, and ask the general question of whether or not we can have a type, either user-defined or language-provided, that, when wrapped in a shared reference, guarantees that no accesses are introduced at the operational semantics level that are not part of the original program (and thus, if volatile accesses only are used, an implementation won't introduce any speculative reads).
If such a type can exist, the second question is what is the operational semantics of retagging as a shared reference to such a type.

@RalfJung RalfJung added the A-dereferenceable Topic: when exactly does a reference need to point to regular dereferenceable memory? label Jun 6, 2023
@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2023

I think the question whether we "can" have is fairly straight-forward -- yes, we can totally do that. I think it wouldn't be too hard either, operationally speaking. For instance we could say that &T where T contains a VolatileCell is exempt from all forms of dereferencability requirements. Most likely those requirements will stem from the aliasing model, not any kind of validity of value representations, but at least in Tree Borrows it would be trivial to skip any kind of "fake read" that might otherwise check a reference for being dereferenceable. (In fact right now Tree Borrows already skips the fake read for &!Frozen, and relies on other parts of Miri checking dereferenceability, but in MiniRust that would not work.)

For UnsafeCell there is a lot of demand for more precise tracking, reflexing in the opsem that only parts of memory allow shared mutation. Does that even make sense for VolatileCell? When doing MMIO, I assume it's always the entire memory block that needs to be treated volatile, and there's no other non-volatile memory behind the same reference?

@Dirbaio
Copy link

Dirbaio commented Jun 6, 2023

Is this something that is actually needed?

The main use case is making "register layout structs" in Peripheral Access Crates (PACs):

struct MyRegisterBlock {
   foo: VolatileCell<u32>,
   bar: VolatileCell<u32>,
   baz: VolatileCell<u32>,
}
struct VolatileCell<T: Copy>(UnsafeCell<T>);
impl<T> VolatileCell<T> {
   fn read(&self) -> T { self.0.get().read_volatile() }
   fn write(&self, val: T) { self.0.get().write_volatile(val) }
}

let regs: &MyRegisterBlock = &*(0x4000_0000 as *const MyRegisterBlock);
regs.foo.write(0x42);

However, you can accomplish the same thing by staying entirely within raw pointers:

struct MyRegisterBlock(*mut u32);
impl MyRegisterBlock {
   fn foo(&self) -> Reg<u32> { Reg(self.0.add(0)) }
   fn bar(&self) -> Reg<u32> { Reg(self.0.add(1)) }
   fn baz(&self) -> Reg<u32> { Reg(self.0.add(2)) }
}
struct Reg<T: Copy>(*mut T);
impl<T> Reg<T> {
   fn read(&self) -> T { self.0.read_volatile() }
   fn write(&self, val: T) { self.0.write_volatile(val) }
}

let regs: MyRegisterBlock = MyRegisterBlock(0x4000_0000 as _);
regs.foo().write(0x42);

Pros and cons:

  • The latter clearly avoids all issues with speculative reads, since it never creates references.
  • They're both equally ergonomic to use. The only change is .foo vs .foo().
  • The former is shorter to write: you get rustc to lay out the struct for you, instead of having to do pointer maths. However, I don't think this is an advantage, because PACs are usually autogenerated with tools like svd2rust or chiptool, rarely written by hand.
  • The struct approach breaks down anyway if registers overlap (can happen if same address has different meaning based on some mode, or if the same reg can be accessed as e.g. u16 and u32). svd2rust has code to detect that, and fall back to generating code similar to the raw pointer approach.

So overall, I don't think there's any reason to prefer the struct approach.

in Embassy we've been using PACs generated by chiptool, which generates code using raw pointers, out of concerns for the dereferenceable / speculative reads issue. It's been working well so far, we haven't found anything that it can't do that the struct approach could.

So IMO it's not worth it to add exceptions to the memory model to support usages like VolatileCell. It's not needed, you can do everything fine without.

@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2023

I'm an outsider here and totally happy with not having VolatileCell, if the domain experts say that the status quo is totally sufficient.

@Lokathor has opinions on this topic, IIRC.

@chorman0773
Copy link
Contributor Author

I specifically would like VolatileCell as it does make exposing MMIO via magic symbols (rather than magic addresses) a bit more ergonomic IMO.

@Lokathor
Copy link
Contributor

Lokathor commented Jun 7, 2023

My opinions are:

  • The voladdress crate has types that store usize+PhantomData<T> because that makes it easy to work with in const contexts. It turns the usize into an actual pointer at the last possible moment, allowing almost everything except the read and write ops themselves to be const fn.
  • My crate works extremely well for me personally. I'm entirely satisfied with it. If you want to find someone who needs more from the language than what is currently possible you'll have to look around. Perhaps @phil-opp has a strong opinion? They're in charge of the volatile crate and have been tinkering with the design of a next version for a while now I think.

@Dirbaio I think what people "really want" is something that also has magical field projection so that you can do code like this:

#[repr(C)]
pub struct Controls {
  display_control: u16,
  display_status: u16,
  vcount: u16,
}

pub const MMIO: &VolatileCell<Controls> = whatever_expression_here!(0x0400_0000);

fn main() {
  // magical field projection from `&VolatileCell<Controls>` into `&VolatileCell<u16>`
  let display: &VolatileCell<u16> = &MMIO.display_control;

  // call some method to assign some value
  display.write(1);

  // or we can chain the expression
  let y: u16 = MMIO.vcount.read();
}

I think without some sort of field projection thing added to the language it's less compelling, though possibly still useful even then. Also note that the above example doesn't bother with when things are unsafe or not, which things are readable or writable, etc. There's a lot of design that can go into an mmio abstraction type.

@chorman0773 can you say more about that linker stuff? I think you told me once but I've forgotten if you did, and more details about that might make a more compelling case for needing VolatileCell or might explain what constraints might apply.

@chorman0773
Copy link
Contributor Author

can you say more about that linker stuff

Yes. In SNES-Dev, I use linker scripts to define access to the MMIO registers. For stuff that is specific to SNES-Dev, the address is even actualy known at compile time, as it is generally assigned by the linker itself, and given a special type in the mapping table.

@Lokathor
Copy link
Contributor

Lokathor commented Jun 7, 2023

That part sounds similar to GBA dev, but with GBA it's generally done with just const values, no linker involved. I'm unclear on why you'd want to have the linker involved at all.

@thejpster
Copy link

@Dirbaio in your example, MyRegisterBlock is no longer a ZST. Can that approach work without holding on to the pointer?

@phil-opp
Copy link

phil-opp commented Jun 7, 2023

Thanks for the mention!

As far as I know, volatile cells are still commonly used in the Rust embedded an OS ecosystems. For example:

Most of these projects are aware that this approach is not sound, but still haven't changed their code. I assume that it's a mix of personal preference (structs are much easier to write than doing manual pointer arithmetic) and migration cost (requires breaking changes across the entire ecosystem). So I think that there is definitely demand for a sound VolatileCell type.

@phil-opp
Copy link

phil-opp commented Jun 7, 2023

@Dirbaio

  • The former is shorter to write: you get rustc to lay out the struct for you, instead of having to do pointer maths. However, I don't think this is an advantage, because PACs are usually autogenerated with tools like svd2rust or chiptool, rarely written by hand.

I agree that it's less of an issue when the pointer arithmetic is generated by tools. However, I think most people write things by hand first and then create tools to autogenerate the boilerplate code later using the same design. So the design that is easier to write by hand is often used for code generation too. For example, I think svd2rust generates code that is based on the volatile_register crate, at least for Cortex M.

@bjorn3
Copy link
Member

bjorn3 commented Jun 7, 2023

  • The voladdress crate has types that store usize+PhantomData<T> because that makes it easy to work with in const contexts. It turns the usize into an actual pointer at the last possible moment, allowing almost everything except the read and write ops themselves to be const fn.

That is not compatible with CHERI, right? For CHERI you did at least have to store a raw pointer to preserve the full capability.

@Dirbaio
Copy link

Dirbaio commented Jun 7, 2023

@thejpster: in your example, MyRegisterBlock is no longer a ZST. Can that approach work without holding on to the pointer?

in the struct approach, you use &MyRegisterBlock, which is pointer-sized.
in the raw pointer approach, you use MyRegisterBlock, which is pointer-sized.

a HAL wouldn't store a MyRegisterBlock, just like it doesn't store a &MyRegisterBlock with today's PACs. They store zero-sized owned singletons instead, that can be deref'd / converted to a &MyRegisterBlock/MyRegisterBlock. Both approaches are equally memory-efficient and generate the same ASM (I've personally checked with svd2rust vs chiptool)

@phil-opp Most of these projects are aware that this approach is not sound, but still haven't changed their code. I assume that it's a mix of personal preference (structs are much easier to write than doing manual pointer arithmetic) and migration cost (requires breaking changes across the entire ecosystem). So I think that there is definitely demand for a sound VolatileCell type.

from the Embedded WG's perspective, we were definitely aware of this and studying possible solutions (it's one of the reasons chiptool exists for example). The push for it has essentially stopped once rust-lang/rust#98017 was merged, which meant VolatileCell is no longer unsound in the current implementation. It was not an "okay, now let's push to make VolatileCell sound in the memory model" decision, it was more like "okay, it's less urgent now, and we don't have much manpower so let's leave it for now".

Moving to pointers is still something I'd personally like to see done, due to the other advantages I mention in this thread. (this is my personal opinion though, not the WG's)

@phil-opp: I agree that it's less of an issue when the pointer arithmetic is generated by tools. However, I think most people write things by hand first and then create tools to autogenerate the boilerplate code later using the same design. So the design that is easier to write by hand is often used for code generation too. For example, I think svd2rust generates code that is based on the volatile_register crate, at least for Cortex M.

my experience is the contrary, everyone starts adding support for a new chip by grabbing a .svd file from the vendor and running it through svd2rust. .svd files are widely available: ARM forces vendors to provide .svd's and it's become some sort of de-facto standard so many risc-v vendors also offer them.

I disagree reg structs are easier to write by hand, too. Documentation always says "register FOO is at offset 0x4c". Translating that to pointer math is trivial, you just add 0x4c. Translating that to a struct you find yourself counting registers manually to infer at which offset each field is, and manually calculating sizes of dummy padding hole arrays.

I think people write registers structs simply because that's how it's always been done in C. There's a good reason to use structs in C: you can use fields with regs->foo syntax, but not methods with regs->foo() syntax. So you can't do the "raw pointers + methods" approach in C. People come to embedded Rust come from C, so that's the first thing they tried in Rust, it somewhat worked, and then it stuck despite the soundness concerns.

@Lokathor
Copy link
Contributor

Lokathor commented Jun 7, 2023

That is not compatible with CHERI, right? For CHERI you did at least have to store a raw pointer to preserve the full capability.

You may be right. I've never seen a hardware manual for any CHERI device.

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2023

The push for it has essentially stopped once rust-lang/rust#98017 was merged, which meant VolatileCell is no longer unsound in the current implementation.

That is concernying from a t-opsem perspective, this is certainly not the outcome we hoped for when resolving the Arc issue that way. :/

Anyway, from my perspective -- what's missing here is a VolatileCell RFC. This doesn't seem like a gap in the language that needs solving to gain some crucial expressiveness, so I don't have writing such an RFC anywhere on my own roadmap. t-opsem would be involved only insofar as we'd make the memory model actually support the desired semantics, but until such an RFC is accepted, I think this is S-status-not-opsem.

@chorman0773
Copy link
Contributor Author

chorman0773 commented Jun 7, 2023

That part sounds similar to GBA dev, but with GBA it's generally done with just const values, no linker involved. I'm unclear on why you'd want to have the linker involved at all.

For the SNES-Dev specific parts, they don't have a fixed address and just live where the mapping table puts them. BEing able to float that arround the rest of the cartridge mapped memory allows for more compact programs and more effective bank-sensitive relaxations. And having the SNES stuff also be defined in the linker, depsite having fixed addresses, is good for consistency.

That is concernying from a t-opsem perspective, this is certainly not the outcome we hoped for when resolving the Arc issue that way.

I'd say there were a lot of consequences from removing dereferenceable from &impl !Freeze from a perspective of extra programs working. I'd say the usefulness of being able to deallocate a &AtomicU64 outweighs the unintended widening of scope on llvm. I would note to my friends with the Embedded Working Group that indeed it is not yet correct to write VolatileCell in rust via UnsafeCell. rustc-llvm is overly permissive here because llvm can't represent exactly the permissiveness that the model presently allows.

@mkroening
Copy link

A VolatileCell with field projections would be wonderful!

Our use case is writing MMIO drivers based on manually written structs. We are currently in an unsound situation in our codebase, anticipating helpers to make volatile accesses easier before rewriting everything with raw pointers.

It would be a blessing if the language allowed to write let mut foo: &mut VolatileCell<u8> = &mut foo.bar (with foo: VolatileCell<Foo>) instead of having to reinvent field projections and indices in libraries (Volatile::map_mut and Volatile::index_mut from rust-osdev/volatile#29).

I would be genuinely interested in driving this forward and perhaps writing an RFC. The right way to start drafting would be the t-lang stream in Zulip, right? 🤔

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2023

Field projections would also be useful for Pin and &Cell, so yeah that would be quite wonderful indeed. And yes this is a t-lang thing. I think proposals have already been posted on IRLO before, not sure how far any of them went.

@digama0
Copy link

digama0 commented Jun 7, 2023

latest I am aware of is rust-lang/rfcs#3318

@Dirbaio
Copy link

Dirbaio commented Jun 7, 2023

Field projections is orthogonal to the issue at hand though?

The question is "do we add some special case to opsem to guarantee no speculative reads on & references pointing to volatile memory?". If the answer to that is "no", then VolatileCell is unsound, either with or without field projections.

@Lokathor
Copy link
Contributor

Lokathor commented Jun 7, 2023

The answer isn't just a yes or no, it's actually "we don't right now, but we could do that, but it's non-zero work to do, and the work won't be of much value without also having field projection, so we should evaluate the cost/benefits with all that in mind"

@digama0
Copy link

digama0 commented Jun 7, 2023

As Ralf said, the opsem side of things is pretty cut-and-dried. If lang approves a VolatileCell type, then there will be some fairly straightforward changes to the opsem to treat &VolatileCell<T> basically like *const T. Users cannot implement VolatileCell themselves because of the opsem changes, but I don't think we will have much to say about it. This is mostly an API design question, the opsem changes are clear.

@Dirbaio
Copy link

Dirbaio commented Jun 7, 2023

ah okay! I see the plan now, thanks for clarifying. Just to confirm:

  • The current vcell::VolatileCell that uses UnsafeCell internally is unsound and will always be unsound?
  • The only ways to soundly do MMIO are either
    • using raw pointers all the way, or
    • using a future VolatileCell in core that's not built out of UnsafeCell, but is instead magically blessed by the compiler to guarantee no spurious reads?

@mkroening
Copy link

mkroening commented Jun 7, 2023

[...] Just to confirm [...]

I think so, yes.

the work won't be of much value without also having field projection

As an VolatileCell might be less worthwhile with the field projections RFC since you might be able to implement an ergonomic wrapper over raw pointers, let's talk about the VolatileMem type from the RFCs Guide-level explanation:

pub struct VolatileMem<T> {
    ptr: NonNull<T>,
}

impl<T: Copy> VolatileMem<T> {
    pub fn get(&self) -> T;
    pub fn put(&mut self, val: T);
    pub fn map<F: Field<Base = T>>(self) -> VolatileMem<F::Type>;
}

get and put are fairly straightforward, I think. Several things come to mind, though:

  1. Are there downsides to modeling a VolatileCell by carrying around a pointer? The additional indirection in &VolatileMem feels more complex than necessary to me.

  2. Can a library type with map be as ergonomic as a type provided by the language? This is essentially config.map::<field_of!(Config, mode)>() vs &mut config.mode.

    Edit: wrapper->field is a future possibility.

  3. Can a library type with map be correct while still being sound and liberally allowing mapping to fields?

    Any solution should be possible to correctly model references. This makes the example actually unsound, if I am not mistaken:

    let mut config = magic();
    let mut mode = config.map::<field_of!(Config, mode)>();
    
    // Essentially two mutable references conceptually, allowing us to create a data race:
    thread::spawn(|| config.put(Config::new()));
    thread::spawn(|| mode.put(Mode::new()));

    Edit: Nevermind, map takes self by value, but this makes multiple mappings impossible.

    An ideal solution should allow creating multiple mappings at the same time to different fields while still being checked by the borrow checker regarding mutability and lifetimes. This is something that @phil-opp's current draft for VolatileRef is also lacking if I am not mistaken.

    This topic reminds me a bit of view types though that would be of a different scope, since the internal field would be the same for a VolatileMem pointer type.

    As this is relevant for the projections RFC, I'll comment on there as well.

@RalfJung
Copy link
Member

RalfJung commented Jun 8, 2023

Looks like we should have a thread for VolatileCell API design somewhere -- this thread here was intended for the opsem questions around it, which are quite orthogonal.

(Or we declare the opsem parts resolved and re-purpose this here about API design -- including whether that design even needs a VolatileCell type.)

@kellda
Copy link

kellda commented Nov 19, 2023

Has anyone considered using MaybeUninit instead or in addition to UnsafeCell ? Would that be a sound solution ?

My understanding is that UnsafeCell<T> is a union of () and T, which means that a reference to one could at most read 0 bytes from the address (as () is zero sized, it can't be assumed that more bytes are valid/readable). I may be wrong through.

Maybe a new type is needed to say "this is not actually dereferenceable" (in addition to UnsafeCell/"this is not actually constant" and MaybeUninit/"this is not actually initialized").

(I'm sorry if this has already been answered or if that's not the right place to ask)

edit: seems that Redox is using MaybeUninit

@RalfJung
Copy link
Member

MaybeUninit has no effect on dereferencability or mutability of references to it, so for this issue here it is irrelevant.

@bravely-beep
Copy link

Has there been any interest in making progress on VolatileCell? It seems to be a reasonably frequently-requested feature, and apparently there aren't any opsem issues with the type existing. This isn't something that can be easily simulated outside of the compiler either.

@chorman0773
Copy link
Contributor Author

I think you'd need to post some kind of proposal (maybe an RFC) that involves T-lang and probably T-libs-api.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025

There are some open opsem questions though -- specifically, how should types like &(VolatileCell<i32>, i32) behave? Do they still have to be dereferenceable for offsets 4..8?

@Lokathor
Copy link
Contributor

Lokathor commented Jan 8, 2025

I think that without some sort of field projection and/or indexing projection system (which is being discussed elsewhere, but which I think hasn't even turned into an RFC yet), then VolatileCell can't really provide a significant improvement over the current capabilities.

@bravely-beep
Copy link

Field projection can be (awkwardly) implemented in userspace via macros (e.g. the munge crate), though I'm not sure how much of an ergonomics gain VolatileCell would be in this case. It might be just as ergonomic for the user to declare a VolatilePtr<'a, T> that wraps a raw pointer, enforces lifetimes on it, and exposes volatile access methods.

There is one capability I'm imagining that VolatileCell could potentially provide which isn't currently manually implementable by users: a VolatileCell::from_mut(value: &mut T) method, similar to the currently unstable method on UnsafeCell. This method would temporarily suspend the compiler's permission to insert spurious writes to the pointee, without requiring the &mut T to be dropped or invalidated. I have no idea what technical or opsem barriers there are to implementing something like this though, and its use-case is quite niche (currently, inline assembly is required to do things like this).

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025

This method would temporarily suspend the compiler's permission to insert spurious writes to the pointee,

I don't think we can have such an operation without LLVM exposing it as a new primitive. If memory is ever dereferenceable, then we consider it to always remain dereferenceable; this is not something you can ever undo (except maybe with something like rust-lang/rfcs#3700, but that cannot be soundly used on a reference you got from someone else).

@Dirbaio
Copy link

Dirbaio commented Jan 8, 2025

I don't think we should have VolatileCell. Reasons:

  • As I've commented before, you can already do everything today with raw pointers + volatile_read/volatile_write.
  • It adds complexity to the language (it's a new "magic" type with special opsem treatment by the compiler)

IMO the "make a repr(C) struct that matches the register layout" thing is a C-ism that isn't needed in Rust. People do it in C because it's the only way to get ergonomic register access (C doesn't have methods, so the only way to get PERIPHERAL.some_reg syntax is struct fields!) and people are trying to carry that to Rust just by inertia, because that's how it's always been done.

Also there's been some news from svd2rust since I last posted here:

  • Today it still uses volatile cells, but the fields are now private and you always access regs by methods like peripheral.some_reg(). So, it isn't getting the syntax benefits of using fields+volatilecell anymore (the new syntax could be implemented with raw pointers as well). It's been out for a while and so far the extra () hasn't been a big deal.
  • there's some consensus that moving it towards using raw pointers would be good, and some implementation work (see here, here)

@Lokathor
Copy link
Contributor

Lokathor commented Jan 8, 2025

Yeah the voladdress crate basically already does that VolatilePtr thing, though it's intended for static mmio access so it doesn't do lifetimes on its own. And it's a great crate, right until you want to manage a device by declaring a whole repr(C) struct with one field per control. Then the lack of field projection bites you.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025

Wouldn't field projections (at least the kind proposed on rust-lang/rfcs#3735) work for a VolatilePtr type? So then one could do C-style layout declaration for MMIO register groups. The only overhead would be an extra load()/store() rather than primitive pointer syntax.

@Lokathor
Copy link
Contributor

Lokathor commented Jan 8, 2025

Yes, that's basically what I was saying: VolatileCell / VolatilePtr / WhateverYouCallIt would be an improvement with field projection capability, but it is not an improvement until then.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025

I am quite confused by this reply. This discussion is about whether VolatilePtr (which can already be defined by users as a newtype around NonNull or *mut) is enough, or whether adding VolatileCell would add significant benefits. What I said is it seems like VolatilePtr + field projections already give all the ergonomics that VolatileCell could hope to provide. This was in reply to your earlier point saying VolatileCell requires field projections to be ergonomic.

IOW:

  • without field projections, VolatileCell isn't fun to use (which is the point you made)
  • with field projections, VolatileCell doesn't beat VolatilePtr (which disagrees with what I think you implied)

I think that without some sort of field projection and/or indexing projection system (which is being discussed elsewhere, but which I think hasn't even turned into an RFC yet), then VolatileCell can't really provide a significant improvement over the current capabilities.

Which improvements can it provide with field projections, given that VolatilePtr with field projections already seems to be pretty nice?

@mkroening
Copy link

The Rust OSDev community has also moved away from Cell-style wrappers to raw pointer-based wrappers (newer versions of the volatile crate), which works well in my experience. Once we have field projections, we can hopefully move away from our macro-based workarounds (map_field and VolatileFieldAccess). So I think we are very much okay without a VolatileCell.

@bravely-beep
Copy link

Assuming we have field projections, one area where VolatileCell would provide ergonomics improvements is with reborrowing. Using VolatilePtr<'a, T> / VolatilePtrMut<'a, T> types, reborrowing would need to be done manually by calling a function:

impl<'a> VolatilePtrMut<'a, T> {
    fn reborrow<'b>(&'b mut self) -> VolatilePtrMut<'b, T> {
        todo!()
    }
}

Using VolatileCell with ordinary references would allow standard implicit reborrowing to be used.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025

I assume VolatilePtr is Copy since this is anyway shared state, so reborrowing should not usually be needed?

@bravely-beep
Copy link

bravely-beep commented Jan 8, 2025

Yeah, VolatilePtr to shared memory would be Copy, but since we don't have volatile atomic operations, you'd need a non-Copy VolatilePtrMut in order to build abstractions that allow safe volatile writes.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025

since we don't have volatile atomic operations

We should definitely fix that.

@Dirbaio
Copy link

Dirbaio commented Jan 8, 2025

since we don't have volatile atomic operations, you'd need a non-Copy VolatilePtrMut in order to build abstractions that allow safe volatile writes.

The current implementation guarantees volatile ops up to a single word are compiled to a single LDR or STR instruction, so they're already atomic. I'm not sure if rust opsem formally guarantees this or not? if not, it should. Otherwise pretty much all existing MMIO code is broken.

So, as long as you only write words (which is usually the case when you do MMIO) you don't need exclusive access to make it sound, it's not UB. (with some caveats, you need extra conditions so it's not UB from the hardware, such as making accesses in uncahced device memory to avoid cache coherency issues. If you're doing MMIO you already fulfill these conditions)

@bravely-beep
Copy link

bravely-beep commented Jan 8, 2025

From the std docs:

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 write_volatile and any other operation (reading or writing) on the same location is undefined behavior.

Not sure if this can be removed and things will "just work"

@Lokathor
Copy link
Contributor

Lokathor commented Jan 8, 2025

Yeah those docs are kinda old and conservatively designed, they're more like a lower bound on what's possible to be confident about right now.

We could definitely work to improve the situation.

@bravely-beep
Copy link

It's probably worth noting as an aside that it may be desirable to create an exclusive borrow of a MMIO register for API design reasons (statically enforcing invariants etc.)

@Lokathor
Copy link
Contributor

Lokathor commented Jan 8, 2025

Honestly we should make creation of the VolatileWhatever be unsafe so that usage can be kept safe as often as possible. That plus an atomic flag or whatever and you've got your "lock this device" api designed already.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025 via email

@Dirbaio
Copy link

Dirbaio commented Jan 8, 2025

I meant "single ldr/str" instruction does make it "atomic" in the sense as "two racing word-sized reads/writes to the same memory address are not UB". So, similar to Relaxed atomics. This guarantee is enough to make register access APIs safe without needing exclusive/mutable references or owned singletons.

and about "how it interacts with the Rust memory model", the answer is just "it doesn't"? you never mix volatile accesses and regular accesses to MMIO registers. If you do want it to interact then yes you do need "real" volatile atomics. (for example do regular writes to a buffer then do an volatile atomic Release write to a DMA register to ensure DMA start happens after the buffer is filled)

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2025

Yeah those docs are kinda old and conservatively designed, they're more like a lower bound on what's possible to be confident about right now.

We could definitely work to improve the situation.

The docs still reflect the current status though. We'd have to change our LLVM codegen to make these operations actually atomic, but that doesn't work with our current API -- my hope is that we can use bytewise atomic volatile memcpy once rust-lang/rfcs#3301 becomes a thing.

I meant "single ldr/str" instruction does make it "atomic" in the sense as "two racing word-sized reads/writes to the same memory address are not UB". So, similar to Relaxed atomics.

What the hardware does is not the point. These operations are not marked as atomic in the memory model so they must not race.

It may be possible to carve out an exception for locations which are never pointed-to by a reference and never accessed using non-volatile loads/stores. (Not sure if we have to worry about inbounds pointer arithmetic.) We don't have a guarantee from LLVM for this case AFAIK though. And as @bravely-beep noted, such accesses can still not establish synchronization the way atomic accesses can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dereferenceable Topic: when exactly does a reference need to point to regular dereferenceable memory?
Projects
None yet
Development

No branches or pull requests