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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jul 08, 2024 at 06:26:22PM +0200, Paolo Bonzini wrote:
> 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.

This series is still missing changes to enable build on all targets
during CI, including cross-compiles, to prove that we're doing the
correct thing on all our targetted platforms. That's a must have
before considering it suitable for merge.

I also believe we should default to enabling rust toolchain by
default in configure, and require and explicit --without-rust
to disable it, *despite* it not technically being a mandatory
feature....yet.

This is to give users a clear message that Rust is likely to
become a fundamental part of QEMU, so they need to give feedback
if they hit any problems / have use cases we've not anticipated
that are problematic wrt Rust.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|