Hello Paolo, thank you for the thorough response,
On Tue, 23 Jul 2024 18:07, Paolo Bonzini <pbonz...@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
