| Message ID | rust-pl011-rfc-v5.git.manos.pitsidianakis@linaro.org |
|---|---|
| Headers | show |
| Series | Add Rust support, implement ARM PL011 | expand |
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 ✅ 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. 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. > .../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: ❌ TODO comments when the code is doing potential undefined behavior ❌ a trait to store the CStr corresponding to the structs ❌ a trait to generate all-zero structs without having to type "unsafe { MaybeUninit::zeroed().assume_init() }" ❌ I'd like to use ctor instead of non-portable linker magic, and the cstr crate instead of CStr statics or c"" 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. 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). - 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. Paolo > .../pl011/vendor/arbitrary-int/CHANGELOG.md | 47 + > .../pl011/vendor/arbitrary-int/Cargo.toml | 54 + > .../pl011/vendor/arbitrary-int/LICENSE.txt | 21 + > .../char/pl011/vendor/arbitrary-int/README.md | 72 + > .../pl011/vendor/arbitrary-int/meson.build | 14 + > .../pl011/vendor/arbitrary-int/src/lib.rs | 1489 +++++ > .../pl011/vendor/arbitrary-int/tests/tests.rs | 1913 ++++++ > .../vendor/bilge-impl/.cargo-checksum.json | 1 + > .../char/pl011/vendor/bilge-impl/Cargo.toml | 54 + > .../hw/char/pl011/vendor/bilge-impl/README.md | 327 ++ > .../char/pl011/vendor/bilge-impl/meson.build | 24 + > .../pl011/vendor/bilge-impl/src/bitsize.rs | 187 + > .../vendor/bilge-impl/src/bitsize/split.rs | 185 + > .../vendor/bilge-impl/src/bitsize_internal.rs | 235 + > .../src/bitsize_internal/struct_gen.rs | 402 ++ > .../pl011/vendor/bilge-impl/src/debug_bits.rs | 55 + > .../vendor/bilge-impl/src/default_bits.rs | 92 + > .../pl011/vendor/bilge-impl/src/fmt_bits.rs | 112 + > .../pl011/vendor/bilge-impl/src/from_bits.rs | 222 + > .../char/pl011/vendor/bilge-impl/src/lib.rs | 79 + > .../pl011/vendor/bilge-impl/src/shared.rs | 196 + > .../src/shared/discriminant_assigner.rs | 56 + > .../vendor/bilge-impl/src/shared/fallback.rs | 92 + > .../vendor/bilge-impl/src/shared/util.rs | 91 + > .../vendor/bilge-impl/src/try_from_bits.rs | 143 + > .../pl011/vendor/bilge/.cargo-checksum.json | 1 + > rust/hw/char/pl011/vendor/bilge/Cargo.toml | 69 + > .../hw/char/pl011/vendor/bilge/LICENSE-APACHE | 176 + > rust/hw/char/pl011/vendor/bilge/LICENSE-MIT | 17 + > rust/hw/char/pl011/vendor/bilge/README.md | 327 ++ > rust/hw/char/pl011/vendor/bilge/meson.build | 17 + > rust/hw/char/pl011/vendor/bilge/src/lib.rs | 80 + > .../pl011/vendor/either/.cargo-checksum.json | 1 + > rust/hw/char/pl011/vendor/either/Cargo.toml | 54 + > .../char/pl011/vendor/either/LICENSE-APACHE | 201 + > rust/hw/char/pl011/vendor/either/LICENSE-MIT | 25 + > .../pl011/vendor/either/README-crates.io.md | 10 + > rust/hw/char/pl011/vendor/either/meson.build | 16 + > .../pl011/vendor/either/src/into_either.rs | 64 + > .../char/pl011/vendor/either/src/iterator.rs | 315 + > rust/hw/char/pl011/vendor/either/src/lib.rs | 1519 +++++ > .../pl011/vendor/either/src/serde_untagged.rs | 69 + > .../either/src/serde_untagged_optional.rs | 74 + > .../vendor/itertools/.cargo-checksum.json | 1 + > .../char/pl011/vendor/itertools/CHANGELOG.md | 409 ++ > .../hw/char/pl011/vendor/itertools/Cargo.lock | 681 +++ > .../hw/char/pl011/vendor/itertools/Cargo.toml | 101 + > .../pl011/vendor/itertools/LICENSE-APACHE | 201 + > .../char/pl011/vendor/itertools/LICENSE-MIT | 25 + > rust/hw/char/pl011/vendor/itertools/README.md | 44 + > .../pl011/vendor/itertools/benches/bench1.rs | 877 +++ > .../vendor/itertools/benches/combinations.rs | 125 + > .../benches/combinations_with_replacement.rs | 40 + > .../vendor/itertools/benches/extra/mod.rs | 2 + > .../itertools/benches/extra/zipslices.rs | 188 + > .../itertools/benches/fold_specialization.rs | 73 + > .../vendor/itertools/benches/powerset.rs | 44 + > .../vendor/itertools/benches/tree_fold1.rs | 144 + > .../itertools/benches/tuple_combinations.rs | 113 + > .../pl011/vendor/itertools/benches/tuples.rs | 213 + > .../pl011/vendor/itertools/examples/iris.data | 150 + > .../pl011/vendor/itertools/examples/iris.rs | 137 + > .../char/pl011/vendor/itertools/meson.build | 18 + > .../vendor/itertools/src/adaptors/coalesce.rs | 235 + > .../vendor/itertools/src/adaptors/map.rs | 124 + > .../vendor/itertools/src/adaptors/mod.rs | 1151 ++++ > .../itertools/src/adaptors/multi_product.rs | 230 + > .../vendor/itertools/src/combinations.rs | 128 + > .../src/combinations_with_replacement.rs | 109 + > .../pl011/vendor/itertools/src/concat_impl.rs | 23 + > .../vendor/itertools/src/cons_tuples_impl.rs | 64 + > .../char/pl011/vendor/itertools/src/diff.rs | 61 + > .../vendor/itertools/src/duplicates_impl.rs | 216 + > .../vendor/itertools/src/either_or_both.rs | 495 ++ > .../vendor/itertools/src/exactly_one_err.rs | 110 + > .../pl011/vendor/itertools/src/extrema_set.rs | 48 + > .../pl011/vendor/itertools/src/flatten_ok.rs | 165 + > .../char/pl011/vendor/itertools/src/format.rs | 168 + > .../char/pl011/vendor/itertools/src/free.rs | 286 + > .../pl011/vendor/itertools/src/group_map.rs | 32 + > .../pl011/vendor/itertools/src/groupbylazy.rs | 579 ++ > .../vendor/itertools/src/grouping_map.rs | 535 ++ > .../pl011/vendor/itertools/src/impl_macros.rs | 29 + > .../pl011/vendor/itertools/src/intersperse.rs | 118 + > .../pl011/vendor/itertools/src/k_smallest.rs | 20 + > .../pl011/vendor/itertools/src/kmerge_impl.rs | 227 + > .../pl011/vendor/itertools/src/lazy_buffer.rs | 63 + > .../hw/char/pl011/vendor/itertools/src/lib.rs | 3967 +++++++++++++ > .../pl011/vendor/itertools/src/merge_join.rs | 220 + > .../char/pl011/vendor/itertools/src/minmax.rs | 115 + > .../vendor/itertools/src/multipeek_impl.rs | 101 + > .../pl011/vendor/itertools/src/pad_tail.rs | 96 + > .../pl011/vendor/itertools/src/peek_nth.rs | 102 + > .../itertools/src/peeking_take_while.rs | 177 + > .../vendor/itertools/src/permutations.rs | 277 + > .../pl011/vendor/itertools/src/powerset.rs | 90 + > .../itertools/src/process_results_impl.rs | 68 + > .../vendor/itertools/src/put_back_n_impl.rs | 61 + > .../pl011/vendor/itertools/src/rciter_impl.rs | 99 + > .../pl011/vendor/itertools/src/repeatn.rs | 59 + > .../pl011/vendor/itertools/src/size_hint.rs | 119 + > .../pl011/vendor/itertools/src/sources.rs | 183 + > .../itertools/src/take_while_inclusive.rs | 68 + > .../hw/char/pl011/vendor/itertools/src/tee.rs | 78 + > .../pl011/vendor/itertools/src/tuple_impl.rs | 331 ++ > .../pl011/vendor/itertools/src/unique_impl.rs | 179 + > .../pl011/vendor/itertools/src/unziptuple.rs | 80 + > .../vendor/itertools/src/with_position.rs | 88 + > .../pl011/vendor/itertools/src/zip_eq_impl.rs | 60 + > .../pl011/vendor/itertools/src/zip_longest.rs | 83 + > .../pl011/vendor/itertools/src/ziptuple.rs | 138 + > .../itertools/tests/adaptors_no_collect.rs | 46 + > .../vendor/itertools/tests/flatten_ok.rs | 76 + > .../vendor/itertools/tests/macros_hygiene.rs | 13 + > .../vendor/itertools/tests/merge_join.rs | 108 + > .../itertools/tests/peeking_take_while.rs | 69 + > .../pl011/vendor/itertools/tests/quick.rs | 1849 ++++++ > .../vendor/itertools/tests/specializations.rs | 153 + > .../pl011/vendor/itertools/tests/test_core.rs | 317 + > .../pl011/vendor/itertools/tests/test_std.rs | 1184 ++++ > .../pl011/vendor/itertools/tests/tuples.rs | 86 + > .../char/pl011/vendor/itertools/tests/zip.rs | 77 + > rust/hw/char/pl011/vendor/meson.build | 18 + > .../.cargo-checksum.json | 1 + > .../vendor/proc-macro-error-attr/Cargo.toml | 33 + > .../proc-macro-error-attr/LICENSE-APACHE | 201 + > .../vendor/proc-macro-error-attr/LICENSE-MIT | 21 + > .../vendor/proc-macro-error-attr/build.rs | 5 + > .../vendor/proc-macro-error-attr/meson.build | 20 + > .../vendor/proc-macro-error-attr/src/lib.rs | 121 + > .../vendor/proc-macro-error-attr/src/parse.rs | 89 + > .../proc-macro-error-attr/src/settings.rs | 72 + > .../proc-macro-error/.cargo-checksum.json | 1 + > .../vendor/proc-macro-error/CHANGELOG.md | 162 + > .../pl011/vendor/proc-macro-error/Cargo.toml | 56 + > .../vendor/proc-macro-error/LICENSE-APACHE | 201 + > .../pl011/vendor/proc-macro-error/LICENSE-MIT | 21 + > .../pl011/vendor/proc-macro-error/README.md | 258 + > .../pl011/vendor/proc-macro-error/build.rs | 11 + > .../pl011/vendor/proc-macro-error/meson.build | 22 + > .../vendor/proc-macro-error/src/diagnostic.rs | 349 ++ > .../vendor/proc-macro-error/src/dummy.rs | 150 + > .../proc-macro-error/src/imp/delegate.rs | 69 + > .../proc-macro-error/src/imp/fallback.rs | 30 + > .../pl011/vendor/proc-macro-error/src/lib.rs | 560 ++ > .../vendor/proc-macro-error/src/macros.rs | 288 + > .../vendor/proc-macro-error/src/sealed.rs | 3 + > .../proc-macro-error/tests/macro-errors.rs | 8 + > .../pl011/vendor/proc-macro-error/tests/ok.rs | 10 + > .../proc-macro-error/tests/runtime-errors.rs | 13 + > .../vendor/proc-macro-error/tests/ui/abort.rs | 11 + > .../proc-macro-error/tests/ui/abort.stderr | 48 + > .../proc-macro-error/tests/ui/append_dummy.rs | 13 + > .../tests/ui/append_dummy.stderr | 5 + > .../tests/ui/children_messages.rs | 6 + > .../tests/ui/children_messages.stderr | 23 + > .../vendor/proc-macro-error/tests/ui/dummy.rs | 13 + > .../proc-macro-error/tests/ui/dummy.stderr | 5 + > .../vendor/proc-macro-error/tests/ui/emit.rs | 7 + > .../proc-macro-error/tests/ui/emit.stderr | 48 + > .../tests/ui/explicit_span_range.rs | 6 + > .../tests/ui/explicit_span_range.stderr | 5 + > .../proc-macro-error/tests/ui/misuse.rs | 11 + > .../proc-macro-error/tests/ui/misuse.stderr | 13 + > .../tests/ui/multiple_tokens.rs | 6 + > .../tests/ui/multiple_tokens.stderr | 5 + > .../tests/ui/not_proc_macro.rs | 4 + > .../tests/ui/not_proc_macro.stderr | 10 + > .../proc-macro-error/tests/ui/option_ext.rs | 6 + > .../tests/ui/option_ext.stderr | 7 + > .../tests/ui/proc_macro_hack.rs | 10 + > .../tests/ui/proc_macro_hack.stderr | 26 + > .../proc-macro-error/tests/ui/result_ext.rs | 7 + > .../tests/ui/result_ext.stderr | 11 + > .../tests/ui/to_tokens_span.rs | 6 + > .../tests/ui/to_tokens_span.stderr | 11 + > .../tests/ui/unknown_setting.rs | 4 + > .../tests/ui/unknown_setting.stderr | 5 + > .../tests/ui/unrelated_panic.rs | 6 + > .../tests/ui/unrelated_panic.stderr | 7 + > .../vendor/proc-macro2/.cargo-checksum.json | 1 + > .../char/pl011/vendor/proc-macro2/Cargo.toml | 104 + > .../pl011/vendor/proc-macro2/LICENSE-APACHE | 176 + > .../char/pl011/vendor/proc-macro2/LICENSE-MIT | 23 + > .../char/pl011/vendor/proc-macro2/README.md | 94 + > .../hw/char/pl011/vendor/proc-macro2/build.rs | 227 + > .../pl011/vendor/proc-macro2/build/probe.rs | 25 + > .../char/pl011/vendor/proc-macro2/meson.build | 19 + > .../vendor/proc-macro2/rust-toolchain.toml | 2 + > .../pl011/vendor/proc-macro2/src/detection.rs | 75 + > .../pl011/vendor/proc-macro2/src/extra.rs | 151 + > .../pl011/vendor/proc-macro2/src/fallback.rs | 1226 ++++ > .../char/pl011/vendor/proc-macro2/src/lib.rs | 1369 +++++ > .../pl011/vendor/proc-macro2/src/location.rs | 29 + > .../pl011/vendor/proc-macro2/src/marker.rs | 17 + > .../pl011/vendor/proc-macro2/src/parse.rs | 996 ++++ > .../pl011/vendor/proc-macro2/src/rcvec.rs | 145 + > .../pl011/vendor/proc-macro2/src/wrapper.rs | 993 ++++ > .../vendor/proc-macro2/tests/comments.rs | 105 + > .../vendor/proc-macro2/tests/features.rs | 8 + > .../pl011/vendor/proc-macro2/tests/marker.rs | 100 + > .../pl011/vendor/proc-macro2/tests/test.rs | 905 +++ > .../vendor/proc-macro2/tests/test_fmt.rs | 28 + > .../vendor/proc-macro2/tests/test_size.rs | 73 + > .../pl011/vendor/quote/.cargo-checksum.json | 1 + > rust/hw/char/pl011/vendor/quote/Cargo.toml | 50 + > .../hw/char/pl011/vendor/quote/LICENSE-APACHE | 176 + > rust/hw/char/pl011/vendor/quote/LICENSE-MIT | 23 + > rust/hw/char/pl011/vendor/quote/README.md | 272 + > rust/hw/char/pl011/vendor/quote/meson.build | 17 + > .../pl011/vendor/quote/rust-toolchain.toml | 2 + > rust/hw/char/pl011/vendor/quote/src/ext.rs | 110 + > rust/hw/char/pl011/vendor/quote/src/format.rs | 168 + > .../pl011/vendor/quote/src/ident_fragment.rs | 88 + > rust/hw/char/pl011/vendor/quote/src/lib.rs | 1464 +++++ > .../hw/char/pl011/vendor/quote/src/runtime.rs | 530 ++ > .../hw/char/pl011/vendor/quote/src/spanned.rs | 50 + > .../char/pl011/vendor/quote/src/to_tokens.rs | 209 + > .../pl011/vendor/quote/tests/compiletest.rs | 7 + > rust/hw/char/pl011/vendor/quote/tests/test.rs | 549 ++ > .../ui/does-not-have-iter-interpolated-dup.rs | 9 + > ...does-not-have-iter-interpolated-dup.stderr | 11 + > .../ui/does-not-have-iter-interpolated.rs | 9 + > .../ui/does-not-have-iter-interpolated.stderr | 11 + > .../tests/ui/does-not-have-iter-separated.rs | 5 + > .../ui/does-not-have-iter-separated.stderr | 10 + > .../quote/tests/ui/does-not-have-iter.rs | 5 + > .../quote/tests/ui/does-not-have-iter.stderr | 10 + > .../vendor/quote/tests/ui/not-quotable.rs | 7 + > .../vendor/quote/tests/ui/not-quotable.stderr | 20 + > .../vendor/quote/tests/ui/not-repeatable.rs | 8 + > .../quote/tests/ui/not-repeatable.stderr | 34 + > .../vendor/quote/tests/ui/wrong-type-span.rs | 7 + > .../quote/tests/ui/wrong-type-span.stderr | 10 + > .../pl011/vendor/syn/.cargo-checksum.json | 1 + > rust/hw/char/pl011/vendor/syn/Cargo.toml | 260 + > rust/hw/char/pl011/vendor/syn/LICENSE-APACHE | 176 + > rust/hw/char/pl011/vendor/syn/LICENSE-MIT | 23 + > rust/hw/char/pl011/vendor/syn/README.md | 284 + > rust/hw/char/pl011/vendor/syn/benches/file.rs | 57 + > rust/hw/char/pl011/vendor/syn/benches/rust.rs | 182 + > rust/hw/char/pl011/vendor/syn/meson.build | 24 + > rust/hw/char/pl011/vendor/syn/src/attr.rs | 793 +++ > rust/hw/char/pl011/vendor/syn/src/bigint.rs | 66 + > rust/hw/char/pl011/vendor/syn/src/buffer.rs | 434 ++ > rust/hw/char/pl011/vendor/syn/src/classify.rs | 377 ++ > .../pl011/vendor/syn/src/custom_keyword.rs | 260 + > .../vendor/syn/src/custom_punctuation.rs | 304 + > rust/hw/char/pl011/vendor/syn/src/data.rs | 423 ++ > rust/hw/char/pl011/vendor/syn/src/derive.rs | 259 + > .../char/pl011/vendor/syn/src/discouraged.rs | 225 + > rust/hw/char/pl011/vendor/syn/src/drops.rs | 58 + > rust/hw/char/pl011/vendor/syn/src/error.rs | 467 ++ > rust/hw/char/pl011/vendor/syn/src/export.rs | 73 + > rust/hw/char/pl011/vendor/syn/src/expr.rs | 3960 +++++++++++++ > rust/hw/char/pl011/vendor/syn/src/ext.rs | 136 + > rust/hw/char/pl011/vendor/syn/src/file.rs | 130 + > rust/hw/char/pl011/vendor/syn/src/fixup.rs | 218 + > .../hw/char/pl011/vendor/syn/src/gen/clone.rs | 2209 +++++++ > .../hw/char/pl011/vendor/syn/src/gen/debug.rs | 3160 ++++++++++ > rust/hw/char/pl011/vendor/syn/src/gen/eq.rs | 2242 +++++++ > rust/hw/char/pl011/vendor/syn/src/gen/fold.rs | 3779 ++++++++++++ > rust/hw/char/pl011/vendor/syn/src/gen/hash.rs | 2807 +++++++++ > .../hw/char/pl011/vendor/syn/src/gen/visit.rs | 3858 ++++++++++++ > .../pl011/vendor/syn/src/gen/visit_mut.rs | 3855 ++++++++++++ > rust/hw/char/pl011/vendor/syn/src/generics.rs | 1286 ++++ > rust/hw/char/pl011/vendor/syn/src/group.rs | 291 + > rust/hw/char/pl011/vendor/syn/src/ident.rs | 108 + > rust/hw/char/pl011/vendor/syn/src/item.rs | 3441 +++++++++++ > rust/hw/char/pl011/vendor/syn/src/lib.rs | 1019 ++++ > rust/hw/char/pl011/vendor/syn/src/lifetime.rs | 156 + > rust/hw/char/pl011/vendor/syn/src/lit.rs | 1830 ++++++ > .../hw/char/pl011/vendor/syn/src/lookahead.rs | 169 + > rust/hw/char/pl011/vendor/syn/src/mac.rs | 223 + > rust/hw/char/pl011/vendor/syn/src/macros.rs | 166 + > rust/hw/char/pl011/vendor/syn/src/meta.rs | 427 ++ > rust/hw/char/pl011/vendor/syn/src/op.rs | 219 + > rust/hw/char/pl011/vendor/syn/src/parse.rs | 1397 +++++ > .../pl011/vendor/syn/src/parse_macro_input.rs | 128 + > .../char/pl011/vendor/syn/src/parse_quote.rs | 210 + > rust/hw/char/pl011/vendor/syn/src/pat.rs | 953 +++ > rust/hw/char/pl011/vendor/syn/src/path.rs | 886 +++ > .../char/pl011/vendor/syn/src/precedence.rs | 163 + > rust/hw/char/pl011/vendor/syn/src/print.rs | 16 + > .../char/pl011/vendor/syn/src/punctuated.rs | 1132 ++++ > .../char/pl011/vendor/syn/src/restriction.rs | 176 + > rust/hw/char/pl011/vendor/syn/src/sealed.rs | 4 + > rust/hw/char/pl011/vendor/syn/src/span.rs | 63 + > rust/hw/char/pl011/vendor/syn/src/spanned.rs | 118 + > rust/hw/char/pl011/vendor/syn/src/stmt.rs | 481 ++ > rust/hw/char/pl011/vendor/syn/src/thread.rs | 60 + > rust/hw/char/pl011/vendor/syn/src/token.rs | 1138 ++++ > rust/hw/char/pl011/vendor/syn/src/tt.rs | 107 + > rust/hw/char/pl011/vendor/syn/src/ty.rs | 1216 ++++ > rust/hw/char/pl011/vendor/syn/src/verbatim.rs | 33 + > .../char/pl011/vendor/syn/src/whitespace.rs | 65 + > .../char/pl011/vendor/syn/tests/common/eq.rs | 900 +++ > .../char/pl011/vendor/syn/tests/common/mod.rs | 28 + > .../pl011/vendor/syn/tests/common/parse.rs | 49 + > .../char/pl011/vendor/syn/tests/debug/gen.rs | 5163 +++++++++++++++++ > .../char/pl011/vendor/syn/tests/debug/mod.rs | 147 + > .../char/pl011/vendor/syn/tests/macros/mod.rs | 93 + > .../char/pl011/vendor/syn/tests/regression.rs | 5 + > .../vendor/syn/tests/regression/issue1108.rs | 5 + > .../vendor/syn/tests/regression/issue1235.rs | 32 + > .../char/pl011/vendor/syn/tests/repo/mod.rs | 461 ++ > .../pl011/vendor/syn/tests/repo/progress.rs | 37 + > .../pl011/vendor/syn/tests/test_asyncness.rs | 43 + > .../pl011/vendor/syn/tests/test_attribute.rs | 225 + > .../vendor/syn/tests/test_derive_input.rs | 781 +++ > .../char/pl011/vendor/syn/tests/test_expr.rs | 692 +++ > .../pl011/vendor/syn/tests/test_generics.rs | 282 + > .../pl011/vendor/syn/tests/test_grouping.rs | 53 + > .../char/pl011/vendor/syn/tests/test_ident.rs | 87 + > .../char/pl011/vendor/syn/tests/test_item.rs | 332 ++ > .../pl011/vendor/syn/tests/test_iterators.rs | 70 + > .../char/pl011/vendor/syn/tests/test_lit.rs | 331 ++ > .../char/pl011/vendor/syn/tests/test_meta.rs | 154 + > .../vendor/syn/tests/test_parse_buffer.rs | 103 + > .../vendor/syn/tests/test_parse_quote.rs | 166 + > .../vendor/syn/tests/test_parse_stream.rs | 187 + > .../char/pl011/vendor/syn/tests/test_pat.rs | 152 + > .../char/pl011/vendor/syn/tests/test_path.rs | 130 + > .../pl011/vendor/syn/tests/test_precedence.rs | 537 ++ > .../pl011/vendor/syn/tests/test_receiver.rs | 321 + > .../pl011/vendor/syn/tests/test_round_trip.rs | 253 + > .../pl011/vendor/syn/tests/test_shebang.rs | 67 + > .../char/pl011/vendor/syn/tests/test_size.rs | 36 + > .../char/pl011/vendor/syn/tests/test_stmt.rs | 322 + > .../vendor/syn/tests/test_token_trees.rs | 32 + > .../hw/char/pl011/vendor/syn/tests/test_ty.rs | 397 ++ > .../pl011/vendor/syn/tests/test_visibility.rs | 185 + > .../char/pl011/vendor/syn/tests/zzz_stable.rs | 33 + > .../vendor/unicode-ident/.cargo-checksum.json | 1 + > .../pl011/vendor/unicode-ident/Cargo.toml | 63 + > .../pl011/vendor/unicode-ident/LICENSE-APACHE | 176 + > .../pl011/vendor/unicode-ident/LICENSE-MIT | 23 + > .../vendor/unicode-ident/LICENSE-UNICODE | 46 + > .../char/pl011/vendor/unicode-ident/README.md | 283 + > .../pl011/vendor/unicode-ident/benches/xid.rs | 124 + > .../pl011/vendor/unicode-ident/meson.build | 14 + > .../pl011/vendor/unicode-ident/src/lib.rs | 269 + > .../pl011/vendor/unicode-ident/src/tables.rs | 651 +++ > .../vendor/unicode-ident/tests/compare.rs | 67 + > .../vendor/unicode-ident/tests/fst/mod.rs | 11 + > .../unicode-ident/tests/fst/xid_continue.fst | Bin 0 -> 73249 bytes > .../unicode-ident/tests/fst/xid_start.fst | Bin 0 -> 65487 bytes > .../vendor/unicode-ident/tests/roaring/mod.rs | 21 + > .../vendor/unicode-ident/tests/static_size.rs | 95 + > .../vendor/unicode-ident/tests/tables/mod.rs | 7 + > .../unicode-ident/tests/tables/tables.rs | 347 ++ > .../vendor/unicode-ident/tests/trie/mod.rs | 7 + > .../vendor/unicode-ident/tests/trie/trie.rs | 445 ++ > .../vendor/version_check/.cargo-checksum.json | 1 + > .../pl011/vendor/version_check/Cargo.toml | 24 + > .../pl011/vendor/version_check/LICENSE-APACHE | 201 + > .../pl011/vendor/version_check/LICENSE-MIT | 19 + > .../char/pl011/vendor/version_check/README.md | 80 + > .../pl011/vendor/version_check/meson.build | 14 + > .../pl011/vendor/version_check/src/channel.rs | 193 + > .../pl011/vendor/version_check/src/date.rs | 203 + > .../pl011/vendor/version_check/src/lib.rs | 493 ++ > .../pl011/vendor/version_check/src/version.rs | 316 + > rust/hw/meson.build | 1 + > rust/meson.build | 15 + > rust/qemu-api/.gitignore | 2 + > rust/qemu-api/Cargo.lock | 7 + > rust/qemu-api/Cargo.toml | 23 + > rust/qemu-api/README.md | 17 + > rust/qemu-api/build.rs | 13 + > rust/qemu-api/meson.build | 19 + > rust/qemu-api/rustfmt.toml | 1 + > rust/qemu-api/src/bindings.rs | 7 + > rust/qemu-api/src/definitions.rs | 107 + > rust/qemu-api/src/device_class.rs | 128 + > rust/qemu-api/src/lib.rs | 100 + > rust/qemu-api/src/tests.rs | 48 + > rust/rustfmt.toml | 7 + > .../ci/setup/ubuntu/ubuntu-2204-aarch64.yaml | 3 + > .../ci/setup/ubuntu/ubuntu-2204-s390x.yaml | 3 + > scripts/meson-buildoptions.sh | 3 + > scripts/rustc_args.py | 84 + > tests/docker/dockerfiles/alpine.docker | 3 + > tests/docker/dockerfiles/centos9.docker | 3 + > .../dockerfiles/debian-amd64-cross.docker | 4 + > .../dockerfiles/debian-arm64-cross.docker | 4 + > .../dockerfiles/debian-armel-cross.docker | 4 + > .../dockerfiles/debian-armhf-cross.docker | 4 + > .../dockerfiles/debian-i686-cross.docker | 4 + > .../dockerfiles/debian-mips64el-cross.docker | 4 + > .../dockerfiles/debian-mipsel-cross.docker | 4 + > .../dockerfiles/debian-ppc64el-cross.docker | 4 + > .../dockerfiles/debian-s390x-cross.docker | 4 + > tests/docker/dockerfiles/debian.docker | 3 + > .../dockerfiles/fedora-win64-cross.docker | 3 + > tests/docker/dockerfiles/fedora.docker | 3 + > tests/docker/dockerfiles/opensuse-leap.docker | 2 + > tests/docker/dockerfiles/ubuntu2204.docker | 3 + > tests/lcitool/libvirt-ci | 2 +- > tests/lcitool/projects/qemu.yml | 3 + > tests/vm/generated/freebsd.json | 2 + > 432 files changed, 111104 insertions(+), 16 deletions(-) > create mode 100644 rust/hw/char/pl011/vendor/either/README.rst > create mode 100644 rust/wrapper.h > create mode 100644 rust/.gitignore > create mode 100644 rust/Kconfig > create mode 100644 rust/hw/Kconfig > create mode 100644 rust/hw/char/Kconfig > create mode 100644 rust/hw/char/meson.build > create mode 100644 rust/hw/char/pl011/.gitignore > create mode 100644 rust/hw/char/pl011/Cargo.lock > create mode 100644 rust/hw/char/pl011/Cargo.toml > create mode 100644 rust/hw/char/pl011/README.md > create mode 100644 rust/hw/char/pl011/meson.build > create mode 120000 rust/hw/char/pl011/rustfmt.toml > create mode 100644 rust/hw/char/pl011/src/definitions.rs > create mode 100644 rust/hw/char/pl011/src/device.rs > create mode 100644 rust/hw/char/pl011/src/device_class.rs > create mode 100644 rust/hw/char/pl011/src/lib.rs > create mode 100644 rust/hw/char/pl011/src/memory_ops.rs > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/CHANGELOG.md > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/LICENSE.txt > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/README.md > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/meson.build > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/arbitrary-int/tests/tests.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/README.md > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/meson.build > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/bitsize.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/bitsize/split.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/bitsize_internal.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/bitsize_internal/struct_gen.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/debug_bits.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/default_bits.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/fmt_bits.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/from_bits.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/shared.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/shared/discriminant_assigner.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/shared/fallback.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/shared/util.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge-impl/src/try_from_bits.rs > create mode 100644 rust/hw/char/pl011/vendor/bilge/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/bilge/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/bilge/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/bilge/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/bilge/README.md > create mode 100644 rust/hw/char/pl011/vendor/bilge/meson.build > create mode 100644 rust/hw/char/pl011/vendor/bilge/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/either/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/either/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/either/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/either/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/either/README-crates.io.md > create mode 100644 rust/hw/char/pl011/vendor/either/meson.build > create mode 100644 rust/hw/char/pl011/vendor/either/src/into_either.rs > create mode 100644 rust/hw/char/pl011/vendor/either/src/iterator.rs > create mode 100644 rust/hw/char/pl011/vendor/either/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/either/src/serde_untagged.rs > create mode 100644 rust/hw/char/pl011/vendor/either/src/serde_untagged_optional.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/itertools/CHANGELOG.md > create mode 100644 rust/hw/char/pl011/vendor/itertools/Cargo.lock > create mode 100644 rust/hw/char/pl011/vendor/itertools/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/itertools/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/itertools/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/itertools/README.md > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/bench1.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/combinations.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/combinations_with_replacement.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/extra/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/extra/zipslices.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/fold_specialization.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/powerset.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/tree_fold1.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/tuple_combinations.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/benches/tuples.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/examples/iris.data > create mode 100644 rust/hw/char/pl011/vendor/itertools/examples/iris.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/meson.build > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/adaptors/coalesce.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/adaptors/map.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/adaptors/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/adaptors/multi_product.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/combinations.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/combinations_with_replacement.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/concat_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/cons_tuples_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/diff.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/duplicates_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/either_or_both.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/exactly_one_err.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/extrema_set.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/flatten_ok.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/format.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/free.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/group_map.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/groupbylazy.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/grouping_map.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/impl_macros.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/intersperse.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/k_smallest.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/kmerge_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/lazy_buffer.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/merge_join.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/minmax.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/multipeek_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/pad_tail.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/peek_nth.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/peeking_take_while.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/permutations.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/powerset.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/process_results_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/put_back_n_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/rciter_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/repeatn.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/size_hint.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/sources.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/take_while_inclusive.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/tee.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/tuple_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/unique_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/unziptuple.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/with_position.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/zip_eq_impl.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/zip_longest.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/src/ziptuple.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/adaptors_no_collect.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/flatten_ok.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/macros_hygiene.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/merge_join.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/peeking_take_while.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/quick.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/specializations.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/test_core.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/test_std.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/tuples.rs > create mode 100644 rust/hw/char/pl011/vendor/itertools/tests/zip.rs > create mode 100644 rust/hw/char/pl011/vendor/meson.build > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/build.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/meson.build > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/src/parse.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error-attr/src/settings.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/CHANGELOG.md > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/README.md > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/build.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/meson.build > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/src/diagnostic.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/src/dummy.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/src/imp/delegate.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/src/imp/fallback.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/src/macros.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/src/sealed.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/macro-errors.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ok.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/runtime-errors.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/abort.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/abort.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/append_dummy.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/append_dummy.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/children_messages.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/children_messages.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/dummy.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/dummy.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/emit.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/emit.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/explicit_span_range.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/explicit_span_range.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/misuse.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/misuse.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/multiple_tokens.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/multiple_tokens.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/not_proc_macro.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/not_proc_macro.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/option_ext.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/option_ext.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/proc_macro_hack.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/proc_macro_hack.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/result_ext.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/result_ext.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/to_tokens_span.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/to_tokens_span.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/unknown_setting.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/unknown_setting.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/unrelated_panic.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro-error/tests/ui/unrelated_panic.stderr > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/README.md > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/build.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/build/probe.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/meson.build > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/rust-toolchain.toml > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/detection.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/extra.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/fallback.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/location.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/marker.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/parse.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/rcvec.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/src/wrapper.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/tests/comments.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/tests/features.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/tests/marker.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/tests/test.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/tests/test_fmt.rs > create mode 100644 rust/hw/char/pl011/vendor/proc-macro2/tests/test_size.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/quote/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/quote/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/quote/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/quote/README.md > create mode 100644 rust/hw/char/pl011/vendor/quote/meson.build > create mode 100644 rust/hw/char/pl011/vendor/quote/rust-toolchain.toml > create mode 100644 rust/hw/char/pl011/vendor/quote/src/ext.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/src/format.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/src/ident_fragment.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/src/runtime.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/src/spanned.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/src/to_tokens.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/compiletest.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/test.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter-interpolated-dup.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter-interpolated-dup.stderr > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter-interpolated.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter-interpolated.stderr > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter-separated.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter-separated.stderr > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/does-not-have-iter.stderr > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/not-quotable.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/not-quotable.stderr > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/not-repeatable.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/not-repeatable.stderr > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/wrong-type-span.rs > create mode 100644 rust/hw/char/pl011/vendor/quote/tests/ui/wrong-type-span.stderr > create mode 100644 rust/hw/char/pl011/vendor/syn/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/syn/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/syn/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/syn/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/syn/README.md > create mode 100644 rust/hw/char/pl011/vendor/syn/benches/file.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/benches/rust.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/meson.build > create mode 100644 rust/hw/char/pl011/vendor/syn/src/attr.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/bigint.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/buffer.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/classify.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/custom_keyword.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/custom_punctuation.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/data.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/derive.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/discouraged.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/drops.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/error.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/export.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/expr.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/ext.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/file.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/fixup.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/gen/clone.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/gen/debug.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/gen/eq.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/gen/fold.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/gen/hash.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/gen/visit.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/gen/visit_mut.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/generics.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/group.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/ident.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/item.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/lifetime.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/lit.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/lookahead.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/mac.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/macros.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/meta.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/op.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/parse.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/parse_macro_input.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/parse_quote.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/pat.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/path.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/precedence.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/print.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/punctuated.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/restriction.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/sealed.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/span.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/spanned.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/stmt.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/thread.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/token.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/tt.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/ty.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/verbatim.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/src/whitespace.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/common/eq.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/common/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/common/parse.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/debug/gen.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/debug/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/macros/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/regression.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/regression/issue1108.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/regression/issue1235.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/repo/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/repo/progress.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_asyncness.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_attribute.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_derive_input.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_expr.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_generics.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_grouping.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_ident.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_item.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_iterators.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_lit.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_meta.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_parse_buffer.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_parse_quote.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_parse_stream.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_pat.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_path.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_precedence.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_receiver.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_round_trip.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_shebang.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_size.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_stmt.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_token_trees.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_ty.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/test_visibility.rs > create mode 100644 rust/hw/char/pl011/vendor/syn/tests/zzz_stable.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/LICENSE-UNICODE > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/README.md > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/benches/xid.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/meson.build > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/src/tables.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/compare.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/fst/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/fst/xid_continue.fst > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/fst/xid_start.fst > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/roaring/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/static_size.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/tables/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/tables/tables.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/trie/mod.rs > create mode 100644 rust/hw/char/pl011/vendor/unicode-ident/tests/trie/trie.rs > create mode 100644 rust/hw/char/pl011/vendor/version_check/.cargo-checksum.json > create mode 100644 rust/hw/char/pl011/vendor/version_check/Cargo.toml > create mode 100644 rust/hw/char/pl011/vendor/version_check/LICENSE-APACHE > create mode 100644 rust/hw/char/pl011/vendor/version_check/LICENSE-MIT > create mode 100644 rust/hw/char/pl011/vendor/version_check/README.md > create mode 100644 rust/hw/char/pl011/vendor/version_check/meson.build > create mode 100644 rust/hw/char/pl011/vendor/version_check/src/channel.rs > create mode 100644 rust/hw/char/pl011/vendor/version_check/src/date.rs > create mode 100644 rust/hw/char/pl011/vendor/version_check/src/lib.rs > create mode 100644 rust/hw/char/pl011/vendor/version_check/src/version.rs > create mode 100644 rust/hw/meson.build > create mode 100644 rust/meson.build > create mode 100644 rust/qemu-api/.gitignore > create mode 100644 rust/qemu-api/Cargo.lock > create mode 100644 rust/qemu-api/Cargo.toml > create mode 100644 rust/qemu-api/README.md > create mode 100644 rust/qemu-api/build.rs > create mode 100644 rust/qemu-api/meson.build > create mode 120000 rust/qemu-api/rustfmt.toml > create mode 100644 rust/qemu-api/src/bindings.rs > create mode 100644 rust/qemu-api/src/definitions.rs > create mode 100644 rust/qemu-api/src/device_class.rs > create mode 100644 rust/qemu-api/src/lib.rs > create mode 100644 rust/qemu-api/src/tests.rs > create mode 100644 rust/rustfmt.toml > create mode 100644 scripts/rustc_args.py > > > base-commit: a7ddb48bd1363c8bcdf42776d320289c42191f01
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
On Wed, Jul 24, 2024 at 11:58 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > 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 Yes, I don't think it's a requirement that they live along the C files. Using rust/hw/... as you did is fine. > 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. Ok, it's good that you agree because that's what I was worried about. :) > > > >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. Ehh, as you say below it's complicated. Sometimes worse is better. Personally I wouldn't have minded keeping the v4 approach as a "known evil"; but if you can make the Cargo subprojects work, that would be fine for me. What I don't like is the vendoring and handwritten (I think?) meson.build, I think that's worse than the v4. > >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? No I meant comments where we have known instances of undefined behavior. The two I had in my emails are (in pl011_init): // TODO: this assumes that "all zeroes" is a valid state for all fields of // PL011State. This is not necessarily true of any #[repr(Rust)] structs, // including bilge-generated types. It should instead use MaybeUninit. (before the call to qemu_chr_fe_accept_input): // TODO: this causes a callback that creates another "&mut self". // This is forbidden by Rust aliasing rules and has to be fixed // using interior mutability. > 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. Why do they have to be #[used]? You have + #[used] + static TYPE_NAME: &::core::ffi::CStr = $tname; + $tname.as_ptr() but since the cstr crate (and c"" literal) promise to return a &'static CStr, I thought it could be just $tname.as_ptr() About traits, I meant something like pub unsafe trait ObjectType: Sized { const TYPE_NAME: &'static CStr; } So that you can put the trait declaration in the pl011 crate and the type_info! macro can do <$t as ObjectType>::TYPE_NAME.as_ptr() (also for the parent). > >❌ 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. Ah, I see. Anyhow, I've been looking at the Linux kernel's pinned-init crate (https://rust-for-linux.com/pinned-init) and it provides a Zeroable macro including #[derive] support. So that has gone lower in my priority. > >❌ 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. I'd still like to give someone else the burden. :) Writing our own constructor macro would be more work for little gain. > >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"? Ah, I meant that it applies to both direct use: pub static CLK_NAME: &CStr = c"clk"; and arguments to macros (for example type_info). > >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. I think it's worth trying it anyway. > >- 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. Ok, thanks! Paolo
On Wed, 24 Jul 2024 13:34, Paolo Bonzini <pbonzini@redhat.com> wrote: >On Wed, Jul 24, 2024 at 11:58 AM Manos Pitsidianakis ><manos.pitsidianakis@linaro.org> wrote: >> >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. > >Ehh, as you say below it's complicated. Sometimes worse is better. >Personally I wouldn't have minded keeping the v4 approach as a "known >evil"; but if you can make the Cargo subprojects work, that would be >fine for me. What I don't like is the vendoring and handwritten (I >think?) meson.build, I think that's worse than the v4. > >> >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? > >No I meant comments where we have known instances of undefined >behavior. The two I had in my emails are > >(in pl011_init): >// TODO: this assumes that "all zeroes" is a valid state for all fields of >// PL011State. This is not necessarily true of any #[repr(Rust)] structs, >// including bilge-generated types. It should instead use MaybeUninit. > >(before the call to qemu_chr_fe_accept_input): >// TODO: this causes a callback that creates another "&mut self". >// This is forbidden by Rust aliasing rules and has to be fixed >// using interior mutability. You mean that we can have a call stack that looks something like... <qemu code> |_ pl011_read |_ PL011State::read() |_ qemu_chr_fe_accept_input() |_ pl011_read |_ PL011State::read() If I understand correctly, this does not create another "&mut self", because a mutable reference of self is passed to qemu_chr_fe_accept_input(), and only exists until it returns. In any case, I agree that these subtleties must be examined thoroughly in general. In this case though PL011State has only Copy fields and no side effects when dropped. This means that adding interior mutability e.g. with Cell would have exactly the same behavior. But your bringing it up makes me wonder whether we can detect any bad behaviors with miri... It does not play well with FFI boundaries but it's possible to mock them in some cases. I will look into the two TODOs you mention and also if it's possible to verify the correct behavior whenever possible! > >> 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. > >Why do they have to be #[used]? You have > >+ #[used] >+ static TYPE_NAME: &::core::ffi::CStr = $tname; >+ $tname.as_ptr() > >but since the cstr crate (and c"" literal) promise to return a >&'static CStr, I thought it could be just > > $tname.as_ptr() > >About traits, I meant something like > >pub unsafe trait ObjectType: Sized { > const TYPE_NAME: &'static CStr; >} > >So that you can put the trait declaration in the pl011 crate and the >type_info! macro can do > ><$t as ObjectType>::TYPE_NAME.as_ptr() > >(also for the parent). That was my approach at the beginning but I was having issues with the linker stripping the <some const value>.as_ptr() and it would point to invalid memory; I checked it again and I think using #[used] just for the TypeInfo struct declaration might be enough so these can be removed. > >> >❌ 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. > >Ah, I see. Anyhow, I've been looking at the Linux kernel's pinned-init >crate (https://rust-for-linux.com/pinned-init) and it provides a >Zeroable macro including #[derive] support. So that has gone lower in >my priority. > >> >❌ 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. > >I'd still like to give someone else the burden. :) Writing our own >constructor macro would be more work for little gain. Well, it's just that I personally don't view adding __attribute__ manually in only two places is a burden but I've no strong preference over it. I'm looking at the ctor dependencies and they are a few: https://github.com/mmastrac/rust-ctor/blob/cc3ab9160ed9dc3bdf20d735352b519abc2913e9/Cargo.lock Do you perhaps agree with adding a FIXME comment to replace the linker attributes with ctor when we get the cargo dependency story in meson sorted out? Manos
Il gio 25 lug 2024, 08:19 Manos Pitsidianakis < manos.pitsidianakis@linaro.org> ha scritto: > >(before the call to qemu_chr_fe_accept_input): > >// TODO: this causes a callback that creates another "&mut self". > >// This is forbidden by Rust aliasing rules and has to be fixed > >// using interior mutability. > > You mean that we can have a call stack that looks something like... > > <qemu code> > |_ pl011_read > |_ PL011State::read() > |_ qemu_chr_fe_accept_input() > |_ pl011_read > |_ PL011State::read() > > If I understand correctly, this does not create another "&mut self", > because a mutable reference of self is passed to > qemu_chr_fe_accept_input(), and only exists until it returns. > Unfortunately that's a *mut, not a &mut. A &mut must be unique, so the cast in pl011_read() is undefined behavior. In any case, I agree that these subtleties must be examined thoroughly > in general. In this case though PL011State has only Copy fields and no > side effects when dropped. This means that adding interior mutability > e.g. with Cell would have exactly the same behavior. > If you add interior mutability you can operate on a &self; and then creating the &mut (via either Cell or RefCell) from the callback is legal. But your bringing it up makes me wonder whether we can detect any bad > behaviors with miri... It does not play well with FFI boundaries but > it's possible to mock them in some cases. I will look into the two TODOs > you mention and also if it's possible to verify the correct behavior > whenever possible! > The most robust way is to write proper bindings to QEMU APIs that enforce use of shared references. That will take time; but we'll get there, for now it's important just to point out the problem. > >> >❌ 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. > > > >Ah, I see. Anyhow, I've been looking at the Linux kernel's pinned-init > >crate (https://rust-for-linux.com/pinned-init) and it provides a > >Zeroable macro including #[derive] support. So that has gone lower in > >my priority. > > > >> >❌ 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. > > > >I'd still like to give someone else the burden. :) Writing our own > >constructor macro would be more work for little gain. > > Well, it's just that I personally don't view adding __attribute__ > manually in only two places is a burden but I've no strong preference > over it. > > I'm looking at the ctor dependencies and they are a few: > > https://github.com/mmastrac/rust-ctor/blob/cc3ab9160ed9dc3bdf20d735352b519abc2913e9/Cargo.lock > > Do you perhaps agree with adding a FIXME comment to replace the linker > attributes with ctor when we get the cargo dependency story in meson > sorted out? > Yes, that's fine. Paolo Manos > >
On Thu, 25 Jul 2024 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote: >Il gio 25 lug 2024, 08:19 Manos Pitsidianakis < >manos.pitsidianakis@linaro.org> ha scritto: > >> >(before the call to qemu_chr_fe_accept_input): >> >// TODO: this causes a callback that creates another "&mut self". >> >// This is forbidden by Rust aliasing rules and has to be fixed >> >// using interior mutability. >> >> You mean that we can have a call stack that looks something like... >> >> <qemu code> >> |_ pl011_read >> |_ PL011State::read() >> |_ qemu_chr_fe_accept_input() >> |_ pl011_read >> |_ PL011State::read() >> >> If I understand correctly, this does not create another "&mut self", >> because a mutable reference of self is passed to >> qemu_chr_fe_accept_input(), and only exists until it returns. >> > >Unfortunately that's a *mut, not a &mut. A &mut must be unique, so the cast >in pl011_read() is undefined behavior. Actually it's: unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; And you can ensure there's no disjoint borrowing by making a wrapper function that mutably borrows self, e.g. fn accept_input(&mut self) { unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; } This is not undefined behavior, since the cast in pl011_read creates a mutable reference that does not outlive the same call to pl011_read. > >In any case, I agree that these subtleties must be examined thoroughly >> in general. In this case though PL011State has only Copy fields and no >> side effects when dropped. This means that adding interior mutability >> e.g. with Cell would have exactly the same behavior. >> > >If you add interior mutability you can operate on a &self; and then >creating the &mut (via either Cell or RefCell) from the callback is legal. > >But your bringing it up makes me wonder whether we can detect any bad >> behaviors with miri... It does not play well with FFI boundaries but >> it's possible to mock them in some cases. I will look into the two TODOs >> you mention and also if it's possible to verify the correct behavior >> whenever possible! >> > >The most robust way is to write proper bindings to QEMU APIs that enforce >use of shared references. That will take time; but we'll get there, for now >it's important just to point out the problem. Please demonstrate a scenario where said problem shows up, otherwise it's just hand-waving on a undefined (pun intended ;) ) hypothetical. > > >> >> >❌ 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. >> > >> >Ah, I see. Anyhow, I've been looking at the Linux kernel's pinned-init >> >crate (https://rust-for-linux.com/pinned-init) and it provides a >> >Zeroable macro including #[derive] support. So that has gone lower in >> >my priority. >> > >> >> >❌ 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. >> > >> >I'd still like to give someone else the burden. :) Writing our own >> >constructor macro would be more work for little gain. >> >> Well, it's just that I personally don't view adding __attribute__ >> manually in only two places is a burden but I've no strong preference >> over it. >> >> I'm looking at the ctor dependencies and they are a few: >> >> https://github.com/mmastrac/rust-ctor/blob/cc3ab9160ed9dc3bdf20d735352b519abc2913e9/Cargo.lock >> >> Do you perhaps agree with adding a FIXME comment to replace the linker >> attributes with ctor when we get the cargo dependency story in meson >> sorted out? >> > >Yes, that's fine. Thanks! We also have to keep in mind we would like Debian to be able to package this, so we should be very conservative on what dependencies we use. > >Paolo > >Manos >> >>
On Thu, Jul 25, 2024 at 12:14 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > >Unfortunately that's a *mut, not a &mut. A &mut must be unique, so the cast > >in pl011_read() is undefined behavior. > > Actually it's: > > unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; > > And you can ensure there's no disjoint borrowing by making a wrapper > function that mutably borrows self, e.g. > > fn accept_input(&mut self) { > unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; > } > > This is not undefined behavior, since the cast in pl011_read creates a > mutable reference that does not outlive the same call to pl011_read. pl011_receive (called by qemu_chr_fe_accept_input) creates a mutable reference that *overlaps* the lifetime of the outer reference created by pl011_read. This is undefined behavior. You're effectively writing: fn main() { let mut foo = String::from("foo"); let x = &mut foo; let y = &mut foo; print!("{}", y); print!("{}", x); // does not compile } And even if you're hiding it like this: fn main() { let mut foo = String::from("foo"); let x = &mut foo; let x_ptr: *mut String = x as *mut _; let y = &mut foo; print!("{}", y); print!("{}", unsafe { &mut *x_ptr }); } It's still wrong. Quoting the docs: https://doc.rust-lang.org/nightly/nomicon/references.html -> "A mutable reference cannot be aliased" https://doc.rust-lang.org/nightly/nomicon/aliasing.html -> "variables and pointers alias if they refer to overlapping regions of memory." https://doc.rust-lang.org/reference/behavior-considered-undefined.html -> "Rust programs must never cause undefined behavior. It is the programmer's responsibility when writing unsafe code to ensure that any safe code interacting with the unsafe code cannot trigger these behaviors." The QEMU code has even more layers hiding the problem but it's the same thing: there are two &mut that are alive at the same time, and refer to the same region of memory. > Please demonstrate a scenario where said problem shows up, otherwise > it's just hand-waving on a undefined (pun intended ;) ) hypothetical. Again: there's no "scenario where the problem shows up", you just can't do that; the code is wrong. I'm not saying you should fix it now, because it's complicated to fix it and in particular to fix it the right way. But I think it is a requirement for merging Rust code that the community understands the extra complexity introduced by writing Rust code that interacts with C APIs, and does not hand wave it away as not a problem. > >> Do you perhaps agree with adding a FIXME comment to replace the linker > >> attributes with ctor when we get the cargo dependency story in meson > >> sorted out? > > > >Yes, that's fine. > > Thanks! We also have to keep in mind we would like Debian to be able to > package this, so we should be very conservative on what dependencies we > use. If we add Rust dependencies, Debian will have to figure out how to package them in order to build QEMU. But I agree that it's a good idea to add them slowly. Paolo
On Thu, 25 Jul 2024 at 14:19, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On Thu, Jul 25, 2024 at 12:14 PM Manos Pitsidianakis > <manos.pitsidianakis@linaro.org> wrote: > > >Unfortunately that's a *mut, not a &mut. A &mut must be unique, so the cast > > >in pl011_read() is undefined behavior. > > > > Actually it's: > > > > unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; > > > > And you can ensure there's no disjoint borrowing by making a wrapper > > function that mutably borrows self, e.g. > > > > fn accept_input(&mut self) { > > unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; > > } > > > > This is not undefined behavior, since the cast in pl011_read creates a > > mutable reference that does not outlive the same call to pl011_read. > > pl011_receive (called by qemu_chr_fe_accept_input) creates a mutable > reference that *overlaps* the lifetime of the outer reference created > by pl011_read. This is undefined behavior. You're effectively writing: There is no overlap there, sorry. Once qemu_chr_fe_accept_input returns, any references it created do not exist anymore.
On Thu, Jul 25, 2024 at 4:48 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > pl011_receive (called by qemu_chr_fe_accept_input) creates a mutable > > reference that *overlaps* the lifetime of the outer reference created > > by pl011_read. This is undefined behavior. You're effectively writing: > > There is no overlap there, sorry. Once qemu_chr_fe_accept_input > returns, any references it created do not exist anymore. read |-------------| receive |-----| That's the overlap. Maybe you're thinking that the outer &mut "goes to sleep" and is reborn when qemu_chr_fe_accept_input() returns, but there's no such thing in the language. You can do it within a function: fn main() { let mut x = 42u32; let a = &mut x; let b = &mut *a; *b = 0; *a = 43; // *b = 42; } But that's because the compiler _knows_ the provenance of b. So it says "ok, 'b' is basically the same as 'a' and 'a' won't be accessed while 'b' is live". This doesn't happen in any other case. In fact miri points out that, the moment you create a &mut in pl011_read, the chardev's opaque becomes effectively unusable[1]. In other words, you're not even allowed to turn it from a *mut into a &mut! Again: it's exactly the same as the example above fn main() { let mut foo = String::from("foo"); let x = &mut foo; // pl011_read has a reference let x_ptr: *mut String = x as *mut _; // pl011_receive starts let y = &mut foo; // pl011_receive creates a reference // at this point, x becomes invalid print!("{}", y); // pl011_receive returns // this is undefined behavior (shown by miri) print!("{}", unsafe { &mut *x_ptr }); } Honestly I don't see how I can provide a proof or explanation that is more definitive than miri. If some specific documentation or discussions gives you the perception that there is no undefined behavior, I'm happy to check them out and provide an explanation in that context. But otherwise, I don't think it's useful to keep debating *whether* it is undefined behavior. Paolo [1] this is the other example I gave a few hours ago, which I'll copy here for reference: // MIRIFLAGS=-Zmiri-ignore-leaks cargo +nightly miri run use std::mem::MaybeUninit; struct S { me: *mut S, them: *mut S } impl S { pub fn chardev_receive(&mut self) { println!("in chardev_receive with &mut"); } pub fn memory_write_good(&mut self) { println!("in memory_write_good, calling qemu_chr_fe_accept_input()"); qemu_chr_fe_accept_input_good(self); } pub fn memory_write_bad(&mut self) { println!("in memory_write_bad, calling qemu_chr_fe_accept_input()"); qemu_chr_fe_accept_input_bad(self); } } fn qemu_chr_fe_accept_input_good(c: &S) { // you can still go from *mut to &mut in _another_ instance of struct S (unsafe { &mut *c.them }).chardev_receive(); } fn qemu_chr_fe_accept_input_bad(c: &S) { // you cannot go from *mut to &mut when it points to _this_ instance; // creating the &mut that is passed to memory_write_bad() has // effectively made that *mut unusable! (unsafe { &mut *c.me }).chardev_receive(); } fn main() { let p: &mut MaybeUninit<S> = Box::leak(Box::new(MaybeUninit::uninit())); let q: &mut MaybeUninit<S> = Box::leak(Box::new(MaybeUninit::uninit())); unsafe { let p_mut_ptr = p.as_mut_ptr(); let q_mut_ptr = q.as_mut_ptr(); *(&mut *p_mut_ptr) = S { me: p_mut_ptr, them: q_mut_ptr }; (&mut *p_mut_ptr).memory_write_bad(); (&mut *p_mut_ptr).memory_write_good(); } }
On Thu, 25 Jul 2024 18:15, Paolo Bonzini <pbonzini@redhat.com> wrote: >On Thu, Jul 25, 2024 at 4:48 PM Manos Pitsidianakis ><manos.pitsidianakis@linaro.org> wrote: >> > pl011_receive (called by qemu_chr_fe_accept_input) creates a mutable >> > reference that *overlaps* the lifetime of the outer reference created >> > by pl011_read. This is undefined behavior. You're effectively writing: >> >> There is no overlap there, sorry. Once qemu_chr_fe_accept_input >> returns, any references it created do not exist anymore. > >read |-------------| >receive |-----| > >That's the overlap. Maybe you're thinking that the outer &mut "goes to >sleep" and is reborn when qemu_chr_fe_accept_input() returns, but >there's no such thing in the language. There's no overlap, because the read scope is not live while receive is active. References are linear types, when you give `&mut sth` as an argument to a function call, you can model it in your mind as "giving the mutual reference to the function call" and "receiving it back" when it returns. There'd be an overlap if qemu_chr_fe_accept_input got two mutual references of the same memory location, such as calling foo(&mut self, &mut self) with foo being `fn foo(a: &mut T, b: &mut T)`. > >You can do it within a function: > These examples you provide are not really equivalent. >Honestly I don't see how I can provide a proof or explanation that is >more definitive than miri. If some specific documentation or >discussions gives you the perception that there is no undefined >behavior, I'm happy to check them out and provide an explanation in >that context. But otherwise, I don't think it's useful to keep >debating *whether* it is undefined behavior. Agreed! but thank you for taking the time to discuss this :) Manos PS: I will explain why your good/bad example behaves like it does: >// MIRIFLAGS=-Zmiri-ignore-leaks cargo +nightly miri run >use std::mem::MaybeUninit; > >struct S { > me: *mut S, > them: *mut S >} > >impl S { > pub fn chardev_receive(&mut self) { > println!("in chardev_receive with &mut"); > } > > pub fn memory_write_good(&mut self) { > println!("in memory_write_good, calling qemu_chr_fe_accept_input()"); > qemu_chr_fe_accept_input_good(self); > } > > pub fn memory_write_bad(&mut self) { > println!("in memory_write_bad, calling qemu_chr_fe_accept_input()"); > qemu_chr_fe_accept_input_bad(self); > } >} > >fn qemu_chr_fe_accept_input_good(c: &S) { > // you can still go from *mut to &mut in _another_ instance of struct S > (unsafe { &mut *c.them }).chardev_receive(); `them` is an uninitialized value, so you are dereferencing it and passing a mutable reference to it, while >} > >fn qemu_chr_fe_accept_input_bad(c: &S) { > // you cannot go from *mut to &mut when it points to _this_ instance; > // creating the &mut that is passed to memory_write_bad() has > // effectively made that *mut unusable! > (unsafe { &mut *c.me }).chardev_receive(); >} `me` is the same memory location as `c`, which is passed as read only (&S) here. > >fn main() { > let p: &mut MaybeUninit<S> = Box::leak(Box::new(MaybeUninit::uninit())); > let q: &mut MaybeUninit<S> = Box::leak(Box::new(MaybeUninit::uninit())); > > unsafe { > let p_mut_ptr = p.as_mut_ptr(); > let q_mut_ptr = q.as_mut_ptr(); > *(&mut *p_mut_ptr) = S { me: p_mut_ptr, them: q_mut_ptr }; > (&mut *p_mut_ptr).memory_write_bad(); > (&mut *p_mut_ptr).memory_write_good(); > } >} >
On Fri, Jul 26, 2024 at 9:26 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > On Thu, 25 Jul 2024 18:15, Paolo Bonzini <pbonzini@redhat.com> wrote: > >On Thu, Jul 25, 2024 at 4:48 PM Manos Pitsidianakis > ><manos.pitsidianakis@linaro.org> wrote: > >> > pl011_receive (called by qemu_chr_fe_accept_input) creates a mutable > >> > reference that *overlaps* the lifetime of the outer reference created > >> > by pl011_read. This is undefined behavior. You're effectively writing: > >> > >> There is no overlap there, sorry. Once qemu_chr_fe_accept_input > >> returns, any references it created do not exist anymore. > > > >read |-------------| > >receive |-----| > > > >That's the overlap. Maybe you're thinking that the outer &mut "goes to > >sleep" and is reborn when qemu_chr_fe_accept_input() returns, but > >there's no such thing in the language. > > There's no overlap, because the read scope is not live while receive is > active. References are linear types, when you give `&mut sth` as an > argument to a function call, you can model it in your mind as "giving > the mutual reference to the function call" and "receiving it back" when > it returns. There is no such thing as "being active", the only exception being when you create an &mut from *the same* &mut. To clarify further: in principle, if you have code like this in pl011_read(): self.a = 42; qemu_chr_accept_input(self.chardev); return self.a The compiler can optimize the second "self.a" to 42. The optimization is incorrect if pl011_receive() overwrites "self.a". But that's not a problem with the optimization, it's a problem with what pl011_receive() does. In other words, the *mere existence* of a &mut in an outer scope makes it invalid to create *another* &mut pointing to the same memory in an inner scope. As I said, I don't see the point in discussing this more, and I'm not going to unless you provide a clear pointer to documentation that states the opposite. Paolo > There'd be an overlap if qemu_chr_fe_accept_input got two mutual > references of the same memory location, such as calling foo(&mut self, > &mut self) with foo being `fn foo(a: &mut T, b: &mut T)`.
On Fri, 26 Jul 2024 at 11:19, Paolo Bonzini <pbonzini@redhat.com> wrote: > As I said, I don't see the point in discussing this more, and I'm not > going to unless you provide a clear pointer to documentation that > states the opposite. Same here.
On Fri, 26 Jul 2024 12:26, Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: >On Fri, 26 Jul 2024 at 11:19, Paolo Bonzini <pbonzini@redhat.com> wrote: >> As I said, I don't see the point in discussing this more, and I'm not >> going to unless you provide a clear pointer to documentation that >> states the opposite. > >Same here. Next patch series version is taking more time than I expected because of unrelated issues, plus I'm on PTO these days, so I am posting the resolution (which I hope satisfies everyone) here early: It's possible to refactor this code (and any other similar cases) to prevent recursive mutable calls: diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 3643b7bdee..449930e34e 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -104,10 +127,14 @@ pub fn init(&mut self) { } } - pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 { + pub fn read( + &mut self, + offset: hwaddr, + _size: core::ffi::c_uint, + ) -> std::ops::ControlFlow<u64, u64> { use RegisterOffset::*; - match RegisterOffset::try_from(offset) { + std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) { Err(v) if (0x3f8..0x400).contains(&v) => { u64::from(PL011_ID_ARM[((offset - 0xfe0) >> 2) as usize]) } @@ -134,10 +161,8 @@ pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 { // Update error bits. self.receive_status_error_clear = c.to_be_bytes()[3].into(); self.update(); - // SAFETY: self.char_backend is a valid CharBackend instance after it's been - // initialized in realize(). - unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) }; - c.into() + // Must call qemu_chr_fe_accept_input, so return Continue: + return std::ops::ControlFlow::Continue(c.into()); } Ok(RSR) => u8::from(self.receive_status_error_clear).into(), Ok(FR) => u16::from(self.flags).into(), @@ -159,7 +184,7 @@ pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 { 0 } Ok(DMACR) => self.dmacr.into(), - } + }) } pub fn write(&mut self, offset: hwaddr, value: u64) { diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs index 6144d28586..5e185b7cd7 100644 --- a/rust/hw/char/pl011/src/memory_ops.rs +++ b/rust/hw/char/pl011/src/memory_ops.rs @@ -29,7 +29,18 @@ ) -> u64 { assert!(!opaque.is_null()); let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>()); - state.as_mut().read(addr, size) + let val = state.as_mut().read(addr, size); + match val { + std::ops::ControlFlow::Break(val) => val, + std::ops::ControlFlow::Continue(val) => { + // SAFETY: self.char_backend is a valid CharBackend instance after it's been + // initialized in realize(). + let cb_ptr = core::ptr::addr_of_mut!(state.as_mut().char_backend); + unsafe { qemu_chr_fe_accept_input(cb_ptr) }; + + val + } + } } When we iterate the APIs further we will find better idiomatic solutions that do not rely on devices themselves to do this kind of stuff, but offer interfaces from the `qemu_api` library crate. Manos
On Wed, Jul 31, 2024 at 11:50 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > On Fri, 26 Jul 2024 12:26, Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > >On Fri, 26 Jul 2024 at 11:19, Paolo Bonzini <pbonzini@redhat.com> wrote: > >> As I said, I don't see the point in discussing this more, and I'm not > >> going to unless you provide a clear pointer to documentation that > >> states the opposite. > > > >Same here. > > Next patch series version is taking more time than I expected because of > unrelated issues, plus I'm on PTO these days, so I am posting the > resolution (which I hope satisfies everyone) here early: I would be okay with just a TODO comment (I think we all agree that the issue is theoretical), but this one's okay too if you prefer. Paolo