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

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3642 bytes --]

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.

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.


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

Paolo

Manos
>
>

[-- Attachment #2: Type: text/html, Size: 5664 bytes --]