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

[Richard Henderson <richard.henderson@linaro.org>]

On 6/11/24 03:33, Manos Pitsidianakis wrote:
> 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

Ah hah.  Nevermind my previous question -- cargo install produces bindgen v0.69.4, quite a 
bit newer than the ubuntu 22.04 packaged version.  I have progressed a bit.

Please bear with me as I attempt to learn Rust in the process of reviewing this.  I 
promise no bikesheding and only to ask silly questions.


>   rust/pl011/.cargo/config.toml  |   2 +
>   rust/pl011/.gitignore          |   2 +
>   rust/pl011/Cargo.lock          | 120 +++++++
>   rust/pl011/Cargo.toml          |  66 ++++
>   rust/pl011/README.md           |  42 +++
>   rust/pl011/build.rs            |  44 +++
>   rust/pl011/deny.toml           |  57 ++++
>   rust/pl011/meson.build         |   7 +
>   rust/pl011/rustfmt.toml        |   1 +

First silly question: how much of this is boiler plate that gets moved the moment that the 
second rust subdirectory is added?


> diff --git a/rust/pl011/.cargo/config.toml b/rust/pl011/.cargo/config.toml
> new file mode 100644
> index 0000000000..241210ffa7
> --- /dev/null
> +++ b/rust/pl011/.cargo/config.toml
> @@ -0,0 +1,2 @@
> +[build]
> +rustflags = ["-Crelocation-model=pic", "-Ctarget-feature=+crt-static"]

It seems certain that this is not specific to pl011, and will be commot to other rust 
subdirectories.  Or, given the .cargo directory name, is this generated by cargo and 
committed by mistake?


> diff --git a/rust/pl011/.gitignore b/rust/pl011/.gitignore
> new file mode 100644
> index 0000000000..28a02c847f
> --- /dev/null
> +++ b/rust/pl011/.gitignore
> @@ -0,0 +1,2 @@
> +target
> +src/generated.rs.inc

Is this a symptom of generating files into the source directory and not build directory?


> diff --git a/rust/pl011/Cargo.lock b/rust/pl011/Cargo.lock
> new file mode 100644
> index 0000000000..d0fa46f9f5
> --- /dev/null
> +++ b/rust/pl011/Cargo.lock
> @@ -0,0 +1,120 @@
> +# This file is automatically @generated by Cargo.
> +# It is not intended for manual editing.

Second silly question: does this really need to be committed to the repository? It 
*appears* to be specific to the host+os-version of the build.  It is certainly very 
specific about versions and checksums...


> 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 @@
> +[package]
> +name = "pl011"
> +version = "0.1.0"
> +edition = "2021"
> +authors = ["Manos Pitsidianakis <manos.pitsidianakis@linaro.org>"]
> +license = "GPL-2.0 OR GPL-3.0-or-later"
> +readme = "README.md"
> +homepage = "https://www.qemu.org"
> +description = "pl011 device model for QEMU"
> +repository = "https://gitlab.com/epilys/rust-for-qemu"
> +resolver = "2"
> +publish = false
> +keywords = []
> +categories = []
> +
> +[lib]
> +crate-type = ["staticlib"]
> +
> +# bilge deps included here to include them with docs
> +[dependencies]
> +arbitrary-int = { version = "1.2.7" }
> +bilge = { version = "0.2.0" }
> +bilge-impl = { version = "0.2.0" }

Likewise.

> diff --git a/rust/pl011/deny.toml b/rust/pl011/deny.toml
> new file mode 100644
> index 0000000000..3992380509
> --- /dev/null
> +++ b/rust/pl011/deny.toml
> @@ -0,0 +1,57 @@
> +# cargo-deny configuration file
> +
> +[graph]
> +targets = [
> +    "aarch64-unknown-linux-gnu",
> +    "x86_64-unknown-linux-gnu",
> +    "x86_64-apple-darwin",
> +    "aarch64-apple-darwin",
> +    "x86_64-pc-windows-gnu",
> +    "aarch64-pc-windows-gnullvm",
> +]

Very much likewise.
Since aarch64-pc-windows-gnullvm is not a host that qemu supports, if this is not 
auto-generated, I am confused.

> 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
> \ No newline at end of file

Newline.  Also, third silly question: is there a way we can avoid replicating this within 
every rust subdirectory?  E.g. some search path option within one of the build tools?


> +++ b/rust/pl011/src/definitions.rs
> +++ b/rust/pl011/src/device.rs
> +++ b/rust/pl011/src/device_class.rs
> +++ b/rust/pl011/src/lib.rs
> +++ b/rust/pl011/src/memory_ops.rs

Eek! Actual Rust! Skipping until I am better educated.


> diff --git a/rust/pl011/src/generated.rs b/rust/pl011/src/generated.rs
> new file mode 100644
> index 0000000000..670e7b541d
> --- /dev/null
> +++ b/rust/pl011/src/generated.rs
> @@ -0,0 +1,5 @@
> +#[cfg(MESON_GENERATED_RS)]
> +include!(concat!(env!("OUT_DIR"), "/generated.rs"));
> +
> +#[cfg(not(MESON_GENERATED_RS))]
> +include!("generated.rs.inc");

Is this indicative of Rust not being prepared to separate source and build directories? 
I'm surprised there would have to be a file in the source to direct the compiler to look 
for a file in the build.


r~