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

[Paolo Bonzini <pbonzini@redhat.com>]

On Tue, Jun 18, 2024 at 1:33 AM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 6/17/24 14:04, Manos Pitsidianakis wrote:
> > 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.
>
> 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!

Don't quote me out of context; I said It's an "awesome" first step,
though I qualified that we should treat it as "a guide towards
providing safe bindings between Rust and C". That is, as long as we
agree that it is not production quality code. Which it doesn't have to
be!

> 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.

I never said that I want perfection before landing upstream. I want _a path_.

When I read "there was consensus in the community call that we won't
be writing Rust APIs for internal C QEMU interfaces; or at least,
that's not the goal"[1], then sorry but I think that it's better to
stick with C.

On the other hand, if there is a path towards proper, maintainable
Rust, then I am even okay even with committing something that
technically contains undefined behavior.

[1] https://lore.kernel.org/qemu-devel/ez270.x96k6aeu0rpw@linaro.org/

> 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!

Let's not exaggerate the benefits: no pointers were converted to RAII
(Box<> or Rc<>) in the course of writing the Rust code; so there are
no memory errors that can be eliminated by the rewrite. In fact, new
memory errors can also be introduced, because safe Rust has stricter
aliasing rules than C.

This is not a problem! It's just that we need to be realistic to
understand where to focus next and why. To build our path.

(Also, a small correction so that we're on the same page on the fix:
it's reentrancy, not concurrency).

Paolo