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
> >
> >I think we're actually in a great position. We have a PoC from Marc-André,
> >plus the experience of glib-rs (see below), that shows us that our goal of
> >idiomatic bindings is doable; and a complementary PoC from you that will
> >guide us and let us reach it in little steps. The first 90% of the work is
> >done, now we only have to do the second 90%... :) but then we can open the
> >floodgates for Rust code in QEMU.
> >
> >For what it's worth, in my opinion looking at glib-rs for inspiration is
> >> a bad idea, because that project has to support an immutable public
> >> interface (glib) while we do not.
> >>
> >
> >glib-rs includes support for GObject, which was itself an inspiration for
> >QOM (with differences, granted).
> >
> >There are a lot of libraries that we can take inspiration from: in addition
> >to glib-rs, zbus has an interesting approach to
> >serialization/deserialization for example that could inform future work on
> >QAPI. It's not a coincidence that these libraries integrate with more
> >traditional C code, because we are doing the same. Rust-vmm crates will
> >also become an interesting topic sooner or later.
> >
> >There's more to discuss about this topic which I am open to continuing
> >> on IRC instead!
> >>
> >
> >Absolutely, I will try to post code soonish and also review the build
> >system integration.
> >
> >Thanks,
> >
> >Paolo
> >
> >
> >> -- Manos Pitsidianakis
> >> Emulation and Virtualization Engineer at Linaro Ltd
> >>
> >> >
> >> >One thing that would be very useful is to have an Error
> >> >implementation. Looking at what Marc-André did for Error*
> >> >(
> >> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/20210907121943.3498701-13-marcandre.lur...@redhat.com/
> >> ),
> >> >his precise implementation relies on his code to go back and forth
> >> >between Rust representation, borrowed C object with Rust bindings and
> >> >owned C object with Rust bindings. But I think we can at least have
> >> >something like this:
> >> >
> >> >// qemu::Error
> >> >pub struct Error {
> >> > msg: String,
> >> > /// Appends the print string of the error to the msg if not None
> >> > cause: Option<Box<dyn std::error::Error>>,
> >> > location: Option<(String, u32)>
> >> >}
> >> >
> >> >impl std::error::Error for Error { ... }
> >> >
> >> >impl Error {
> >> > ...
> >> > fn into_c_error(self) -> *const bindings::Error { ... }
> >> >}
> >> >
> >> >// qemu::Result<T>
> >> >type Result<T> = Result<T, Error>;
> >> >
> >> >which can be implemented without too much code. This way any "bool
> >> >f(..., Error *)" function (example: realize :)) could be implemented
> >> >as returning qemu::Result<()>.
> >>
> >>
>
