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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 6/17/24 07:32, Paolo Bonzini wrote:
> On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
> <manos.pitsidianakis@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.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<()>.
>>>>
>>>>
>>
>