-
Notifications
You must be signed in to change notification settings - Fork 150
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
Do the current semantics for referencing registers interfere with composability? #151
Comments
@kjetilkjeka Thanks for bringing the topic up. I've never too happy about the way we handle Preamble: on safetyI think it's important to remember that safety in Rust means memory safety and that race Thus an API like this is totally (memory) safe: // generated by svd2rust. Usart1 has interior mutability.
static USART1: &'static Usart1 = /* .. */;
fn isr1() {
USART1.dr.write(0xAA);
}
fn isr2() {
USART1.dr.write(0x55);
} It is racy but it's memory safe (there's no data race) per Rust definition. The example above is no different than this one: static FOO: AtomicUsize = ATOMIC_USIZE_INIT;
fn isr1() {
FOO.store(FOO.load(Ordering::Relaxed) + 1, Ordering::Relaxed)
}
fn isr2() {
FOO.store(FOO.load(Ordering::Relaxed) * 2, Ordering::Relaxed)
} Which is also memory safe (no Both examples have no data races because the read / write operations, in isolation, are performed in What I'm trying to say with this example is that An old ideaOK. So what should we do here? The problem of race conditions arises from peripherals having I think it may be worthwhile to retry an idea that I discarded some time ago because it's not zero // # svd2rust generated code
// proxy
pub struct USART1 { _0: () }
impl ops::Deref for USART1 {
type Target = usart1::RegisterBlock;
fn deref(&self) -> &usart1::RegisterBlock {
unsafe { &*(0x4000_0000 as *const _) }
}
}
// omitted: DerefMut implementation
pub mod usart1 {
pub struct RegisterBlock { sr: Sr, dr: Dr, /* more registers */ }
}
// here's the singleton "constructor"
pub fn usart1() -> Option<USART1> {
static USED: Mutex<Cell<bool>> = Mutex::new(Cell::new(false));
let free = interrupt::free(|cs| {
let used = USED.borrow(cs);
if used.get() {
false
} else {
used.set(true);
true
}
});
if free {
Some(USART1 { _0: () })
} else {
None
}
}
// # application code
fn main() {
// get peripheral singleton instance
let usart1 = stm32f103xx::usart1().unwrap();
// let usart1_alias = stm32f103xx::usart1().unwrap(); // would `panic!`
// construct driver
let serial: Serial<USART1> = Serial::new(usart1);
// do your thing
} (Alternatively we could make the singleton "constructor" return Basically we move from a global singleton with synchronized access ( Leaving the non zero cost problem aside. Recent improvements in RTFM (japaric/cortex-m-rtfm#43) open app! {
resources: {
static SERIAL: Serial<Usart1>;
},
tasks: {
USART1: {
path: usart1,
resources: [SERIAL],
},
}
}
fn init() -> init::LateResources {
let usart1 = stm32f103xx::usart1().unwrap();
let serial = Serial::new(usart1);
init::LateResources {
SERIAL: serial,
}
}
fn usart1(_t: &mut Threshold, r: USART1::Resources) {
r.SERIAL.write(r.SERIAL.read().unwrap()).unwrap();
} This also means that your driver would now be able to hold state and preserve it across task runs. Making it zero costThe singleton constructor I showed above is certainly non zero cost but I think that with the // svd2rust generated code
static mut USED: bool = false;
pub fn usart1() -> Option<USART1> {
let free = interrupt::free(|cs| {
if unsafe { USED } {
false
} else {
unsafe { USED = true; }
true
}
});
if free {
Some(USART1 { _0: () })
} else {
None
}
}
// NOTE(unsafe) aliases the singleton. Must be called once.
pub unsafe fn _usart1() -> USART1 {
// NOTE this will be mutated within a critical section (see below)
USED = true;
USART1 { _0: () }
}
// rtfm generated code
fn main() {
interrupt::free(|_| {
let usart1 = unsafe { stm32f103xx::_usart1() };
let p = stm32f103xx::Peripherals { USART1: usart1, /* more peripherals */ };
init(p);
});
}
// user code
fn init(p: stm32f103xx::Peripherals) {
let serial = Serial::new(p.USART1);
// let usart1 = stm32f103xx::usart1(); // this would `panic!`
} I think LLVM is smart enough to optimize away static variables that are only written to as it's Shared peripherals?This non-global singleton approach lets you "hide the existence of peripherals" (for the most part fn exti0(t: &mut Threshold, r: EXTI0::Resources) {
r.DMA1.claim(t, |dma1| r.SERIAL1.write_all(dma1, some_buffer));
}
fn exti1(t: &mut Threshold, r: EXTI0::Resources) {
r.DMA1.claim(t, |dma1| r.SERIAL2.write_all(dma1, some_buffer));
} Can't think of a way to hide the Thoughts on the above proposal? cc @hannobraun @thejpster Interior mutabilityInterior mutability was required for RTFMv1 because it was not safe / sound to get a mutable At this point I'm not sure what the "immutable" ( re (a) you mentioned this
This makes sense in this particular case but in general the hardware may change the contents of a re (b) if you know that a read operation on a register modifies its contents should You had some questions as well:
Because it's memory safe to do so.
They are certainly racy but so are statically allocated atomics as shown in the second example
Then, I think, you don't have any way to use peripherals unless you are using RTFM. This would not
It would signal the wrong message. They are as memory safe to use as atomics, even if racy.
You mean having a // `free` needs to be tweaked a little for `get_mut` to work at all
interrupt::free(|cs: CriticalSection| {
let usart1: &mut Usart1 = USART1.get_mut(&mut cs);
interrupt::free(|cs: CriticalSection| {
let aliased_usart1: &mut Usart1 = USART1.get_mut(&mut cs);
// two mutable references to the same memory region; this is undefined behavior
});
}); |
Let me try to refine my argument of why the const references to peripherals are unsafe.
The nomicon lists 8 bullet points for what should be considered undefined behaviour. One of them is "Causing a data race" and another one is "Breaking the pointer aliasing rules". As you point out the consts should not be made unsafe based on them beeing racy. But I think they do break the pointer aliasing rule. Let's look at an analog example based on "normal memory" rather than memory mapped IO. (Same example on playground)
Creating this U32Cell would be sound if we knew that there could never exist mutable references to this location in memory. When it comes to memory, it's obvious that all bets are off since the compiler might generate references. When it comes to memory mapped IO we know that the compiler won't generate references, but we don't know what other crates will do. This means that by making these consts safe we either:
Since we cannot make assumptions about libraries out of our control (as this would break composability) we're actually (potentially) breaking the pointer aliasing rules by exposing these consts safely. It's, of course, the argument that "No one should safely hand out mutable references to memory mapped registers when they don't know if others will do the same". But this could easily be extended to "No one should safely hand out references to memory mapped registers when they don't know others will hand out mutable references to registers", reaching a contradiction. I like your idea of turning the global singleton into a scoped singleton. But I won't be able to think through everything and write up an intelligent comment tonight. So I will rather save it for one of the upcoming days. But I will comment one more thing tonight
Let's overlook the fact that it's (allegedly) breaking the aliasing rules. If I were writing a TCP wrapper that had two equivalent send methods, As long as only one was used everything would work fine, but when they were used together there would be a chance of deadlocking (depending on racy stuff). It would be totally safe to expose such interface. Wouldn't it still be a bad idea? |
No, they are not. To explain it, it's better to read language reference, not a nomicon https://doc.rust-lang.org/reference/behavior-considered-undefined.html It has a precise definition on what is allowed and what is not. For example
No it would be unsound in any case, because your example is breaking rules 5 and 6:
Also there is a note in the docs:
So in a nutshell, you are not allowed to have more than one &mut during execution of a function, but you are allowed to mutate data using non mutable (shared) reference if your data is inside UnsafeCell. Our registers all have UnsafeCell inside, thats why it is OK to mutate them using shared reference. That is why our current model is sound. |
Thanks for making the UD and "safe rust" more clear to me @pftbest. There's just one more thing I don't understand, that is: Doesn't the argument of exposing const peripheral in an |
I think modelling our types in the way you describe is highly desirable, but I really don't see how this could be achieved on the register level (see @japaric's comment about the hardware changing registers under our nose). I think this should be left to a higher-level layer, maybe an implementation of embedded-hal. I think at a higher level, you could effectively use
This looks very good to me. I really like how you can use the This follows from the "must be called once" requirement, but maybe it should be made clear in the documentation that the One caveat: I've only taken a cursory glance at RTFM so far. I think I fully understand your proposal, but maybe I'm missing some implication.
I've definitely seen this. The LPC82x UART error flags clear on read (see user manual, section 13.6.3). |
@japaric, I like your "non-global singleton" idea. If I've understood things correctly, it's assumed that libraries generated with svd2rust will be the only way people are going to access hardware from rust. If someone were to make a "competing" library with other semantics then this singleton might not be a singleton after all. This will be the case as long as memory mapped IO is not handled in the language itself. The end game should probably be to have some sort of register allocation onto some address space that is compile-time guaranteed to only happen once for each compilation. To do this (less safely) in runtime, for now, totally makes sense.
This is the case for some registers. On the other side, you have registers that can only be mutated by CPU (not peripheral). For these, it might be valuable to have a It's also known how peripherals can mutate registers, making it possible to write drivers on top of as long as you know that you're the only part of code that have access to the registers. If you need interior mutability it won't be a problem to introduce this on a layer above the code generated with svd2rust.
What I'm currently thinking:
yes!
It would probably work fine since most people only use one driver at the time for their peripherals anyway. Perhaps it could let some bugs through in the cases of peripherals used for different drivers (GPIO, Power, Clock gating). It would be better to just let some reads mutate without a mutable reference than letting all writes do it as well.
In theory, yes! The svd specification requires This is definitely going to be the case in practice as well, as we all know how good chip manufacturers are to write up to spec SVD 🙄
This is not only a benefit for RTFM, but rather all rust code targetting embedded as it's enabling composability.
RTFM doesn't have monopoly on macros calling
@hannobraun, this is exactly what I'm talking about. The problem is that with the current code generated from svd2rust code can safely mess up the module settings and it is thus impossible to construct an interface with such guarantees. |
@kjetilkjeka Sorry, but I don't see where would you get the Also even when there is only one library present, there are still many corner cases. How would you share the same register between two drivers? Suppose we have i2c peripheral and spi peripheral and their clocks are in the same clock control register, how would it work? How would you share a There is also a danger for users that might want to do more than a library has provided. For example we have an SPI library that consumes |
Yes, handing out My argument is: If it's ok for svd2rust to hand out references under the assumption that it's the only library doing it, then it must be ok for other libraries as well. And if you're handing out references to registers under the assumption that you're the only one doing it, you might as well hand out Well, it actually can be done safely. But it would require global analysis. This can be done in the If constructing the
Most of the time they will only need non-mutable references after the initialization is done. This means that I can do the init and hand out If several drivers needs In your specific example, I would assume that the clock control driver handed out some
I wouldn't say that this is the worst price to pay. Unless you're doing something mega hacky you should probably fork the driver and add the feature anyway. Alternatively, if it's a config (and the library doesn't clear registers), you could just set it before handing off the |
I don't think there is such assumption. You can have two or more copies of svd2rust generated peripherals in your application and it would work fine. There is nothing special in svd2rust that requires a guarantee that there is only one copy of it. |
You're right, the assumption is rather "No other libraries hands out references to registers without putting them in an UnsafeCell". How do we know that is the case? I guess the almost good enough answer is "This can interfere with other libraries. And they shouldn't do that without doing a global analysis first. But they can't do a global analysis since they're libraries. So they shouldn't do it at all". But the same argument is applicable with handing out references inside an UnsafeCell as well. You should do the global analysis to see that no one hands out these references without putting them in an It's like having a closed down one-way street. You can drive down it in the night because there are not any cops around. And you're totally fine even if other people drive down it as well. Except when a guy wants to drive it the wrong way since the streets closed down and there shouldn't be any traffic anyway. I for sure know who is "the bigger idiot" but it doesn't make the guy driving down a closed down street right. Any of these actions could only be made safe if the driver had the ability to do a global analysis (see every other cars intention) |
Well, I still think that this is also not the case here. I think svd2rust would work correctly even if &mut library present. svd2rust doesn't care about other libraries, it would be compiled correctly in any case. |
I'm partially agreeing. I'm also partially thinking that this is (in a more subtle way) the same as handing out a pointer of an |
Honestly, I'm perfectly fine with an API that just promises to work correctly, as long as the user makes sure nothing else touches the peripheral. |
I'm inclined to agree. But my main problem is that other libraries might mess things up as well. A real-life example: On the other hand, wouldn't be amazing if I could configure the pll first and give a read-only reference (without interior mutability) to the can driver. It would both know how the pll was configured and know that as long as it held this reference no one could modify the pll. It's not like it's impossible to write embedded systems without these features. We've (probably) all written embedded systems in C. But I see this as an opportunity to get some "bang for my bucks" using Rust. And I don't see being able to mutate everything in a critical section as a specially good idea or valuable feature. |
@kjetilkjeka It might be possible to combine static analysis of the Rust source code with static analysis of the compiled program and use device-specific knowledge about the address space, to determine if there are any rogue actors. If such a tool existed, that would be great, but it seems more like a long-term vision, if anything. All I'm saying is this:
Why does it need to be a a reference to the PLL register(s) though? Write a driver for the PLL. Tell the user (via documentation) that only that driver is allowed to access the PLL. Pass references to the driver (mutable or immutable, as required) to your other drivers. Of course, those drivers need to cooperate for this to work, and I think embedded-hal is a good first step here. If we can do more to help on the svd2rust level (like @japaric suggested above), I'm all for it. |
Rust is pretty young language and on the infant stage for use in embedded. Isn't it now we should ask ourself what features that would be constructive to have for development close to hardware? When linking to a c library all bets are off. That's why calling c functions are "unsafe". The library author for this code needs to make sure it's safe. Any library can also mess anything up by calling unsafe and provoke UB. So let's not nail everything down to the lowest level. But let's not accept status quo, discuss this thoroughly and try to make it even better than it currently is. Jorge Aparicio suggested a way to remove the static peripheral definitions that create a more composable interface that will make it harder to create unwanted race conditions. This is great! All I'm asking is, what if we had a compiler plugin that could hand out references, would this be 1: possible or impossible, 2: better or worse?
It doesn't have to be a reference. But for this to work the library writers need to agree that there needs to be some control of who is allowed to do what. I think having a reference is an intuitive way to express "who is allowed to do what". The compile-time checking is also a nice feature. Handing out mutable references in critical sections does the exact opposite. It sends the message that you're allowed to do everything in the world(like reconfiguring the pll) as long as you do it in a critical section. Using a reference is just my suggestion to how one could use the ownership model to disallow reconfiguration. If you think that using a reference will make things worse, please explain. If you think there's a better way to solve the problem, please explain. If you think this is just an inconvenience and not really a problem, and would prefer to not fix the inconvenience, please explain. Removing the static peripheral definitions is probably of higher importance than the move away from interior mutability. This will (hopefully) force people writing drivers to at least "ask for permission" before changing it.
I support the idea of cooperating against making things consistent and nice on the driver level. But I also think we should improve things when possible on the register/peripheral level. Someone is going to have to write the drivers implementing the embedded-hal traits, and they're going to appreciate this effort. Sometimes the traits in My point is that just because things are consistent on the driver level we shouldn't forget to make things nice on the lower levels as well. |
I fully agree. I never meant to inhibit discussion, and I apologize if it came across that way. All I meant to express is that there exists a solution that is, in my opinion, "good enough for now". That doesn't mean we shouldn't think about better solutions.
I think there has been a misunderstanding at some point. I wasn't arguing at all against using the ownership model. I was merely pointing out that you can use the ownership model and references on the HAL/driver layer (whatever you want to call it), even if the registeres themselves use interior mutability.
I agree. |
(Sorry that I've been away from this discussion for several days)
I also want to note another problem: even if the hardware doesn't modify some registers in parallel
Definitively. Actually I didn't properly document the safety requirements of calling that
Thanks for providing an example.
Indeed. That's a problem. You don't even need a competing library to run into this problem. This can #[no_mangle] // important attribute
static mut USART1: bool = false;
pub fn usart1() -> Option<USART1> {
// uses the `USART1` static as a guard
}
pub unsafe fn _usart1() -> USART1 {
USART1 = true;
USART1 { _0: () }
} This way the This unmangled symbol idea could be extended to competing libraries but it would require some sort
From what I have heard A language feature would be an ideal solution but I don't think this (peripheral / register block
This still has problems. See the "spooky action at a distance" problem mentioned at the top.
How would you model the "spooky action at a distance" phenomena using Note that my main concern here is not being able to do this correctly, and in an automated
TIL. Unfortunately only 23 of 548 files in the SVD database are making use of this field. If you |
I believe this has been fixed by the new singletons approach (cf. #158) |
Memory mapped IO is a completely different animal than the normal memory. It may mask bits when reading/writing, it oftens changes values of registers "asynchronously", may be inherently stateful and may even trigger hard faults (or similar) when a read is attempted. It's not given how rusts notion of safety should be extended or extrapolated into something meaningful when hardware access is thrown into the mix.
Code generated with svd2rust currently exposes two interfaces to the hardware. One of them is the static
Peripheral
structs. These give a strong feeling of interior mutability and lets you safely (no unsafe) borrow one in exchange for aCriticalSection
token. This is great! Except for when it isn't. Which unfortunately is a lot of the time. What I'm referring to is the composability problem ofLet's look at number two first. Imagine a token representing that a clock source has been activated for a peripheral. To use the peripheral you would have to present such token with a longer lifetime than the peripheral. The first problem is that, with safe rust, the peripheral would have lifetime of the
CriticalSection
and so would theClockSource
token and the actual peripheral as well. This would force you to either reinitialize everything the next time you're going to use the unit (which makes the library *useless in practice) or always do everything in a critical section all the time (which makes the library *useless in practice).The other way to access IO is by generating a struct with references to all io. With this method, you don't have a present a
CriticalSection
token. This is obviously unsafe for the following reasons:CriticalSection
badge and they will be able to mess up the hw module without calling unsafe code.My first question is. Why do we generate these Peripherals that will safely trade a
CriticalSection
for register access? It seems like they are of limited use in practice and by allowing this as "safe behavior" destroys composability by making it safe for a library to change the state of io registers as long as they have a critical section.Is this not a threat to composability? What is the drawback of removing these constants? What is the drawback of making them unsafe to use?
The next problem I see is that there is no way to get a mutable reference to the registers. Let's look at the clock gating example again. If several peripherals share a common clock source it would make sense to require a read reference so they can't be changed while the peripheral is in use. This only holds as long as you need to use a mutable reference to mutate the registers. Due to never being able to get a mutable reference It's currently impossible to write the drivers in such way (without using
transmute
).What would be the drawback of the
Peripheral
struct containing mutable references instead of non-mutable references?I see that "state altering" calls in
std
often doesn't require mutable references, this in turns requires more error handling capabilities (no functions are assumed "not fail" and thus have to return aResult
). An example isstd::net::TcpStream
. I guess it could be achievable by checking all relevant configurations before doing anything, but i don't think it will be worth it.(I'm aware that you can currently create several
Peripheral
structs. It's not ideal, but due to requiring unsafe code it's ok. And in the future, mechanisms for ensuring that only one mutable reference exists to each register can be implemented, theapp!
macro from rtfm seems like a step in the right direction)*(Exaggeration for artistic purposes. The library is not useless, it's actually great! The safe constant definitions is a bit weird though.)
The text was updated successfully, but these errors were encountered: