| Message ID | rust-pl011-rfc-v4.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Rust support, implement ARM PL011 | expand |
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
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
Il lun 8 lug 2024, 18:33 Daniel P. Berrangé <berrange@redhat.com> ha scritto: > 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. > But we're not—in particular it's still using several features not in all supported distros. 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. > I guess the detection could be done, but actually enabling the build part needs to wait until the minimum supported version is low enough. Paolo > 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 :| > >
On Mon, Jul 08, 2024 at 06:55:40PM +0200, Paolo Bonzini wrote: > Il lun 8 lug 2024, 18:33 Daniel P. Berrangé <berrange@redhat.com> ha > scritto: > > > 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. > > > > But we're not—in particular it's still using several features not in all > supported distros. That's exactly why I suggest its a pre-requisite for merging this. Unless we're able to demonstrate that we can enable Rust on all our CI platforms, the benefits of Rust will not be realized in QEMU, and we'll have never ending debates about whether each given feature needs to be in C or Rust. > 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. > > > > I guess the detection could be done, but actually enabling the build part > needs to wait until the minimum supported version is low enough. With regards, Daniel
Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha scritto: > That's exactly why I suggest its a pre-requisite for merging > this. Unless we're able to demonstrate that we can enable > Rust on all our CI platforms, the benefits of Rust will > not be realized in QEMU, and we'll have never ending debates > about whether each given feature needs to be in C or Rust. > In that case we should develop it on a branch, so that more than one person can contribute (unlike if we keep iterating on this RFC). Paolo > > > 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. > > > > > > > I guess the detection could be done, but actually enabling the build part > > needs to wait until the minimum supported version is low enough. > > 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 :| > >
On Mon, 8 Jul 2024, 21:34 Paolo Bonzini, <pbonzini@redhat.com> wrote: > > > Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha > scritto: > >> That's exactly why I suggest its a pre-requisite for merging >> this. Unless we're able to demonstrate that we can enable >> Rust on all our CI platforms, the benefits of Rust will >> not be realized in QEMU, and we'll have never ending debates >> about whether each given feature needs to be in C or Rust. >> > > In that case we should develop it on a branch, so that more than one > person can contribute (unlike if we keep iterating on this RFC). > > Paolo > If you do, I'd really appreciate it if you did not use any part of my patches. Thanks, Manos
On Mon, 8 Jul 2024 at 21:49, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > Il lun 8 lug 2024, 20:39 Manos Pitsidianakis <manos.pitsidianakis@linaro.org> ha scritto: >> >> >> >> On Mon, 8 Jul 2024, 21:34 Paolo Bonzini, <pbonzini@redhat.com> wrote: >>> >>> >>> >>> Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha scritto: >>>> >>>> That's exactly why I suggest its a pre-requisite for merging >>>> this. Unless we're able to demonstrate that we can enable >>>> Rust on all our CI platforms, the benefits of Rust will >>>> not be realized in QEMU, and we'll have never ending debates >>>> about whether each given feature needs to be in C or Rust. >>> >>> >>> In that case we should develop it on a branch, so that more than one person can contribute (unlike if we keep iterating on this RFC). >> >> >> If you do, I'd really appreciate it if you did not use any part of my patches. > > > "We" means that you would accept patches, review them and apply them until any agreed-upon conditions for merging are satisfied, and then send either a final series or a pull request for inclusion in QEMU. Ah, alright. That wasn't obvious because that e-mail was not directed to me nor did it mention my name :) I do not want to do that, in any case. I do not think it's the right approach.
On Tue, Jul 9, 2024 at 9:38 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > Ah, alright. That wasn't obvious because that e-mail was not directed > to me nor did it mention my name :) Oh, ok. Sorry about that. Generally when I say "we" I include as large a part of the community as applicable. > I do not want to do that, in any case. I do not think it's the right approach. No problem with that (and in fact I agree, as I'd prefer a speedy merge and doing the work on the QEMU master branch); however, we need to reach an agreement on that and everybody (including Daniel) needs to explain the reason for their position. Daniel's proposed criteria for merging include: - CI integration - CI passing for all supported targets (thus lowering the MSRV to 1.63.0) - plus any the code changes that were or will be requested during review That seems to be a pretty high amount of work, and until it's done everyone else is unable to contribute, not even in directions orthogonal to the above (cross compilation support, less unsafe code, porting more devices). So something has to give: either we decide for an early merge, where the code is marked as experimental and disabled by default. Personally I think it's fine, the contingency plan is simply to "git rm -rf rust/". Or we can keep the above stringent requirements for merging, but then I don't see it as a one-person job. If I can say so, developing on a branch would also be a useful warm-up for you in the maintainer role, if we expect that there will be significant community contributions to Rust. Paolo
On Mon, 8 Jul 2024 at 21:39, Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > > > On Mon, 8 Jul 2024, 21:34 Paolo Bonzini, <pbonzini@redhat.com> wrote: >> >> >> >> Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha scritto: >>> >>> That's exactly why I suggest its a pre-requisite for merging >>> this. Unless we're able to demonstrate that we can enable >>> Rust on all our CI platforms, the benefits of Rust will >>> not be realized in QEMU, and we'll have never ending debates >>> about whether each given feature needs to be in C or Rust. >> >> >> In that case we should develop it on a branch, so that more than one person can contribute (unlike if we keep iterating on this RFC). >> >> Paolo > > > > > If you do, I'd really appreciate it if you did not use any part of my patches. Someone pointed out to me that my wording is really bad here and I agree, I apologize. I did not express myself with the right words. What I wanted to say is that hypothetically my work can be used as a base just not the patches/sign offs. The changes in the patches are all open source and part of the QEMU community now, of course.
On Tue, Jul 09, 2024 at 09:54:43AM +0200, Paolo Bonzini wrote: > On Tue, Jul 9, 2024 at 9:38 AM Manos Pitsidianakis > <manos.pitsidianakis@linaro.org> wrote: > > Ah, alright. That wasn't obvious because that e-mail was not directed > > to me nor did it mention my name :) > > Oh, ok. Sorry about that. Generally when I say "we" I include as large > a part of the community as applicable. > > > I do not want to do that, in any case. I do not think it's the right approach. > > No problem with that (and in fact I agree, as I'd prefer a speedy > merge and doing the work on the QEMU master branch); however, we need > to reach an agreement on that and everybody (including Daniel) needs > to explain the reason for their position. > > Daniel's proposed criteria for merging include: > - CI integration > - CI passing for all supported targets (thus lowering the MSRV to 1.63.0) > - plus any the code changes that were or will be requested during review > > That seems to be a pretty high amount of work, and until it's done > everyone else is unable to contribute, not even in directions > orthogonal to the above (cross compilation support, less unsafe code, > porting more devices). My thought is that the initial merge focuses only on the build system integration. So that's basically patches 1 + 2 in this series. IMHO that is small enough that we should be able to demonstrate that we detect Rust, run bindgen & compile its result, on all our supported platforms without an unreasonable amount of effort. > So something has to give: either we decide for > an early merge, where the code is marked as experimental and disabled > by default. Personally I think it's fine, the contingency plan is > simply to "git rm -rf rust/". Or we can keep the above stringent > requirements for merging, but then I don't see it as a one-person job. Patch 3, the high level APIs is where I see most of the work and collaboration being needed, but that doesn't need to be rushed into the first merge. We would have a "rust" subsystem + maintainer who would presumably have a staging tree, etc in the normal way we work and collaborate With regards, Daniel
On Tue, Jul 9, 2024 at 2:18 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > My thought is that the initial merge focuses only on the build system > integration. So that's basically patches 1 + 2 in this series. > > Patch 3, the high level APIs is where I see most of the work and > collaboration being needed, but that doesn't need to be rushed into > the first merge. We would have a "rust" subsystem + maintainer who > would presumably have a staging tree, etc in the normal way we work > and collaborate It's complicated. A "perfect" build system integration would include integration with Kconfig, but it's not simple work and it may not be Manos's preference for what to work on (or maybe it is), and it's also not a blocker for further work on patches 3-4. On the other hand, patches 3 and 4 are _almost_ ready except for requiring a very new Rust - we know how to tackle that, but again it may take some time and it's completely separate work from better build system integration. In other words, improving build system integration is harder until merge, but merge is blocked by independent work on lowering the minimum supported Rust version. This is why I liked the idea of having either a development tree to allow a merge into early 9.2. On the other hand, given the exceptional scope (completely new code that can be disabled at will) and circumstances, even a very early merge into 9.1 (disabled by default) might be better to provide developers with the easiest base for experimenting. The requirements for merging, here, would basically amount to a good roadmap and some established good habits. An merge into early 9.2 would be a bit harder for experimenting, while merging it now would sacrifice CI integration in the initial stages of the work but make cooperation easier. However, it's difficult to say what's best without knowing Manos's rationale for preferring not to have a development tree yet. Paolo
On 7/9/24 09:51, Paolo Bonzini wrote: > On Tue, Jul 9, 2024 at 2:18 PM Daniel P. Berrangé <berrange@redhat.com> wrote: >> My thought is that the initial merge focuses only on the build system >> integration. So that's basically patches 1 + 2 in this series. >> >> Patch 3, the high level APIs is where I see most of the work and >> collaboration being needed, but that doesn't need to be rushed into >> the first merge. We would have a "rust" subsystem + maintainer who >> would presumably have a staging tree, etc in the normal way we work >> and collaborate > > It's complicated. A "perfect" build system integration would include > integration with Kconfig, but it's not simple work and it may not be > Manos's preference for what to work on (or maybe it is), and it's also > not a blocker for further work on patches 3-4. > > On the other hand, patches 3 and 4 are _almost_ ready except for > requiring a very new Rust - we know how to tackle that, but again it > may take some time and it's completely separate work from better build > system integration. > > In other words, improving build system integration is harder until > merge, but merge is blocked by independent work on lowering the > minimum supported Rust version. This is why I liked the idea of having > either a development tree to allow a merge into early 9.2. > > On the other hand, given the exceptional scope (completely new code > that can be disabled at will) and circumstances, even a very early > merge into 9.1 (disabled by default) might be better to provide > developers with the easiest base for experimenting. The requirements > for merging, here, would basically amount to a good roadmap and some > established good habits. > > An merge into early 9.2 would be a bit harder for experimenting, while > merging it now would sacrifice CI integration in the initial stages of > the work but make cooperation easier. I would be in favor of a 9.1 merge (disabled, not auto). Noting that soft-freeze is two weeks from today, so no dilly dallying. :-) The only reasonable alternative that I see is the kind of development branch that qemu is not used to having: a long-term shared branch on qemu-projects. With which I would be ok, up to and including a branch merge at the end, as I'm used to working that way with other projects. I think the only reason to delay the current development work until 9.2 is if we were still questioning whether using Rust *at all* is a good idea. I think we're beyond that point. r~