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

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

On Wed, 24 Jul 2024 13:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
>On Wed, Jul 24, 2024 at 11:58 AM Manos Pitsidianakis
><manos.pitsidianakis@linaro.org> wrote:
>> >In my opinion we should start with cargo workspaces as the
>> >known-imperfect (but good enough) solution, so that it could be evolved
>> >later.  It is important that any change that deviates from common Rust
>> >conventions is documented, and v4 provided a nice basis to build upon,
>> >with documentation coming as things settle.  This is why I explicitly
>> >didn't include Kconfig in the TODO list in my review of v4.
>>
>> After working with the latest meson releases, it seems we will soon have
>> a good enough way of handling all this with meson. It makes me sceptical
>> of adding cargo wrappers and using a build system out of meson when we
>> might be able to drop all that soonish. We might as well bite the bullet
>> now to avoid working on something we know we will remove.
>
>Ehh, as you say below it's complicated. Sometimes worse is better.
>Personally I wouldn't have minded keeping the v4 approach as a "known
>evil"; but if you can make the Cargo subprojects work, that would be
>fine for me. What I don't like is the vendoring and handwritten (I
>think?) meson.build, I think that's worse than the v4.
>
>> >Also, of the code changes (as opposed to the build system changes) that
>> >I had asked for in the review of v4, almost none of them have been
>> >applied.  I'm copying them here for future reference:
>>
>> Thanks, this helps a lot.
>>
>> >❌ TODO comments when the code is doing potential undefined behavior
>>
>> Do you mean like Rust's safety comments?
>
>No I meant comments where we have known instances of undefined
>behavior. The two I had in my emails are
>
>(in pl011_init):
>// TODO: this assumes that "all zeroes" is a valid state for all fields of
>// PL011State. This is not necessarily true of any #[repr(Rust)] structs,
>// including bilge-generated types. It should instead use MaybeUninit.
>
>(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.

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.

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!


>
>> https://std-dev-guide.rust-lang.org/policy/safety-comments.html
>>
>> These can be required by lints which is really helpful. At this point
>> the UART library has safety comments (which needs to be reviewed for
>> validity). I plan on adding some at the macros in qemu-api as well.
>>
>> >
>> >❌ a trait to store the CStr corresponding to the structs
>>
>> I don't know yet if that is helpful in our usecase, because the strings
>> must be visible from C, thus be (rust, not c) statics, unmangled and
>> marked as #[used] for the linker. It makes sense from the Rust POV but
>> must also be FFI-accessible.
>
>Why do they have to be #[used]? You have
>
>+                #[used]
>+                static TYPE_NAME: &::core::ffi::CStr = $tname;
>+                $tname.as_ptr()
>
>but since the cstr crate (and c"" literal) promise to return a
>&'static CStr, I thought it could be just
>
>    $tname.as_ptr()
>
>About traits, I meant something like
>
>pub unsafe trait ObjectType: Sized {
>     const TYPE_NAME: &'static CStr;
>}
>
>So that you can put the trait declaration in the pl011 crate and the
>type_info! macro can do
>
><$t as ObjectType>::TYPE_NAME.as_ptr()
>
>(also for the parent).

That was my approach at the beginning but I was having issues with the 
linker stripping the <some const value>.as_ptr() and it would point to 
invalid memory; I checked it again and I think using #[used] just for 
the TypeInfo struct declaration might be enough so these can be removed.

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

Manos