On 6/17/24 07:32, Paolo Bonzini 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.
You are raising an interesting point, and should be a central discussion
when designing the future Rust API for this subsystem.
It may not be intuitive to people that even a code without any unsafe
section could still call code in a sequence that will violate some
invariants, and break Rust rules.
IMHO, this is one of the big challenge with the Rust/C interfacing,
including for Linux.
But it's *not yet* the goal of this series. First, we should see how to
build one device (I would even like to see a second, to factorize all
the boilerplate), and then, focus on which interface we want to have to
make it really better than the C version.
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.
I think that Manos did a great amount of work to reduce the use of
unsafe code. Basically, he wrote the device as Rusty as possible, while
still using the QEMU C API part that is inevitable today.
In more, given the current design, yes some race conditions are possible
(depends on how QEMU calls callbacks installed), but a whole class of
problems is still eliminated.
From the start of this series, it was precised that focus was on build
system, and it seemed to me that we shifted on the hot debate of "How to
interface with C code?".
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.
I agree, it should be the next direction after a first device was
written: How to remove almost all usage of unsafe code and maintain good
invariants in the API?
While talking about idiomatic, Rust tends to favor message passing to
memory share when it comes to concurrency (and IMHO, it's the right
thing). And it's definitely now how QEMU is architected at this time.
With extra locking, we should be able to have a first correct interface
that offers strong guarantees, and we can then work on making it faster
with concurrency.
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...
Not the same, you still get other compile/runtime guarantees:
- strong typed enums, so no case is forgotten
- good error handling
- safe type conversions
- bound check of fifo access
The only open issue by calling unsafe code in this context is related to
(potential) concurrency. If a first step to have a good Rust API is to
run everything under a lock, there is good chance you already started to
design the device in the right way to support real concurrency later, so
it's still useful.
Pierrick
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<()>.
