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

[Manos Pitsidianakis]

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, but given that the bindings depend on various headers that 
are only gathered in the common source set in meson by the time all the 
subdirs are evaluated, it results in the Rust device being dependendant 
on a meson target (bindings_rs) that cannot be declared yet.

It's kind of a code smell, ideally we should not use bindgen for QEMU 
internal headers but update bindings along with the C headers (with 
extra tests to ensure definitions and layout match). Not for this 
patchset, but something to keep in mind.

> ✅ only generate bindings.rs.inc once
>
> ✅ a couple lints are too broad and should be enabled per-file. (though 
> there are still some issues with duplication of lints, I consider this 
> mostly done)
>
> ✅ please check if -Wl,--whole-archive can be replaced with link_whole 
> (as discussed on IRC, unfortunately it cannot)
>
>
> The hot point here is how to handle dependencies.  I appreciate that you 
> found a way to avoid repeated building of dependent crates, and to 
> integrate with Kconfig, but at the same time this is a huge change which 
> in my opinion is premature.
>
> For example if we can (sooner or later) use the automatic Cargo 
> subprojects, we do not need any vendoring and we can use cargo in the 
> meanwhile (we can drop --cargo and CARGO at any point, just like we 
> dropped --meson and --sphinx-build in QEMU 8.1).
>
> On the other hand, committing to using meson's "raw" (meson.build-level) 
> rust support and vendoring everything is premature in my opinion is very 
> different for people who are already comfortable with Cargo, so it makes 
> it harder to add new dependencies.  In fact, because the huge patch 8 
> did not reach the mailing list, it's really hard to understand what's 
> going on, what had to be done by hand and what is done automatically by 
> meson.

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.

> 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. It's not that 
of a clear-cut decision, so I'd like feedback on this. What do you 
think?

> >   .../vendor/arbitrary-int/.cargo-checksum.json |    1 +
>
> In any case, vendoring should not be done inside hw/char/pl011.
>
> 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?

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.

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

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

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

> If you have a tree that I can look at, to understand more of patch 8, 
> please send a pointer.  However, honestly I am not comfortable with the 
> build system integration as done in this patch.

Ah forgot to mention it in the cover letter, everything is under the 
rust-pl011-rfc-v5 tag here: 

https://gitlab.com/epilys/rust-for-qemu/-/tree/rust-pl011-rfc-v5?ref_type=tags

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

Thanks!
Manos