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

[Paolo Bonzini]

On Wed, Jul 24, 2024 at 11:58 AM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> Hello Paolo, thank you for the thorough response,
>
> On Tue, 23 Jul 2024 18:07, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >On 7/22/24 13:43, Manos Pitsidianakis wrote:
> >> Changes from v4->v5:
> >> - Added CI patch from Alex Benee
> >> - Removed all cargo use, use meson rust support
> >> - Added Kconfig logic
> >
> >The following requests from the v4 review have also been evaluated (good!):
> >
> >✅ module structure should resemble the C part of the tree
>
> To expand on this, I tried really hard to make the rust code live along
> the c files

Yes, I don't think it's a requirement that they live along the C
files. Using rust/hw/... as you did is fine.

> I agree. I personally prefer using meson wraps and fetch the
> dependencies via network to be honest. While also providing Cargo.toml
> and Cargo.lock manifests for developers.

Ok, it's good that you agree because that's what I was worried about. :)

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

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

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

> >and the cstr crate instead of CStr statics or c""
>
> Oh yes, the c"" literals must be replaced. The cstr! macro is the same,
> semantically, can you explain what you mean by "CStr statics"?

Ah, I meant that it applies to both direct use:

pub static CLK_NAME: &CStr = c"clk";

and arguments to macros (for example type_info).

> >My suggestion is to do one of the following, or both:
> >
> >- start from this version; try using Cargo subproject support in 1.5.0
> >and see if it works, so that vendoring can be dropped.  We can require
> >Meson 1.5.0 to work on Rust support.  In this case it's okay not to do
> >any further code changes (the four that were marked ❌ above).
>
> This is my preference as stated above, if everyone also agrees.

I think it's worth trying it anyway.

> >- go back to the build system integration of v4, and do *only* the
> >changes that were requested during review (in this case, all of them
> >except link_whole, with you checked it does not work).
> >
> >If you try using Cargo subproject support, please provide the running
> >time for configure and make, for both "v4" and "v5+subproject".  When I
> >tried it, the processing of the subprojects was very slow.
>
> Hmmm thanks for mentioning that, I did not notice any slow times
> locally. Will check.

Ok, thanks!

Paolo