Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011

[Paolo Bonzini <pbonzini@redhat.com>]

On 7/4/24 14:15, Manos Pitsidianakis wrote:
> Changes from v3->v4:
> - Add rust-specific files to .gitattributes
> - Added help text to scripts/cargo_wrapper.py arguments (thanks Stephan)
> - Split bindings separate crate
> - Add declarative macros for symbols exported to QEMU to said crate
> - Lowered MSRV to 1.77.2
> - Removed auto-download and install of bindgen-cli
> - Fixed re-compilation of Rust objects in case they are missing from
>    filesystem
> - Fixed optimized builds by adding #[used] (thanks Pierrick for the help
>    debugging this)

I think the largest issue is that I'd rather have a single cargo build 
using a virtual manifest, because my hunch is that it'd be the easiest 
path towards Kconfig integration.  But it's better to do this after 
merge, as the changes are pretty large.  It's also independent from any 
other changes targeted at removing unsafe code, so no need to hold back 
on merging.

Other comments I made that should however be addressed before merging, 
from most to least important:

- TODO comments when the code is doing potential undefined behavior

- module structure should IMO resemble the C part of the tree

- only generate bindings.rs.inc once

- a couple abstractions that I'd like to have now: a trait to store the 
CStr corresponding to the structs, and one to generate all-zero structs 
without having to type "unsafe { MaybeUninit::zeroed().assume_init() }"

- I pointed out a couple lints that are too broad and should be enabled 
per-file, even if right now it's basically all files that include them.

- add support for --cargo and CARGO environment variables (if my patch 
works without too much hassle)

- I'd like to use ctor instead of non-portable linker magic, and the 
cstr crate instead of CStr statics or c""

- please check if -Wl,--whole-archive can be replaced with link_whole:

- probably, until Rust is enabled by default we should treat 
dependencies as a moving target and not commit Cargo.lock files.  In the 
meanwhile we can discuss how to handle them.

And a few aesthetic changes on top of this.

With respect to lints, marking entire groups as "deny" is problematic. 
Before merge, I'd rather have the groups as just "warn", and add a long 
list of denied lints[1].  After merge we can also add a non-fatal CI job 
that runs clippy with nightly rust and with groups marked as "deny". 
This matches the check-python-tox job in python/.

[1] https://github.com/bonzini/rust-qemu/commit/95b25f7c5f4e

Thanks,

Paolo