Re: [RFC PATCH v5 0/8] Add Rust support, implement ARM PL011

[Manos Pitsidianakis]

On Thu, 25 Jul 2024 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il gio 25 lug 2024, 08:19 Manos Pitsidianakis <
> manos.pitsidianakis@linaro.org> ha scritto:
>
> > >(before the call to qemu_chr_fe_accept_input):
> > >// TODO: this causes a callback that creates another "&mut self".
> > >// This is forbidden by Rust aliasing rules and has to be fixed
> > >// using interior mutability.
> >
> > You mean that we can have a call stack that looks something like...
> >
> > <qemu code>
> > |_ pl011_read
> >   |_ PL011State::read()
> >      |_ qemu_chr_fe_accept_input()
> >       |_ pl011_read
> >         |_ PL011State::read()
> >
> > If I understand correctly, this does not create another "&mut self",
> > because a mutable reference of self is passed to
> > qemu_chr_fe_accept_input(), and only exists until it returns.
>
> Unfortunately that's a *mut, not a &mut. A &mut must be unique, so the cast
> in pl011_read() is undefined behavior.

Actually it's:

 unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) };

And you can ensure there's no disjoint borrowing by making a wrapper 
function that mutably borrows self, e.g.

fn accept_input(&mut self) {
 unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) };
}

This is not undefined behavior, since the cast in pl011_read creates a 
mutable reference that does not outlive the same call to pl011_read.


> In any case, I agree that these subtleties must be examined thoroughly
> > in general. In this case though PL011State has only Copy fields and no
> > side effects when dropped. This means that adding interior mutability
> > e.g. with Cell would have exactly the same behavior.
>
> If you add interior mutability you can operate on a &self; and then
> creating the &mut (via either Cell or RefCell) from the callback is legal.
>
> But your bringing it up makes me wonder whether we can detect any bad
> > behaviors with miri... It does not play well with FFI boundaries but
> > it's possible to mock them in some cases. I will look into the two TODOs
> > you mention and also if it's possible to verify the correct behavior
> > whenever possible!
>
> The most robust way is to write proper bindings to QEMU APIs that enforce
> use of shared references. That will take time; but we'll get there, for now
> it's important just to point out the problem.

Please demonstrate a scenario where said problem shows up, otherwise 
it's just hand-waving on a undefined (pun intended ;) ) hypothetical.

> > >> >❌ a trait to generate all-zero structs without having to type "unsafe {
> > >> >MaybeUninit::zeroed().assume_init() }"
> > >>
> > >> Traits cannot have const fns at the moment (traits cannot have
> > >> type-level effects like const or async but it's WIP to get them into
> > >> rustc), so this cannot be used for statics and consts.
> > >
> > >Ah, I see. Anyhow, I've been looking at the Linux kernel's pinned-init
> > >crate (https://rust-for-linux.com/pinned-init) and it provides a
> > >Zeroable macro including #[derive] support. So that has gone lower in
> > >my priority.
> > >
> > >> >❌ I'd like to use ctor instead of non-portable linker magic,
> > >>
> > >> The linker sections are pretty much standard and in fact ctor uses the
> > >> same linker attributes. Would writing our own constructor macro be a
> > >> solution for you? My reasoning is that 1) we support our own specific
> > >> platforms and it's better for portability to reflect that in our source
> > >> tree and 2) it avoids the external dependency, linker sections do not
> > >> change so any ctor update would be in the API or adding more platform
> > >> support,  not fixes in what we target.
> > >
> > >I'd still like to give someone else the burden. :) Writing our own
> > >constructor macro would be more work for little gain.
> >
> > Well, it's just that I personally don't view adding __attribute__
> > manually in only two places is a burden but I've no strong preference
> > over it.
> >
> > I'm looking at the ctor dependencies and they are a few:
> >
> > https://github.com/mmastrac/rust-ctor/blob/cc3ab9160ed9dc3bdf20d735352b519abc2913e9/Cargo.lock
> >
> > Do you perhaps agree with adding a FIXME comment to replace the linker
> > attributes with ctor when we get the cargo dependency story in meson
> > sorted out?
>
> Yes, that's fine.

Thanks! We also have to keep in mind we would like Debian to be able to 
package this, so we should be very conservative on what dependencies we 
use.

> Paolo
>
> Manos
> >