Re: [RFC PATCH v2 3/5] rust: add PL011 device model

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Mon, 17 Jun 2024 14:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>Il lun 17 giu 2024, 10:59 Manos Pitsidianakis <
>manos.pitsidianakis@linaro.org> ha scritto:
>
>> >qdev_define_type!(c"test-device", TestDevice);
>> >impl ObjectImpl for TestDevice {}
>> >impl DeviceImpl for TestDevice {}
>> >
>> >fn main() {
>> >    let d = TestDevice::new();
>> >    d.cold_reset();
>> >}
>> >
>> >Of course the code makes no sense but it's a start.
>>
>> Let's not rush into making interfaces without the need for them arising
>> first. It's easy to wander off into bikeshedding territory; case in
>> point, there has been little discussion on the code of this RFC and much
>> more focus on hypotheticals.
>>
>
>I see your point but I think it's important to understand the road ahead of
>us.
>
>Knowing that we can build and maintain a usable (does not have to be
>perfect) interface to QOM is important, and in fact it's already useful for
>the pl011 device's chardev. It's also important to play with more advanced
>usage of the language to ascertain what features of the language will be
>useful; for example, my current implementation uses generic associated
>types which are not available on Debian Bookworm—it should be easy to
>remove them but if I am wrong that's also a data point, and an important
>one.
>
>Don't get me wrong: *for this first device* only, it makes a lot of sense
>to have a very C-ish implementation. It lets us sort out the build system
>integration, tackle idiomatic bindings one piece at a time, and is easier
>to review than Marc-André's approach of building the whole QAPI bindings.
>But at the same time, I don't consider a C-ish device better just because
>it's written in Rust: as things stand your code has all the disadvantages
>of C and all the disadvantages of Rust. What makes it (extremely) valuable
>is that your code can lead us along the path towards reaping the advantages
>of Rust.

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.

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.

>
>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.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@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<()>.
>>
>>