Re: [RFC PATCH v2 3/5] rust: add PL011 device model

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

Hello Zhao :)

On Thu, 13 Jun 2024 11:30, Zhao Liu <zhao1.liu@intel.com> wrote:
>On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote:
>> Date: Tue, 11 Jun 2024 13:33:32 +0300
>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> Subject: [RFC PATCH v2 3/5] rust: add PL011 device model
>> X-Mailer: git-send-email 2.44.0
>> 
>> This commit adds a re-implementation of hw/char/pl011.c in Rust.
>> 
>> It uses generated Rust bindings (produced by `ninja
>> aarch64-softmmu-generated.rs`) to
>> register itself as a QOM type/class.
>> 
>> How to build:
>> 
>> 1. Make sure rust, cargo and bindgen (cargo install bindgen-cli) are
>>    installed
>> 2. Configure a QEMU build with:
>>    --enable-system --target-list=aarch64-softmmu --enable-with-rust
>> 3. Launching a VM with qemu-system-aarch64 should use the Rust version
>>    of the pl011 device (unless it is not set up so in hw/arm/virt.c; the
>>    type of the UART device is hardcoded).
>> 
>>    To confirm, inspect `info qom-tree` in the monitor and look for an
>>    `x-pl011-rust` device.
>> 
>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> ---
>
>Hi Manos,
>
>Thanks for your example!
>
>> diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
>> new file mode 100644
>> index 0000000000..db74f2b59f
>> --- /dev/null
>> +++ b/rust/pl011/Cargo.toml
>> @@ -0,0 +1,66 @@
>
>...
>
>> +[lints]
>> +[lints.rustdoc]
>> +broken_intra_doc_links = "deny"
>> +redundant_explicit_links = "deny"
>> +[lints.clippy]
>> +# lint groups
>> +correctness = { level = "deny", priority = -1 }
>> +suspicious = { level = "deny", priority = -1 }
>> +complexity = { level = "deny", priority = -1 }
>> +perf = { level = "deny", priority = -1 }
>> +cargo = { level = "deny", priority = -1 }
>> +nursery = { level = "deny", priority = -1 }
>> +style = { level = "deny", priority = -1 }
>> +# restriction group
>> +dbg_macro = "deny"
>> +rc_buffer = "deny"
>> +as_underscore = "deny"
>> +assertions_on_result_states = "deny"
>> +# pedantic group
>> +doc_markdown = "deny"
>> +expect_fun_call = "deny"
>> +borrow_as_ptr = "deny"
>> +case_sensitive_file_extension_comparisons = "deny"
>> +cast_lossless = "deny"
>> +cast_ptr_alignment = "allow"
>> +large_futures = "deny"
>> +waker_clone_wake = "deny"
>> +unused_enumerate_index = "deny"
>> +unnecessary_fallible_conversions = "deny"
>> +struct_field_names = "deny"
>> +manual_hash_one = "deny"
>> +into_iter_without_iter = "deny"
>> +option_if_let_else = "deny"
>> +missing_const_for_fn = "deny"
>> +significant_drop_tightening = "deny"
>> +multiple_crate_versions = "deny"
>> +significant_drop_in_scrutinee = "deny"
>> +cognitive_complexity = "deny"
>> +missing_safety_doc = "allow"
>
>...
>
>> diff --git a/rust/pl011/rustfmt.toml b/rust/pl011/rustfmt.toml
>> new file mode 120000
>> index 0000000000..39f97b043b
>> --- /dev/null
>> +++ b/rust/pl011/rustfmt.toml
>> @@ -0,0 +1 @@
>> +../rustfmt.toml
>
>...
>
>> diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml
>> new file mode 100644
>> index 0000000000..ebecb99fe0
>> --- /dev/null
>> +++ b/rust/rustfmt.toml
>> @@ -0,0 +1,7 @@
>> +edition = "2021"
>> +format_generated_files = false
>> +format_code_in_doc_comments = true
>> +format_strings = true
>> +imports_granularity = "Crate"
>> +group_imports = "StdExternalCrate"
>> +wrap_comments = true
>> 
>
>About the Rust style, inspired from the discussion on my previous
>simpletrace-rust [1], it looks like people prefer the default rust style
>and use the default check without custom configurations.
>
>More style requirements are also an open, especially for unstable ones,
>and it would be better to split this part into a separate patch, so that
>the discussion about style doesn't overshadow the focus on your example.
>
>[1]: https://lore.kernel.org/qemu-devel/ZlnBGwk29Ds9FjUA@redhat.com/
>

I had read that discussion and had that in mind. There's no need to 
worry about format inconsistencies; these options are unstable  -nightly 
only- format options and they don't affect the default rust style (they 
actually follow it). If you run a stable cargo fmt you will see the code 
won't change (but might complain that these settings are nightly only).

What they do is extra work on top of the default style. If anything ends 
up incompatible with stable I agree it must be removed, there's no sense 
in having a custom Rust style when the defaults are so reasonable.

To sum it up, the style is essentially the default one, so there's no 
problem here!

Manos