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

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

On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonzini@redhat.com> 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.
>
>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