On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonz...@redhat.com> wrote:
On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
<manos.pitsidiana...@linaro.org> wrote:
I respectfully disagree and recommend taking another look at the code.
The device actually performs all logic in non-unsafe methods and is
typed instead of operating on raw integers as fields/state. The C stuff
is the FFI boundary calls which you cannot avoid; they are the same
things you'd wrap under these bindings we're talking about.
Indeed, but the whole point is that the bindings wrap unsafe code in
such a way that the safety invariants hold. Not doing this, especially
for a device that does not do DMA (so that there are very few ways
that things can go wrong in the first place), runs counter to the
whole philosophy of Rust.
For example, you have:
pub fn realize(&mut self) {
unsafe {
qemu_chr_fe_set_handlers(
addr_of_mut!(self.char_backend),
Some(pl011_can_receive),
Some(pl011_receive),
Some(pl011_event),
None,
addr_of_mut!(*self).cast::<c_void>(),
core::ptr::null_mut(),
true,
);
}
}
where you are implicitly relying on the fact that pl011_can_receive(),
pl011_receive(), pl011_event() are never called from the
MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
mutable references at the same time, one as an argument to the
callbacks:
pub fn read(&mut self, offset: hwaddr, ...
and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
This is not Rust code. It makes no attempt at enforcing the whole
"shared XOR mutable" which is the basis of Rust's reference semantics.
In other words, this is as safe as C code---sure, it can use nice
abstractions for register access, it has "unsafe" added in front of
pointer dereferences, but it is not safe.
Again, I'm not saying it's a bad first step. It's *awesome* if we
treat it as what it is: a guide towards providing safe bindings
between Rust and C (which often implies them being idiomatic). But if
we don't accept this, if there is no plan to make the code safe, it is
a potential huge source of technical debt.
QEMU calls the device's FFI callbacks with its pointer and arguments,
the pointer gets dereferenced to the actual Rust type which qemu
guarantees is valid, then the Rust struct's methods are called to handle
each functionality. There is nothing actually unsafe here, assuming
QEMU's invariants and code are correct.
The same can be said of C code, can't it? There is nothing unsafe in C
code, assuming there are no bugs...
Paolo, first please tone down your condescending tone, it's incredibly
offensive. I'm honestly certain this is not on purpose otherwise I'd not
engage at all.
Secondly, are you implying that these callbacks are not operated under
the BQL? I'm not seeing the C UART devices using mutexes. If they are
not running under the BQL, then gladly we add mutexes, big deal. Just
say this directly instead of writing all these amounts of text. If it's
true I'd just accept it and move on with a new iteration. Isn't that the
point of code review? It is really that simple. Why not do this right
away? I'm frankly puzzled.
Finally, this is Rust code. You cannot turn off the type system, you
cannot turn off the borrow checker, you can only go around creating new
types and references out of raw memory addresses and tell the compiler
'trust me on this'. Ignoring that misses the entire point of Rust. The
statement 'this is not Rust code but as good as C' is dishonest and
misguided. Check for example the source code of the nix crate, which
exposes libc and various POSIX/*nix APIs. Is it the same as C and not
Rust code?
If you have specific scenarios in mind where such things exist in the
code and end up doing invalid behavior please be kind and write them
down explicitly and demonstrate them on code review. This approach of
'yes but no' is not constructive because it is not addressing any
specific problems directly. Instead it comes out as vague dismissive FUD
and I'm sure that is not what you or anyone else wants.
Please take some time to understand my POV here, it'd help both of us
immensely.
Sincerely thank you in advance,
Manos
