glitchy has quit [Remote host closed the connection]
glitchy has joined #rust-embedded
bandini has joined #rust-embedded
rainbyte has quit [Read error: Connection reset by peer]
rainbyte has joined #rust-embedded
<thejpster[m]>
is racing on an MMIO register unsound?
<thejpster[m]>
assuming you use volatile reads/write and only use pointers and not references
<thejpster[m]>
(also I think you can s/MMIO register/static mut u32/ and the problem is unchanged)
diondokter[m] has joined #rust-embedded
<diondokter[m]>
It's outside the abstract machine afaik, so who knows! :P
jfsimon has joined #rust-embedded
<diondokter[m]>
I think it's unlikely to break anything, even if it's strictly UB
<thejpster[m]>
my underlying question is - should a freestanding function like SomeArmCP15Register::modify be unsafe. Because concurrent calls to it will lose one of the updates.
<thejpster[m]>
the CP15 register is basically a static mut u32, with fancier assembler syntax.
jfsimon has quit [Client Quit]
jfsimon has joined #rust-embedded
<thejpster[m]>
now if it was some_arm_register.modify(), and some_arm_register was a value of a type that was unsafe to create, no problem. You promised not to race when you made the object.
<thejpster[m]>
but it's only a function
<diondokter[m]>
Well, a static variable is part of the abstract machine and thus you're not allowed to have data races for it
<whitequark[cis]>
i thought it's an architectural register
<diondokter[m]>
diondokter[m]: I guess that function doesn't do a RMW, but only write
<whitequark[cis]>
* are CP15 registers MMIO?
<whitequark[cis]>
* i thought they're architectural registers
<whitequark[cis]>
in this case, I'd say the access itself is always safe (since the compiler can't reason about it at all unless it looks in the inline assembly, and if it does do that it better respect the mrc/mcr semantics), but the function as a whole may be unsafe if a race on this register can violate memory safety as a consequence (let's say you are moving VTOR using an RMW op for some reason, that would definitely be unsafe)
<thejpster[m]>
whitequark[cis]: they are, you read them with an MRC instruction. Hence "fancier assembler syntax". But it just puts a u32 value into a register for you, like a LDR does.
<whitequark[cis]>
well one difference here is that you can't alias those registers, unlike with MMIO
<whitequark[cis]>
the other is that unlike memory they are by definition core-local
<whitequark[cis]>
so really it's closer to a thread-local variable than an MMIO location
<thejpster[m]>
yeah the core-local part is interesting.
<thejpster[m]>
because the only concurrency is an interrupt, which takes many cycles to enter
<thejpster[m]>
but I'm sensing here the general feeling that "losing an update when you called modify re-entrantly is not a memory safety issue", unless the update is a pointer to something and the bad pointer would later be a memory safety issue.
<whitequark[cis]>
yep
<diondokter[m]>
Agreed
<thejpster[m]>
I suppose an MRC/MCR is just a fancy Cell
<diondokter[m]>
Cell doesn't have a modify API though
<thejpster[m]>
yeah :/
<thejpster[m]>
I could just take it away. It's only syntatic sugar for a read/write pair.
<thejpster[m]>
and then the re-entrancy problem would be much more obvious.
raymondr has quit [Read error: Connection reset by peer]
bandini has quit [Quit: WeeChat 4.9.0]
raymondr has joined #rust-embedded
raymondr has quit [Ping timeout: 244 seconds]
<thejpster[m]>
<thejpster[m]> whats with str to [sp, #4] followed by an immediate load? I don't have any volatiles on the stack. My MMIO register address is in r6.
<thejpster[m]>
This is caused by doing a volatile write with a struct type. It becomes a memcpy where both the input and the output are volatile.
<thejpster[m]>
Which is just horrendous for performance and probably means I need to rethink how derive-mmio works with bitbybit::bitfield.
<thejpster[m]>
Does svd2rust have the same problem?
raymondr has joined #rust-embedded
raymondr has quit [Remote host closed the connection]
<adamgreig[m]>
<thejpster[m]> Which is just horrendous for performance and probably means I need to rethink how derive-mmio works with bitbybit::bitfield.
<adamgreig[m]>
huh, can't it convert the bitbybit struct into a primitive value and volatile write that?
<adamgreig[m]>
or is it because the mmio registers struct gets a raw pointer to a bitbybit struct
<adamgreig[m]>
ok, let's begin! any announcements before we start with the agenda?
<JonathanKeller[m>
thejpster: probably the easiest thing to do would be to send a PR to bitbybit to make the struct `repr(transparent)` instead of `repr(C)`, which fixes the codegen and is semantically equivalent since the generated structs only have one member
<adamgreig[m]>
ok, then let's go through agenda, thanks for adding items thejpster
<adamgreig[m]>
do you want to talk about the eabi tests briefly?
therealprof[m] has joined #rust-embedded
<therealprof[m]>
Hm, have we lost him? 😅
<adamgreig[m]>
looks like it 😅 most of the rest of the agenda is various points from him too though
<adamgreig[m]>
question about cortex-m release status: I think we're now very close, I think we need to merge https://github.com/rust-embedded/cortex-m/pull/531 soon which is probably now ready, I wanted to check if we can do without the new proc macro crate for cortex-m somehow, and then i think that's it beyond a quick cargo-semver-checks run
bartmassey[m] has joined #rust-embedded
<bartmassey[m]>
I still have the stale business from before the break of inviting someone from rustlang here for a meeting to talk about the stuff we've piled up on that issue. This might be a good time/reason to do that.
<adamgreig[m]>
bartmassey[m]: the volatile atomics stuff in particular?
<adamgreig[m]>
otherwise, triage (thanks jannic); I think https://github.com/rust-embedded/heapless/pull/655 is probably ok to merge to heapless 0.8 backports if someone from libs wants to have a look?
<bartmassey[m]>
Yes, volatile atomics in particular.
<adamgreig[m]>
and possibly we need to discuss https://github.com/rust-embedded/embedded-hal/issues/719 but i wonder if it's possible to have meaningful cancellation safety on these types in the face of being able to leak the futures, and that might be why we didn't to start with
jannic[m] has joined #rust-embedded
<jannic[m]>
I think cancellation safety is mostly unrelated to what rust usually calls safety, isn't it? It's more like "it works as expected".
<jannic[m]>
Like, if you wrap a read with a timeout, and then put the result in a loop, you don't want to lose characters.
<bartmassey[m]>
I think cancellation safety in this context refers to the possibility of UB when a future is cancelled and a new future is spawned. You don't want to end up with multiple mutable pointers to the same data, each of which could be active. Does that sound right?
<jannic[m]>
That's not what the cited comments are about.
<bartmassey[m]>
Ah.
<adamgreig[m]>
yea, it's as jannic says, more about unexpectedly losing data if you cancel a future
<bartmassey[m]>
Sorry, didn't carefully read the thing. Apologies.
<adamgreig[m]>
as in, a future might pop a byte off a hardware fifo, get cancelled because a timeout became ready first and they were in the same select call, then next time you poll that hardware the byte is gone
<adamgreig[m]>
any idea why we'd allow non-cancel-safe impls in general?
<jannic[m]>
For a single byte read, it's quite unlikely to have a non-cancel-safe implementation: Once the byte is available, you can read it and immediately return it, without another .await point, so there's no way the byte can get lost.
<jannic[m]>
But if you read into a DMA buffer, at the moment the future gets cancelled, you may not even know how many bytes were read from the peripheral and written into the buffer.
<adamgreig[m]>
if you're processing the cancellation you have to stop the dma anyway, at which point you should know how many bytes it transferred?
<adamgreig[m]>
but do you get a chance to tell the user about them?
<jannic[m]>
Yes, but then you are already in drop, so what should you do with that knowledge?
<adamgreig[m]>
right, yea
<adamgreig[m]>
so all the performant implementations are likely to be non-cancel-safe due to dma
<jannic[m]>
You may be technically able to make it cancel-safe by having an additional memory buffer somewhere, and drop could store the already-read bytes there for the next call to read.
<jannic[m]>
But that adds memory overhead and makes drop slow.
<bartmassey[m]>
Do we / could we have a way to explicitly stop the DMA and restore invariants before cancelling the future by dropping? If so, this would be much less concerning I think?
<dirbaio[m]>
Impls can and do stop dma on drop. There's no ub on cancel on any impl I've seen, definitely not in embassy
<bartmassey[m]>
I'm no longer concerned about UB — just thinking that anyone who is about to intentionally cancel but wanted to be able to pick up again could achieve that behavior this way?
<dirbaio[m]>
There's no way to do that if you're dmaing directly to the users buffer
<jannic[m]>
Not without another copy to some buffer owned by the driver.
<dirbaio[m]>
Some bytes might have been read into the buffer but you can't communicate how many to the user since the future doesn't get to complete and return the usize
<bartmassey[m]>
You can report to the user how much of the buffer is valid at the point you stop the DMA, no?
<dirbaio[m]>
So those bytes are lost
<bartmassey[m]>
No, call something before you drop the future.
<dirbaio[m]>
It's still not cancel-safe
<dirbaio[m]>
You're inventing some sort of custom "graceful cancel"
<bartmassey[m]>
But it seems less obnoxiously non-cancel-safe, no? Yes, exactly
<dirbaio[m]>
But the standard cancel (dropping the future) will still cause losing data
<bartmassey[m]>
Yes, absolutely
<dirbaio[m]>
And the cancel can come from many many layers up the call stack, from code that's unaware that underneath there's an uart being read
<dirbaio[m]>
The compiler wires up cancel across the futures stack for you, but it won't wire your custom graceful cancel
<bartmassey[m]>
Correct. Are there examples out there where people want to cancel from many layers up the stack without losing data? I guess I'm probably forgetting some common case.
<dirbaio[m]>
And you can't wire it manually, you'd have to hand write all the futures between the cancel site and the uart, no "async fn"
<bartmassey[m]>
Anyway, sounds like a non-starter for you. Fair enough.
<dirbaio[m]>
bartmassey[m]: The typical example is with_timeout(read_packet(), T)
<dirbaio[m]>
Where read_packet() reads from a uart
<dirbaio[m]>
The only solution is to not do these things, instead run the read packet looop in another task and use a channel or something
<dirbaio[m]>
That being said, read_packet() would typically do many uart read calls. So even if we mandate read() is cancel safe, the whole read_packet() is not
<adamgreig[m]>
Consequently, do you think there's any point having two traits, one that's read safe and one that might not be?
<adamgreig[m]>
s/read/cancel/
<adamgreig[m]>
I wonder if anyone would actually implement the cancel safe version, it would be a pretty serious perf or memory hit
<dirbaio[m]>
If read() was cancel-safe you could in theory write read_packet() in a way that is "resumable" if it's allowed to keep the buffer between calls
<dirbaio[m]>
While if read() itself is not cancel-safe then you have no hope
<jannic[m]>
It could avoid a whole class of bugs. So I think there is a point. Question is whether it's reason enough to solve it on the type system level, or if documentation is sufficient.
<dirbaio[m]>
In practice I've never ran into a problem where I needed read to be cancel-safe. But it is a recurring thing people complain about so I have no idea what to think :D
<dirbaio[m]>
I'm not sure how many bugs it avoids since basically anything you write on top of read will be non-cancel-safe if you write it the obvious way
<dirbaio[m]>
And not always trivial in std. You can't impl these traits on top of io-uring due to the cancel-safe mandate
<dirbaio[m]>
(unless you copy through a temp buffer. Same issue as dma basically)
<adamgreig[m]>
Shame you can't wrap a non-cancel-safe type in something with a temporary buffer to make it cancel safe huh
<adamgreig[m]>
Well, thanks for the input, probably no clear answer to the issue though.
<jannic[m]>
If we now decide that Read does not guarantee cancel safety. Can we later (without a breaking change) add a trait CancelSafeRead: Read that does nothing but documenting "implementations of this trait have a cancel-safe read method"?
<adamgreig[m]>
I don't see why not
<jannic[m]>
So if we learn that there's value in having separate types, they could be added later?
<adamgreig[m]>
And it's non breaking to both the trait crate and impls can just add it as a marker later too, right
<dirbaio[m]>
Yea that'd work. In practice what I suspect would happen is people would not require it and instead ensure they're not canceling reads.
<dirbaio[m]>
Because it's not that hard and you have to do it anyway if you're using e.g. Read_exact
<adamgreig[m]>
Probably because almost all impls turn out to be not cancel safe for embedded huh
<thejpster[m]>
Sorry I was unavailable. I hope the notes I left were useful anyway.
<adamgreig[m]>
They were, thanks!
<adamgreig[m]>
I guess that's all we have time for anyway, thanks everyone!
<jannic[m]>
<thejpster[m]> Sorry I was unavailable. I hope the notes I left were useful anyway.
<jannic[m]>
They were!
glitchy has quit [Ping timeout: 245 seconds]
glitchy has joined #rust-embedded
glitchy has quit [Remote host closed the connection]