On 6/17/24 14:04, Manos Pitsidianakis wrote:
On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonz...@redhat.com> 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. 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.
The best compliment you had was "I'm not saying it's a bad first step", and I would say this differently: It's a great first step!
We should have a first version where we stick to the C API, with all the Rust code being as Rusty as possible: benefit from typed enums, error handling, bounds checking and other nice things.
It's useless to iterate/debate for two years on the list before landing something upstream. We can start with this, have one or two devices that use this build system, and then focus on designing a good interface for this.
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.
As I mentioned in my previous answer, this device already makes a good progress: it eliminates a whole class of memory errors, and the only issue brought by unsafe code is concurrency issues. And this should be our focus once we get the build infrastructure done!
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
