* [RFC PATCH v2 0/5] Implement ARM PL011 in Rust
@ 2024-06-11 10:33 Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 1/5] build-sys: Add rust feature option Manos Pitsidianakis
` (6 more replies)
0 siblings, 7 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-11 10:33 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
Changes from v1->v2:
- Create bindgen target first, then add commit for device (thanks
Pierrick)
- Create a special named generated.rs for each target as compilation
would fail if more than one targets were defined. The generated.rs
target names would clash.
- Add more descriptive commit messages
- Update MAINTAINERS
- Cleanup patch order for better review, hopefully
Previous series was
<cover.rust-pl011-rfc-v1.git.manos.pitsidianakis@linaro.org>
--
Hello everyone,
This is an early draft of my work on implementing a very simple device,
in this case the ARM PL011 (which in C code resides in hw/char/pl011.c
and is used in hw/arm/virt.c).
The device is functional, with copied logic from the C code but with
effort not to make a direct C to Rust translation. In other words, do
not write Rust as a C developer would.
That goal is not complete but a best-effort case. To give a specific
example, register values are typed but interrupt bit flags are not (but
could be). I will leave such minutiae for later iterations.
By the way, the wiki page for Rust was revived to keep track of all
current series on the mailing list https://wiki.qemu.org/RustInQemu
a #qemu-rust IRC channel was also created for rust-specific discussion
that might flood #qemu
------------------------------------------------------------------------
A request: keep comments to Rust in relation to the QEMU project and no
debates on the merits of the language itself. These are valid concerns,
but it'd be better if they were on separate mailing list threads.
------------------------------------------------------------------------
Table of contents: [TOC]
- How can I try it? [howcanItryit]
- What are the most important points to focus on, at this point?
[whatarethemostimportant]
- What are the issues with not using the compiler, rustc, directly?
[whataretheissueswith]
1. Tooling
2. Rust dependencies
- Should QEMU use third-party dependencies? [shouldqemuusethirdparty]
- Should QEMU provide wrapping Rust APIs over QEMU internals?
[qemuprovidewrappingrustapis]
- Will QEMU now depend on Rust and thus not build on my XYZ platform?
[qemudependonrustnotbuildonxyz]
- How is the compilation structured? [howisthecompilationstructured]
- The generated.rs rust file includes a bunch of junk definitions?
[generatedrsincludesjunk]
- The staticlib artifact contains a bunch of mangled .o objects?
[staticlibmangledobjects]
How can I try it?
=================
[howcanItryit] Back to [TOC]
Hopefully applying this patches (or checking out `master` branch from
https://gitlab.com/epilys/rust-for-qemu/ )
Tag for this RFC is rust-pl011-rfc-v2
Rustdoc documentation is hosted on
https://rust-for-qemu-epilys-aebb06ca9f9adfe6584811c14ae44156501d935ba4.gitlab.io/pl011/index.html
If `cargo` and `bindgen` is installed in your system, you should be able
to build qemu-system-aarch64 with configure flag --enable-rust and
launch an arm virt VM. One of the patches hardcodes the default UART of
the machine to the Rust one, so if something goes wrong you will see it
upon launching qemu-system-aarch64.
To confirm it is there for sure, run e.g. info qom-tree on the monitor
and look for x-pl011-rust.
What are the most important points to focus on, at this point?
==============================================================
[whatarethemostimportant] Back to [TOC]
In my opinion, integration of the go-to Rust build system (Cargo and
crates.io) with the build system we use in QEMU. This is "easily" done
in some definition of the word with a python wrapper script.
What are the issues with not using the compiler, rustc, directly?
-----------------------------------------------------------------
[whataretheissueswith] Back to [TOC]
1. Tooling
Mostly writing up the build-sys tooling to do so. Ideally we'd
compile everything without cargo but rustc directly.
If we decide we need Rust's `std` library support, we could
investigate whether building it from scratch is a good solution. This
will only build the bits we need in our devices.
2. Rust dependencies
We could go without them completely. I chose deliberately to include
one dependency in my UART implementation, `bilge`[0], because it has
an elegant way of representing typed bitfields for the UART's
registers.
[0]: Article: https://hecatia-elegua.github.io/blog/no-more-bit-fiddling/
Crates.io page: https://crates.io/crates/bilge
Repository: https://github.com/hecatia-elegua/bilge
Should QEMU use third-party dependencies?
-----------------------------------------
[shouldqemuusethirdparty] Back to [TOC]
In my personal opinion, if we need a dependency we need a strong
argument for it. A dependency needs a trusted upstream source, a QEMU
maintainer to make sure it us up-to-date in QEMU etc.
We already fetch some projects with meson subprojects, so this is not a
new reality. Cargo allows you to define "locked" dependencies which is
the same as only fetching specific commits by SHA. No suspicious
tarballs, and no disappearing dependencies a la left-pad in npm.
However, I believe it's worth considering vendoring every dependency by
default, if they prove to be few, for the sake of having a local QEMU
git clone buildable without network access.
Should QEMU provide wrapping Rust APIs over QEMU internals?
-----------------------------------------------------------
[qemuprovidewrappingrustapis] Back to [TOC]
My personal opinion is no, with the reasoning being that QEMU internals
are not documented or stable. However I do not see why creating stable
opt-in interfaces is bad. It just needs someone to volunteer to maintain
it and ensure there are no breakages through versions.
Will QEMU now depend on Rust and thus not build on my XYZ platform?
-------------------------------------------------------------------
[qemudependonrustnotbuildonxyz] Back to [TOC]
No, worry about this in some years if this experiment takes off. Rust
has broad platform support and is present in most distro package
managers. In the future we might have gcc support for it as well.
For now, Rust will have an experimental status, and will be aimed to
those who wish to try it. I leave it to the project leaders to make
proper decisions and statements on this if necessary.
How is the compilation structured?
==================================
[howisthecompilationstructured] Back to [TOC]
First, a meson target that runs `bindgen` on a bunch of header files
(defined in `rust/wrapper.h`) is created as a target and as a dependency
for any rust hardware device that needs it. You can see the generated
bindings by running
ninja aarch64-softmmu-generated.rs
inside your build directory.
The devices are defined as dictionaries in rust/meson.build because they
depend on the bindgen dependency, which is available much later in the
meson process (when the static qemu lib and target emulator executables
are defined).
A cargo wrapper python script under scripts/ exists to build the crate
library, by providing the path to the generated.rs bindings via the
environment. Then, the qemu-system-aarch64 binary links against the
staticlib archive (i.e. libpl011.a)
The generated.rs rust file includes a bunch of junk definitions?
================================================================
[generatedrsincludesjunk] Back to [TOC]
Yes, bindgen allows you to block certain types and identifiers from
being generated but they are simply too many. I have trimmed some of the
fat but vast improvements can be made.
The staticlib artifact contains a bunch of mangled .o objects?
==============================================================
[staticlibmangledobjects] Back to [TOC]
Yes, until we compile without the `std` module library or we compile it
manually instead of linking it, we will have some junk in it.
--
Manos Pitsidianakis (5):
build-sys: Add rust feature option
rust: add bindgen step as a meson dependency
rust: add PL011 device model
DO NOT MERGE: add rustdoc build for gitlab pages
DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine
.gitignore | 2 +
.gitlab-ci.d/buildtest.yml | 64 ++--
MAINTAINERS | 13 +
configure | 12 +
hw/arm/virt.c | 4 +
meson.build | 102 ++++++
meson_options.txt | 4 +
rust/meson.build | 93 ++++++
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 +
rust/pl011/src/definitions.rs | 95 ++++++
rust/pl011/src/device.rs | 531 ++++++++++++++++++++++++++++++
rust/pl011/src/device_class.rs | 95 ++++++
rust/pl011/src/generated.rs | 5 +
rust/pl011/src/lib.rs | 581 +++++++++++++++++++++++++++++++++
rust/pl011/src/memory_ops.rs | 38 +++
rust/rustfmt.toml | 7 +
rust/wrapper.h | 39 +++
scripts/cargo_wrapper.py | 221 +++++++++++++
scripts/meson-buildoptions.sh | 6 +
27 files changed, 2234 insertions(+), 19 deletions(-)
create mode 100644 rust/meson.build
create mode 100644 rust/pl011/.cargo/config.toml
create mode 100644 rust/pl011/.gitignore
create mode 100644 rust/pl011/Cargo.lock
create mode 100644 rust/pl011/Cargo.toml
create mode 100644 rust/pl011/README.md
create mode 100644 rust/pl011/build.rs
create mode 100644 rust/pl011/deny.toml
create mode 100644 rust/pl011/meson.build
create mode 120000 rust/pl011/rustfmt.toml
create mode 100644 rust/pl011/src/definitions.rs
create mode 100644 rust/pl011/src/device.rs
create mode 100644 rust/pl011/src/device_class.rs
create mode 100644 rust/pl011/src/generated.rs
create mode 100644 rust/pl011/src/lib.rs
create mode 100644 rust/pl011/src/memory_ops.rs
create mode 100644 rust/rustfmt.toml
create mode 100644 rust/wrapper.h
create mode 100644 scripts/cargo_wrapper.py
base-commit: 01782d6b294f95bcde334386f0aaac593cd28c0d
--
γαῖα πυρί μιχθήτω
^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC PATCH v2 1/5] build-sys: Add rust feature option
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
@ 2024-06-11 10:33 ` Manos Pitsidianakis
2024-06-19 4:44 ` Richard Henderson
2024-06-19 16:52 ` Richard Henderson
2024-06-11 10:33 ` [RFC PATCH v2 2/5] rust: add bindgen step as a meson dependency Manos Pitsidianakis
` (5 subsequent siblings)
6 siblings, 2 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-11 10:33 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier, John Snow,
Cleber Rosa
Add options for Rust in meson_options.txt, meson.build, configure to
prepare for adding Rust code in the followup commits.
`rust` is a reserved meson name, so we have to use an alternative.
`with_rust` was chosen.
A cargo_wrapper.py script is added that is heavily based on the work of
Marc-André Lureau from 2021.
https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
.gitignore | 2 +
MAINTAINERS | 5 +
configure | 12 ++
meson.build | 11 ++
meson_options.txt | 4 +
scripts/cargo_wrapper.py | 211 ++++++++++++++++++++++++++++++++++
scripts/meson-buildoptions.sh | 6 +
7 files changed, 251 insertions(+)
create mode 100644 scripts/cargo_wrapper.py
diff --git a/.gitignore b/.gitignore
index 61fa39967b..f42b0d937e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,8 @@
/build/
/.cache/
/.vscode/
+/target/
+rust/**/target
*.pyc
.sdk
.stgit-*
diff --git a/MAINTAINERS b/MAINTAINERS
index 1b79767d61..431010ddbf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4223,6 +4223,11 @@ F: docs/sphinx/
F: docs/_templates/
F: docs/devel/docs.rst
+Rust build system integration
+M: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+S: Maintained
+F: scripts/cargo_wrapper.py
+
Miscellaneous
-------------
Performance Tools and Tests
diff --git a/configure b/configure
index 38ee257701..6baa63b443 100755
--- a/configure
+++ b/configure
@@ -302,6 +302,9 @@ else
objcc="${objcc-${cross_prefix}clang}"
fi
+with_rust="auto"
+with_rust_target_triple=""
+
ar="${AR-${cross_prefix}ar}"
as="${AS-${cross_prefix}as}"
ccas="${CCAS-$cc}"
@@ -760,6 +763,12 @@ for opt do
;;
--gdb=*) gdb_bin="$optarg"
;;
+ --enable-with-rust) with_rust=enabled
+ ;;
+ --disable-with-rust) with_rust=disabled
+ ;;
+ --with-rust-target-triple=*) with_rust_target_triple="$optarg"
+ ;;
# everything else has the same name in configure and meson
--*) meson_option_parse "$opt" "$optarg"
;;
@@ -1796,6 +1805,9 @@ if test "$skip_meson" = no; then
test -n "${LIB_FUZZING_ENGINE+xxx}" && meson_option_add "-Dfuzzing_engine=$LIB_FUZZING_ENGINE"
test "$plugins" = yes && meson_option_add "-Dplugins=true"
test "$tcg" != enabled && meson_option_add "-Dtcg=$tcg"
+ test "$with_rust" != enabled && meson_option_add "-Dwith_rust=$with_rust"
+ test "$with_rust" != enabled && meson_option_add "-Dwith_rust=$with_rust"
+ test "$with_rust_target_triple" != "" && meson_option_add "-Dwith_rust_target_triple=$with_rust_target_triple"
run_meson() {
NINJA=$ninja $meson setup "$@" "$PWD" "$source_path"
}
diff --git a/meson.build b/meson.build
index a9de71d450..3533889852 100644
--- a/meson.build
+++ b/meson.build
@@ -290,6 +290,12 @@ foreach lang : all_languages
endif
endforeach
+cargo = not_found
+if get_option('with_rust').allowed()
+ cargo = find_program('cargo', required: get_option('with_rust'))
+endif
+with_rust = cargo.found()
+
# default flags for all hosts
# We use -fwrapv to tell the compiler that we require a C dialect where
# left shift of signed integers is well defined and has the expected
@@ -2066,6 +2072,7 @@ endif
config_host_data = configuration_data()
+config_host_data.set('CONFIG_WITH_RUST', with_rust)
audio_drivers_selected = []
if have_system
audio_drivers_available = {
@@ -4190,6 +4197,10 @@ if 'objc' in all_languages
else
summary_info += {'Objective-C compiler': false}
endif
+summary_info += {'Rust support': with_rust}
+if with_rust and get_option('with_rust_target_triple') != ''
+ summary_info += {'Rust target': get_option('with_rust_target_triple')}
+endif
option_cflags = (get_option('debug') ? ['-g'] : [])
if get_option('optimization') != 'plain'
option_cflags += ['-O' + get_option('optimization')]
diff --git a/meson_options.txt b/meson_options.txt
index 4c1583eb40..223491b731 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -366,3 +366,7 @@ option('qemu_ga_version', type: 'string', value: '',
option('hexagon_idef_parser', type : 'boolean', value : true,
description: 'use idef-parser to automatically generate TCG code for the Hexagon frontend')
+option('with_rust', type: 'feature', value: 'auto',
+ description: 'Enable Rust support')
+option('with_rust_target_triple', type : 'string', value: '',
+ description: 'Rust target triple')
diff --git a/scripts/cargo_wrapper.py b/scripts/cargo_wrapper.py
new file mode 100644
index 0000000000..d338effdaa
--- /dev/null
+++ b/scripts/cargo_wrapper.py
@@ -0,0 +1,211 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 Red Hat, Inc.
+# Copyright (c) 2023 Linaro Ltd.
+#
+# Authors:
+# Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+# Marc-André Lureau <marcandre.lureau@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later. See the COPYING file in the top-level directory.
+
+import argparse
+import configparser
+import distutils.file_util
+import json
+import logging
+import os
+import os.path
+import re
+import subprocess
+import sys
+import pathlib
+import shutil
+import tomllib
+
+from pathlib import Path
+from typing import Any, Dict, List, Tuple
+
+RUST_TARGET_TRIPLES = (
+ "aarch64-unknown-linux-gnu",
+ "x86_64-unknown-linux-gnu",
+ "x86_64-apple-darwin",
+ "aarch64-apple-darwin",
+)
+
+
+def cfg_name(name: str) -> str:
+ if (
+ name.startswith("CONFIG_")
+ or name.startswith("TARGET_")
+ or name.startswith("HAVE_")
+ ):
+ return name
+ return ""
+
+
+def generate_cfg_flags(header: str) -> List[str]:
+ with open(header, encoding="utf-8") as cfg:
+ config = [l.split()[1:] for l in cfg if l.startswith("#define")]
+
+ cfg_list = []
+ for cfg in config:
+ name = cfg_name(cfg[0])
+ if not name:
+ continue
+ if len(cfg) >= 2 and cfg[1] != "1":
+ continue
+ cfg_list.append("--cfg")
+ cfg_list.append(name)
+ return cfg_list
+
+
+def cargo_target_dir(args: argparse.Namespace) -> pathlib.Path:
+ return args.meson_build_dir
+
+
+def manifest_path(args: argparse.Namespace) -> pathlib.Path:
+ return args.crate_dir / "Cargo.toml"
+
+
+def get_cargo_rustc(args: argparse.Namespace) -> tuple[Dict[str, Any], List[str]]:
+ # See https://doc.rust-lang.org/cargo/reference/environment-variables.html
+ # Item `CARGO_ENCODED_RUSTFLAGS — A list of custom flags separated by
+ # 0x1f (ASCII Unit Separator) to pass to all compiler invocations that Cargo
+ # performs`
+ cfg = chr(0x1F).join(
+ [c for h in args.config_headers for c in generate_cfg_flags(h)]
+ )
+ target_dir = cargo_target_dir(args)
+ cargo_path = manifest_path(args)
+
+ cargo_cmd = [
+ "cargo",
+ "build",
+ "--target-dir",
+ str(target_dir),
+ "--manifest-path",
+ str(cargo_path),
+ ]
+ if args.target_triple:
+ cargo_cmd += ["--target", args.target_triple]
+ if args.profile == "release":
+ cargo_cmd += ["--release"]
+
+ env = os.environ
+ env["CARGO_ENCODED_RUSTFLAGS"] = cfg
+
+ return (env, cargo_cmd)
+
+
+def run_cargo(env: Dict[str, Any], cargo_cmd: List[str]) -> str:
+ envlog = " ".join(["{}={}".format(k, v) for k, v in env.items()])
+ cmdlog = " ".join(cargo_cmd)
+ logging.debug("Running %s %s", envlog, cmdlog)
+ try:
+ out = subprocess.check_output(
+ cargo_cmd,
+ env=dict(os.environ, **env),
+ stderr=subprocess.STDOUT,
+ universal_newlines=True,
+ )
+ except subprocess.CalledProcessError as err:
+ print("Environment: " + envlog)
+ print("Command: " + cmdlog)
+ print(err.output)
+ sys.exit(1)
+
+ return out
+
+
+def build_lib(args: argparse.Namespace) -> None:
+ logging.debug("build-lib")
+ target_dir = cargo_target_dir(args)
+ cargo_toml_path = manifest_path(args)
+
+ with open(cargo_toml_path, "rb") as f:
+ config = tomllib.load(f)
+
+ package_name = config["package"]["name"].strip('"').replace("-", "_")
+
+ liba_filename = "lib" + package_name + ".a"
+ liba = target_dir / args.target_triple / args.profile / liba_filename
+
+ env, cargo_cmd = get_cargo_rustc(args)
+ out = run_cargo(env, cargo_cmd)
+ logging.debug("cp %s %s", liba, args.outdir)
+ shutil.copy2(liba, args.outdir)
+
+
+def main() -> None:
+ parser = argparse.ArgumentParser()
+ parser.add_argument("-v", "--verbose", action="store_true")
+ parser.add_argument(
+ "--color",
+ metavar="WHEN",
+ choices=["auto", "always", "never"],
+ default="auto",
+ help="Coloring: auto, always, never",
+ )
+ parser.add_argument(
+ "--config-headers",
+ metavar="CONFIG_HEADER",
+ action="append",
+ dest="config_headers",
+ required=False,
+ default=[],
+ )
+ parser.add_argument(
+ "--meson-build-dir",
+ metavar="BUILD_DIR",
+ help="meson.current_build_dir()",
+ type=pathlib.Path,
+ dest="meson_build_dir",
+ required=True,
+ )
+ parser.add_argument(
+ "--meson-source-dir",
+ metavar="SOURCE_DIR",
+ help="meson.current_source_dir()",
+ type=pathlib.Path,
+ dest="meson_source_dir",
+ required=True,
+ )
+ parser.add_argument(
+ "--crate-dir",
+ metavar="CRATE_DIR",
+ type=pathlib.Path,
+ dest="crate_dir",
+ help="Absolute path that contains the manifest file of the crate to compile",
+ required=True,
+ )
+ parser.add_argument(
+ "--outdir",
+ metavar="OUTDIR",
+ type=pathlib.Path,
+ dest="outdir",
+ help="Path to copy compiled artifacts to for Meson to use.",
+ required=True,
+ )
+ parser.add_argument(
+ "--profile", type=str, choices=["release", "debug"], required=True
+ )
+ parser.add_argument(
+ "--target-triple", type=str, choices=RUST_TARGET_TRIPLES, required=True
+ )
+
+ subparsers = parser.add_subparsers()
+
+ buildlib = subparsers.add_parser("build-lib")
+ buildlib.set_defaults(func=build_lib)
+
+ args = parser.parse_args()
+ if args.verbose:
+ logging.basicConfig(level=logging.DEBUG)
+ logging.debug("args: %s", args)
+
+ args.func(args)
+
+
+if __name__ == "__main__":
+ main()
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 6ce5a8b72a..fdcf17129e 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -79,6 +79,8 @@ meson_options_help() {
printf "%s\n" ' auto/sigaltstack/ucontext/windows)'
printf "%s\n" ' --with-pkgversion=VALUE use specified string as sub-version of the'
printf "%s\n" ' package'
+ printf "%s\n" ' --with-rust-target-triple=VALUE'
+ printf "%s\n" ' Specify Rust host target triple'
printf "%s\n" ' --with-suffix=VALUE Suffix for QEMU data/modules/config directories'
printf "%s\n" ' (can be empty) [qemu]'
printf "%s\n" ' --with-trace-file=VALUE Trace file prefix for simple backend [trace]'
@@ -215,6 +217,7 @@ meson_options_help() {
printf "%s\n" ' vvfat vvfat image format support'
printf "%s\n" ' werror Treat warnings as errors'
printf "%s\n" ' whpx WHPX acceleration support'
+ printf "%s\n" ' with-rust Enable Rust support'
printf "%s\n" ' xen Xen backend support'
printf "%s\n" ' xen-pci-passthrough'
printf "%s\n" ' Xen PCI passthrough support'
@@ -550,6 +553,9 @@ _meson_option_parse() {
--disable-werror) printf "%s" -Dwerror=false ;;
--enable-whpx) printf "%s" -Dwhpx=enabled ;;
--disable-whpx) printf "%s" -Dwhpx=disabled ;;
+ --enable-with-rust) printf "%s" -Dwith_rust=enabled ;;
+ --disable-with-rust) printf "%s" -Dwith_rust=disabled ;;
+ --with-rust-target-triple=*) quote_sh "-Dwith_rust_target_triple=$2" ;;
--enable-xen) printf "%s" -Dxen=enabled ;;
--disable-xen) printf "%s" -Dxen=disabled ;;
--enable-xen-pci-passthrough) printf "%s" -Dxen_pci_passthrough=enabled ;;
--
γαῖα πυρί μιχθήτω
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [RFC PATCH v2 2/5] rust: add bindgen step as a meson dependency
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 1/5] build-sys: Add rust feature option Manos Pitsidianakis
@ 2024-06-11 10:33 ` Manos Pitsidianakis
2024-06-17 21:01 ` Paolo Bonzini
2024-06-11 10:33 ` [RFC PATCH v2 3/5] rust: add PL011 device model Manos Pitsidianakis
` (4 subsequent siblings)
6 siblings, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-11 10:33 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier, John Snow,
Cleber Rosa
Add mechanism to generate rust hw targets that depend on a custom
bindgen target for rust bindings to C.
This way bindings will be created before the rust crate is compiled.
The bindings will end up in BUILDDIR/{target}-generated.rs and have the same name
as a target:
ninja aarch64-softmmu-generated.rs
The way the bindings are generated is:
1. All required C headers are included in a single file, in our case
rust/wrapper.h for convenience. Otherwise we'd have to provide a list
of headers every time to the bindgen tool.
2. Meson creates a generated_rs target that runs bindgen making sure
the architecture etc header dependencies are present.
3. The generated_rs target takes a list of files, type symbols,
function symbols to block from being generated. This is not necessary
for the bindings to work, but saves us time and space.
4. Meson creates rust hardware target dependencies from the rust_targets
dictionary defined in rust/meson.build.
Since we cannot declare a dependency on generated_rs before it is
declared in meson.build, the rust crate targets must be defined after
the generated_rs target for each target architecture is defined. This
way meson sets up the dependency tree properly.
5. After compiling each rust crate with the cargo_wrapper.py script,
its static library artifact is linked as a `whole-archive` with the
final binary.
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
MAINTAINERS | 2 +
meson.build | 87 ++++++++++++++++++++++++++++++++++++++
rust/meson.build | 91 ++++++++++++++++++++++++++++++++++++++++
rust/wrapper.h | 39 +++++++++++++++++
scripts/cargo_wrapper.py | 10 +++++
5 files changed, 229 insertions(+)
create mode 100644 rust/meson.build
create mode 100644 rust/wrapper.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 431010ddbf..ff6117f41d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4227,6 +4227,8 @@ Rust build system integration
M: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
S: Maintained
F: scripts/cargo_wrapper.py
+F: rust/meson.build
+F: rust/wrapper.h
Miscellaneous
-------------
diff --git a/meson.build b/meson.build
index 3533889852..a08c975ef9 100644
--- a/meson.build
+++ b/meson.build
@@ -3876,6 +3876,93 @@ foreach target : target_dirs
lib_deps += dep.partial_dependency(compile_args: true, includes: true)
endforeach
+ if with_rust and target_type == 'system'
+ rust_bindgen = import('unstable-rust')
+ rust_bindgen_dep = declare_dependency(
+ dependencies: arch_deps + lib_deps,
+ include_directories: target_inc,
+ sources: arch_srcs + genh)
+
+ # We need one generated_rs target per target, so give them
+ # target-specific names
+ copy = fs.copyfile('rust/wrapper.h', target + '_wrapper.h')
+ generated_rs = rust_bindgen.bindgen(
+ input: copy,
+ dependencies: arch_deps + rust_bindgen_dep,
+ output: target + '-generated.rs',
+ include_directories: target_inc + include_directories('.') + include_directories('include'),
+ args: [
+ '--formatter',
+ 'rustfmt',
+ '--no-doc-comments',
+ '--merge-extern-blocks',
+ '--generate-block',
+ '--generate-cstr',
+ '--impl-debug',
+ '--with-derive-default',
+ '--use-core',
+ '--ctypes-prefix',
+ 'core::ffi',
+ '--blocklist-var',
+ 'IPPORT_RESERVED',
+ '--blocklist-type',
+ '^.+_.*autoptr$',
+ '--blocklist-type',
+ 'max_align_t',
+ '--blocklist-function',
+ 'str*',
+ '--blocklist-function',
+ 'ato*',
+ '--blocklist-function',
+ 'p?select',
+ '--blocklist-file',
+ '.+inttypes.h$',
+ '--blocklist-file',
+ '.+limits.h$',
+ '--blocklist-file',
+ '.+ctype.h$',
+ '--blocklist-file',
+ '.+errno.h$',
+ '--blocklist-file',
+ '.+fcntl.h$',
+ '--blocklist-file',
+ '.+getopt.h$',
+ '--blocklist-file',
+ '.+assert.h$',
+ '--blocklist-file',
+ '.+stdbool.h$',
+ '--blocklist-file',
+ '.+stdio.h$',
+ '--blocklist-file',
+ '.+stdint.h$',
+ '--blocklist-file',
+ '.+stdalign.h$',
+ ],
+ c_args: c_args,
+ )
+
+ if target in rust_targets
+ rust_hw = ss.source_set()
+ foreach t: rust_targets[target]
+ rust_device_cargo = custom_target(t['name'],
+ output: t['output'],
+ depends: [generated_rs],
+ build_always_stale: true,
+ command: t['command'])
+ rust_dep = declare_dependency(link_args: [
+ '-Wl,--whole-archive',
+ t['output-path'],
+ '-Wl,--no-whole-archive'
+ ],
+ sources: [rust_device_cargo])
+ rust_hw.add(rust_dep)
+ endforeach
+ rust_hw_config = rust_hw.apply(config_target, strict: false)
+ arch_srcs += rust_hw_config.sources()
+ arch_deps += rust_hw_config.dependencies()
+ endif
+ endif
+
lib = static_library('qemu-' + target,
sources: arch_srcs + genh,
dependencies: lib_deps,
diff --git a/rust/meson.build b/rust/meson.build
new file mode 100644
index 0000000000..e9660a3045
--- /dev/null
+++ b/rust/meson.build
@@ -0,0 +1,91 @@
+rust_targets = {}
+
+cargo_wrapper = [
+ find_program(meson.global_source_root() / 'scripts/cargo_wrapper.py'),
+ '--config-headers', meson.project_build_root() / 'config-host.h',
+ '--meson-build-root', meson.project_build_root(),
+ '--meson-build-dir', meson.current_build_dir(),
+ '--meson-source-dir', meson.current_source_dir(),
+]
+
+if get_option('b_colorout') != 'never'
+ cargo_wrapper += ['--color', 'always']
+endif
+
+rust_supported_oses = {'linux': '-unknown-linux-gnu', 'darwin': '-apple-darwin', 'windows': '-pc-windows-gnu'}
+rust_supported_cpus = ['x86_64', 'aarch64']
+
+# Future-proof the above definitions against any change in the root meson.build file:
+foreach rust_os: rust_supported_oses.keys()
+ if not supported_oses.contains(rust_os)
+ message()
+ warning('UNSUPPORTED OS VALUES IN ' + meson.current_source_dir() + '/meson.build')
+ message()
+ message('This meson.build file claims OS `+' + rust_os + '` is supported but')
+ message('it is not included in the global supported OSes list in')
+ message(meson.source_root() + '/meson.build.')
+ endif
+endforeach
+foreach rust_cpu: rust_supported_cpus
+ if not supported_cpus.contains(rust_cpu)
+ message()
+ warning('UNSUPPORTED CPU VALUES IN ' + meson.current_source_dir() + '/meson.build')
+ message()
+ message('This meson.build file claims CPU `+' + rust_cpu + '` is supported but')
+ message('it is not included in the global supported CPUs list in')
+ message(meson.source_root() + '/meson.build.')
+ endif
+endforeach
+
+if meson.is_cross_build()
+ message()
+ error('QEMU does not support cross compiling with Rust enabled.')
+endif
+
+rust_target_triple = get_option('with_rust_target_triple')
+
+# TODO: verify rust_target_triple if given as an option
+if rust_target_triple == ''
+ if not supported_oses.contains(host_os)
+ message()
+ error('QEMU does not support `' + host_os +'` as a Rust platform.')
+ elif not supported_cpus.contains(host_arch)
+ message()
+ error('QEMU does not support `' + host_arch +'` as a Rust architecture.')
+ endif
+ rust_target_triple = host_arch + rust_supported_oses[host_os]
+ if host_os == 'windows' and host_arch == 'aarch64'
+ rust_target_triple += 'llvm'
+ endif
+endif
+
+if get_option('optimization') in ['0', '1', 'g']
+ rs_build_type = 'debug'
+else
+ rs_build_type = 'release'
+endif
+
+rust_hw_target_list = {}
+
+foreach rust_hw_target, rust_hws: rust_hw_target_list
+ foreach rust_hw_dev: rust_hws
+ output = meson.current_build_dir() / rust_target_triple / rs_build_type / rust_hw_dev['output']
+ crate_metadata = {
+ 'name': rust_hw_dev['name'],
+ 'output': [rust_hw_dev['output']],
+ 'output-path': output,
+ 'command': [cargo_wrapper,
+ '--crate-dir',
+ meson.current_source_dir() / rust_hw_dev['dirname'],
+ '--profile',
+ rs_build_type,
+ '--target-triple',
+ rust_target_triple,
+ '--outdir',
+ '@OUTDIR@',
+ 'build-lib'
+ ]
+ }
+ rust_targets += { rust_hw_target: [crate_metadata] }
+ endforeach
+endforeach
diff --git a/rust/wrapper.h b/rust/wrapper.h
new file mode 100644
index 0000000000..bcf808c8d7
--- /dev/null
+++ b/rust/wrapper.h
@@ -0,0 +1,39 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2020 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu-io.h"
+#include "sysemu/sysemu.h"
+#include "hw/sysbus.h"
+#include "exec/memory.h"
+#include "chardev/char-fe.h"
+#include "hw/clock.h"
+#include "hw/qdev-clock.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "hw/irq.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+#include "chardev/char-serial.h"
diff --git a/scripts/cargo_wrapper.py b/scripts/cargo_wrapper.py
index d338effdaa..a36a4fc86d 100644
--- a/scripts/cargo_wrapper.py
+++ b/scripts/cargo_wrapper.py
@@ -94,6 +94,8 @@ def get_cargo_rustc(args: argparse.Namespace) -> tuple[Dict[str, Any], List[str]
env = os.environ
env["CARGO_ENCODED_RUSTFLAGS"] = cfg
+ env["MESON_BUILD_DIR"] = str(target_dir)
+ env["MESON_BUILD_ROOT"] = str(args.meson_build_root)
return (env, cargo_cmd)
@@ -164,6 +166,14 @@ def main() -> None:
required=True,
)
parser.add_argument(
+ "--meson-build-root",
+ metavar="BUILD_ROOT",
+ help="meson.project_build_root()",
+ type=pathlib.Path,
+ dest="meson_build_root",
+ required=True,
+ )
+ parser.add_argument(
"--meson-source-dir",
metavar="SOURCE_DIR",
help="meson.current_source_dir()",
--
γαῖα πυρί μιχθήτω
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 1/5] build-sys: Add rust feature option Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 2/5] rust: add bindgen step as a meson dependency Manos Pitsidianakis
@ 2024-06-11 10:33 ` Manos Pitsidianakis
2024-06-12 12:29 ` Paolo Bonzini
` (3 more replies)
2024-06-11 10:33 ` [RFC PATCH v2 4/5] DO NOT MERGE: add rustdoc build for gitlab pages Manos Pitsidianakis
` (3 subsequent siblings)
6 siblings, 4 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-11 10:33 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
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>
---
MAINTAINERS | 6 +
meson.build | 4 +
rust/meson.build | 2 +
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 +
rust/pl011/src/definitions.rs | 95 ++++++
rust/pl011/src/device.rs | 531 ++++++++++++++++++++++++++++++
rust/pl011/src/device_class.rs | 95 ++++++
rust/pl011/src/generated.rs | 5 +
rust/pl011/src/lib.rs | 581 +++++++++++++++++++++++++++++++++
rust/pl011/src/memory_ops.rs | 38 +++
rust/rustfmt.toml | 7 +
19 files changed, 1705 insertions(+)
create mode 100644 rust/pl011/.cargo/config.toml
create mode 100644 rust/pl011/.gitignore
create mode 100644 rust/pl011/Cargo.lock
create mode 100644 rust/pl011/Cargo.toml
create mode 100644 rust/pl011/README.md
create mode 100644 rust/pl011/build.rs
create mode 100644 rust/pl011/deny.toml
create mode 100644 rust/pl011/meson.build
create mode 120000 rust/pl011/rustfmt.toml
create mode 100644 rust/pl011/src/definitions.rs
create mode 100644 rust/pl011/src/device.rs
create mode 100644 rust/pl011/src/device_class.rs
create mode 100644 rust/pl011/src/generated.rs
create mode 100644 rust/pl011/src/lib.rs
create mode 100644 rust/pl011/src/memory_ops.rs
create mode 100644 rust/rustfmt.toml
diff --git a/MAINTAINERS b/MAINTAINERS
index ff6117f41d..e77a3d4169 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1181,6 +1181,11 @@ F: include/hw/*/microbit*.h
F: tests/qtest/microbit-test.c
F: docs/system/arm/nrf.rst
+ARM PL011 Rust device
+M: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+S: Maintained
+F: rust/pl011/
+
AVR Machines
-------------
@@ -4229,6 +4234,7 @@ S: Maintained
F: scripts/cargo_wrapper.py
F: rust/meson.build
F: rust/wrapper.h
+F: rust/rustfmt.toml
Miscellaneous
-------------
diff --git a/meson.build b/meson.build
index a08c975ef9..23e8c43562 100644
--- a/meson.build
+++ b/meson.build
@@ -296,6 +296,10 @@ if get_option('with_rust').allowed()
endif
with_rust = cargo.found()
+if with_rust
+ subdir('rust')
+endif
+
# default flags for all hosts
# We use -fwrapv to tell the compiler that we require a C dialect where
# left shift of signed integers is well defined and has the expected
diff --git a/rust/meson.build b/rust/meson.build
index e9660a3045..264787198f 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -67,6 +67,8 @@ endif
rust_hw_target_list = {}
+subdir('pl011')
+
foreach rust_hw_target, rust_hws: rust_hw_target_list
foreach rust_hw_dev: rust_hws
output = meson.current_build_dir() / rust_target_triple / rs_build_type / rust_hw_dev['output']
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"]
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
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.
+version = 3
+
+[[package]]
+name = "arbitrary-int"
+version = "1.2.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c84fc003e338a6f69fbd4f7fe9f92b535ff13e9af8997f3b14b6ddff8b1df46d"
+
+[[package]]
+name = "bilge"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "dc707ed8ebf81de5cd6c7f48f54b4c8621760926cdf35a57000747c512e67b57"
+dependencies = [
+ "arbitrary-int",
+ "bilge-impl",
+]
+
+[[package]]
+name = "bilge-impl"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "feb11e002038ad243af39c2068c8a72bcf147acf05025dcdb916fcc000adb2d8"
+dependencies = [
+ "itertools",
+ "proc-macro-error",
+ "proc-macro2",
+ "quote",
+ "syn",
+]
+
+[[package]]
+name = "either"
+version = "1.12.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b"
+
+[[package]]
+name = "itertools"
+version = "0.11.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "b1c173a5686ce8bfa551b3563d0c2170bf24ca44da99c7ca4bfdab5418c3fe57"
+dependencies = [
+ "either",
+]
+
+[[package]]
+name = "pl011"
+version = "0.1.0"
+dependencies = [
+ "arbitrary-int",
+ "bilge",
+ "bilge-impl",
+]
+
+[[package]]
+name = "proc-macro-error"
+version = "1.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c"
+dependencies = [
+ "proc-macro-error-attr",
+ "proc-macro2",
+ "quote",
+ "version_check",
+]
+
+[[package]]
+name = "proc-macro-error-attr"
+version = "1.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "version_check",
+]
+
+[[package]]
+name = "proc-macro2"
+version = "1.0.84"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ec96c6a92621310b51366f1e28d05ef11489516e93be030060e5fc12024a49d6"
+dependencies = [
+ "unicode-ident",
+]
+
+[[package]]
+name = "quote"
+version = "1.0.36"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0fa76aaf39101c457836aec0ce2316dbdc3ab723cdda1c6bd4e6ad4208acaca7"
+dependencies = [
+ "proc-macro2",
+]
+
+[[package]]
+name = "syn"
+version = "2.0.66"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c42f3f41a2de00b01c0aaad383c5a45241efc8b2d1eda5661812fda5f3cdcff5"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "unicode-ident",
+]
+
+[[package]]
+name = "unicode-ident"
+version = "1.0.12"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b"
+
+[[package]]
+name = "version_check"
+version = "0.9.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f"
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" }
+
+[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"
+
+# Do not include in any global workspace
+[workspace]
diff --git a/rust/pl011/README.md b/rust/pl011/README.md
new file mode 100644
index 0000000000..54efe3c0cb
--- /dev/null
+++ b/rust/pl011/README.md
@@ -0,0 +1,42 @@
+# PL011 QEMU Device Model
+
+This library implements a device model for the PrimeCell® UART (PL011)
+device in QEMU.
+
+The C bindings were generated for commit `01782d6b29`:
+
+```console
+$ git describe 01782d6b29
+v9.0.0-769-g01782d6b29
+```
+
+with `bindgen`, using this build target:
+
+```console
+$ ninja aarch64-softmmu-generated.rs
+```
+
+## Build static lib
+
+```sh
+cargo build --target x86_64-unknown-linux-gnu
+```
+
+Replace host target triplet if necessary.
+
+## Generate Rust documentation
+
+To generate docs for this crate, including private items:
+
+```sh
+cargo doc --no-deps --document-private-items --target x86_64-unknown-linux-gnu
+```
+
+To include direct dependencies like `bilge` (bitmaps for register types):
+
+```sh
+cargo tree --depth 1 -e normal --prefix none \
+ | cut -d' ' -f1 \
+ | xargs printf -- '-p %s\n' \
+ | xargs cargo doc --no-deps --document-private-items --target x86_64-unknown-linux-gnu
+```
diff --git a/rust/pl011/build.rs b/rust/pl011/build.rs
new file mode 100644
index 0000000000..169516d8ab
--- /dev/null
+++ b/rust/pl011/build.rs
@@ -0,0 +1,44 @@
+use std::{env, path::Path};
+
+fn main() {
+ println!("cargo::rerun-if-env-changed=MESON_BUILD_DIR");
+ println!("cargo::rerun-if-env-changed=MESON_BUILD_ROOT");
+ println!("cargo::rerun-if-changed=src/generated.rs.inc");
+
+ let out_dir = env::var_os("OUT_DIR").unwrap();
+
+ if let Some(build_dir) = std::env::var_os("MESON_BUILD_ROOT") {
+ let mut build_dir = Path::new(&build_dir).to_path_buf();
+ let mut out_dir = Path::new(&out_dir).to_path_buf();
+ assert!(
+ build_dir.exists(),
+ "MESON_BUILD_ROOT value does not exist on filesystem: {}",
+ build_dir.display()
+ );
+ assert!(
+ build_dir.is_dir(),
+ "MESON_BUILD_ROOT value is not actually a directory: {}",
+ build_dir.display()
+ );
+ build_dir.push("aarch64-softmmu-generated.rs");
+ let generated_rs = build_dir;
+ assert!(
+ generated_rs.exists(),
+ "MESON_BUILD_ROOT/aarch64-softmmu-generated.rs does not exist on filesystem: {}",
+ generated_rs.display()
+ );
+ assert!(
+ generated_rs.is_file(),
+ "MESON_BUILD_ROOT/aarch64-softmmu-generated.rs is not a file: {}",
+ generated_rs.display()
+ );
+ out_dir.push("generated.rs");
+ std::fs::copy(generated_rs, out_dir).unwrap();
+ println!("cargo::rustc-cfg=MESON_GENERATED_RS");
+ } else if !Path::new("src/generated.rs.inc").exists() {
+ panic!(
+ "No generated C bindings found! Either build them manually with bindgen or with meson \
+ (`ninja aarch64-softmmu-generated.rs`) and copy them to src/generated.rs.inc, or build through meson."
+ );
+ }
+}
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",
+]
+#exclude = []
+all-features = false
+no-default-features = false
+#features = []
+
+[output]
+feature-depth = 1
+
+[advisories]
+db-path = "$CARGO_HOME/advisory-dbs"
+db-urls = ["https://github.com/rustsec/advisory-db"]
+ignore = []
+
+[licenses]
+allow = [
+ "GPL-2.0",
+ "MIT",
+ "Apache-2.0",
+ "Unicode-DFS-2016",
+]
+confidence-threshold = 0.8
+exceptions = []
+
+[licenses.private]
+ignore = false
+registries = []
+
+[bans]
+multiple-versions = "warn"
+wildcards = "deny"
+# The graph highlighting used when creating dotgraphs for crates
+# with multiple versions
+# * lowest-version - The path to the lowest versioned duplicate is highlighted
+# * simplest-path - The path to the version with the fewest edges is highlighted
+# * all - Both lowest-version and simplest-path are used
+highlight = "all"
+workspace-default-features = "allow"
+external-default-features = "allow"
+allow = []
+deny = []
+
+[sources]
+unknown-registry = "deny"
+unknown-git = "deny"
+allow-registry = ["https://github.com/rust-lang/crates.io-index"]
+allow-git = []
diff --git a/rust/pl011/meson.build b/rust/pl011/meson.build
new file mode 100644
index 0000000000..70d1ca1148
--- /dev/null
+++ b/rust/pl011/meson.build
@@ -0,0 +1,7 @@
+rust_pl011 = {
+ 'name': 'rust_pl011_cargo',
+ 'dirname': 'pl011',
+ 'output': 'libpl011.a',
+ }
+
+rust_hw_target_list += {'aarch64-softmmu': [rust_pl011]}
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
diff --git a/rust/pl011/src/definitions.rs b/rust/pl011/src/definitions.rs
new file mode 100644
index 0000000000..f02604c114
--- /dev/null
+++ b/rust/pl011/src/definitions.rs
@@ -0,0 +1,95 @@
+//! Definitions required by QEMU when registering the device.
+
+use core::{ffi::CStr, mem::MaybeUninit, ptr::NonNull};
+
+use crate::{device::PL011State, device_class::pl011_class_init, generated::*};
+
+pub const TYPE_PL011: &CStr = unsafe { CStr::from_bytes_with_nul_unchecked(b"x-pl011-rust\0") };
+
+pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
+ name: TYPE_PL011.as_ptr(),
+ parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
+ instance_size: core::mem::size_of::<PL011State>(),
+ instance_align: core::mem::align_of::<PL011State>(),
+ instance_init: Some(pl011_init),
+ instance_post_init: None,
+ instance_finalize: None,
+ abstract_: false,
+ class_size: 0,
+ class_init: Some(pl011_class_init),
+ class_base_init: None,
+ class_data: core::ptr::null_mut(),
+ interfaces: core::ptr::null_mut(),
+};
+
+pub const VMSTATE_PL011: VMStateDescription = VMStateDescription {
+ name: PL011_ARM_INFO.name,
+ unmigratable: true,
+ ..unsafe { MaybeUninit::<VMStateDescription>::zeroed().assume_init() }
+};
+//version_id : 2,
+//minimum_version_id : 2,
+//post_load : pl011_post_load,
+//fields = (const VMStateField[]) {
+// VMSTATE_UINT32(readbuff, PL011State),
+// VMSTATE_UINT32(flags, PL011State),
+// VMSTATE_UINT32(lcr, PL011State),
+// VMSTATE_UINT32(rsr, PL011State),
+// VMSTATE_UINT32(cr, PL011State),
+// VMSTATE_UINT32(dmacr, PL011State),
+// VMSTATE_UINT32(int_enabled, PL011State),
+// VMSTATE_UINT32(int_level, PL011State),
+// VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH),
+// VMSTATE_UINT32(ilpr, PL011State),
+// VMSTATE_UINT32(ibrd, PL011State),
+// VMSTATE_UINT32(fbrd, PL011State),
+// VMSTATE_UINT32(ifl, PL011State),
+// VMSTATE_INT32(read_pos, PL011State),
+// VMSTATE_INT32(read_count, PL011State),
+// VMSTATE_INT32(read_trigger, PL011State),
+// VMSTATE_END_OF_LIST()
+//},
+//.subsections = (const VMStateDescription * const []) {
+// &vmstate_pl011_clock,
+// NULL
+//}
+
+#[no_mangle]
+pub unsafe extern "C" fn pl011_init(obj: *mut Object) {
+ assert!(!obj.is_null());
+ let mut state = NonNull::new_unchecked(obj.cast::<PL011State>());
+ state.as_mut().init();
+}
+
+use crate::generated::module_init_type_MODULE_INIT_QOM;
+
+#[macro_export]
+macro_rules! module_init {
+ ($func:expr, $type:expr) => {
+ #[cfg_attr(target_os = "linux", link_section = ".ctors")]
+ #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
+ #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
+ pub static LOAD_MODULE: extern "C" fn() = {
+ assert!($type < $crate::generated::module_init_type_MODULE_INIT_MAX);
+
+ extern "C" fn __load() {
+ // ::std::panic::set_hook(::std::boxed::Box::new(|_| {}));
+
+ unsafe {
+ $crate::generated::register_module_init(Some($func), $type);
+ }
+ }
+
+ __load
+ };
+ };
+}
+
+#[no_mangle]
+unsafe extern "C" fn register_type() {
+ crate::generated::type_register_static(&PL011_ARM_INFO);
+}
+
+module_init! {
+ register_type, module_init_type_MODULE_INIT_QOM
+}
diff --git a/rust/pl011/src/device.rs b/rust/pl011/src/device.rs
new file mode 100644
index 0000000000..405f590f03
--- /dev/null
+++ b/rust/pl011/src/device.rs
@@ -0,0 +1,531 @@
+use core::{
+ ffi::{c_int, c_uchar, c_uint, c_void, CStr},
+ mem::MaybeUninit,
+ ptr::{addr_of, addr_of_mut, NonNull},
+};
+
+use crate::{
+ definitions::PL011_ARM_INFO,
+ generated::{self, *},
+ memory_ops::PL011_OPS,
+ registers::{self, Interrupt},
+ RegisterOffset,
+};
+
+static PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
+
+const DATA_BREAK: u32 = 1 << 10;
+
+/// QEMU sourced constant.
+pub const PL011_FIFO_DEPTH: usize = 16_usize;
+
+#[repr(C)]
+#[derive(Debug)]
+/// PL011 Device Model in QEMU
+pub struct PL011State {
+ pub parent_obj: SysBusDevice,
+ pub iomem: MemoryRegion,
+ pub readbuff: u32,
+ #[doc(alias = "fr")]
+ pub flags: registers::Flags,
+ #[doc(alias = "lcr")]
+ pub line_control: registers::LineControl,
+ #[doc(alias = "rsr")]
+ pub receive_status_error_clear: registers::ReceiveStatusErrorClear,
+ #[doc(alias = "cr")]
+ pub control: registers::Control,
+ pub dmacr: u32,
+ pub int_enabled: u32,
+ pub int_level: u32,
+ pub read_fifo: [u32; PL011_FIFO_DEPTH],
+ pub ilpr: u32,
+ pub ibrd: u32,
+ pub fbrd: u32,
+ pub ifl: u32,
+ pub read_pos: usize,
+ pub read_count: usize,
+ pub read_trigger: usize,
+ #[doc(alias = "chr")]
+ pub char_backend: CharBackend,
+ /// QEMU interrupts
+ ///
+ /// ```text
+ /// * sysbus MMIO region 0: device registers
+ /// * sysbus IRQ 0: `UARTINTR` (combined interrupt line)
+ /// * sysbus IRQ 1: `UARTRXINTR` (receive FIFO interrupt line)
+ /// * sysbus IRQ 2: `UARTTXINTR` (transmit FIFO interrupt line)
+ /// * sysbus IRQ 3: `UARTRTINTR` (receive timeout interrupt line)
+ /// * sysbus IRQ 4: `UARTMSINTR` (momem status interrupt line)
+ /// * sysbus IRQ 5: `UARTEINTR` (error interrupt line)
+ /// ```
+ #[doc(alias = "irq")]
+ pub interrupts: [qemu_irq; 6usize],
+ #[doc(alias = "clk")]
+ pub clock: NonNull<Clock>,
+ #[doc(alias = "migrate_clk")]
+ pub migrate_clock: bool,
+}
+
+impl PL011State {
+ pub fn init(&mut self) {
+ unsafe {
+ memory_region_init_io(
+ addr_of_mut!(self.iomem),
+ addr_of_mut!(*self).cast::<Object>(),
+ &PL011_OPS,
+ addr_of_mut!(*self).cast::<c_void>(),
+ PL011_ARM_INFO.name,
+ 0x1000,
+ );
+ let sbd = addr_of_mut!(*self).cast::<SysBusDevice>();
+ let dev = addr_of_mut!(*self).cast::<DeviceState>();
+ sysbus_init_mmio(sbd, addr_of_mut!(self.iomem));
+ for irq in self.interrupts.iter_mut() {
+ sysbus_init_irq(sbd, irq);
+ }
+ const CLK_NAME: &CStr = unsafe { CStr::from_bytes_with_nul_unchecked(b"clk\0") };
+ self.clock = NonNull::new(qdev_init_clock_in(
+ dev,
+ CLK_NAME.as_ptr(),
+ None, /* pl011_clock_update */
+ addr_of_mut!(*self).cast::<c_void>(),
+ ClockEvent_ClockUpdate,
+ ))
+ .unwrap();
+ }
+ }
+
+ pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 {
+ use RegisterOffset::*;
+
+ match RegisterOffset::try_from(offset) {
+ Err(v) if (0x3f8..0x400).contains(&v) => {
+ u64::from(PL011_ID_ARM[((offset - 0xfe0) >> 2) as usize])
+ }
+ Err(_) => {
+ // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
+ 0
+ }
+ Ok(DR) => {
+ // s->flags &= ~PL011_FLAG_RXFF;
+ self.flags.set_receive_fifo_full(false);
+ let c = self.read_fifo[self.read_pos];
+ if self.read_count > 0 {
+ self.read_count -= 1;
+ self.read_pos = (self.read_pos + 1) & (self.fifo_depth() - 1);
+ }
+ if self.read_count == 0 {
+ // self.flags |= PL011_FLAG_RXFE;
+ self.flags.set_receive_fifo_empty(true);
+ }
+ if self.read_count + 1 == self.read_trigger {
+ //self.int_level &= ~ INT_RX;
+ self.int_level &= !registers::INT_RX;
+ }
+ // Update error bits.
+ self.receive_status_error_clear = c.to_be_bytes()[3].into();
+ self.update();
+ unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) };
+ c.into()
+ }
+ Ok(RSR) => u8::from(self.receive_status_error_clear).into(),
+ Ok(FR) => u16::from(self.flags).into(),
+ Ok(FBRD) => self.fbrd.into(),
+ Ok(ILPR) => self.ilpr.into(),
+ Ok(IBRD) => self.ibrd.into(),
+ Ok(LCR_H) => u16::from(self.line_control).into(),
+ Ok(CR) => {
+ // We exercise our self-control.
+ u16::from(self.control).into()
+ }
+ Ok(FLS) => self.ifl.into(),
+ Ok(IMSC) => self.int_enabled.into(),
+ Ok(RIS) => self.int_level.into(),
+ Ok(MIS) => u64::from(self.int_level & self.int_enabled),
+ Ok(ICR) => {
+ // "The UARTICR Register is the interrupt clear register and is write-only"
+ // Source: ARM DDI 0183G 3.3.13 Interrupt Clear Register, UARTICR
+ 0
+ }
+ Ok(DMACR) => self.dmacr.into(),
+ }
+ }
+
+ pub fn write(&mut self, offset: hwaddr, value: u64) {
+ // eprintln!("write offset {offset} value {value}");
+ use RegisterOffset::*;
+ let value: u32 = value as u32;
+ match RegisterOffset::try_from(offset) {
+ Err(_bad_offset) => {
+ eprintln!("write bad offset {offset} value {value}");
+ }
+ Ok(DR) => {
+ // ??? Check if transmitter is enabled.
+ let ch: u8 = value as u8;
+ // XXX this blocks entire thread. Rewrite to use
+ // qemu_chr_fe_write and background I/O callbacks
+ unsafe {
+ qemu_chr_fe_write_all(addr_of_mut!(self.char_backend), &ch, 1);
+ }
+ self.loopback_tx(value);
+ self.int_level |= registers::INT_TX;
+ self.update();
+ }
+ Ok(RSR) => {
+ self.receive_status_error_clear = 0.into();
+ }
+ Ok(FR) => {
+ // flag writes are ignored
+ }
+ Ok(ILPR) => {
+ self.ilpr = value;
+ }
+ Ok(IBRD) => {
+ self.ibrd = value;
+ }
+ Ok(FBRD) => {
+ self.fbrd = value;
+ }
+ Ok(LCR_H) => {
+ let value = value as u16;
+ let new_val: registers::LineControl = value.into();
+ // Reset the FIFO state on FIFO enable or disable
+ if bool::from(self.line_control.fifos_enabled())
+ ^ bool::from(new_val.fifos_enabled())
+ {
+ self.reset_fifo();
+ }
+ if self.line_control.send_break() ^ new_val.send_break() {
+ let mut break_enable: c_int = new_val.send_break().into();
+ unsafe {
+ qemu_chr_fe_ioctl(
+ addr_of_mut!(self.char_backend),
+ CHR_IOCTL_SERIAL_SET_BREAK as i32,
+ addr_of_mut!(break_enable).cast::<c_void>(),
+ );
+ }
+ self.loopback_break(break_enable > 0);
+ }
+ self.line_control = new_val;
+ self.set_read_trigger();
+ }
+ Ok(CR) => {
+ // ??? Need to implement the enable bit.
+ let value = value as u16;
+ self.control = value.into();
+ self.loopback_mdmctrl();
+ }
+ Ok(FLS) => {
+ self.ifl = value;
+ self.set_read_trigger();
+ }
+ Ok(IMSC) => {
+ self.int_enabled = value;
+ self.update();
+ }
+ Ok(RIS) => {}
+ Ok(MIS) => {}
+ Ok(ICR) => {
+ self.int_level &= !value;
+ self.update();
+ }
+ Ok(DMACR) => {
+ self.dmacr = value;
+ if value & 3 > 0 {
+ // qemu_log_mask(LOG_UNIMP, "pl011: DMA not implemented\n");
+ eprintln!("pl011: DMA not implemented");
+ }
+ }
+ }
+ }
+
+ #[inline]
+ fn loopback_tx(&mut self, value: u32) {
+ if !self.loopback_enabled() {
+ return;
+ }
+
+ // Caveat:
+ //
+ // In real hardware, TX loopback happens at the serial-bit level
+ // and then reassembled by the RX logics back into bytes and placed
+ // into the RX fifo. That is, loopback happens after TX fifo.
+ //
+ // Because the real hardware TX fifo is time-drained at the frame
+ // rate governed by the configured serial format, some loopback
+ // bytes in TX fifo may still be able to get into the RX fifo
+ // that could be full at times while being drained at software
+ // pace.
+ //
+ // In such scenario, the RX draining pace is the major factor
+ // deciding which loopback bytes get into the RX fifo, unless
+ // hardware flow-control is enabled.
+ //
+ // For simplicity, the above described is not emulated.
+ self.put_fifo(value);
+ }
+
+ fn loopback_mdmctrl(&mut self) {
+ if !self.loopback_enabled() {
+ return;
+ }
+
+ /*
+ * Loopback software-driven modem control outputs to modem status inputs:
+ * FR.RI <= CR.Out2
+ * FR.DCD <= CR.Out1
+ * FR.CTS <= CR.RTS
+ * FR.DSR <= CR.DTR
+ *
+ * The loopback happens immediately even if this call is triggered
+ * by setting only CR.LBE.
+ *
+ * CTS/RTS updates due to enabled hardware flow controls are not
+ * dealt with here.
+ */
+
+ //fr = s->flags & ~(PL011_FLAG_RI | PL011_FLAG_DCD |
+ // PL011_FLAG_DSR | PL011_FLAG_CTS);
+ //fr |= (cr & CR_OUT2) ? PL011_FLAG_RI : 0;
+ //fr |= (cr & CR_OUT1) ? PL011_FLAG_DCD : 0;
+ //fr |= (cr & CR_RTS) ? PL011_FLAG_CTS : 0;
+ //fr |= (cr & CR_DTR) ? PL011_FLAG_DSR : 0;
+ //
+ self.flags.set_ring_indicator(self.control.out_2());
+ self.flags.set_data_carrier_detect(self.control.out_1());
+ self.flags.set_clear_to_send(self.control.request_to_send());
+ self.flags
+ .set_data_set_ready(self.control.data_transmit_ready());
+
+ // Change interrupts based on updated FR
+ let mut il = self.int_level;
+
+ il &= !Interrupt::MS;
+ //il |= (fr & PL011_FLAG_DSR) ? INT_DSR : 0;
+ //il |= (fr & PL011_FLAG_DCD) ? INT_DCD : 0;
+ //il |= (fr & PL011_FLAG_CTS) ? INT_CTS : 0;
+ //il |= (fr & PL011_FLAG_RI) ? INT_RI : 0;
+
+ if self.flags.data_set_ready() {
+ il |= Interrupt::DSR as u32;
+ }
+ if self.flags.data_carrier_detect() {
+ il |= Interrupt::DCD as u32;
+ }
+ if self.flags.clear_to_send() {
+ il |= Interrupt::CTS as u32;
+ }
+ if self.flags.ring_indicator() {
+ il |= Interrupt::RI as u32;
+ }
+ self.int_level = il;
+ self.update();
+ }
+
+ fn loopback_break(&mut self, enable: bool) {
+ if enable {
+ self.loopback_tx(DATA_BREAK);
+ }
+ }
+
+ fn set_read_trigger(&mut self) {
+ //#if 0
+ // /* The docs say the RX interrupt is triggered when the FIFO exceeds
+ // the threshold. However linux only reads the FIFO in response to an
+ // interrupt. Triggering the interrupt when the FIFO is non-empty seems
+ // to make things work. */
+ // if (s->lcr & LCR_FEN)
+ // s->read_trigger = (s->ifl >> 1) & 0x1c;
+ // else
+ //#endif
+ self.read_trigger = 1;
+ }
+
+ pub fn realize(&mut self) {
+ unsafe {
+ qemu_chr_fe_set_handlers(
+ addr_of_mut!(self.char_backend),
+ Some(pl011_can_receive),
+ Some(pl011_receive),
+ Some(pl011_event),
+ None,
+ addr_of_mut!(*self).cast::<c_void>(),
+ core::ptr::null_mut(),
+ true,
+ );
+ }
+ }
+
+ pub fn reset(&mut self) {
+ self.line_control.reset();
+ self.receive_status_error_clear.reset();
+ self.dmacr = 0;
+ self.int_enabled = 0;
+ self.int_level = 0;
+ self.ilpr = 0;
+ self.ibrd = 0;
+ self.fbrd = 0;
+ self.read_trigger = 1;
+ self.ifl = 0x12;
+ self.control.reset();
+ self.flags = 0.into();
+ self.reset_fifo();
+ }
+
+ pub fn reset_fifo(&mut self) {
+ self.read_count = 0;
+ self.read_pos = 0;
+
+ /* Reset FIFO flags */
+ self.flags.reset();
+ }
+
+ pub fn can_receive(&self) -> bool {
+ // trace_pl011_can_receive(s->lcr, s->read_count, r);
+ self.read_count < self.fifo_depth()
+ }
+
+ pub fn event(&mut self, event: QEMUChrEvent) {
+ if event == generated::QEMUChrEvent_CHR_EVENT_BREAK && !self.fifo_enabled() {
+ self.put_fifo(DATA_BREAK);
+ self.receive_status_error_clear.set_break_error(true);
+ }
+ }
+
+ #[inline]
+ pub fn fifo_enabled(&self) -> bool {
+ matches!(self.line_control.fifos_enabled(), registers::Mode::FIFO)
+ }
+
+ #[inline]
+ pub fn loopback_enabled(&self) -> bool {
+ self.control.enable_loopback()
+ }
+
+ #[inline]
+ pub fn fifo_depth(&self) -> usize {
+ // Note: FIFO depth is expected to be power-of-2
+ if self.fifo_enabled() {
+ return PL011_FIFO_DEPTH;
+ }
+ 1
+ }
+
+ pub fn put_fifo(&mut self, value: c_uint) {
+ let depth = self.fifo_depth();
+ assert!(depth > 0);
+ let slot = (self.read_pos + self.read_count) & (depth - 1);
+ self.read_fifo[slot] = value;
+ self.read_count += 1;
+ // s->flags &= ~PL011_FLAG_RXFE;
+ self.flags.set_receive_fifo_empty(false);
+ if self.read_count == depth {
+ //s->flags |= PL011_FLAG_RXFF;
+ self.flags.set_receive_fifo_full(true);
+ }
+
+ if self.read_count == self.read_trigger {
+ self.int_level |= registers::INT_RX;
+ self.update();
+ }
+ }
+
+ pub fn update(&mut self) {
+ let flags = self.int_level & self.int_enabled;
+ for (irq, i) in self.interrupts.iter().zip(IRQMASK) {
+ unsafe { qemu_set_irq(*irq, i32::from(flags & i != 0)) };
+ }
+ }
+}
+
+/// Which bits in the interrupt status matter for each outbound IRQ line ?
+pub const IRQMASK: [u32; 6] = [
+ /* combined IRQ */
+ Interrupt::E
+ | Interrupt::MS
+ | Interrupt::RT as u32
+ | Interrupt::TX as u32
+ | Interrupt::RX as u32,
+ Interrupt::RX as u32,
+ Interrupt::TX as u32,
+ Interrupt::RT as u32,
+ Interrupt::MS,
+ Interrupt::E,
+];
+
+pub unsafe extern "C" fn pl011_can_receive(opaque: *mut c_void) -> c_int {
+ assert!(!opaque.is_null());
+ let state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+ state.as_ref().can_receive().into()
+}
+pub unsafe extern "C" fn pl011_receive(
+ opaque: *mut core::ffi::c_void,
+ buf: *const u8,
+ size: core::ffi::c_int,
+) {
+ assert!(!opaque.is_null());
+ let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+ if state.as_ref().loopback_enabled() {
+ return;
+ }
+ if size > 0 {
+ assert!(!buf.is_null());
+ state.as_mut().put_fifo(*buf.cast::<c_uint>())
+ }
+}
+
+pub unsafe extern "C" fn pl011_event(opaque: *mut core::ffi::c_void, event: QEMUChrEvent) {
+ assert!(!opaque.is_null());
+ let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+ state.as_mut().event(event)
+}
+
+pub const VMSTATE_PL011: VMStateDescription = VMStateDescription {
+ name: PL011_ARM_INFO.name,
+ unmigratable: true,
+ ..unsafe { MaybeUninit::<VMStateDescription>::zeroed().assume_init() }
+};
+//version_id : 2,
+//minimum_version_id : 2,
+//post_load : pl011_post_load,
+//fields = (const VMStateField[]) {
+// VMSTATE_UINT32(readbuff, PL011State),
+// VMSTATE_UINT32(flags, PL011State),
+// VMSTATE_UINT32(lcr, PL011State),
+// VMSTATE_UINT32(rsr, PL011State),
+// VMSTATE_UINT32(cr, PL011State),
+// VMSTATE_UINT32(dmacr, PL011State),
+// VMSTATE_UINT32(int_enabled, PL011State),
+// VMSTATE_UINT32(int_level, PL011State),
+// VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH),
+// VMSTATE_UINT32(ilpr, PL011State),
+// VMSTATE_UINT32(ibrd, PL011State),
+// VMSTATE_UINT32(fbrd, PL011State),
+// VMSTATE_UINT32(ifl, PL011State),
+// VMSTATE_INT32(read_pos, PL011State),
+// VMSTATE_INT32(read_count, PL011State),
+// VMSTATE_INT32(read_trigger, PL011State),
+// VMSTATE_END_OF_LIST()
+//},
+//.subsections = (const VMStateDescription * const []) {
+// &vmstate_pl011_clock,
+// NULL
+//}
+
+pub unsafe extern "C" fn pl011_create(
+ addr: u64,
+ irq: qemu_irq,
+ chr: *mut Chardev,
+) -> *mut DeviceState {
+ let dev: *mut DeviceState = unsafe { qdev_new(PL011_ARM_INFO.name) };
+ assert!(!dev.is_null());
+ let sysbus: *mut SysBusDevice = dev as *mut SysBusDevice;
+
+ unsafe {
+ qdev_prop_set_chr(dev, generated::TYPE_CHARDEV.as_ptr(), chr);
+ sysbus_realize_and_unref(sysbus, addr_of!(error_fatal) as *mut *mut Error);
+ sysbus_mmio_map(sysbus, 0, addr);
+ sysbus_connect_irq(sysbus, 0, irq);
+ }
+ dev
+}
diff --git a/rust/pl011/src/device_class.rs b/rust/pl011/src/device_class.rs
new file mode 100644
index 0000000000..50d91eb527
--- /dev/null
+++ b/rust/pl011/src/device_class.rs
@@ -0,0 +1,95 @@
+use core::{mem::MaybeUninit, ptr::NonNull};
+use std::sync::OnceLock;
+
+use crate::{
+ device::{PL011State, VMSTATE_PL011},
+ generated::*,
+};
+
+#[no_mangle]
+pub unsafe extern "C" fn pl011_class_init(klass: *mut ObjectClass, _: *mut core::ffi::c_void) {
+ let mut dc = NonNull::new(klass.cast::<DeviceClass>()).unwrap();
+ dc.as_mut().realize = Some(pl011_realize);
+ dc.as_mut().reset = Some(pl011_reset);
+ dc.as_mut().vmsd = &VMSTATE_PL011;
+ _ = PL011_PROPERTIES.get_or_init(make_pl011_properties);
+ device_class_set_props(
+ dc.as_mut(),
+ PL011_PROPERTIES.get_mut().unwrap().as_mut_ptr(),
+ );
+}
+
+#[macro_export]
+macro_rules! define_property {
+ ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr) => {
+ $crate::generated::Property {
+ name: $name,
+ info: $prop,
+ offset: ::core::mem::offset_of!($state, $field) as _,
+ bitnr: 0,
+ bitmask: 0,
+ set_default: true,
+ defval: $crate::generated::Property__bindgen_ty_1 { u: $defval.into() },
+ arrayoffset: 0,
+ arrayinfo: ::core::ptr::null(),
+ arrayfieldsize: 0,
+ link_type: ::core::ptr::null(),
+ }
+ };
+ ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr) => {
+ $crate::generated::Property {
+ name: $name,
+ info: $prop,
+ offset: ::core::mem::offset_of!($state, $field) as _,
+ bitnr: 0,
+ bitmask: 0,
+ set_default: false,
+ defval: $crate::generated::Property__bindgen_ty_1 { i: 0 },
+ arrayoffset: 0,
+ arrayinfo: ::core::ptr::null(),
+ arrayfieldsize: 0,
+ link_type: ::core::ptr::null(),
+ }
+ };
+}
+
+#[no_mangle]
+pub static mut PL011_PROPERTIES: OnceLock<[Property; 3]> = OnceLock::new();
+
+unsafe impl Send for Property {}
+unsafe impl Sync for Property {}
+
+#[no_mangle]
+fn make_pl011_properties() -> [Property; 3] {
+ [
+ define_property!(
+ c"chardev".as_ptr(),
+ PL011State,
+ char_backend,
+ unsafe { &qdev_prop_chr },
+ CharBackend
+ ),
+ define_property!(
+ c"migrate-clk".as_ptr(),
+ PL011State,
+ migrate_clock,
+ unsafe { &qdev_prop_bool },
+ bool
+ ),
+ unsafe { MaybeUninit::<Property>::zeroed().assume_init() },
+ ]
+}
+
+#[no_mangle]
+pub unsafe extern "C" fn pl011_realize(dev: *mut DeviceState, _errp: *mut *mut Error) {
+ assert!(!dev.is_null());
+ let mut state = NonNull::new_unchecked(dev.cast::<PL011State>());
+ state.as_mut().realize();
+}
+
+#[no_mangle]
+pub unsafe extern "C" fn pl011_reset(dev: *mut DeviceState) {
+ assert!(!dev.is_null());
+ let mut state = NonNull::new_unchecked(dev.cast::<PL011State>());
+ state.as_mut().reset();
+}
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");
diff --git a/rust/pl011/src/lib.rs b/rust/pl011/src/lib.rs
new file mode 100644
index 0000000000..bb2899dbc2
--- /dev/null
+++ b/rust/pl011/src/lib.rs
@@ -0,0 +1,581 @@
+// PL011 QEMU Device Model
+//
+// This library implements a device model for the PrimeCell® UART (PL011)
+// device in QEMU.
+//
+#![doc = include_str!("../README.md")]
+//! # Library crate
+//!
+//! See [`PL011State`](crate::device::PL011State) for the device model type and
+//! the [`registers`] module for register types.
+
+// FIXME: remove improper_ctypes
+#[allow(
+ improper_ctypes_definitions,
+ improper_ctypes,
+ non_camel_case_types,
+ non_snake_case,
+ non_upper_case_globals
+)]
+#[allow(
+ clippy::missing_const_for_fn,
+ clippy::useless_transmute,
+ clippy::too_many_arguments,
+ clippy::approx_constant,
+ clippy::use_self,
+ clippy::cast_lossless,
+)]
+#[rustfmt::skip]
+pub mod generated;
+
+pub mod definitions;
+pub mod device;
+pub mod device_class;
+pub mod memory_ops;
+
+/// Offset of each register from the base memory address of the device.
+///
+/// # Source
+/// ARM DDI 0183G, Table 3-1 p.3-3
+#[doc(alias = "offset")]
+#[allow(non_camel_case_types)]
+#[repr(u64)]
+#[derive(Debug)]
+pub enum RegisterOffset {
+ /// Data Register
+ ///
+ /// A write to this register initiates the actual data transmission
+ #[doc(alias = "UARTDR")]
+ DR = 0x000,
+ /// Receive Status Register or Error Clear Register
+ #[doc(alias = "UARTRSR")]
+ #[doc(alias = "UARTECR")]
+ RSR = 0x004,
+ /// Flag Register
+ ///
+ /// A read of this register shows if transmission is complete
+ #[doc(alias = "UARTFR")]
+ FR = 0x018,
+ /// Fractional Baud Rate Register
+ ///
+ /// responsible for baud rate speed
+ #[doc(alias = "UARTFBRD")]
+ FBRD = 0x028,
+ /// `IrDA` Low-Power Counter Register
+ #[doc(alias = "UARTILPR")]
+ ILPR = 0x020,
+ /// Integer Baud Rate Register
+ ///
+ /// Responsible for baud rate speed
+ #[doc(alias = "UARTIBRD")]
+ IBRD = 0x024,
+ /// line control register (data frame format)
+ #[doc(alias = "UARTLCR_H")]
+ LCR_H = 0x02C,
+ /// Toggle UART, transmission or reception
+ #[doc(alias = "UARTCR")]
+ CR = 0x030,
+ /// Interrupt FIFO Level Select Register
+ #[doc(alias = "UARTIFLS")]
+ FLS = 0x034,
+ /// Interrupt Mask Set/Clear Register
+ #[doc(alias = "UARTIMSC")]
+ IMSC = 0x038,
+ /// Raw Interrupt Status Register
+ #[doc(alias = "UARTRIS")]
+ RIS = 0x03C,
+ /// Masked Interrupt Status Register
+ #[doc(alias = "UARTMIS")]
+ MIS = 0x040,
+ /// Interrupt Clear Register
+ #[doc(alias = "UARTICR")]
+ ICR = 0x044,
+ /// DMA control Register
+ #[doc(alias = "UARTDMACR")]
+ DMACR = 0x048,
+ ///// Reserved, offsets `0x04C` to `0x07C`.
+ //Reserved = 0x04C,
+}
+
+impl core::convert::TryFrom<u64> for RegisterOffset {
+ type Error = u64;
+
+ fn try_from(value: u64) -> Result<Self, Self::Error> {
+ macro_rules! case {
+ ($($discriminant:ident),*$(,)*) => {
+ /* check that matching on all macro arguments compiles, which means we are not
+ * missing any enum value; if the type definition ever changes this will stop
+ * compiling.
+ */
+ const fn _assert_exhaustive(val: RegisterOffset) {
+ match val {
+ $(RegisterOffset::$discriminant => (),)*
+ }
+ }
+
+ match value {
+ $(x if x == Self::$discriminant as u64 => Ok(Self::$discriminant),)*
+ _ => Err(value),
+ }
+ }
+ }
+ case! { DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, ICR, DMACR }
+ }
+}
+
+pub mod registers {
+ //! Device registers exposed as typed structs which are backed by arbitrary
+ //! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
+ //!
+ //! All PL011 registers are essentially 32-bit wide, but are typed here as
+ //! bitmaps with only the necessary width. That is, if a struct bitmap
+ //! in this module is for example 16 bits long, it should be conceived
+ //! as a 32-bit register where the unmentioned higher bits are always
+ //! unused thus treated as zero when read or written.
+ use bilge::prelude::*;
+
+ // TODO: FIFO Mode has different semantics
+ /// Data Register, `UARTDR`
+ ///
+ /// The `UARTDR` register is the data register.
+ ///
+ /// For words to be transmitted:
+ ///
+ /// - if the FIFOs are enabled, data written to this location is pushed onto
+ /// the transmit
+ /// FIFO
+ /// - if the FIFOs are not enabled, data is stored in the transmitter
+ /// holding register (the
+ /// bottom word of the transmit FIFO).
+ ///
+ /// The write operation initiates transmission from the UART. The data is
+ /// prefixed with a start bit, appended with the appropriate parity bit
+ /// (if parity is enabled), and a stop bit. The resultant word is then
+ /// transmitted.
+ ///
+ /// For received words:
+ ///
+ /// - if the FIFOs are enabled, the data byte and the 4-bit status (break,
+ /// frame, parity,
+ /// and overrun) is pushed onto the 12-bit wide receive FIFO
+ /// - if the FIFOs are not enabled, the data byte and status are stored in
+ /// the receiving
+ /// holding register (the bottom word of the receive FIFO).
+ ///
+ /// The received data byte is read by performing reads from the `UARTDR`
+ /// register along with the corresponding status information. The status
+ /// information can also be read by a read of the `UARTRSR/UARTECR`
+ /// register.
+ ///
+ /// # Note
+ ///
+ /// You must disable the UART before any of the control registers are
+ /// reprogrammed. When the UART is disabled in the middle of
+ /// transmission or reception, it completes the current character before
+ /// stopping.
+ ///
+ /// # Source
+ /// ARM DDI 0183G 3.3.1 Data Register, UARTDR
+ #[bitsize(16)]
+ #[derive(Clone, Copy, DebugBits, FromBits)]
+ #[doc(alias = "UARTDR")]
+ pub struct Data {
+ _reserved: u4,
+ pub data: u8,
+ pub framing_error: bool,
+ pub parity_error: bool,
+ pub break_error: bool,
+ pub overrun_error: bool,
+ }
+
+ // TODO: FIFO Mode has different semantics
+ /// Receive Status Register / Error Clear Register, `UARTRSR/UARTECR`
+ ///
+ /// The UARTRSR/UARTECR register is the receive status register/error clear
+ /// register. Receive status can also be read from the `UARTRSR`
+ /// register. If the status is read from this register, then the status
+ /// information for break, framing and parity corresponds to the
+ /// data character read from the [Data register](Data), `UARTDR` prior to
+ /// reading the UARTRSR register. The status information for overrun is
+ /// set immediately when an overrun condition occurs.
+ ///
+ ///
+ /// # Note
+ /// The received data character must be read first from the [Data
+ /// Register](Data), `UARTDR` before reading the error status associated
+ /// with that data character from the `UARTRSR` register. This read
+ /// sequence cannot be reversed, because the `UARTRSR` register is
+ /// updated only when a read occurs from the `UARTDR` register. However,
+ /// the status information can also be obtained by reading the `UARTDR`
+ /// register
+ ///
+ /// # Source
+ /// ARM DDI 0183G 3.3.2 Receive Status Register/Error Clear Register,
+ /// UARTRSR/UARTECR
+ #[bitsize(8)]
+ #[derive(Clone, Copy, DebugBits, FromBits)]
+ pub struct ReceiveStatusErrorClear {
+ pub framing_error: bool,
+ pub parity_error: bool,
+ pub break_error: bool,
+ pub overrun_error: bool,
+ _reserved_unpredictable: u4,
+ }
+
+ impl ReceiveStatusErrorClear {
+ pub fn reset(&mut self) {
+ // All the bits are cleared to 0 on reset.
+ *self = 0.into();
+ }
+ }
+
+ impl Default for ReceiveStatusErrorClear {
+ fn default() -> Self {
+ 0.into()
+ }
+ }
+
+ #[bitsize(16)]
+ #[derive(Clone, Copy, DebugBits, FromBits)]
+ /// Flag Register, `UARTFR`
+ #[doc(alias = "UARTFR")]
+ pub struct Flags {
+ /// CTS Clear to send. This bit is the complement of the UART clear to
+ /// send, `nUARTCTS`, modem status input. That is, the bit is 1
+ /// when `nUARTCTS` is LOW.
+ pub clear_to_send: bool,
+ /// DSR Data set ready. This bit is the complement of the UART data set
+ /// ready, `nUARTDSR`, modem status input. That is, the bit is 1 when
+ /// `nUARTDSR` is LOW.
+ pub data_set_ready: bool,
+ /// DCD Data carrier detect. This bit is the complement of the UART data
+ /// carrier detect, `nUARTDCD`, modem status input. That is, the bit is
+ /// 1 when `nUARTDCD` is LOW.
+ pub data_carrier_detect: bool,
+ /// BUSY UART busy. If this bit is set to 1, the UART is busy
+ /// transmitting data. This bit remains set until the complete
+ /// byte, including all the stop bits, has been sent from the
+ /// shift register. This bit is set as soon as the transmit FIFO
+ /// becomes non-empty, regardless of whether the UART is enabled
+ /// or not.
+ pub busy: bool,
+ /// RXFE Receive FIFO empty. The meaning of this bit depends on the
+ /// state of the FEN bit in the UARTLCR_H register. If the FIFO
+ /// is disabled, this bit is set when the receive holding
+ /// register is empty. If the FIFO is enabled, the RXFE bit is
+ /// set when the receive FIFO is empty.
+ pub receive_fifo_empty: bool,
+ /// TXFF Transmit FIFO full. The meaning of this bit depends on the
+ /// state of the FEN bit in the UARTLCR_H register. If the FIFO
+ /// is disabled, this bit is set when the transmit holding
+ /// register is full. If the FIFO is enabled, the TXFF bit is
+ /// set when the transmit FIFO is full.
+ pub transmit_fifo_full: bool,
+ /// RXFF Receive FIFO full. The meaning of this bit depends on the state
+ /// of the FEN bit in the UARTLCR_H register. If the FIFO is
+ /// disabled, this bit is set when the receive holding register
+ /// is full. If the FIFO is enabled, the RXFF bit is set when
+ /// the receive FIFO is full.
+ pub receive_fifo_full: bool,
+ /// Transmit FIFO empty. The meaning of this bit depends on the state of
+ /// the FEN bit in the [Line Control register](LineControl),
+ /// `UARTLCR_H`. If the FIFO is disabled, this bit is set when the
+ /// transmit holding register is empty. If the FIFO is enabled,
+ /// the TXFE bit is set when the transmit FIFO is empty. This
+ /// bit does not indicate if there is data in the transmit shift
+ /// register.
+ pub transmit_fifo_empty: bool,
+ /// `RI`, is `true` when `nUARTRI` is `LOW`.
+ pub ring_indicator: bool,
+ _reserved_zero_no_modify: u7,
+ }
+
+ impl Flags {
+ pub fn reset(&mut self) {
+ // After reset TXFF, RXFF, and BUSY are 0, and TXFE and RXFE are 1
+ self.set_receive_fifo_full(false);
+ self.set_transmit_fifo_full(false);
+ self.set_busy(false);
+ self.set_receive_fifo_empty(true);
+ self.set_transmit_fifo_empty(true);
+ }
+ }
+
+ impl Default for Flags {
+ fn default() -> Self {
+ let mut ret: Self = 0.into();
+ ret.reset();
+ ret
+ }
+ }
+
+ #[bitsize(16)]
+ #[derive(Clone, Copy, DebugBits, FromBits)]
+ /// Line Control Register, `UARTLCR_H`
+ #[doc(alias = "UARTLCR_H")]
+ pub struct LineControl {
+ /// 15:8 - Reserved, do not modify, read as zero.
+ _reserved_zero_no_modify: u8,
+ /// 7 SPS Stick parity select.
+ /// 0 = stick parity is disabled
+ /// 1 = either:
+ /// • if the EPS bit is 0 then the parity bit is transmitted and checked
+ /// as a 1 • if the EPS bit is 1 then the parity bit is
+ /// transmitted and checked as a 0. This bit has no effect when
+ /// the PEN bit disables parity checking and generation. See Table 3-11
+ /// on page 3-14 for the parity truth table.
+ pub sticky_parity: bool,
+ /// WLEN Word length. These bits indicate the number of data bits
+ /// transmitted or received in a frame as follows: b11 = 8 bits
+ /// b10 = 7 bits
+ /// b01 = 6 bits
+ /// b00 = 5 bits.
+ pub word_length: WordLength,
+ /// FEN Enable FIFOs:
+ /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
+ /// 1-byte-deep holding registers 1 = transmit and receive FIFO
+ /// buffers are enabled (FIFO mode).
+ pub fifos_enabled: Mode,
+ /// 3 STP2 Two stop bits select. If this bit is set to 1, two stop bits
+ /// are transmitted at the end of the frame. The receive
+ /// logic does not check for two stop bits being received.
+ pub two_stops_bits: bool,
+ /// EPS Even parity select. Controls the type of parity the UART uses
+ /// during transmission and reception:
+ /// - 0 = odd parity. The UART generates or checks for an odd number of
+ /// 1s in the data and parity bits.
+ /// - 1 = even parity. The UART generates or checks for an even number
+ /// of 1s in the data and parity bits.
+ /// This bit has no effect when the `PEN` bit disables parity checking
+ /// and generation. See Table 3-11 on page 3-14 for the parity
+ /// truth table.
+ pub parity: Parity,
+ /// 1 PEN Parity enable:
+ ///
+ /// - 0 = parity is disabled and no parity bit added to the data frame
+ /// - 1 = parity checking and generation is enabled.
+ ///
+ /// See Table 3-11 on page 3-14 for the parity truth table.
+ pub parity_enabled: bool,
+ /// BRK Send break.
+ ///
+ /// If this bit is set to `1`, a low-level is continually output on the
+ /// `UARTTXD` output, after completing transmission of the
+ /// current character. For the proper execution of the break command,
+ /// the software must set this bit for at least two complete
+ /// frames. For normal use, this bit must be cleared to `0`.
+ pub send_break: bool,
+ }
+
+ impl LineControl {
+ pub fn reset(&mut self) {
+ // All the bits are cleared to 0 when reset.
+ *self = 0.into();
+ }
+ }
+
+ impl Default for LineControl {
+ fn default() -> Self {
+ 0.into()
+ }
+ }
+
+ #[bitsize(1)]
+ #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+ /// `EPS` "Even parity select", field of [Line Control
+ /// register](LineControl).
+ pub enum Parity {
+ /// - 0 = odd parity. The UART generates or checks for an odd number of
+ /// 1s in the data and parity bits.
+ Odd = 0,
+ /// - 1 = even parity. The UART generates or checks for an even number
+ /// of 1s in the data and parity bits.
+ Even = 1,
+ }
+
+ #[bitsize(1)]
+ #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+ /// `FEN` "Enable FIFOs" or Device mode, field of [Line Control
+ /// register](LineControl).
+ pub enum Mode {
+ /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
+ /// 1-byte-deep holding registers
+ Character = 0,
+ /// 1 = transmit and receive FIFO buffers are enabled (FIFO mode).
+ FIFO = 1,
+ }
+
+ impl From<Mode> for bool {
+ fn from(val: Mode) -> Self {
+ matches!(val, Mode::FIFO)
+ }
+ }
+
+ #[bitsize(2)]
+ #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+ /// `WLEN` Word length, field of [Line Control register](LineControl).
+ ///
+ /// These bits indicate the number of data bits transmitted or received in a
+ /// frame as follows:
+ pub enum WordLength {
+ /// b11 = 8 bits
+ _8Bits = 0b11,
+ /// b10 = 7 bits
+ _7Bits = 0b10,
+ /// b01 = 6 bits
+ _6Bits = 0b01,
+ /// b00 = 5 bits.
+ _5Bits = 0b00,
+ }
+
+ /// Control Register, `UARTCR`
+ ///
+ /// The `UARTCR` register is the control register. All the bits are cleared
+ /// to `0` on reset except for bits `9` and `8` that are set to `1`.
+ ///
+ /// # Source
+ /// ARM DDI 0183G, 3.3.8 Control Register, `UARTCR`, Table 3-12
+ #[bitsize(16)]
+ #[doc(alias = "UARTCR")]
+ #[derive(Clone, Copy, DebugBits, FromBits)]
+ pub struct Control {
+ /// `UARTEN` UART enable: 0 = UART is disabled. If the UART is disabled
+ /// in the middle of transmission or reception, it completes the current
+ /// character before stopping. 1 = the UART is enabled. Data
+ /// transmission and reception occurs for either UART signals or SIR
+ /// signals depending on the setting of the SIREN bit.
+ pub enable_uart: bool,
+ /// `SIREN` `SIR` enable: 0 = IrDA SIR ENDEC is disabled. `nSIROUT`
+ /// remains LOW (no light pulse generated), and signal transitions on
+ /// SIRIN have no effect. 1 = IrDA SIR ENDEC is enabled. Data is
+ /// transmitted and received on nSIROUT and SIRIN. UARTTXD remains HIGH,
+ /// in the marking state. Signal transitions on UARTRXD or modem status
+ /// inputs have no effect. This bit has no effect if the UARTEN bit
+ /// disables the UART.
+ pub enable_sir: bool,
+ /// `SIRLP` SIR low-power IrDA mode. This bit selects the IrDA encoding
+ /// mode. If this bit is cleared to 0, low-level bits are transmitted as
+ /// an active high pulse with a width of 3/ 16th of the bit period. If
+ /// this bit is set to 1, low-level bits are transmitted with a pulse
+ /// width that is 3 times the period of the IrLPBaud16 input signal,
+ /// regardless of the selected bit rate. Setting this bit uses less
+ /// power, but might reduce transmission distances.
+ pub sir_lowpower_irda_mode: u1,
+ /// Reserved, do not modify, read as zero.
+ _reserved_zero_no_modify: u4,
+ /// `LBE` Loopback enable. If this bit is set to 1 and the SIREN bit is
+ /// set to 1 and the SIRTEST bit in the Test Control register, UARTTCR
+ /// on page 4-5 is set to 1, then the nSIROUT path is inverted, and fed
+ /// through to the SIRIN path. The SIRTEST bit in the test register must
+ /// be set to 1 to override the normal half-duplex SIR operation. This
+ /// must be the requirement for accessing the test registers during
+ /// normal operation, and SIRTEST must be cleared to 0 when loopback
+ /// testing is finished. This feature reduces the amount of external
+ /// coupling required during system test. If this bit is set to 1, and
+ /// the SIRTEST bit is set to 0, the UARTTXD path is fed through to the
+ /// UARTRXD path. In either SIR mode or UART mode, when this bit is set,
+ /// the modem outputs are also fed through to the modem inputs. This bit
+ /// is cleared to 0 on reset, to disable loopback.
+ pub enable_loopback: bool,
+ /// `TXE` Transmit enable. If this bit is set to 1, the transmit section
+ /// of the UART is enabled. Data transmission occurs for either UART
+ /// signals, or SIR signals depending on the setting of the SIREN bit.
+ /// When the UART is disabled in the middle of transmission, it
+ /// completes the current character before stopping.
+ pub enable_transmit: bool,
+ /// `RXE` Receive enable. If this bit is set to 1, the receive section
+ /// of the UART is enabled. Data reception occurs for either UART
+ /// signals or SIR signals depending on the setting of the SIREN bit.
+ /// When the UART is disabled in the middle of reception, it completes
+ /// the current character before stopping.
+ pub enable_receive: bool,
+ /// `DTR` Data transmit ready. This bit is the complement of the UART
+ /// data transmit ready, `nUARTDTR`, modem status output. That is, when
+ /// the bit is programmed to a 1 then `nUARTDTR` is LOW.
+ pub data_transmit_ready: bool,
+ /// `RTS` Request to send. This bit is the complement of the UART
+ /// request to send, `nUARTRTS`, modem status output. That is, when the
+ /// bit is programmed to a 1 then `nUARTRTS` is LOW.
+ pub request_to_send: bool,
+ /// `Out1` This bit is the complement of the UART Out1 (`nUARTOut1`)
+ /// modem status output. That is, when the bit is programmed to a 1 the
+ /// output is 0. For DTE this can be used as Data Carrier Detect (DCD).
+ pub out_1: bool,
+ /// `Out2` This bit is the complement of the UART Out2 (`nUARTOut2`)
+ /// modem status output. That is, when the bit is programmed to a 1, the
+ /// output is 0. For DTE this can be used as Ring Indicator (RI).
+ pub out_2: bool,
+ /// `RTSEn` RTS hardware flow control enable. If this bit is set to 1,
+ /// RTS hardware flow control is enabled. Data is only requested when
+ /// there is space in the receive FIFO for it to be received.
+ pub rts_hardware_flow_control_enable: bool,
+ /// `CTSEn` CTS hardware flow control enable. If this bit is set to 1,
+ /// CTS hardware flow control is enabled. Data is only transmitted when
+ /// the `nUARTCTS` signal is asserted.
+ pub cts_hardware_flow_control_enable: bool,
+ }
+
+ impl Control {
+ pub fn reset(&mut self) {
+ *self = 0.into();
+ self.set_enable_receive(true);
+ self.set_enable_transmit(true);
+ }
+ }
+
+ impl Default for Control {
+ fn default() -> Self {
+ let mut ret: Self = 0.into();
+ ret.reset();
+ ret
+ }
+ }
+
+ /// Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC
+ pub const INT_OE: u32 = 1 << 10;
+ pub const INT_BE: u32 = 1 << 9;
+ pub const INT_PE: u32 = 1 << 8;
+ pub const INT_FE: u32 = 1 << 7;
+ pub const INT_RT: u32 = 1 << 6;
+ pub const INT_TX: u32 = 1 << 5;
+ pub const INT_RX: u32 = 1 << 4;
+ pub const INT_DSR: u32 = 1 << 3;
+ pub const INT_DCD: u32 = 1 << 2;
+ pub const INT_CTS: u32 = 1 << 1;
+ pub const INT_RI: u32 = 1 << 0;
+ pub const INT_E: u32 = INT_OE | INT_BE | INT_PE | INT_FE;
+ pub const INT_MS: u32 = INT_RI | INT_DSR | INT_DCD | INT_CTS;
+
+ #[repr(u32)]
+ pub enum Interrupt {
+ OE = 1 << 10,
+ BE = 1 << 9,
+ PE = 1 << 8,
+ FE = 1 << 7,
+ RT = 1 << 6,
+ TX = 1 << 5,
+ RX = 1 << 4,
+ DSR = 1 << 3,
+ DCD = 1 << 2,
+ CTS = 1 << 1,
+ RI = 1 << 0,
+ }
+
+ impl Interrupt {
+ pub const E: u32 = INT_OE | INT_BE | INT_PE | INT_FE;
+ pub const MS: u32 = INT_RI | INT_DSR | INT_DCD | INT_CTS;
+ }
+}
+
+// State
+// TODO You must disable the UART before any of the control registers are
+// reprogrammed. When the UART is disabled in the middle of transmission or
+// reception, it completes the current character before stopping
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn it_works() {}
+}
diff --git a/rust/pl011/src/memory_ops.rs b/rust/pl011/src/memory_ops.rs
new file mode 100644
index 0000000000..4c7d4577c7
--- /dev/null
+++ b/rust/pl011/src/memory_ops.rs
@@ -0,0 +1,38 @@
+use core::{mem::MaybeUninit, ptr::NonNull};
+
+use crate::{device::PL011State, generated::*};
+
+pub const PL011_OPS: MemoryRegionOps = MemoryRegionOps {
+ read: Some(pl011_read),
+ write: Some(pl011_write),
+ read_with_attrs: None,
+ write_with_attrs: None,
+ endianness: device_endian_DEVICE_NATIVE_ENDIAN,
+ valid: unsafe { MaybeUninit::<MemoryRegionOps__bindgen_ty_1>::zeroed().assume_init() },
+ impl_: MemoryRegionOps__bindgen_ty_2 {
+ min_access_size: 4,
+ max_access_size: 4,
+ ..unsafe { MaybeUninit::<MemoryRegionOps__bindgen_ty_2>::zeroed().assume_init() }
+ },
+};
+
+unsafe extern "C" fn pl011_read(
+ opaque: *mut core::ffi::c_void,
+ addr: hwaddr,
+ size: core::ffi::c_uint,
+) -> u64 {
+ assert!(!opaque.is_null());
+ let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+ state.as_mut().read(addr, size)
+}
+
+unsafe extern "C" fn pl011_write(
+ opaque: *mut core::ffi::c_void,
+ addr: hwaddr,
+ data: u64,
+ _size: core::ffi::c_uint,
+) {
+ assert!(!opaque.is_null());
+ let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+ state.as_mut().write(addr, data)
+}
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
--
γαῖα πυρί μιχθήτω
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [RFC PATCH v2 4/5] DO NOT MERGE: add rustdoc build for gitlab pages
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
` (2 preceding siblings ...)
2024-06-11 10:33 ` [RFC PATCH v2 3/5] rust: add PL011 device model Manos Pitsidianakis
@ 2024-06-11 10:33 ` Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 5/5] DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine Manos Pitsidianakis
` (2 subsequent siblings)
6 siblings, 0 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-11 10:33 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier,
Wainer dos Santos Moschetta, Beraldo Leal
Deploy the generated rustdocs for my personal rust qemu fork on gitlab.
The URL is:
https://rust-for-qemu-epilys-aebb06ca9f9adfe6584811c14ae44156501d935ba4.gitlab.io/pl011/index.html
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
.gitlab-ci.d/buildtest.yml | 64 +++++++++++++++++++++++++++-----------
1 file changed, 45 insertions(+), 19 deletions(-)
diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 91c57efded..380c24897d 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -715,31 +715,57 @@ build-tools-and-docs-debian:
# For contributor forks we want to publish from any repo so
# that users can see the results of their commits, regardless
# of what topic branch they're currently using
+# pages:
+# extends: .base_job_template
+# image: $CI_REGISTRY_IMAGE/qemu/debian:$QEMU_CI_CONTAINER_TAG
+# stage: test
+# needs:
+# - job: build-tools-and-docs-debian
+# script:
+# - mkdir -p public
+# # HTML-ised source tree
+# - make gtags
+# # We unset variables to work around a bug in some htags versions
+# # which causes it to fail when the environment is large
+# - CI_COMMIT_MESSAGE= CI_COMMIT_TAG_MESSAGE= htags
+# -anT --tree-view=filetree -m qemu_init
+# -t "Welcome to the QEMU sourcecode"
+# - mv HTML public/src
+# # Project documentation
+# - make -C build install DESTDIR=$(pwd)/temp-install
+# - mv temp-install/usr/local/share/doc/qemu/* public/
+# artifacts:
+# when: on_success
+# paths:
+# - public
+# variables:
+# QEMU_JOB_PUBLISH: 1
+# The Docker image that will be used to build your app
pages:
- extends: .base_job_template
- image: $CI_REGISTRY_IMAGE/qemu/debian:$QEMU_CI_CONTAINER_TAG
- stage: test
- needs:
- - job: build-tools-and-docs-debian
+ image: rust:latest
script:
- - mkdir -p public
- # HTML-ised source tree
- - make gtags
- # We unset variables to work around a bug in some htags versions
- # which causes it to fail when the environment is large
- - CI_COMMIT_MESSAGE= CI_COMMIT_TAG_MESSAGE= htags
- -anT --tree-view=filetree -m qemu_init
- -t "Welcome to the QEMU sourcecode"
- - mv HTML public/src
- # Project documentation
- - make -C build install DESTDIR=$(pwd)/temp-install
- - mv temp-install/usr/local/share/doc/qemu/* public/
+ - rustup component add rustfmt
+ - DEBIAN_FRONTEND=noninteractive apt-get update -y
+ - DEBIAN_FRONTEND=noninteractive apt-get install -y python3-venv meson libgcrypt20-dev zlib1g-dev autoconf automake libtool bison flex git libglib2.0-dev libfdt-dev libpixman-1-dev ninja-build make libclang-14-dev
+ - cargo install bindgen-cli
+ - mkdir ./build/
+ - cd ./build/
+ - ../configure --enable-system --disable-kvm --target-list=aarch64-softmmu --enable-with-rust
+ - ninja "aarch64-softmmu-generated.rs"
+ - cp ./aarch64-softmmu-generated.rs ../rust/pl011/src/generated.rs.inc
+ - cd ../rust/pl011/
+ - cargo tree --depth 1 -e normal --prefix none | cut -d' ' -f1 | xargs
+ printf -- '-p %s\n' | xargs cargo doc --no-deps --document-private-items --target x86_64-unknown-linux-gnu
+ - cd ./../..
+ - mv ./rust/pl011/target/x86_64-unknown-linux-gnu/doc ./public
artifacts:
when: on_success
paths:
- public
- variables:
- QEMU_JOB_PUBLISH: 1
+ rules:
+ # This ensures that only pushes to the default branch will trigger
+ # a pages deploy
+ - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH
coverity:
image: $CI_REGISTRY_IMAGE/qemu/fedora:$QEMU_CI_CONTAINER_TAG
--
γαῖα πυρί μιχθήτω
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [RFC PATCH v2 5/5] DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
` (3 preceding siblings ...)
2024-06-11 10:33 ` [RFC PATCH v2 4/5] DO NOT MERGE: add rustdoc build for gitlab pages Manos Pitsidianakis
@ 2024-06-11 10:33 ` Manos Pitsidianakis
2024-06-12 8:37 ` [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Daniel P. Berrangé
2024-06-19 3:31 ` Richard Henderson
6 siblings, 0 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-11 10:33 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier, qemu-arm
Convenience patch for testing the rust device.
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
hw/arm/virt.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3c93c0c0a6..f33b58ae0d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -912,7 +912,11 @@ static void create_uart(const VirtMachineState *vms, int uart,
int irq = vms->irqmap[uart];
const char compat[] = "arm,pl011\0arm,primecell";
const char clocknames[] = "uartclk\0apb_pclk";
+#ifdef CONFIG_WITH_RUST
+ DeviceState *dev = qdev_new("x-pl011-rust");
+#else
DeviceState *dev = qdev_new(TYPE_PL011);
+#endif
SysBusDevice *s = SYS_BUS_DEVICE(dev);
MachineState *ms = MACHINE(vms);
--
γαῖα πυρί μιχθήτω
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 0/5] Implement ARM PL011 in Rust
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
` (4 preceding siblings ...)
2024-06-11 10:33 ` [RFC PATCH v2 5/5] DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine Manos Pitsidianakis
@ 2024-06-12 8:37 ` Daniel P. Berrangé
2024-06-13 5:13 ` Manos Pitsidianakis
2024-06-19 3:31 ` Richard Henderson
6 siblings, 1 reply; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-12 8:37 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Bennée, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Tue, Jun 11, 2024 at 01:33:29PM +0300, Manos Pitsidianakis wrote:
>
> .gitignore | 2 +
> .gitlab-ci.d/buildtest.yml | 64 ++--
> MAINTAINERS | 13 +
> configure | 12 +
> hw/arm/virt.c | 4 +
> meson.build | 102 ++++++
> meson_options.txt | 4 +
> rust/meson.build | 93 ++++++
> 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 +
> rust/pl011/src/definitions.rs | 95 ++++++
> rust/pl011/src/device.rs | 531 ++++++++++++++++++++++++++++++
> rust/pl011/src/device_class.rs | 95 ++++++
> rust/pl011/src/generated.rs | 5 +
> rust/pl011/src/lib.rs | 581 +++++++++++++++++++++++++++++++++
> rust/pl011/src/memory_ops.rs | 38 +++
> rust/rustfmt.toml | 7 +
> rust/wrapper.h | 39 +++
> scripts/cargo_wrapper.py | 221 +++++++++++++
> scripts/meson-buildoptions.sh | 6 +
Given the priority of getting the build system correct, what's missing
here is updates/integration into our standard GitLab CI pipeline. If
that can be shown to be working, that'll give alot more confidence in
the overall solution.
Ideally this should not require anything more than updating the docker
container definitions to add in the rust toolchain, plus the appropriate
std library build for the given target - we cross compiler for every
arch we officially care about.
Most of our dockerfiles these days are managed by lcitool, and it has
nearly sufficient support for cross compiling with the rust std library.
So to start with, this series should modify tests/lcitool/projects/qemu.yml
to add
- rust
- rust-std
to the package list, and run 'make lcitool-refresh' to re-create the
dockerfiles - see the docs/devel/testing.rst for more info about
lcitool if needed.
Assuming these 2 rust packages are in the container, I would then
expect QEMU to just "do the right thing" when building this rust
code. If it does not, then that's a sign of gaps that need closing.
Getting rid of the need to use --rust-target-triple will be the
immediate gap that needs fixing, as CI just passes --cross-prefix
for cross-builds and expects everything to be set from that.
The main gap we have is that for Windows I need to update lcitool
to pull in the mingw std lib target for rust, which I something I
missed when adding rust cross compiler support.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-11 10:33 ` [RFC PATCH v2 3/5] rust: add PL011 device model Manos Pitsidianakis
@ 2024-06-12 12:29 ` Paolo Bonzini
2024-06-12 14:14 ` Manos Pitsidianakis
2024-06-13 8:30 ` Zhao Liu
` (2 subsequent siblings)
3 siblings, 1 reply; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-12 12:29 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
I think this is extremely useful to show where we could go in the task
of creating more idiomatic bindings.
On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> +fn main() {
> + println!("cargo::rerun-if-env-changed=MESON_BUILD_DIR");
> + println!("cargo::rerun-if-env-changed=MESON_BUILD_ROOT");
> + println!("cargo::rerun-if-changed=src/generated.rs.inc");
Why do you need this rerun-if-changed?
> +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
> + name: TYPE_PL011.as_ptr(),
> + parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
> + instance_size: core::mem::size_of::<PL011State>(),
> + instance_align: core::mem::align_of::<PL011State>(),
> + instance_init: Some(pl011_init),
> + instance_post_init: None,
> + instance_finalize: None,
> + abstract_: false,
> + class_size: 0,
> + class_init: Some(pl011_class_init),
> + class_base_init: None,
> + class_data: core::ptr::null_mut(),
> + interfaces: core::ptr::null_mut(),
> +};
QOM is certainly a major part of what we need to do for idiomatic
bindings. This series includes both using QOM objects (chardev) and
defining them.
For using QOM objects, there is only one strategy that we need to
implement for both Chardev and DeviceState/SysBusDevice which is nice.
We can probably take inspiration from glib-rs, see for example
- https://docs.rs/glib/latest/glib/object/trait.Cast.html
- https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
- https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
For definining new classes I think it's okay if Rust does not support
writing superclasses yet, only leaves.
I would make a QOM class written in Rust a struct that only contains
the new fields. The struct must implement Default and possibly Drop
(for finalize).
struct PL011_Inner {
iomem: MemoryRegion,
readbuff: u32.
...
}
and then a macro defines a wrapper struct that includes just two
fields, one for the superclass and one for the Rust struct.
instance_init can initialize the latter with Default::default().
struct PL011 {
parent_obj: qemu::bindings::SysBusDevice,
private: PL011_Inner,
}
"private" probably should be RefCell<PL011_Inner>, avoiding the unsafe
state.as_mut().read(addr, size)
There should also be macros to define the wrappers for MMIO MemoryRegions.
> + pub fn realize(&mut self) {
> + unsafe {
> + qemu_chr_fe_set_handlers(
> + addr_of_mut!(self.char_backend),
> + Some(pl011_can_receive),
> + Some(pl011_receive),
> + Some(pl011_event),
> + None,
> + addr_of_mut!(*self).cast::<c_void>(),
> + core::ptr::null_mut(),
> + true,
> + );
> + }
Probably each set of callbacks (MemoryRegion, Chardev) needs to be a
struct that implements a trait.
> +#[macro_export]
> +macro_rules! define_property {
> + ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr) => {
> + $crate::generated::Property {
> + name: $name,
> + info: $prop,
> + offset: ::core::mem::offset_of!($state, $field) as _,
> + bitnr: 0,
> + bitmask: 0,
> + set_default: true,
> + defval: $crate::generated::Property__bindgen_ty_1 { u: $defval.into() },
> + arrayoffset: 0,
> + arrayinfo: ::core::ptr::null(),
> + arrayfieldsize: 0,
> + link_type: ::core::ptr::null(),
> + }
> + };
Perhaps we can define macros similar to the C DEFINE_PROP_*, and const
functions can be used to enforce some kind of type safety.
pub const fn check_type<T>(_x: &T, y: T) -> T { y }
static VAR: i16 = 42i16;
static TEST: i8 = check_type(&VAR, 43i8);
pub fn main() { println!("{}", TEST); }
error[E0308]: mismatched types
--> ff.rs:4:30
|
4 | static TEST: i8 = check_type(&VAR, 43i8);
| ---------- ^^^^ expected `&i8`, found `&i16`
| |
| arguments to this function are incorrect
|
= note: expected reference `&i8`
found reference `&i16`
Anyhow I think this is already very useful, because as the
abstractions are developed, it is possible to see how the device code
evolves.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-12 12:29 ` Paolo Bonzini
@ 2024-06-12 14:14 ` Manos Pitsidianakis
2024-06-12 15:20 ` Paolo Bonzini
2024-06-12 16:06 ` Daniel P. Berrangé
0 siblings, 2 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-12 14:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Peter Maydell,
Alex Benné e, Daniel P. Berrangé ,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé , Zhao Liu, Gustavo Romero,
Pierrick Bouvier
On Wed, 12 Jun 2024 15:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
>I think this is extremely useful to show where we could go in the task
>of creating more idiomatic bindings.
>
>On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
><manos.pitsidianakis@linaro.org> wrote:
>> +fn main() {
>> + println!("cargo::rerun-if-env-changed=MESON_BUILD_DIR");
>> + println!("cargo::rerun-if-env-changed=MESON_BUILD_ROOT");
>> + println!("cargo::rerun-if-changed=src/generated.rs.inc");
>
>Why do you need this rerun-if-changed?
To show an error from build.rs in case the file is deleted and the build
is not via meson
>
>> +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
>> + name: TYPE_PL011.as_ptr(),
>> + parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
>> + instance_size: core::mem::size_of::<PL011State>(),
>> + instance_align: core::mem::align_of::<PL011State>(),
>> + instance_init: Some(pl011_init),
>> + instance_post_init: None,
>> + instance_finalize: None,
>> + abstract_: false,
>> + class_size: 0,
>> + class_init: Some(pl011_class_init),
>> + class_base_init: None,
>> + class_data: core::ptr::null_mut(),
>> + interfaces: core::ptr::null_mut(),
>> +};
>
>QOM is certainly a major part of what we need to do for idiomatic
>bindings. This series includes both using QOM objects (chardev) and
>defining them.
>
>For using QOM objects, there is only one strategy that we need to
>implement for both Chardev and DeviceState/SysBusDevice which is nice.
>We can probably take inspiration from glib-rs, see for example
>- https://docs.rs/glib/latest/glib/object/trait.Cast.html
>- https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
>- https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
There was consensus in the community call that we won't be writing Rust
APIs for internal C QEMU interfaces; or at least, that's not the goal
It's not necessary to make bindings to write idiomatic Rust. If you
notice, most callbacks QEMU calls cast the pointer into the Rust struct
which then calls its idiomatic type methods. I like abstraction a lot,
but it has diminishing returns. We'll see how future Rust devices look
like and we can then decide if extra code for abstractions is a good
trade-off.
>
>For definining new classes I think it's okay if Rust does not support
>writing superclasses yet, only leaves.
>
>I would make a QOM class written in Rust a struct that only contains
>the new fields. The struct must implement Default and possibly Drop
>(for finalize).
The object is allocated and freed from C, hence it is not Dropped. We're
only ever accessing it from a reference retrieved from a QEMU provided
raw pointer. If the struct gains heap object fields like Box or Vec or
String, they'd have to be dropped manually on _unrealize.
>
> struct PL011_Inner {
> iomem: MemoryRegion,
> readbuff: u32.
> ...
> }
>
>and then a macro defines a wrapper struct that includes just two
>fields, one for the superclass and one for the Rust struct.
>instance_init can initialize the latter with Default::default().
>
> struct PL011 {
> parent_obj: qemu::bindings::SysBusDevice,
> private: PL011_Inner,
> }
a nested struct is not necessary for using the Default trait. Consider a
Default impl that sets parent_obj as a field memset to zero; on instance
initialization we can do
*self = Self {
parent_obj: self.parent_obj,
..Self::default(),
};
>"private" probably should be RefCell<PL011_Inner>, avoiding the unsafe
>
> state.as_mut().read(addr, size)
RefCell etc are not FFI safe. Also, nested fields must be visible so
that the offset_of! macro works, for QOM properties. Finally,
state.as_mut().read(addr, size)
Is safe since we receive a valid pointer from QEMU. This fact cannot be
derived by the compiler, which is why it has an `unsafe` keyword. That
does not mean that the use here is unsafe.
>
>There should also be macros to define the wrappers for MMIO MemoryRegions.
Do you mean the MemoryRegionOps?
>
>> + pub fn realize(&mut self) {
>> + unsafe {
>> + qemu_chr_fe_set_handlers(
>> + addr_of_mut!(self.char_backend),
>> + Some(pl011_can_receive),
>> + Some(pl011_receive),
>> + Some(pl011_event),
>> + None,
>> + addr_of_mut!(*self).cast::<c_void>(),
>> + core::ptr::null_mut(),
>> + true,
>> + );
>> + }
>
>Probably each set of callbacks (MemoryRegion, Chardev) needs to be a
>struct that implements a trait.
>
>> +#[macro_export]
>> +macro_rules! define_property {
>> + ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr) => {
>> + $crate::generated::Property {
>> + name: $name,
>> + info: $prop,
>> + offset: ::core::mem::offset_of!($state, $field) as _,
>> + bitnr: 0,
>> + bitmask: 0,
>> + set_default: true,
>> + defval: $crate::generated::Property__bindgen_ty_1 { u: $defval.into() },
>> + arrayoffset: 0,
>> + arrayinfo: ::core::ptr::null(),
>> + arrayfieldsize: 0,
>> + link_type: ::core::ptr::null(),
>> + }
>> + };
>
>Perhaps we can define macros similar to the C DEFINE_PROP_*,
Hopefully if I end up doing something like this, it'd be in a standalone
crate for other devices to use
>
>and const
>functions can be used to enforce some kind of type safety.
>
>pub const fn check_type<T>(_x: &T, y: T) -> T { y }
>
>static VAR: i16 = 42i16;
>static TEST: i8 = check_type(&VAR, 43i8);
>
>pub fn main() { println!("{}", TEST); }
>
>error[E0308]: mismatched types
> --> ff.rs:4:30
> |
>4 | static TEST: i8 = check_type(&VAR, 43i8);
> | ---------- ^^^^ expected `&i8`, found `&i16`
> | |
> | arguments to this function are incorrect
> |
> = note: expected reference `&i8`
> found reference `&i16`
Yes this kind of type safety trick is easy to do (already done for a
register macro in src/lib.rs).
I wanted to focus on the build system integration for the first RFC
which is why there are some macros but not in every place it makes
sense.
Thanks Paolo,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-12 14:14 ` Manos Pitsidianakis
@ 2024-06-12 15:20 ` Paolo Bonzini
2024-06-12 16:06 ` Daniel P. Berrangé
1 sibling, 0 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-12 15:20 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Peter Maydell,
Alex Benné e, Daniel P. Berrangé,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé, Zhao Liu, Gustavo Romero,
Pierrick Bouvier
On Wed, Jun 12, 2024 at 4:42 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> There was consensus in the community call that we won't be writing Rust
> APIs for internal C QEMU interfaces; or at least, that's not the goal
I disagree with that. We need _some_ kind of bindings, otherwise we
have too much unsafe code, and the benefit of Rust becomes so much
lower that I doubt the utility.
If something is used by only one device then fine, but when some kind
of unsafe code repeats across most if not all devices, that is a
problem. It can be macros, it can be smart pointers, that remains to
be seen---but repetition should be a warning signal that _something_
is necessary.
> >For definining new classes I think it's okay if Rust does not support
> >writing superclasses yet, only leaves.
> >
> >I would make a QOM class written in Rust a struct that only contains
> >the new fields. The struct must implement Default and possibly Drop
> >(for finalize).
>
> The object is allocated and freed from C, hence it is not Dropped. We're
> only ever accessing it from a reference retrieved from a QEMU provided
> raw pointer. If the struct gains heap object fields like Box or Vec or
> String, they'd have to be dropped manually on _unrealize.
That's my point, if you have
struct MyDevice_Inner {
data: Vec<u8>,
}
struct MyDevice {
parent_obj: qemu::bindings::SysBusDevice,
private: ManuallyDrop<PL011_Inner>,
}
then the instance_finalize method can simply do
pub instance_finalize(self: *c_void)
{
let dev = self as *mut MyDevice;
unsafe { ManuallyDrop::drop(dev.private) }
}
Don't do it on _unrealize, create macros that do it for you.
> >and then a macro defines a wrapper struct that includes just two
> >fields, one for the superclass and one for the Rust struct.
> >instance_init can initialize the latter with Default::default().
> >
> > struct PL011 {
> > parent_obj: qemu::bindings::SysBusDevice,
> > private: PL011_Inner,
> > }
>
> a nested struct is not necessary for using the Default trait
Agreed, but a nested struct is nice anyway in my opinion as a boundary
between the C-ish and Rust idiomatic code.
> >"private" probably should be RefCell<PL011_Inner>, avoiding the unsafe
> >
> > state.as_mut().read(addr, size)
>
>
> RefCell etc are not FFI safe.
Why does it matter? Everything after the SysBusDevice is private.
> Also, nested fields must be visible so that the offset_of! macro works, for QOM properties.
Note that QOM properties do not use offset_of; qdev properties do.
Using qdev properties is much easier because they hide visitors, but
again - not necessary, sometimes going lower-level can be easier if
the API you wrap is less C-ish.
Also, you can define constants (including properties) in contexts
where non-public fields are visible:
use std::mem;
pub struct Foo {
_x: i32,
y: i32,
}
impl Foo {
pub const OFFSET_Y: usize = mem::offset_of!(Foo, y);
}
fn main() {
println!("{}", Foo::OFFSET_Y);
}
Any offset needed to go past the SysBusDevice and any other fields
before MyDevice_Inner can be added via macros. Also note that it
doesn't _have_ to be RefCell; RefCell isn't particularly magic. We can
implement our own interior mutability thingy that is more friendly to
qdev properties, or that includes the ManuallyDrop<> thing from above,
or both.
For example you could have
type PL011 = QOMImpl<qemu::bindings::SysBusDevice, PL011_Inner>;
and all the magic (for example Borrow<PL011_Inner>, the TypeInfo, the
instance_init and instance_finalize function) would be in QOMImpl.
My point is: let's not focus on having a C-like API. It's the easiest
thing to do but not the target.
> Finally,
>
> state.as_mut().read(addr, size)
>
> Is safe since we receive a valid pointer from QEMU. This fact cannot be
> derived by the compiler, which is why it has an `unsafe` keyword. That
> does not mean that the use here is unsafe.
Yes, it is safe otherwise it would be undefined behavior, but there
are no checks of the kind that you have in Rust whenever you have
&mut.
state.as_mut() implies that no other references to state are in use;
but there are (you pass it as the opaque value to both the
MemoryRegionOps and the chardev frontend callbacks). This is why I
think something like RefCell is needed to go from a shared reference
to an exclusive one (interior mutability).
> >There should also be macros to define the wrappers for MMIO MemoryRegions.
>
> Do you mean the MemoryRegionOps?
Yes.
> I wanted to focus on the build system integration for the first RFC
> which is why there are some macros but not in every place it makes
> sense.
Yes, absolutely. We need to start somewhere.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-12 14:14 ` Manos Pitsidianakis
2024-06-12 15:20 ` Paolo Bonzini
@ 2024-06-12 16:06 ` Daniel P. Berrangé
2024-06-12 20:57 ` Manos Pitsidianakis
1 sibling, 1 reply; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-12 16:06 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: Paolo Bonzini, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Wed, Jun 12, 2024 at 05:14:56PM +0300, Manos Pitsidianakis wrote:
> On Wed, 12 Jun 2024 15:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > I think this is extremely useful to show where we could go in the task
> > of creating more idiomatic bindings.
> >
> > On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
> > <manos.pitsidianakis@linaro.org> wrote:
> > > +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
> > > + name: TYPE_PL011.as_ptr(),
> > > + parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
> > > + instance_size: core::mem::size_of::<PL011State>(),
> > > + instance_align: core::mem::align_of::<PL011State>(),
> > > + instance_init: Some(pl011_init),
> > > + instance_post_init: None,
> > > + instance_finalize: None,
> > > + abstract_: false,
> > > + class_size: 0,
> > > + class_init: Some(pl011_class_init),
> > > + class_base_init: None,
> > > + class_data: core::ptr::null_mut(),
> > > + interfaces: core::ptr::null_mut(),
> > > +};
> >
> > QOM is certainly a major part of what we need to do for idiomatic
> > bindings. This series includes both using QOM objects (chardev) and
> > defining them.
> >
> > For using QOM objects, there is only one strategy that we need to
> > implement for both Chardev and DeviceState/SysBusDevice which is nice.
> > We can probably take inspiration from glib-rs, see for example
> > - https://docs.rs/glib/latest/glib/object/trait.Cast.html
> > - https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
> > - https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
>
>
> There was consensus in the community call that we won't be writing Rust APIs
> for internal C QEMU interfaces; or at least, that's not the goal
I think that is over-stating things. This was only mentioned in passing
and my feeling was that we didn't have that detailed of a discussion
because at this stage such a discussion is a bit like putting the cart
before the horse.
While the initial focus might be to just consume a Rust API that is a
1:1 mapping of the C API, I expect that over time we'll end up writing
various higher level Rust wrappers around the C API. If we didn't do that,
then in effect we'd be making ourselves write psuedo-C code in Rust,
undermining many of the benefits we could get.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-12 16:06 ` Daniel P. Berrangé
@ 2024-06-12 20:57 ` Manos Pitsidianakis
2024-06-12 21:27 ` Paolo Bonzini
0 siblings, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-12 20:57 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé ,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Wed, 12 Jun 2024 19:06, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>On Wed, Jun 12, 2024 at 05:14:56PM +0300, Manos Pitsidianakis wrote:
>> On Wed, 12 Jun 2024 15:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > I think this is extremely useful to show where we could go in the task
>> > of creating more idiomatic bindings.
>> >
>> > On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
>> > <manos.pitsidianakis@linaro.org> wrote:
>> > > +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
>> > > + name: TYPE_PL011.as_ptr(),
>> > > + parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
>> > > + instance_size: core::mem::size_of::<PL011State>(),
>> > > + instance_align: core::mem::align_of::<PL011State>(),
>> > > + instance_init: Some(pl011_init),
>> > > + instance_post_init: None,
>> > > + instance_finalize: None,
>> > > + abstract_: false,
>> > > + class_size: 0,
>> > > + class_init: Some(pl011_class_init),
>> > > + class_base_init: None,
>> > > + class_data: core::ptr::null_mut(),
>> > > + interfaces: core::ptr::null_mut(),
>> > > +};
>> >
>> > QOM is certainly a major part of what we need to do for idiomatic
>> > bindings. This series includes both using QOM objects (chardev) and
>> > defining them.
>> >
>> > For using QOM objects, there is only one strategy that we need to
>> > implement for both Chardev and DeviceState/SysBusDevice which is nice.
>> > We can probably take inspiration from glib-rs, see for example
>> > - https://docs.rs/glib/latest/glib/object/trait.Cast.html
>> > - https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
>> > - https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html
>>
>>
>> There was consensus in the community call that we won't be writing Rust APIs
>> for internal C QEMU interfaces; or at least, that's not the goal
>
>I think that is over-stating things. This was only mentioned in passing
>and my feeling was that we didn't have that detailed of a discussion
>because at this stage such a discussion is a bit like putting the cart
>before the horse.
>
>While the initial focus might be to just consume a Rust API that is a
>1:1 mapping of the C API, I expect that over time we'll end up writing
>various higher level Rust wrappers around the C API. If we didn't do that,
>then in effect we'd be making ourselves write psuedo-C code in Rust,
>undermining many of the benefits we could get.
>
In any case, it is out of scope for this RFC. Introducing wrappers would
be a gradual process.
Thanks,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-12 20:57 ` Manos Pitsidianakis
@ 2024-06-12 21:27 ` Paolo Bonzini
2024-06-13 5:09 ` Manos Pitsidianakis
2024-06-13 7:13 ` Daniel P. Berrangé
0 siblings, 2 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-12 21:27 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
[-- Attachment #1: Type: text/plain, Size: 359 bytes --]
Il mer 12 giu 2024, 22:58 Manos Pitsidianakis <
manos.pitsidianakis@linaro.org> ha scritto:
> In any case, it is out of scope for this RFC. Introducing wrappers would
> be a gradual process.
>
Sure, how would you feel about such bindings being developed on list, and
maintained in a (somewhat) long-lived experimental branch?
Paolo
> Thanks,
> Manos
>
>
[-- Attachment #2: Type: text/html, Size: 988 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-12 21:27 ` Paolo Bonzini
@ 2024-06-13 5:09 ` Manos Pitsidianakis
2024-06-13 7:13 ` Daniel P. Berrangé
1 sibling, 0 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-13 5:09 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniel P. Berrangé , qemu-devel, Stefan Hajnoczi,
Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé , Zhao Liu, Gustavo Romero,
Pierrick Bouvier
Good morning Paolo,
On Thu, 13 Jun 2024 00:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
>Il mer 12 giu 2024, 22:58 Manos Pitsidianakis <
>manos.pitsidianakis@linaro.org> ha scritto:
>
>> In any case, it is out of scope for this RFC. Introducing wrappers
>> would be a gradual process.
>>
>
>Sure, how would you feel about such bindings being developed on list,
>and maintained in a (somewhat) long-lived experimental branch?
Hm the only thing that worries me is keeping it synced and postponing
merge indefinitely. If we declare the rust parts as "experimental" we
could evolve them quickly even on master. What do the other maintainers
think?
Thanks,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 0/5] Implement ARM PL011 in Rust
2024-06-12 8:37 ` [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Daniel P. Berrangé
@ 2024-06-13 5:13 ` Manos Pitsidianakis
2024-06-13 7:56 ` Daniel P. Berrangé
0 siblings, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-13 5:13 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé ,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
Good morning Daniel,
On Wed, 12 Jun 2024 11:37, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>On Tue, Jun 11, 2024 at 01:33:29PM +0300, Manos Pitsidianakis wrote:
>
>>
>> .gitignore | 2 +
>> .gitlab-ci.d/buildtest.yml | 64 ++--
>> MAINTAINERS | 13 +
>> configure | 12 +
>> hw/arm/virt.c | 4 +
>> meson.build | 102 ++++++
>> meson_options.txt | 4 +
>> rust/meson.build | 93 ++++++
>> 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 +
>> rust/pl011/src/definitions.rs | 95 ++++++
>> rust/pl011/src/device.rs | 531 ++++++++++++++++++++++++++++++
>> rust/pl011/src/device_class.rs | 95 ++++++
>> rust/pl011/src/generated.rs | 5 +
>> rust/pl011/src/lib.rs | 581 +++++++++++++++++++++++++++++++++
>> rust/pl011/src/memory_ops.rs | 38 +++
>> rust/rustfmt.toml | 7 +
>> rust/wrapper.h | 39 +++
>> scripts/cargo_wrapper.py | 221 +++++++++++++
>> scripts/meson-buildoptions.sh | 6 +
>
>Given the priority of getting the build system correct, what's missing
>here is updates/integration into our standard GitLab CI pipeline. If
>that can be shown to be working, that'll give alot more confidence in
>the overall solution.
>
>Ideally this should not require anything more than updating the docker
>container definitions to add in the rust toolchain, plus the appropriate
>std library build for the given target - we cross compiler for every
>arch we officially care about.
>
>Most of our dockerfiles these days are managed by lcitool, and it has
>nearly sufficient support for cross compiling with the rust std library.
>So to start with, this series should modify tests/lcitool/projects/qemu.yml
>to add
>
> - rust
> - rust-std
>
>to the package list, and run 'make lcitool-refresh' to re-create the
>dockerfiles - see the docs/devel/testing.rst for more info about
>lcitool if needed.
>
>Assuming these 2 rust packages are in the container, I would then
>expect QEMU to just "do the right thing" when building this rust
>code. If it does not, then that's a sign of gaps that need closing.
>
>Getting rid of the need to use --rust-target-triple will be the
>immediate gap that needs fixing, as CI just passes --cross-prefix
>for cross-builds and expects everything to be set from that.
>
>The main gap we have is that for Windows I need to update lcitool
>to pull in the mingw std lib target for rust, which I something I
>missed when adding rust cross compiler support.
>
Thanks very much for the pointers! I will start dealing with this in the
next RFC version.
Re: the target triple, I agree 100%. In fact it wasn't my addition, I
kept it from the previous rust RFC patchset that was posted on the list
some years ago. It should be possible to construct the triplets
ourselves and let the user override if they want to as mentioned in
another email.
The rust project has official Docker images, do you think it's something
we could use or is it unnecessary?
https://github.com/rust-lang/docker-rust
Thanks,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-12 21:27 ` Paolo Bonzini
2024-06-13 5:09 ` Manos Pitsidianakis
@ 2024-06-13 7:13 ` Daniel P. Berrangé
2024-06-13 7:56 ` Paolo Bonzini
1 sibling, 1 reply; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-13 7:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote:
> Il mer 12 giu 2024, 22:58 Manos Pitsidianakis <
> manos.pitsidianakis@linaro.org> ha scritto:
>
> > In any case, it is out of scope for this RFC. Introducing wrappers would
> > be a gradual process.
> >
>
> Sure, how would you feel about such bindings being developed on list, and
> maintained in a (somewhat) long-lived experimental branch?
IMHO any higher level binding APIs for Rust should be acceptable in the
main QEMU tree as soon as we accept Rust functionality. They can evolve
in-tree based on the needs of whomever is creating and/or consuming them.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 0/5] Implement ARM PL011 in Rust
2024-06-13 5:13 ` Manos Pitsidianakis
@ 2024-06-13 7:56 ` Daniel P. Berrangé
0 siblings, 0 replies; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-13 7:56 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Thu, Jun 13, 2024 at 08:13:01AM +0300, Manos Pitsidianakis wrote:
> Good morning Daniel,
>
> On Wed, 12 Jun 2024 11:37, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
> > On Tue, Jun 11, 2024 at 01:33:29PM +0300, Manos Pitsidianakis wrote:
> >
> > >
> > > .gitignore | 2 +
> > > .gitlab-ci.d/buildtest.yml | 64 ++--
> > > MAINTAINERS | 13 +
> > > configure | 12 +
> > > hw/arm/virt.c | 4 +
> > > meson.build | 102 ++++++
> > > meson_options.txt | 4 +
> > > rust/meson.build | 93 ++++++
> > > 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 +
> > > rust/pl011/src/definitions.rs | 95 ++++++
> > > rust/pl011/src/device.rs | 531 ++++++++++++++++++++++++++++++
> > > rust/pl011/src/device_class.rs | 95 ++++++
> > > rust/pl011/src/generated.rs | 5 +
> > > rust/pl011/src/lib.rs | 581 +++++++++++++++++++++++++++++++++
> > > rust/pl011/src/memory_ops.rs | 38 +++
> > > rust/rustfmt.toml | 7 +
> > > rust/wrapper.h | 39 +++
> > > scripts/cargo_wrapper.py | 221 +++++++++++++
> > > scripts/meson-buildoptions.sh | 6 +
> >
> > Given the priority of getting the build system correct, what's missing
> > here is updates/integration into our standard GitLab CI pipeline. If
> > that can be shown to be working, that'll give alot more confidence in
> > the overall solution.
> >
> > Ideally this should not require anything more than updating the docker
> > container definitions to add in the rust toolchain, plus the appropriate
> > std library build for the given target - we cross compiler for every
> > arch we officially care about.
> >
> > Most of our dockerfiles these days are managed by lcitool, and it has
> > nearly sufficient support for cross compiling with the rust std library.
> > So to start with, this series should modify tests/lcitool/projects/qemu.yml
> > to add
> >
> > - rust
> > - rust-std
> >
> > to the package list, and run 'make lcitool-refresh' to re-create the
> > dockerfiles - see the docs/devel/testing.rst for more info about
> > lcitool if needed.
> >
> > Assuming these 2 rust packages are in the container, I would then
> > expect QEMU to just "do the right thing" when building this rust
> > code. If it does not, then that's a sign of gaps that need closing.
> >
> > Getting rid of the need to use --rust-target-triple will be the
> > immediate gap that needs fixing, as CI just passes --cross-prefix
> > for cross-builds and expects everything to be set from that.
> >
> > The main gap we have is that for Windows I need to update lcitool
> > to pull in the mingw std lib target for rust, which I something I
> > missed when adding rust cross compiler support.
I've updated lcitool for this gap:
https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/492
> The rust project has official Docker images, do you think it's something we
> could use or is it unnecessary?
>
> https://github.com/rust-lang/docker-rust
They're irrelevant - we need the containers with the full set of QEMU build
dependencies present, and it is the Rust versions that are in the distros
that matter to us.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 7:13 ` Daniel P. Berrangé
@ 2024-06-13 7:56 ` Paolo Bonzini
2024-06-13 8:49 ` Manos Pitsidianakis
` (2 more replies)
0 siblings, 3 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-13 7:56 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]
Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:
> On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote:
> > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis <
> > manos.pitsidianakis@linaro.org> ha scritto:
> >
> > > In any case, it is out of scope for this RFC. Introducing wrappers
> would
> > > be a gradual process.
> > >
> >
> > Sure, how would you feel about such bindings being developed on list, and
> > maintained in a (somewhat) long-lived experimental branch?
>
> IMHO any higher level binding APIs for Rust should be acceptable in the
> main QEMU tree as soon as we accept Rust functionality. They can evolve
> in-tree based on the needs of whomever is creating and/or consuming them.
>
My question is the opposite, should we accept Rust functionality without
proper high level bindings? I am afraid that, if more Rust devices are
contributed, it becomes technical debt to have a mix of idiomatic and C-ish
code. If the answer is no, then this PL011 device has to be developed out
of tree.
Paolo
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o-
> https://www.instagram.com/dberrange :|
>
>
[-- Attachment #2: Type: text/html, Size: 2671 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-11 10:33 ` [RFC PATCH v2 3/5] rust: add PL011 device model Manos Pitsidianakis
2024-06-12 12:29 ` Paolo Bonzini
@ 2024-06-13 8:30 ` Zhao Liu
2024-06-13 8:41 ` Manos Pitsidianakis
2024-06-19 5:34 ` Richard Henderson
2024-07-11 4:21 ` Zhao Liu
3 siblings, 1 reply; 54+ messages in thread
From: Zhao Liu @ 2024-06-13 8:30 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Bennée, Daniel P. Berrangé,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé, Gustavo Romero, Pierrick Bouvier
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/
Regards,
Zhao
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 8:30 ` Zhao Liu
@ 2024-06-13 8:41 ` Manos Pitsidianakis
2024-06-13 8:53 ` Daniel P. Berrangé
0 siblings, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-13 8:41 UTC (permalink / raw)
To: Zhao Liu
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Benné e, Daniel P. Berrangé ,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé , Gustavo Romero, Pierrick Bouvier
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 7:56 ` Paolo Bonzini
@ 2024-06-13 8:49 ` Manos Pitsidianakis
2024-06-13 9:16 ` Daniel P. Berrangé
2024-06-13 16:20 ` Zhao Liu
2 siblings, 0 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-13 8:49 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé ,
Zhao Liu, Gustavo Romero, Pierrick Bouvier,
Daniel P. Berrangé
On Thu, 13 Jun 2024 10:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
>Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> ha
>scritto:
>
>> On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote:
>> > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis <
>> > manos.pitsidianakis@linaro.org> ha scritto:
>> >
>> > > In any case, it is out of scope for this RFC. Introducing wrappers
>> would
>> > > be a gradual process.
>> > >
>> >
>> > Sure, how would you feel about such bindings being developed on list, and
>> > maintained in a (somewhat) long-lived experimental branch?
>>
>> IMHO any higher level binding APIs for Rust should be acceptable in the
>> main QEMU tree as soon as we accept Rust functionality. They can evolve
>> in-tree based on the needs of whomever is creating and/or consuming them.
>>
>
>My question is the opposite, should we accept Rust functionality without
>proper high level bindings? I am afraid that, if more Rust devices are
>contributed, it becomes technical debt to have a mix of idiomatic and C-ish
>code. If the answer is no, then this PL011 device has to be developed out
>of tree.
>
>Paolo
Getting Rust into QEMU, at least for our team at Linaro, is a long term
commitment, so we will be responsible for preventing and fixing
technical debt. And it will be up to the hypothetical rust maintainers
as well to "keep the garden tidy" so to speak.
To put it another way, I personally plan on making sure any bindings and
any QEMU-ffi idioms that arise are all homogeneous and don't end up
being a burden for the code base.
Your concern is valid, and thank you for raising it. I feel it is
important to figure out how this will be managed, since it's also an
argument for the final say in whether any of this code ends up in the
upstream tree.
Thanks Paolo,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 8:41 ` Manos Pitsidianakis
@ 2024-06-13 8:53 ` Daniel P. Berrangé
2024-06-13 8:59 ` Manos Pitsidianakis
0 siblings, 1 reply; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-13 8:53 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: Zhao Liu, qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Gustavo Romero, Pierrick Bouvier
On Thu, Jun 13, 2024 at 11:41:38AM +0300, Manos Pitsidianakis wrote:
> > > 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.
This doesn't make sense. One the one hand you're saying the rules don't
have any effect on the code style vs the default, but on the otherhand
saying they do "extra work" on top of the default style. Those can't
both be true.
> To sum it up, the style is essentially the default one, so there's no
> problem here!
If the style config is essentially the default one, then just remove
this config and make it actually the default.
Having this file exist sets the (incorrect) expectation that we would
be willing to accept changes that diverge from the default rust style,
and that's not desirable IMHO.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 8:53 ` Daniel P. Berrangé
@ 2024-06-13 8:59 ` Manos Pitsidianakis
2024-06-13 9:20 ` Daniel P. Berrangé
0 siblings, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-13 8:59 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Zhao Liu, qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé ,
Gustavo Romero, Pierrick Bouvier
On Thu, 13 Jun 2024 11:53, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>On Thu, Jun 13, 2024 at 11:41:38AM +0300, Manos Pitsidianakis wrote:
>> > > 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.
>
>This doesn't make sense. One the one hand you're saying the rules don't
>have any effect on the code style vs the default, but on the otherhand
>saying they do "extra work" on top of the default style. Those can't
>both be true.
No, I fear there's a confusion here. It means that if you run the stable
rustfmt with the default options the code doesn't change. I.e. it does
not conflict with the default style.
What it does is group imports, format text in doc comments (which stable
rustfmt doesn't touch at all) and also splits long strings into several
lines, which are all helpful for e-mail patch reviews.
Thanks,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 7:56 ` Paolo Bonzini
2024-06-13 8:49 ` Manos Pitsidianakis
@ 2024-06-13 9:16 ` Daniel P. Berrangé
2024-06-13 20:57 ` Paolo Bonzini
2024-06-13 16:20 ` Zhao Liu
2 siblings, 1 reply; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-13 9:16 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Thu, Jun 13, 2024 at 09:56:39AM +0200, Paolo Bonzini wrote:
> Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> ha
> scritto:
>
> > On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote:
> > > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis <
> > > manos.pitsidianakis@linaro.org> ha scritto:
> > >
> > > > In any case, it is out of scope for this RFC. Introducing wrappers
> > would
> > > > be a gradual process.
> > > >
> > >
> > > Sure, how would you feel about such bindings being developed on list, and
> > > maintained in a (somewhat) long-lived experimental branch?
> >
> > IMHO any higher level binding APIs for Rust should be acceptable in the
> > main QEMU tree as soon as we accept Rust functionality. They can evolve
> > in-tree based on the needs of whomever is creating and/or consuming them.
> >
>
> My question is the opposite, should we accept Rust functionality without
> proper high level bindings? I am afraid that, if more Rust devices are
> contributed, it becomes technical debt to have a mix of idiomatic and C-ish
> code. If the answer is no, then this PL011 device has to be developed out
> of tree.
I guess there's a balance to be had somewhere on the spectrum between doing
everything against the raw C binding, vs everything against a perfectly
idiomatic Rust API wrapping the C bniding. The latter might be the ideal,
but from a pragmmatic POV I doubt we want the barrier to entry to be that
high.
Is this not something we can figure out organically as part of the code
design and review processes ?
e.g. if during review we see a device impl doing something where a higher
level API would have unambiguous benefits, and creatino of such a higher
level API is a practically achieveable task, then ask for it. If a higher
level API is desirable, but possibly not practical, then raise it as an
potential idea, but be willing to accept the technical debt.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 8:59 ` Manos Pitsidianakis
@ 2024-06-13 9:20 ` Daniel P. Berrangé
0 siblings, 0 replies; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-13 9:20 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: Zhao Liu, qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Gustavo Romero, Pierrick Bouvier
On Thu, Jun 13, 2024 at 11:59:12AM +0300, Manos Pitsidianakis wrote:
> On Thu, 13 Jun 2024 11:53, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
> > On Thu, Jun 13, 2024 at 11:41:38AM +0300, Manos Pitsidianakis wrote:
> > > > > 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.
> >
> > This doesn't make sense. One the one hand you're saying the rules don't
> > have any effect on the code style vs the default, but on the otherhand
> > saying they do "extra work" on top of the default style. Those can't
> > both be true.
>
> No, I fear there's a confusion here. It means that if you run the stable
> rustfmt with the default options the code doesn't change. I.e. it does not
> conflict with the default style.
>
> What it does is group imports, format text in doc comments (which stable
> rustfmt doesn't touch at all) and also splits long strings into several
> lines, which are all helpful for e-mail patch reviews.
Ah ok. Are we expecting these options to become part of stable rustfmt ?
I would expect our contributors to primarily be using the Rust toolchain
that comes with their distro, and not unstable -nightly toolchain. So I
still wonder if this rustfmt config will have much real world benefit ?
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 7:56 ` Paolo Bonzini
2024-06-13 8:49 ` Manos Pitsidianakis
2024-06-13 9:16 ` Daniel P. Berrangé
@ 2024-06-13 16:20 ` Zhao Liu
2024-06-13 17:56 ` Paolo Bonzini
2 siblings, 1 reply; 54+ messages in thread
From: Zhao Liu @ 2024-06-13 16:20 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniel P. Berrangé, Manos Pitsidianakis, qemu-devel,
Stefan Hajnoczi, Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé, Gustavo Romero, Pierrick Bouvier
Hi Paolo,
On Thu, Jun 13, 2024 at 09:56:39AM +0200, Paolo Bonzini wrote:
> Date: Thu, 13 Jun 2024 09:56:39 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC PATCH v2 3/5] rust: add PL011 device model
>
> Il gio 13 giu 2024, 09:13 Daniel P. Berrangé <berrange@redhat.com> ha
> scritto:
>
> > On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote:
> > > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis <
> > > manos.pitsidianakis@linaro.org> ha scritto:
> > >
> > > > In any case, it is out of scope for this RFC. Introducing wrappers
> > would
> > > > be a gradual process.
> > > >
> > >
> > > Sure, how would you feel about such bindings being developed on list, and
> > > maintained in a (somewhat) long-lived experimental branch?
> >
> > IMHO any higher level binding APIs for Rust should be acceptable in the
> > main QEMU tree as soon as we accept Rust functionality. They can evolve
> > in-tree based on the needs of whomever is creating and/or consuming them.
> >
>
> My question is the opposite, should we accept Rust functionality without
> proper high level bindings? I am afraid that, if more Rust devices are
> contributed, it becomes technical debt to have a mix of idiomatic and C-ish
> code. If the answer is no, then this PL011 device has to be developed out
> of tree.
I think deeper and higher level bindings will have more opens and will
likely require more discussion and exploration. So could we explore this
direction on another reference Rust device?
I also think there won’t be too many Rust devices in the short term.
Going back to tweak or enhance existing Rust devices may not require too
much effort, once we have a definitive answer.
I wonder if x86 could also implement a rust device (like the x86 timer
you mentioned before, hw/i386/kvm/i8254.c or hw/timer/i8254.c IIRC) to
try this? Or would you recommend another x86 device? :-)
I'd be willing to help Manos try it if you think it's fine.
Regards,
Zhao
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 16:20 ` Zhao Liu
@ 2024-06-13 17:56 ` Paolo Bonzini
0 siblings, 0 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-13 17:56 UTC (permalink / raw)
To: Zhao Liu
Cc: Daniel P. Berrangé, Manos Pitsidianakis, qemu-devel,
Stefan Hajnoczi, Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé, Gustavo Romero, Pierrick Bouvier
On Thu, Jun 13, 2024 at 6:06 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> I think deeper and higher level bindings will have more opens and will
> likely require more discussion and exploration. So could we explore this
> direction on another reference Rust device?
>
> I also think there won’t be too many Rust devices in the short term.
> Going back to tweak or enhance existing Rust devices may not require too
> much effort, once we have a definitive answer.
>
> I wonder if x86 could also implement a rust device (like the x86 timer
> you mentioned before, hw/i386/kvm/i8254.c or hw/timer/i8254.c IIRC) to
> try this? Or would you recommend another x86 device? :-)
A timer device is a good idea, just because it's another pretty stable
low-level QEMU API.
The problem with hw/timer/i8254.c is that it has the KVM version, as
you found. The HPET is an alternative though.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 9:16 ` Daniel P. Berrangé
@ 2024-06-13 20:57 ` Paolo Bonzini
2024-06-14 6:38 ` Manos Pitsidianakis
0 siblings, 1 reply; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-13 20:57 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> I guess there's a balance to be had somewhere on the spectrum between doing
> everything against the raw C binding, vs everything against a perfectly
> idiomatic Rust API wrapping the C bniding. The latter might be the ideal,
> but from a pragmmatic POV I doubt we want the barrier to entry to be that
> high.
Yes, I agree. I guess we could make things work step by step, even
committing something that only focuses on the build system like
Manos's work (I'll review it).
I can try to look at the basic QOM interface.
Manos, can you create a page on the wiki? Something like
https://wiki.qemu.org/Features/Meson.
Paolo
> Is this not something we can figure out organically as part of the code
> design and review processes ?
>
> e.g. if during review we see a device impl doing something where a higher
> level API would have unambiguous benefits, and creatino of such a higher
> level API is a practically achieveable task, then ask for it. If a higher
> level API is desirable, but possibly not practical, then raise it as an
> potential idea, but be willing to accept the technical debt.
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-13 20:57 ` Paolo Bonzini
@ 2024-06-14 6:38 ` Manos Pitsidianakis
2024-06-14 17:50 ` Paolo Bonzini
0 siblings, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-14 6:38 UTC (permalink / raw)
To: Paolo Bonzini, Daniel P. Berrangé
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé ,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Thu, 13 Jun 2024 23:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>> I guess there's a balance to be had somewhere on the spectrum between doing
>> everything against the raw C binding, vs everything against a perfectly
>> idiomatic Rust API wrapping the C bniding. The latter might be the ideal,
>> but from a pragmmatic POV I doubt we want the barrier to entry to be that
>> high.
>
>Yes, I agree. I guess we could make things work step by step, even
>committing something that only focuses on the build system like
>Manos's work (I'll review it).
>
>I can try to look at the basic QOM interface.
>
>Manos, can you create a page on the wiki? Something like
>https://wiki.qemu.org/Features/Meson.
Certainly! Just to make sure I understood correctly, you mean a wiki
page describing how things work and tracking the progress?
I added https://wiki.qemu.org/Features/Meson/Rust
And a Meson category https://wiki.qemu.org/Category:Meson
Thanks,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-14 6:38 ` Manos Pitsidianakis
@ 2024-06-14 17:50 ` Paolo Bonzini
2024-06-17 8:45 ` Manos Pitsidianakis
0 siblings, 1 reply; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-14 17:50 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Fri, Jun 14, 2024 at 9:04 AM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Thu, 13 Jun 2024 23:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >> I guess there's a balance to be had somewhere on the spectrum between doing
> >> everything against the raw C binding, vs everything against a perfectly
> >> idiomatic Rust API wrapping the C bniding. The latter might be the ideal,
> >> but from a pragmmatic POV I doubt we want the barrier to entry to be that
> >> high.
> >
> >Yes, I agree. I guess we could make things work step by step, even
> >committing something that only focuses on the build system like
> >Manos's work (I'll review it).
> >
> >I can try to look at the basic QOM interface.
> >
> >Manos, can you create a page on the wiki? Something like
> >https://wiki.qemu.org/Features/Meson.
>
>
> Certainly! Just to make sure I understood correctly, you mean a wiki
> page describing how things work and tracking the progress?
>
> I added https://wiki.qemu.org/Features/Meson/Rust
I moved it to https://wiki.qemu.org/Features/Rust/Meson :) and wrote
https://wiki.qemu.org/Features/Rust/QOM. I got to the point where at
least this compiles:
qdev_define_type!(c"test-device", TestDevice);
impl ObjectImpl for TestDevice {}
impl DeviceImpl for TestDevice {}
fn main() {
let d = TestDevice::new();
d.cold_reset();
}
Of course the code makes no sense but it's a start.
One thing that would be very useful is to have an Error
implementation. Looking at what Marc-André did for Error*
(https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@redhat.com/),
his precise implementation relies on his code to go back and forth
between Rust representation, borrowed C object with Rust bindings and
owned C object with Rust bindings. But I think we can at least have
something like this:
// qemu::Error
pub struct Error {
msg: String,
/// Appends the print string of the error to the msg if not None
cause: Option<Box<dyn std::error::Error>>,
location: Option<(String, u32)>
}
impl std::error::Error for Error { ... }
impl Error {
...
fn into_c_error(self) -> *const bindings::Error { ... }
}
// qemu::Result<T>
type Result<T> = Result<T, Error>;
which can be implemented without too much code. This way any "bool
f(..., Error *)" function (example: realize :)) could be implemented
as returning qemu::Result<()>.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-14 17:50 ` Paolo Bonzini
@ 2024-06-17 8:45 ` Manos Pitsidianakis
2024-06-17 11:32 ` Paolo Bonzini
0 siblings, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-17 8:45 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé , Stefan Hajnoczi,
Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé , Zhao Liu, Gustavo Romero,
Pierrick Bouvier
On Fri, 14 Jun 2024 20:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
>On Fri, Jun 14, 2024 at 9:04 AM Manos Pitsidianakis
><manos.pitsidianakis@linaro.org> wrote:
>>
>> On Thu, 13 Jun 2024 23:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >On Thu, Jun 13, 2024 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>> >> I guess there's a balance to be had somewhere on the spectrum between doing
>> >> everything against the raw C binding, vs everything against a perfectly
>> >> idiomatic Rust API wrapping the C bniding. The latter might be the ideal,
>> >> but from a pragmmatic POV I doubt we want the barrier to entry to be that
>> >> high.
>> >
>> >Yes, I agree. I guess we could make things work step by step, even
>> >committing something that only focuses on the build system like
>> >Manos's work (I'll review it).
>> >
>> >I can try to look at the basic QOM interface.
>> >
>> >Manos, can you create a page on the wiki? Something like
>> >https://wiki.qemu.org/Features/Meson.
>>
>>
>> Certainly! Just to make sure I understood correctly, you mean a wiki
>> page describing how things work and tracking the progress?
>>
>> I added https://wiki.qemu.org/Features/Meson/Rust
>
>I moved it to https://wiki.qemu.org/Features/Rust/Meson :) and wrote
>https://wiki.qemu.org/Features/Rust/QOM. I got to the point where at
>least this compiles:
>
>qdev_define_type!(c"test-device", TestDevice);
>impl ObjectImpl for TestDevice {}
>impl DeviceImpl for TestDevice {}
>
>fn main() {
> let d = TestDevice::new();
> d.cold_reset();
>}
>
>Of course the code makes no sense but it's a start.
Let's not rush into making interfaces without the need for them arising
first. It's easy to wander off into bikeshedding territory; case in
point, there has been little discussion on the code of this RFC and much
more focus on hypotheticals.
For what it's worth, in my opinion looking at glib-rs for inspiration is
a bad idea, because that project has to support an immutable public
interface (glib) while we do not.
There's more to discuss about this topic which I am open to continuing
on IRC instead!
-- Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
>
>One thing that would be very useful is to have an Error
>implementation. Looking at what Marc-André did for Error*
>(https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@redhat.com/),
>his precise implementation relies on his code to go back and forth
>between Rust representation, borrowed C object with Rust bindings and
>owned C object with Rust bindings. But I think we can at least have
>something like this:
>
>// qemu::Error
>pub struct Error {
> msg: String,
> /// Appends the print string of the error to the msg if not None
> cause: Option<Box<dyn std::error::Error>>,
> location: Option<(String, u32)>
>}
>
>impl std::error::Error for Error { ... }
>
>impl Error {
> ...
> fn into_c_error(self) -> *const bindings::Error { ... }
>}
>
>// qemu::Result<T>
>type Result<T> = Result<T, Error>;
>
>which can be implemented without too much code. This way any "bool
>f(..., Error *)" function (example: realize :)) could be implemented
>as returning qemu::Result<()>.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 8:45 ` Manos Pitsidianakis
@ 2024-06-17 11:32 ` Paolo Bonzini
2024-06-17 13:54 ` Manos Pitsidianakis
2024-06-18 9:13 ` Daniel P. Berrangé
0 siblings, 2 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-17 11:32 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Daniel P. Berrangé, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
[-- Attachment #1: Type: text/plain, Size: 4428 bytes --]
Il lun 17 giu 2024, 10:59 Manos Pitsidianakis <
manos.pitsidianakis@linaro.org> ha scritto:
> >qdev_define_type!(c"test-device", TestDevice);
> >impl ObjectImpl for TestDevice {}
> >impl DeviceImpl for TestDevice {}
> >
> >fn main() {
> > let d = TestDevice::new();
> > d.cold_reset();
> >}
> >
> >Of course the code makes no sense but it's a start.
>
> Let's not rush into making interfaces without the need for them arising
> first. It's easy to wander off into bikeshedding territory; case in
> point, there has been little discussion on the code of this RFC and much
> more focus on hypotheticals.
>
I see your point but I think it's important to understand the road ahead of
us.
Knowing that we can build and maintain a usable (does not have to be
perfect) interface to QOM is important, and in fact it's already useful for
the pl011 device's chardev. It's also important to play with more advanced
usage of the language to ascertain what features of the language will be
useful; for example, my current implementation uses generic associated
types which are not available on Debian Bookworm—it should be easy to
remove them but if I am wrong that's also a data point, and an important
one.
Don't get me wrong: *for this first device* only, it makes a lot of sense
to have a very C-ish implementation. It lets us sort out the build system
integration, tackle idiomatic bindings one piece at a time, and is easier
to review than Marc-André's approach of building the whole QAPI bindings.
But at the same time, I don't consider a C-ish device better just because
it's written in Rust: as things stand your code has all the disadvantages
of C and all the disadvantages of Rust. What makes it (extremely) valuable
is that your code can lead us along the path towards reaping the advantages
of Rust.
I think we're actually in a great position. We have a PoC from Marc-André,
plus the experience of glib-rs (see below), that shows us that our goal of
idiomatic bindings is doable; and a complementary PoC from you that will
guide us and let us reach it in little steps. The first 90% of the work is
done, now we only have to do the second 90%... :) but then we can open the
floodgates for Rust code in QEMU.
For what it's worth, in my opinion looking at glib-rs for inspiration is
> a bad idea, because that project has to support an immutable public
> interface (glib) while we do not.
>
glib-rs includes support for GObject, which was itself an inspiration for
QOM (with differences, granted).
There are a lot of libraries that we can take inspiration from: in addition
to glib-rs, zbus has an interesting approach to
serialization/deserialization for example that could inform future work on
QAPI. It's not a coincidence that these libraries integrate with more
traditional C code, because we are doing the same. Rust-vmm crates will
also become an interesting topic sooner or later.
There's more to discuss about this topic which I am open to continuing
> on IRC instead!
>
Absolutely, I will try to post code soonish and also review the build
system integration.
Thanks,
Paolo
> -- Manos Pitsidianakis
> Emulation and Virtualization Engineer at Linaro Ltd
>
> >
> >One thing that would be very useful is to have an Error
> >implementation. Looking at what Marc-André did for Error*
> >(
> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@redhat.com/
> ),
> >his precise implementation relies on his code to go back and forth
> >between Rust representation, borrowed C object with Rust bindings and
> >owned C object with Rust bindings. But I think we can at least have
> >something like this:
> >
> >// qemu::Error
> >pub struct Error {
> > msg: String,
> > /// Appends the print string of the error to the msg if not None
> > cause: Option<Box<dyn std::error::Error>>,
> > location: Option<(String, u32)>
> >}
> >
> >impl std::error::Error for Error { ... }
> >
> >impl Error {
> > ...
> > fn into_c_error(self) -> *const bindings::Error { ... }
> >}
> >
> >// qemu::Result<T>
> >type Result<T> = Result<T, Error>;
> >
> >which can be implemented without too much code. This way any "bool
> >f(..., Error *)" function (example: realize :)) could be implemented
> >as returning qemu::Result<()>.
>
>
[-- Attachment #2: Type: text/html, Size: 6334 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 11:32 ` Paolo Bonzini
@ 2024-06-17 13:54 ` Manos Pitsidianakis
2024-06-17 14:32 ` Paolo Bonzini
2024-06-18 9:13 ` Daniel P. Berrangé
1 sibling, 1 reply; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-17 13:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, Daniel P. Berrangé , Stefan Hajnoczi,
Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé , Zhao Liu, Gustavo Romero,
Pierrick Bouvier
On Mon, 17 Jun 2024 14:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>Il lun 17 giu 2024, 10:59 Manos Pitsidianakis <
>manos.pitsidianakis@linaro.org> ha scritto:
>
>> >qdev_define_type!(c"test-device", TestDevice);
>> >impl ObjectImpl for TestDevice {}
>> >impl DeviceImpl for TestDevice {}
>> >
>> >fn main() {
>> > let d = TestDevice::new();
>> > d.cold_reset();
>> >}
>> >
>> >Of course the code makes no sense but it's a start.
>>
>> Let's not rush into making interfaces without the need for them arising
>> first. It's easy to wander off into bikeshedding territory; case in
>> point, there has been little discussion on the code of this RFC and much
>> more focus on hypotheticals.
>>
>
>I see your point but I think it's important to understand the road ahead of
>us.
>
>Knowing that we can build and maintain a usable (does not have to be
>perfect) interface to QOM is important, and in fact it's already useful for
>the pl011 device's chardev. It's also important to play with more advanced
>usage of the language to ascertain what features of the language will be
>useful; for example, my current implementation uses generic associated
>types which are not available on Debian Bookworm—it should be easy to
>remove them but if I am wrong that's also a data point, and an important
>one.
>
>Don't get me wrong: *for this first device* only, it makes a lot of sense
>to have a very C-ish implementation. It lets us sort out the build system
>integration, tackle idiomatic bindings one piece at a time, and is easier
>to review than Marc-André's approach of building the whole QAPI bindings.
>But at the same time, I don't consider a C-ish device better just because
>it's written in Rust: as things stand your code has all the disadvantages
>of C and all the disadvantages of Rust. What makes it (extremely) valuable
>is that your code can lead us along the path towards reaping the advantages
>of Rust.
I respectfully disagree and recommend taking another look at the code.
The device actually performs all logic in non-unsafe methods and is
typed instead of operating on raw integers as fields/state. The C stuff
is the FFI boundary calls which you cannot avoid; they are the same
things you'd wrap under these bindings we're talking about.
QEMU calls the device's FFI callbacks with its pointer and arguments,
the pointer gets dereferenced to the actual Rust type which qemu
guarantees is valid, then the Rust struct's methods are called to handle
each functionality. There is nothing actually unsafe here, assuming
QEMU's invariants and code are correct.
>
>I think we're actually in a great position. We have a PoC from Marc-André,
>plus the experience of glib-rs (see below), that shows us that our goal of
>idiomatic bindings is doable; and a complementary PoC from you that will
>guide us and let us reach it in little steps. The first 90% of the work is
>done, now we only have to do the second 90%... :) but then we can open the
>floodgates for Rust code in QEMU.
>
>For what it's worth, in my opinion looking at glib-rs for inspiration is
>> a bad idea, because that project has to support an immutable public
>> interface (glib) while we do not.
>>
>
>glib-rs includes support for GObject, which was itself an inspiration for
>QOM (with differences, granted).
>
>There are a lot of libraries that we can take inspiration from: in addition
>to glib-rs, zbus has an interesting approach to
>serialization/deserialization for example that could inform future work on
>QAPI. It's not a coincidence that these libraries integrate with more
>traditional C code, because we are doing the same. Rust-vmm crates will
>also become an interesting topic sooner or later.
>
>There's more to discuss about this topic which I am open to continuing
>> on IRC instead!
>>
>
>Absolutely, I will try to post code soonish and also review the build
>system integration.
>
>Thanks,
>
>Paolo
>
>
>> -- Manos Pitsidianakis
>> Emulation and Virtualization Engineer at Linaro Ltd
>>
>> >
>> >One thing that would be very useful is to have an Error
>> >implementation. Looking at what Marc-André did for Error*
>> >(
>> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@redhat.com/
>> ),
>> >his precise implementation relies on his code to go back and forth
>> >between Rust representation, borrowed C object with Rust bindings and
>> >owned C object with Rust bindings. But I think we can at least have
>> >something like this:
>> >
>> >// qemu::Error
>> >pub struct Error {
>> > msg: String,
>> > /// Appends the print string of the error to the msg if not None
>> > cause: Option<Box<dyn std::error::Error>>,
>> > location: Option<(String, u32)>
>> >}
>> >
>> >impl std::error::Error for Error { ... }
>> >
>> >impl Error {
>> > ...
>> > fn into_c_error(self) -> *const bindings::Error { ... }
>> >}
>> >
>> >// qemu::Result<T>
>> >type Result<T> = Result<T, Error>;
>> >
>> >which can be implemented without too much code. This way any "bool
>> >f(..., Error *)" function (example: realize :)) could be implemented
>> >as returning qemu::Result<()>.
>>
>>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 13:54 ` Manos Pitsidianakis
@ 2024-06-17 14:32 ` Paolo Bonzini
2024-06-17 21:04 ` Manos Pitsidianakis
2024-06-17 23:18 ` Pierrick Bouvier
0 siblings, 2 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-17 14:32 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Daniel P. Berrangé, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> I respectfully disagree and recommend taking another look at the code.
>
> The device actually performs all logic in non-unsafe methods and is
> typed instead of operating on raw integers as fields/state. The C stuff
> is the FFI boundary calls which you cannot avoid; they are the same
> things you'd wrap under these bindings we're talking about.
Indeed, but the whole point is that the bindings wrap unsafe code in
such a way that the safety invariants hold. Not doing this, especially
for a device that does not do DMA (so that there are very few ways
that things can go wrong in the first place), runs counter to the
whole philosophy of Rust.
For example, you have:
pub fn realize(&mut self) {
unsafe {
qemu_chr_fe_set_handlers(
addr_of_mut!(self.char_backend),
Some(pl011_can_receive),
Some(pl011_receive),
Some(pl011_event),
None,
addr_of_mut!(*self).cast::<c_void>(),
core::ptr::null_mut(),
true,
);
}
}
where you are implicitly relying on the fact that pl011_can_receive(),
pl011_receive(), pl011_event() are never called from the
MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
mutable references at the same time, one as an argument to the
callbacks:
pub fn read(&mut self, offset: hwaddr, ...
and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
This is not Rust code. It makes no attempt at enforcing the whole
"shared XOR mutable" which is the basis of Rust's reference semantics.
In other words, this is as safe as C code---sure, it can use nice
abstractions for register access, it has "unsafe" added in front of
pointer dereferences, but it is not safe.
Again, I'm not saying it's a bad first step. It's *awesome* if we
treat it as what it is: a guide towards providing safe bindings
between Rust and C (which often implies them being idiomatic). But if
we don't accept this, if there is no plan to make the code safe, it is
a potential huge source of technical debt.
> QEMU calls the device's FFI callbacks with its pointer and arguments,
> the pointer gets dereferenced to the actual Rust type which qemu
> guarantees is valid, then the Rust struct's methods are called to handle
> each functionality. There is nothing actually unsafe here, assuming
> QEMU's invariants and code are correct.
The same can be said of C code, can't it? There is nothing unsafe in C
code, assuming there are no bugs...
Paolo
> >
> >I think we're actually in a great position. We have a PoC from Marc-André,
> >plus the experience of glib-rs (see below), that shows us that our goal of
> >idiomatic bindings is doable; and a complementary PoC from you that will
> >guide us and let us reach it in little steps. The first 90% of the work is
> >done, now we only have to do the second 90%... :) but then we can open the
> >floodgates for Rust code in QEMU.
> >
> >For what it's worth, in my opinion looking at glib-rs for inspiration is
> >> a bad idea, because that project has to support an immutable public
> >> interface (glib) while we do not.
> >>
> >
> >glib-rs includes support for GObject, which was itself an inspiration for
> >QOM (with differences, granted).
> >
> >There are a lot of libraries that we can take inspiration from: in addition
> >to glib-rs, zbus has an interesting approach to
> >serialization/deserialization for example that could inform future work on
> >QAPI. It's not a coincidence that these libraries integrate with more
> >traditional C code, because we are doing the same. Rust-vmm crates will
> >also become an interesting topic sooner or later.
> >
> >There's more to discuss about this topic which I am open to continuing
> >> on IRC instead!
> >>
> >
> >Absolutely, I will try to post code soonish and also review the build
> >system integration.
> >
> >Thanks,
> >
> >Paolo
> >
> >
> >> -- Manos Pitsidianakis
> >> Emulation and Virtualization Engineer at Linaro Ltd
> >>
> >> >
> >> >One thing that would be very useful is to have an Error
> >> >implementation. Looking at what Marc-André did for Error*
> >> >(
> >> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@redhat.com/
> >> ),
> >> >his precise implementation relies on his code to go back and forth
> >> >between Rust representation, borrowed C object with Rust bindings and
> >> >owned C object with Rust bindings. But I think we can at least have
> >> >something like this:
> >> >
> >> >// qemu::Error
> >> >pub struct Error {
> >> > msg: String,
> >> > /// Appends the print string of the error to the msg if not None
> >> > cause: Option<Box<dyn std::error::Error>>,
> >> > location: Option<(String, u32)>
> >> >}
> >> >
> >> >impl std::error::Error for Error { ... }
> >> >
> >> >impl Error {
> >> > ...
> >> > fn into_c_error(self) -> *const bindings::Error { ... }
> >> >}
> >> >
> >> >// qemu::Result<T>
> >> >type Result<T> = Result<T, Error>;
> >> >
> >> >which can be implemented without too much code. This way any "bool
> >> >f(..., Error *)" function (example: realize :)) could be implemented
> >> >as returning qemu::Result<()>.
> >>
> >>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 2/5] rust: add bindgen step as a meson dependency
2024-06-11 10:33 ` [RFC PATCH v2 2/5] rust: add bindgen step as a meson dependency Manos Pitsidianakis
@ 2024-06-17 21:01 ` Paolo Bonzini
0 siblings, 0 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-17 21:01 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Peter Maydell,
Alex Bennée, Daniel P. Berrangé, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier, John Snow,
Cleber Rosa
Just one somewhat larger request, otherwise just a collection of ideas.
On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> diff --git a/rust/meson.build b/rust/meson.build
> new file mode 100644
> index 0000000000..e9660a3045
> --- /dev/null
> +++ b/rust/meson.build
> @@ -0,0 +1,91 @@
> +rust_targets = {}
> +
> +cargo_wrapper = [
> + find_program(meson.global_source_root() / 'scripts/cargo_wrapper.py'),
> + '--config-headers', meson.project_build_root() / 'config-host.h',
> + '--meson-build-root', meson.project_build_root(),
> + '--meson-build-dir', meson.current_build_dir(),
> + '--meson-source-dir', meson.current_source_dir(),
> +]
>
> +
> +# TODO: verify rust_target_triple if given as an option
> +if rust_target_triple == ''
> + if not supported_oses.contains(host_os)
> + message()
> + error('QEMU does not support `' + host_os +'` as a Rust platform.')
> + elif not supported_cpus.contains(host_arch)
> + message()
> + error('QEMU does not support `' + host_arch +'` as a Rust architecture.')
> + endif
> + rust_target_triple = host_arch + rust_supported_oses[host_os]
> + if host_os == 'windows' and host_arch == 'aarch64'
> + rust_target_triple += 'llvm'
> + endif
> +endif
> +
> +if get_option('optimization') in ['0', '1', 'g']
> + rs_build_type = 'debug'
> +else
> + rs_build_type = 'release'
> +endif
> +
> +rust_hw_target_list = {}
> +
> +foreach rust_hw_target, rust_hws: rust_hw_target_list
> + foreach rust_hw_dev: rust_hws
> + output = meson.current_build_dir() / rust_target_triple / rs_build_type / rust_hw_dev['output']
> + crate_metadata = {
> + 'name': rust_hw_dev['name'],
> + 'output': [rust_hw_dev['output']],
> + 'output-path': output,
> + 'command': [cargo_wrapper,
> + '--crate-dir',
> + meson.current_source_dir() / rust_hw_dev['dirname'],
> + '--profile',
> + rs_build_type,
> + '--target-triple',
> + rust_target_triple,
> + '--outdir',
> + '@OUTDIR@',
> + 'build-lib'
Probably needs to add config-devices.h as well to --config-headers? I
think that we can have just one crate that is built per target,
instead of many small crates like your rust_pl011_cargo. And then that
crate is built many times with a rust/src/ hierarchy that resembles
the meson.build files we use for C.
rust/src/mod.rs:
pub mod hw;
rust/src/hw/mod.rs:
pub mod char;
rust/src/hw/char/mod.rs:
#[cfg(feature = "CONFIG_PL011")] pub mod pl011;
(perhaps just src/? And we could even move all sources down one level
to src/ for QEMU as well? But that can be done later, for now it can
be rust/src/). This basically gives Kconfig integration for free if it
works, so I'd ask you to try doing this for the next version?
Also, sooner or later we'll have to tackle building a set of common
dependencies (similar to common_ss), and then bringing those as a
dependency to the build of the per-target crates. Even if we don't get
to the point of building devices once for all targets, I'd rather at
least avoid having to build a dozen times the common deps of
procedural macros.
This is not trivial, but should not be hard either, by using build.rs
(cargo::rustc-link-search) to add the -Ldependency=/path/to/rlibs
option to the per-target rustc invocations. This may also require
grabbing the compilation log via "cargo build
--message-format=json-render-diagnostics", but that's not hard to do
in cargo_wrapper.py. And unlike the previous request about Kconfig, it
can be done after the first merge.
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "qemu-io.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/sysbus.h"
> +#include "exec/memory.h"
> +#include "chardev/char-fe.h"
> +#include "hw/clock.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "hw/irq.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "chardev/char-serial.h"
Please check for any headers included indirectly and whether it's
worth including them here (e.g. it seems that qapi/error.h,
qom/object.h, hw/qdev-core.h should be there).
> diff --git a/scripts/cargo_wrapper.py b/scripts/cargo_wrapper.py
> index d338effdaa..a36a4fc86d 100644
> --- a/scripts/cargo_wrapper.py
> +++ b/scripts/cargo_wrapper.py
> @@ -94,6 +94,8 @@ def get_cargo_rustc(args: argparse.Namespace) -> tuple[Dict[str, Any], List[str]
>
> env = os.environ
> env["CARGO_ENCODED_RUSTFLAGS"] = cfg
> + env["MESON_BUILD_DIR"] = str(target_dir)
> + env["MESON_BUILD_ROOT"] = str(args.meson_build_root)
>
> return (env, cargo_cmd)
>
> @@ -164,6 +166,14 @@ def main() -> None:
> required=True,
> )
> parser.add_argument(
> + "--meson-build-root",
> + metavar="BUILD_ROOT",
> + help="meson.project_build_root()",
> + type=pathlib.Path,
> + dest="meson_build_root",
> + required=True,
> + )
> + parser.add_argument(
> "--meson-source-dir",
> metavar="SOURCE_DIR",
> help="meson.current_source_dir()",
Probably all of cargo_wrapper.py can be moved to this patch.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 14:32 ` Paolo Bonzini
@ 2024-06-17 21:04 ` Manos Pitsidianakis
2024-06-17 23:33 ` Pierrick Bouvier
2024-06-18 6:00 ` Paolo Bonzini
2024-06-17 23:18 ` Pierrick Bouvier
1 sibling, 2 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-17 21:04 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé , Stefan Hajnoczi,
Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé , Zhao Liu, Gustavo Romero,
Pierrick Bouvier, rowan.hart
On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
><manos.pitsidianakis@linaro.org> wrote:
>> I respectfully disagree and recommend taking another look at the code.
>>
>> The device actually performs all logic in non-unsafe methods and is
>> typed instead of operating on raw integers as fields/state. The C stuff
>> is the FFI boundary calls which you cannot avoid; they are the same
>> things you'd wrap under these bindings we're talking about.
>
>Indeed, but the whole point is that the bindings wrap unsafe code in
>such a way that the safety invariants hold. Not doing this, especially
>for a device that does not do DMA (so that there are very few ways
>that things can go wrong in the first place), runs counter to the
>whole philosophy of Rust.
>
>For example, you have:
>
> pub fn realize(&mut self) {
> unsafe {
> qemu_chr_fe_set_handlers(
> addr_of_mut!(self.char_backend),
> Some(pl011_can_receive),
> Some(pl011_receive),
> Some(pl011_event),
> None,
> addr_of_mut!(*self).cast::<c_void>(),
> core::ptr::null_mut(),
> true,
> );
> }
> }
>
>where you are implicitly relying on the fact that pl011_can_receive(),
>pl011_receive(), pl011_event() are never called from the
>MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
>mutable references at the same time, one as an argument to the
>callbacks:
>
> pub fn read(&mut self, offset: hwaddr, ...
>
>and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
>
>This is not Rust code. It makes no attempt at enforcing the whole
>"shared XOR mutable" which is the basis of Rust's reference semantics.
>In other words, this is as safe as C code---sure, it can use nice
>abstractions for register access, it has "unsafe" added in front of
>pointer dereferences, but it is not safe.
>
>Again, I'm not saying it's a bad first step. It's *awesome* if we
>treat it as what it is: a guide towards providing safe bindings
>between Rust and C (which often implies them being idiomatic). But if
>we don't accept this, if there is no plan to make the code safe, it is
>a potential huge source of technical debt.
>
>> QEMU calls the device's FFI callbacks with its pointer and arguments,
>> the pointer gets dereferenced to the actual Rust type which qemu
>> guarantees is valid, then the Rust struct's methods are called to handle
>> each functionality. There is nothing actually unsafe here, assuming
>> QEMU's invariants and code are correct.
>
>The same can be said of C code, can't it? There is nothing unsafe in C
>code, assuming there are no bugs...
Paolo, first please tone down your condescending tone, it's incredibly
offensive. I'm honestly certain this is not on purpose otherwise I'd not
engage at all.
Secondly, are you implying that these callbacks are not operated under
the BQL? I'm not seeing the C UART devices using mutexes. If they are
not running under the BQL, then gladly we add mutexes, big deal. Just
say this directly instead of writing all these amounts of text. If it's
true I'd just accept it and move on with a new iteration. Isn't that the
point of code review? It is really that simple. Why not do this right
away? I'm frankly puzzled.
Finally, this is Rust code. You cannot turn off the type system, you
cannot turn off the borrow checker, you can only go around creating new
types and references out of raw memory addresses and tell the compiler
'trust me on this'. Ignoring that misses the entire point of Rust. The
statement 'this is not Rust code but as good as C' is dishonest and
misguided. Check for example the source code of the nix crate, which
exposes libc and various POSIX/*nix APIs. Is it the same as C and not
Rust code?
If you have specific scenarios in mind where such things exist in the
code and end up doing invalid behavior please be kind and write them
down explicitly and demonstrate them on code review. This approach of
'yes but no' is not constructive because it is not addressing any
specific problems directly. Instead it comes out as vague dismissive FUD
and I'm sure that is not what you or anyone else wants.
Please take some time to understand my POV here, it'd help both of us
immensely.
Sincerely thank you in advance,
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 14:32 ` Paolo Bonzini
2024-06-17 21:04 ` Manos Pitsidianakis
@ 2024-06-17 23:18 ` Pierrick Bouvier
1 sibling, 0 replies; 54+ messages in thread
From: Pierrick Bouvier @ 2024-06-17 23:18 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis
Cc: qemu-devel, Daniel P. Berrangé, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero
On 6/17/24 07:32, Paolo Bonzini wrote:
> On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
>> I respectfully disagree and recommend taking another look at the code.
>>
>> The device actually performs all logic in non-unsafe methods and is
>> typed instead of operating on raw integers as fields/state. The C stuff
>> is the FFI boundary calls which you cannot avoid; they are the same
>> things you'd wrap under these bindings we're talking about.
>
> Indeed, but the whole point is that the bindings wrap unsafe code in
> such a way that the safety invariants hold. Not doing this, especially
> for a device that does not do DMA (so that there are very few ways
> that things can go wrong in the first place), runs counter to the
> whole philosophy of Rust.
>
You are raising an interesting point, and should be a central discussion
when designing the future Rust API for this subsystem.
It may not be intuitive to people that even a code without any unsafe
section could still call code in a sequence that will violate some
invariants, and break Rust rules.
IMHO, this is one of the big challenge with the Rust/C interfacing,
including for Linux.
But it's *not yet* the goal of this series. First, we should see how to
build one device (I would even like to see a second, to factorize all
the boilerplate), and then, focus on which interface we want to have to
make it really better than the C version.
> For example, you have:
>
> pub fn realize(&mut self) {
> unsafe {
> qemu_chr_fe_set_handlers(
> addr_of_mut!(self.char_backend),
> Some(pl011_can_receive),
> Some(pl011_receive),
> Some(pl011_event),
> None,
> addr_of_mut!(*self).cast::<c_void>(),
> core::ptr::null_mut(),
> true,
> );
> }
> }
>
> where you are implicitly relying on the fact that pl011_can_receive(),
> pl011_receive(), pl011_event() are never called from the
> MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
> mutable references at the same time, one as an argument to the
> callbacks:
>
> pub fn read(&mut self, offset: hwaddr, ...
>
> and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
>
> This is not Rust code. It makes no attempt at enforcing the whole
> "shared XOR mutable" which is the basis of Rust's reference semantics.
> In other words, this is as safe as C code---sure, it can use nice
> abstractions for register access, it has "unsafe" added in front of
> pointer dereferences, but it is not safe.
>
I think that Manos did a great amount of work to reduce the use of
unsafe code. Basically, he wrote the device as Rusty as possible, while
still using the QEMU C API part that is inevitable today.
In more, given the current design, yes some race conditions are possible
(depends on how QEMU calls callbacks installed), but a whole class of
problems is still eliminated.
From the start of this series, it was precised that focus was on build
system, and it seemed to me that we shifted on the hot debate of "How to
interface with C code?".
> Again, I'm not saying it's a bad first step. It's *awesome* if we
> treat it as what it is: a guide towards providing safe bindings
> between Rust and C (which often implies them being idiomatic). But if
> we don't accept this, if there is no plan to make the code safe, it is
> a potential huge source of technical debt.
>
I agree, it should be the next direction after a first device was
written: How to remove almost all usage of unsafe code and maintain good
invariants in the API?
While talking about idiomatic, Rust tends to favor message passing to
memory share when it comes to concurrency (and IMHO, it's the right
thing). And it's definitely now how QEMU is architected at this time.
With extra locking, we should be able to have a first correct interface
that offers strong guarantees, and we can then work on making it faster
with concurrency.
>> QEMU calls the device's FFI callbacks with its pointer and arguments,
>> the pointer gets dereferenced to the actual Rust type which qemu
>> guarantees is valid, then the Rust struct's methods are called to handle
>> each functionality. There is nothing actually unsafe here, assuming
>> QEMU's invariants and code are correct.
>
> The same can be said of C code, can't it? There is nothing unsafe in C
> code, assuming there are no bugs...
>
Not the same, you still get other compile/runtime guarantees:
- strong typed enums, so no case is forgotten
- good error handling
- safe type conversions
- bound check of fifo access
The only open issue by calling unsafe code in this context is related to
(potential) concurrency. If a first step to have a good Rust API is to
run everything under a lock, there is good chance you already started to
design the device in the right way to support real concurrency later, so
it's still useful.
Pierrick
> Paolo
>
>>>
>>> I think we're actually in a great position. We have a PoC from Marc-André,
>>> plus the experience of glib-rs (see below), that shows us that our goal of
>>> idiomatic bindings is doable; and a complementary PoC from you that will
>>> guide us and let us reach it in little steps. The first 90% of the work is
>>> done, now we only have to do the second 90%... :) but then we can open the
>>> floodgates for Rust code in QEMU.
>>>
>>> For what it's worth, in my opinion looking at glib-rs for inspiration is
>>>> a bad idea, because that project has to support an immutable public
>>>> interface (glib) while we do not.
>>>>
>>>
>>> glib-rs includes support for GObject, which was itself an inspiration for
>>> QOM (with differences, granted).
>>>
>>> There are a lot of libraries that we can take inspiration from: in addition
>>> to glib-rs, zbus has an interesting approach to
>>> serialization/deserialization for example that could inform future work on
>>> QAPI. It's not a coincidence that these libraries integrate with more
>>> traditional C code, because we are doing the same. Rust-vmm crates will
>>> also become an interesting topic sooner or later.
>>>
>>> There's more to discuss about this topic which I am open to continuing
>>>> on IRC instead!
>>>>
>>>
>>> Absolutely, I will try to post code soonish and also review the build
>>> system integration.
>>>
>>> Thanks,
>>>
>>> Paolo
>>>
>>>
>>>> -- Manos Pitsidianakis
>>>> Emulation and Virtualization Engineer at Linaro Ltd
>>>>
>>>>>
>>>>> One thing that would be very useful is to have an Error
>>>>> implementation. Looking at what Marc-André did for Error*
>>>>> (
>>>> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-13-marcandre.lureau@redhat.com/
>>>> ),
>>>>> his precise implementation relies on his code to go back and forth
>>>>> between Rust representation, borrowed C object with Rust bindings and
>>>>> owned C object with Rust bindings. But I think we can at least have
>>>>> something like this:
>>>>>
>>>>> // qemu::Error
>>>>> pub struct Error {
>>>>> msg: String,
>>>>> /// Appends the print string of the error to the msg if not None
>>>>> cause: Option<Box<dyn std::error::Error>>,
>>>>> location: Option<(String, u32)>
>>>>> }
>>>>>
>>>>> impl std::error::Error for Error { ... }
>>>>>
>>>>> impl Error {
>>>>> ...
>>>>> fn into_c_error(self) -> *const bindings::Error { ... }
>>>>> }
>>>>>
>>>>> // qemu::Result<T>
>>>>> type Result<T> = Result<T, Error>;
>>>>>
>>>>> which can be implemented without too much code. This way any "bool
>>>>> f(..., Error *)" function (example: realize :)) could be implemented
>>>>> as returning qemu::Result<()>.
>>>>
>>>>
>>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 21:04 ` Manos Pitsidianakis
@ 2024-06-17 23:33 ` Pierrick Bouvier
2024-06-18 6:00 ` Paolo Bonzini
2024-06-18 6:00 ` Paolo Bonzini
1 sibling, 1 reply; 54+ messages in thread
From: Pierrick Bouvier @ 2024-06-17 23:33 UTC (permalink / raw)
To: Manos Pitsidianakis, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Stefan Hajnoczi,
Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-Andr é Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daud é, Zhao Liu, Gustavo Romero,
rowan.hart
On 6/17/24 14:04, Manos Pitsidianakis wrote:
> On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
>> <manos.pitsidianakis@linaro.org> wrote:
>>> I respectfully disagree and recommend taking another look at the code.
>>>
>>> The device actually performs all logic in non-unsafe methods and is
>>> typed instead of operating on raw integers as fields/state. The C stuff
>>> is the FFI boundary calls which you cannot avoid; they are the same
>>> things you'd wrap under these bindings we're talking about.
>>
>> Indeed, but the whole point is that the bindings wrap unsafe code in
>> such a way that the safety invariants hold. Not doing this, especially
>> for a device that does not do DMA (so that there are very few ways
>> that things can go wrong in the first place), runs counter to the
>> whole philosophy of Rust.
>>
>> For example, you have:
>>
>> pub fn realize(&mut self) {
>> unsafe {
>> qemu_chr_fe_set_handlers(
>> addr_of_mut!(self.char_backend),
>> Some(pl011_can_receive),
>> Some(pl011_receive),
>> Some(pl011_event),
>> None,
>> addr_of_mut!(*self).cast::<c_void>(),
>> core::ptr::null_mut(),
>> true,
>> );
>> }
>> }
>>
>> where you are implicitly relying on the fact that pl011_can_receive(),
>> pl011_receive(), pl011_event() are never called from the
>> MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
>> mutable references at the same time, one as an argument to the
>> callbacks:
>>
>> pub fn read(&mut self, offset: hwaddr, ...
>>
>> and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
>>
>> This is not Rust code. It makes no attempt at enforcing the whole
>> "shared XOR mutable" which is the basis of Rust's reference semantics.
>> In other words, this is as safe as C code---sure, it can use nice
>> abstractions for register access, it has "unsafe" added in front of
>> pointer dereferences, but it is not safe.
>>
>> Again, I'm not saying it's a bad first step. It's *awesome* if we
>> treat it as what it is: a guide towards providing safe bindings
>> between Rust and C (which often implies them being idiomatic). But if
>> we don't accept this, if there is no plan to make the code safe, it is
>> a potential huge source of technical debt.
>>
>>> QEMU calls the device's FFI callbacks with its pointer and arguments,
>>> the pointer gets dereferenced to the actual Rust type which qemu
>>> guarantees is valid, then the Rust struct's methods are called to handle
>>> each functionality. There is nothing actually unsafe here, assuming
>>> QEMU's invariants and code are correct.
>>
>> The same can be said of C code, can't it? There is nothing unsafe in C
>> code, assuming there are no bugs...
>
> Paolo, first please tone down your condescending tone, it's incredibly
> offensive. I'm honestly certain this is not on purpose otherwise I'd not
> engage at all.
The best compliment you had was "I'm not saying it's a bad first step",
and I would say this differently: It's a great first step!
We should have a first version where we stick to the C API, with all the
Rust code being as Rusty as possible: benefit from typed enums, error
handling, bounds checking and other nice things.
It's useless to iterate/debate for two years on the list before landing
something upstream. We can start with this, have one or two devices that
use this build system, and then focus on designing a good interface for
this.
>
> Secondly, are you implying that these callbacks are not operated under
> the BQL? I'm not seeing the C UART devices using mutexes. If they are
> not running under the BQL, then gladly we add mutexes, big deal. Just
> say this directly instead of writing all these amounts of text. If it's
> true I'd just accept it and move on with a new iteration. Isn't that the
> point of code review? It is really that simple. Why not do this right
> away? I'm frankly puzzled.
>
As I mentioned in my previous answer, this device already makes a good
progress: it eliminates a whole class of memory errors, and the only
issue brought by unsafe code is concurrency issues. And this should be
our focus once we get the build infrastructure done!
> Finally, this is Rust code. You cannot turn off the type system, you
> cannot turn off the borrow checker, you can only go around creating new
> types and references out of raw memory addresses and tell the compiler
> 'trust me on this'. Ignoring that misses the entire point of Rust. The
> statement 'this is not Rust code but as good as C' is dishonest and
> misguided. Check for example the source code of the nix crate, which
> exposes libc and various POSIX/*nix APIs. Is it the same as C and not
> Rust code?
>
> If you have specific scenarios in mind where such things exist in the
> code and end up doing invalid behavior please be kind and write them
> down explicitly and demonstrate them on code review. This approach of
> 'yes but no' is not constructive because it is not addressing any
> specific problems directly. Instead it comes out as vague dismissive FUD
> and I'm sure that is not what you or anyone else wants.
>
> Please take some time to understand my POV here, it'd help both of us
> immensely.
>
> Sincerely thank you in advance,
> Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 23:33 ` Pierrick Bouvier
@ 2024-06-18 6:00 ` Paolo Bonzini
0 siblings, 0 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-18 6:00 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: Manos Pitsidianakis, qemu-devel, Daniel P. Berrangé,
Stefan Hajnoczi, Mads Ynddal, Peter Maydell, Alex Benné e,
Marc-Andr é Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daud é, Zhao Liu, Gustavo Romero,
rowan.hart
On Tue, Jun 18, 2024 at 1:33 AM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 6/17/24 14:04, Manos Pitsidianakis wrote:
> > On Mon, 17 Jun 2024 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> On Mon, Jun 17, 2024 at 4:04 PM Manos Pitsidianakis
> >> <manos.pitsidianakis@linaro.org> wrote:
> >>> I respectfully disagree and recommend taking another look at the code.
> >>>
> >>> The device actually performs all logic in non-unsafe methods and is
> >>> typed instead of operating on raw integers as fields/state. The C stuff
> >>> is the FFI boundary calls which you cannot avoid; they are the same
> >>> things you'd wrap under these bindings we're talking about.
> >>
> >> Indeed, but the whole point is that the bindings wrap unsafe code in
> >> such a way that the safety invariants hold. Not doing this, especially
> >> for a device that does not do DMA (so that there are very few ways
> >> that things can go wrong in the first place), runs counter to the
> >> whole philosophy of Rust.
> >>
> >> For example, you have:
> >>
> >> pub fn realize(&mut self) {
> >> unsafe {
> >> qemu_chr_fe_set_handlers(
> >> addr_of_mut!(self.char_backend),
> >> Some(pl011_can_receive),
> >> Some(pl011_receive),
> >> Some(pl011_event),
> >> None,
> >> addr_of_mut!(*self).cast::<c_void>(),
> >> core::ptr::null_mut(),
> >> true,
> >> );
> >> }
> >> }
> >>
> >> where you are implicitly relying on the fact that pl011_can_receive(),
> >> pl011_receive(), pl011_event() are never called from the
> >> MemoryRegionOps read() and write() callbacks. Otherwise you'd have two
> >> mutable references at the same time, one as an argument to the
> >> callbacks:
> >>
> >> pub fn read(&mut self, offset: hwaddr, ...
> >>
> >> and one from e.g. "state.as_mut().put_fifo()" in pl011_receive().
> >>
> >> This is not Rust code. It makes no attempt at enforcing the whole
> >> "shared XOR mutable" which is the basis of Rust's reference semantics.
> >> In other words, this is as safe as C code---sure, it can use nice
> >> abstractions for register access, it has "unsafe" added in front of
> >> pointer dereferences, but it is not safe.
> >>
> >> Again, I'm not saying it's a bad first step. It's *awesome* if we
> >> treat it as what it is: a guide towards providing safe bindings
> >> between Rust and C (which often implies them being idiomatic). But if
> >> we don't accept this, if there is no plan to make the code safe, it is
> >> a potential huge source of technical debt.
> >>
> >>> QEMU calls the device's FFI callbacks with its pointer and arguments,
> >>> the pointer gets dereferenced to the actual Rust type which qemu
> >>> guarantees is valid, then the Rust struct's methods are called to handle
> >>> each functionality. There is nothing actually unsafe here, assuming
> >>> QEMU's invariants and code are correct.
> >>
> >> The same can be said of C code, can't it? There is nothing unsafe in C
> >> code, assuming there are no bugs...
> >
> > Paolo, first please tone down your condescending tone, it's incredibly
> > offensive. I'm honestly certain this is not on purpose otherwise I'd not
> > engage at all.
>
> The best compliment you had was "I'm not saying it's a bad first step",
> and I would say this differently: It's a great first step!
Don't quote me out of context; I said It's an "awesome" first step,
though I qualified that we should treat it as "a guide towards
providing safe bindings between Rust and C". That is, as long as we
agree that it is not production quality code. Which it doesn't have to
be!
> We should have a first version where we stick to the C API, with all the
> Rust code being as Rusty as possible: benefit from typed enums, error
> handling, bounds checking and other nice things.
>
> It's useless to iterate/debate for two years on the list before landing
> something upstream. We can start with this, have one or two devices that
> use this build system, and then focus on designing a good interface for
> this.
I never said that I want perfection before landing upstream. I want _a path_.
When I read "there was consensus in the community call that we won't
be writing Rust APIs for internal C QEMU interfaces; or at least,
that's not the goal"[1], then sorry but I think that it's better to
stick with C.
On the other hand, if there is a path towards proper, maintainable
Rust, then I am even okay even with committing something that
technically contains undefined behavior.
[1] https://lore.kernel.org/qemu-devel/ez270.x96k6aeu0rpw@linaro.org/
> As I mentioned in my previous answer, this device already makes a good
> progress: it eliminates a whole class of memory errors, and the only
> issue brought by unsafe code is concurrency issues. And this should be
> our focus once we get the build infrastructure done!
Let's not exaggerate the benefits: no pointers were converted to RAII
(Box<> or Rc<>) in the course of writing the Rust code; so there are
no memory errors that can be eliminated by the rewrite. In fact, new
memory errors can also be introduced, because safe Rust has stricter
aliasing rules than C.
This is not a problem! It's just that we need to be realistic to
understand where to focus next and why. To build our path.
(Also, a small correction so that we're on the same page on the fix:
it's reentrancy, not concurrency).
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 21:04 ` Manos Pitsidianakis
2024-06-17 23:33 ` Pierrick Bouvier
@ 2024-06-18 6:00 ` Paolo Bonzini
1 sibling, 0 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-18 6:00 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Daniel P. Berrangé, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier, rowan.hart
Il lun 17 giu 2024, 23:45 Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> ha scritto:
> Secondly, are you implying that these callbacks are not operated under
> the BQL?
No, I'm implying that if you had the following nested calls:
unsafe read callback receives the opaque point
-> cast to &mut to call safe read callback
-> chardev function accessing the opaque value
-> unsafe chardev callback receives the opaque pointer
-> cast to & or &mut to call safe chardev callback
Then you would violate the Rust invariant that there can only be one
active reference at a single time if it is &mut. The only exception is
that you can create an &mut UnsafeCell<T> for an active
&UnsafeCell<T>.
It doesn't matter if this cannot happen because of invariants in the
QEMU code. If you are writing safe Rust, every cast to &mut requires
very strong justification. The way you do it instead is to use
RefCell, and borrow_mut() instead of casting to &mut. This way, at
least, the invariant is checked at runtime.
And in fact this _can_ happen. I hadn't checked until now because I
was mostly worried about the conceptual problem, not any specific way
undefined behavior can happen. But here it is:
PL011State::read takes &mut
-> qemu_chr_fe_accept_input
-> mux_chr_accept_input
-> pl011_can_receive
-> creates & which is undefined behavior
You probably can write a 20 lines test case in pure Rust that miri
complains about.
Should we make it a requirement for v3, that all callback methods in
PL011State (read, write, can_receive, receive, event) take a &self and
achieve interior mutability through RefCell? Probably not, a comment
is enough even though technically this is _not_ valid Rust. But it is
a high priority task after the initial commit.
> Just say this directly instead of writing all these amounts of text
I said in the first review that the state should be behind a RefCell.
https://lore.kernel.org/qemu-devel/CABgObfY8BS0yCw2CxgDQTBA4np9BZgGJF3N=t6eoBcdACAE=NA@mail.gmail.com/
> Finally, this is Rust code. You cannot turn off the type system, you
> cannot turn off the borrow checker, you can only go around creating new
> types and references out of raw memory addresses and tell the compiler
> 'trust me on this'
I am sorry if this sounds condescending. But this is _exactly_ what
I'm complaining about: that there is too much trust placed in the
programmer.
Converting from * to & does not turn off the borrow checker, but still
you cannot trust anymore what the borrow checker says; and that's why
you do it inside an abstraction, not in each and every callback of
each and every device. This is what Marc-André did for QAPI; and he
probably erred in the other direction for a PoC, but we _must_ agree
that something as complete as his work is the direction that we _have_
to take.
Again: I am sorry that you feel this way about my remark, because I
thought I had only praised your work. I didn't think I was being
condescending or dismissing. But I am worried that without the proper
abstractions this is going to be a technical debt magnet with only
marginal benefits over C.
And frankly I am saying this from experience. Check out CVE-2019-18960
which was an out of bounds access in Firecracker caused by not using
proper abstractions for DMA. And that's in a 100% Rust code base.
Since we're starting from and interfacing with C it's only going to be
worse; skimping on abstractions is simply something that we cannot
afford.
It's also the way Linux is introducing Rust in the code base. Whenever
something needs access to C functionality they introduce bindings to
it. It's slower, but it's sustainable.
> Ignoring that misses the entire point of Rust. The
> statement 'this is not Rust code but as good as C' is dishonest and
> misguided. Check for example the source code of the nix crate, which
> exposes libc and various POSIX/*nix APIs. Is it the same as C and not
> Rust code?
That's exactly my point. Libc provides mostly unsafe functions, which
is on the level of what bindgen generates. Other crates on top provide
*safe abstractions* such as nix's Dir
(https://docs.rs/nix/0.29.0/nix/dir/struct.Dir.html). If possible,
leaf crates use safe Rust (nix), and avoid unsafe (libc) as much as
possible.
Instead you're using the unsafe functions in the device itself. It's
missing an equivalent of nix.
> If you have specific scenarios in mind where such things exist in the
> code and end up doing invalid behavior please be kind and write them
> down explicitly and demonstrate them on code review.
It doesn't matter whether they exist or not. The point of Rust is that
the type system ensures that these invalid behaviors are caught either
at compile time or (with RefCell) at run time. As things stand, your
code cannot catch them and the language provides a false sense of
security.
On the other hand, I want to be clear agin that this is not a problem
at all—but only as long as we agree that it's not the desired final
state
> This approach of
> 'yes but no' is not constructive because it is not addressing any
> specific problems directly. Instead it comes out as vague dismissive FUD
> and I'm sure that is not what you or anyone else wants.
>
> Please take some time to understand my POV here, it'd help both of us
> immensely.
I can ask the same though. Please take the time to understand that I
am not being dismissive! My position is exactly "yes but no". Yes,
this is a great way to introduce Rust in the code base. No, this is
not a sustainable way to mix Rust and C—but I am willing to help.
Paolo
>
> Sincerely thank you in advance,
> Manos
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-17 11:32 ` Paolo Bonzini
2024-06-17 13:54 ` Manos Pitsidianakis
@ 2024-06-18 9:13 ` Daniel P. Berrangé
2024-06-18 9:29 ` Paolo Bonzini
1 sibling, 1 reply; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-18 9:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Mon, Jun 17, 2024 at 01:32:54PM +0200, Paolo Bonzini wrote:
> Il lun 17 giu 2024, 10:59 Manos Pitsidianakis <
> manos.pitsidianakis@linaro.org> ha scritto:
>
> > >qdev_define_type!(c"test-device", TestDevice);
> > >impl ObjectImpl for TestDevice {}
> > >impl DeviceImpl for TestDevice {}
> > >
> > >fn main() {
> > > let d = TestDevice::new();
> > > d.cold_reset();
> > >}
> > >
> > >Of course the code makes no sense but it's a start.
> >
> > Let's not rush into making interfaces without the need for them arising
> > first. It's easy to wander off into bikeshedding territory; case in
> > point, there has been little discussion on the code of this RFC and much
> > more focus on hypotheticals.
> >
>
> I see your point but I think it's important to understand the road ahead of
> us.
>
> Knowing that we can build and maintain a usable (does not have to be
> perfect) interface to QOM is important, and in fact it's already useful for
> the pl011 device's chardev. It's also important to play with more advanced
> usage of the language to ascertain what features of the language will be
> useful; for example, my current implementation uses generic associated
> types which are not available on Debian Bookworm—it should be easy to
> remove them but if I am wrong that's also a data point, and an important
> one.
>
> Don't get me wrong: *for this first device* only, it makes a lot of sense
> to have a very C-ish implementation. It lets us sort out the build system
> integration, tackle idiomatic bindings one piece at a time, and is easier
> to review than Marc-André's approach of building the whole QAPI bindings.
> But at the same time, I don't consider a C-ish device better just because
> it's written in Rust: as things stand your code has all the disadvantages
> of C and all the disadvantages of Rust. What makes it (extremely) valuable
> is that your code can lead us along the path towards reaping the advantages
> of Rust.
I wonder if starting with a device implementation is perhaps the
wrong idea, in terms of a practical yet simple first step.
As devices go, the pl011 device is simple, but compared to other
QOM impls in QEMU, devices are still relatively complex things,
especially if we want to write against safe abstraction.
How about we go simpler still, and focus on one of the object
backends. For example, the RNG backend interface is practically
the most trivial QOM impl we can do in QEMU. It has one virtual
method that needs to be implemented, which is passed a callback
to receive entropy, and one native method to call to indicate
completion.
Providing a safe Rust abstraction for implementing an RNG
backend looks like a much quicker proposition that a safe
abstraction for implementing a device. The various RNG impls
have a few places where they touch other QEMU code (rng-builtin
uses qemu_bh, rng-egd lightly touches chardev APIs, rng-random
touches main loop FD handlers). Each of those things though, are
small & useful API problems to look it solving.
If we did this I think we would not have to give a "free pass"
for a hackish C-like first Rust impl. We would have something
designed well from day 1, showing small, but tangible benefits,
with a path to incrementally broadening the effort.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-18 9:13 ` Daniel P. Berrangé
@ 2024-06-18 9:29 ` Paolo Bonzini
2024-06-18 9:49 ` Peter Maydell
0 siblings, 1 reply; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-18 9:29 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Manos Pitsidianakis, qemu-devel, Stefan Hajnoczi, Mads Ynddal,
Peter Maydell, Alex Benné e, Marc-André Lureau,
Thomas Huth, Markus Armbruster, Philippe Mathieu-Daudé,
Zhao Liu, Gustavo Romero, Pierrick Bouvier
On Tue, Jun 18, 2024 at 11:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> I wonder if starting with a device implementation is perhaps the
> wrong idea, in terms of a practical yet simple first step.
>
> As devices go, the pl011 device is simple, but compared to other
> QOM impls in QEMU, devices are still relatively complex things,
> especially if we want to write against safe abstraction.
It's true, but I think _some_ complexity provides a better guide as to
what are the next step.
I think it's clear that they are, not in this order:
* calling QOM methods (Chardev)
* implementing QOM objects
* implementing QOM devices
** qdev properties
** MemoryRegion callbacks
* implementing Chardev callbacks
* general technique for bindings for C structs (Error, QAPI)
> If we did this I think we would not have to give a "free pass"
> for a hackish C-like first Rust impl. We would have something
> designed well from day 1, showing small, but tangible benefits,
> with a path to incrementally broadening the effort.
I don't think it's that easy to have something self contained for a
single submission. Reviewing the build system is a completely
different proposition than reviewing generic-heavy QOM bindings.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-18 9:29 ` Paolo Bonzini
@ 2024-06-18 9:49 ` Peter Maydell
0 siblings, 0 replies; 54+ messages in thread
From: Peter Maydell @ 2024-06-18 9:49 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniel P. Berrangé, Manos Pitsidianakis, qemu-devel,
Stefan Hajnoczi, Mads Ynddal, Alex Benné e,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé, Zhao Liu, Gustavo Romero,
Pierrick Bouvier
On Tue, 18 Jun 2024 at 10:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Tue, Jun 18, 2024 at 11:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > I wonder if starting with a device implementation is perhaps the
> > wrong idea, in terms of a practical yet simple first step.
> >
> > As devices go, the pl011 device is simple, but compared to other
> > QOM impls in QEMU, devices are still relatively complex things,
> > especially if we want to write against safe abstraction.
>
> It's true, but I think _some_ complexity provides a better guide as to
> what are the next step.
>
> I think it's clear that they are, not in this order:
> * calling QOM methods (Chardev)
> * implementing QOM objects
> * implementing QOM devices
> ** qdev properties
> ** MemoryRegion callbacks
> * implementing Chardev callbacks
> * general technique for bindings for C structs (Error, QAPI)
Right, this is why I suggested the pl011 as a device: I felt
it provided enough complexity in terms of where it interconnects
to the rest of QEMU to be a realistic way to find out where
the points of difficulty are, without being super complicated
simply as a device. We don't need to have fully worked out
solutions to these things in the first pass, but I agree with
Paolo that we do want to have a clear path forward that says
"this is what we're expecting the solutions to these points
of difficulty to end up looking like and how we plan to get there".
thanks
-- PMM
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 0/5] Implement ARM PL011 in Rust
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
` (5 preceding siblings ...)
2024-06-12 8:37 ` [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Daniel P. Berrangé
@ 2024-06-19 3:31 ` Richard Henderson
2024-06-19 17:36 ` Manos Pitsidianakis
6 siblings, 1 reply; 54+ messages in thread
From: Richard Henderson @ 2024-06-19 3:31 UTC (permalink / raw)
To: Manos Pitsidianakis, qemu-devel
On 6/11/24 03:33, Manos Pitsidianakis wrote:
> If `cargo` and `bindgen` is installed in your system, you should be able
> to build qemu-system-aarch64 with configure flag --enable-rust and
> launch an arm virt VM. One of the patches hardcodes the default UART of
> the machine to the Rust one, so if something goes wrong you will see it
> upon launching qemu-system-aarch64.
What version is required?
On my stock ubuntu 22.04 system, I get
/usr/bin/bindgen aarch64-softmmu_wrapper.h ...
error: Found argument '--formatter' which wasn't expected, or isn't valid in this context
USAGE:
bindgen [FLAGS] [OPTIONS] <header> -- <clang-args>...
$ bindgen --version
bindgen 0.59.1
r~
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 1/5] build-sys: Add rust feature option
2024-06-11 10:33 ` [RFC PATCH v2 1/5] build-sys: Add rust feature option Manos Pitsidianakis
@ 2024-06-19 4:44 ` Richard Henderson
2024-06-19 16:52 ` Richard Henderson
1 sibling, 0 replies; 54+ messages in thread
From: Richard Henderson @ 2024-06-19 4:44 UTC (permalink / raw)
To: Manos Pitsidianakis, qemu-devel
On 6/11/24 03:33, Manos Pitsidianakis wrote:
> Add options for Rust in meson_options.txt, meson.build, configure to
> prepare for adding Rust code in the followup commits.
>
> `rust` is a reserved meson name, so we have to use an alternative.
> `with_rust` was chosen.
>
> A cargo_wrapper.py script is added that is heavily based on the work of
> Marc-André Lureau from 2021.
>
> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/
>
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>
> Signed-off-by: Manos Pitsidianakis<manos.pitsidianakis@linaro.org>
> ---
> .gitignore | 2 +
> MAINTAINERS | 5 +
> configure | 12 ++
> meson.build | 11 ++
> meson_options.txt | 4 +
> scripts/cargo_wrapper.py | 211 ++++++++++++++++++++++++++++++++++
> scripts/meson-buildoptions.sh | 6 +
> 7 files changed, 251 insertions(+)
> create mode 100644 scripts/cargo_wrapper.py
[13/5185] Generating rust_pl011_cargo with a custom command
FAILED: libpl011.a
/home/rth/chroot-home/qemu/bld-x/pyvenv/bin/python3
/home/rth/chroot-home/qemu/src/scripts/cargo_wrapper.py --config-headers
/home/rth/chroot-home/qemu/bld-x/config-host.h --meson-build-root
/home/rth/chroot-home/qemu/bld-x --meson-build-dir /home/rth/chroot-home/qemu/bld-x/rust
--meson-source-dir /home/rth/chroot-home/qemu/src/rust --color always --crate-dir
/home/rth/chroot-home/qemu/src/rust/pl011 --profile release --target-triple
x86_64-unknown-linux-gnu --outdir . build-lib
/home/rth/chroot-home/qemu/src/scripts/cargo_wrapper.py:14: DeprecationWarning: The
distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or
check PEP 632 for potential alternatives
r~
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-11 10:33 ` [RFC PATCH v2 3/5] rust: add PL011 device model Manos Pitsidianakis
2024-06-12 12:29 ` Paolo Bonzini
2024-06-13 8:30 ` Zhao Liu
@ 2024-06-19 5:34 ` Richard Henderson
2024-06-19 16:43 ` Paolo Bonzini
2024-07-11 4:21 ` Zhao Liu
3 siblings, 1 reply; 54+ messages in thread
From: Richard Henderson @ 2024-06-19 5:34 UTC (permalink / raw)
To: Manos Pitsidianakis, qemu-devel
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~
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-19 5:34 ` Richard Henderson
@ 2024-06-19 16:43 ` Paolo Bonzini
2024-06-19 16:54 ` Daniel P. Berrangé
0 siblings, 1 reply; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-19 16:43 UTC (permalink / raw)
To: Richard Henderson, Manos Pitsidianakis, qemu-devel
On 6/19/24 07:34, Richard Henderson wrote:
> First silly question: how much of this is boiler plate that gets moved
> the moment that the second rust subdirectory is added?
If my suggestion at
https://lore.kernel.org/qemu-devel/CABgObfaP7DRD8dbSKNmUzhZNyxeHWO0MztaW3_EFYt=vf6SbzA@mail.gmail.com/
works, we'd have only two directories that have a Cargo.toml in it. For
example it could be rust/qemu/ (common code) and rust/hw/ (code that
depends on Kconfig).
I think we can also have a rust/Cargo.toml file as in
https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace,
and then the various configuration files under rust/ will be valid for
all subpackages.
>> +[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?
-Crelocation-mode should be pie. But also, I am not sure it works
because I think it's always going to be overridden by cargo_wrapper.py?
See https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags.
(I'm not sure what +crt-static is for).
I think the generate_cfg_flags() mechanism of cargo_wrapper.py has to be
rewritten from Python to Rust and moved to build.rs (using
cargo::rustc-cfg). By doing this, the cfg flags are added to whatever
is in .cargo/config.toml, rather than replaced.
>> 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?
If I understand correctly, Manos considered two possible ways to invoke
cargo on the Rust code:
- directly, in which case you need to copy the generated source file to
rust/pl011/src/generated.rs.inc, because cargo does not know where the
build tree
- do everything through meson, which does the right thing because
cargo_wrapper.py knows about the build tree and passes the information.
To avoid this, the first workflow could be adjusted so that cargo can
still be invoked directly, _but from the build tree_, not the source
tree. For example configure could generate a
.../build/.cargo/config.toml with
[env]
MESON_BUILD_ROOT = ".../build"
(extra advantage: -Crelocation-model=pie can be added based on
--enable-pie/--disable-pie). configure can also create symlinks in the
build tree for the source tree's rust/, Cargo.toml and Cargo.lock.
This allows rust/pl011/src/generated.rs (which probably will become
something like rust/common/src/generated.rs) to be:
include!(concat!(env!("MESON_BUILD_ROOT"), "/generated.rs.inc"));
when cargo is invoked from the build tree.
Putting everything together you'd have
build/
rust/
.cargo/
config.toml # generated by configure or meson.build
Cargo.toml # workspace generated by configure or meson.build
Cargo.lock # can be either linked to srctree or generated
qemu/ # symlink to srctree/rust/qemu
aarch64-softmmu-hw/
Cargo.toml # generated by meson.build (*)
src/ # symlink to srctree/rust/hw/
i386-softmmu-hw/
Cargo.toml # generated by meson.build
src/ # symlink to srctree/rust/hw/
generated/ # files generated by rust/generated/meson.build
(*) these have to be generated to change the package name, so
configure_file() seems like a good match for it.
This is suspiciously similar to what tests/tcg/ looks like, except that
tests/tcg/*/Makefile is just a symbolic link. I tried creating a
similar directory structure in a toy project, and it seemed to work...
> 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...
Generally the idea is that libraries should not commit it, while
applications should commit it. The idea is that the Cargo.lock file
reproduces a working configuration, and dependencies are updated to
known-good releases when CI passes. I don't think I like this, but it
is what it is. I ascribe it to me being from the Jurassic.
But for now I would consider not committing Cargo.lock, because we don't
have the infrastructure to do that automatic dependency update (assuming
we want it). But we could generate it at release time so that it is
included in tarballs, and create the symlink from
srctree/rust/Cargo.lock into build/rust/ only if the file is present in
the source tree.
>> diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
>> [...]
>> +# 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" }
This one has to be included, but it is less restrictive than it seems.
It is equivalent to
arbitrary-int = { version = ">=1.2.7, <2.0.0" }
bilge = { version = ">=0.2.0, <0.3.0" }
bilge-impl = { version = ">=0.2.0, <0.3.0" }
That is, it assumes an API breakage when the first nonzero component
changes in the version. It is also possible to put a caret in front of
the version, so that it's clearer that this is a range; but the behavior
is the same.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 1/5] build-sys: Add rust feature option
2024-06-11 10:33 ` [RFC PATCH v2 1/5] build-sys: Add rust feature option Manos Pitsidianakis
2024-06-19 4:44 ` Richard Henderson
@ 2024-06-19 16:52 ` Richard Henderson
2024-06-19 17:32 ` Manos Pitsidianakis
1 sibling, 1 reply; 54+ messages in thread
From: Richard Henderson @ 2024-06-19 16:52 UTC (permalink / raw)
To: Manos Pitsidianakis, qemu-devel
On 6/11/24 03:33, Manos Pitsidianakis wrote:
> +++ b/scripts/cargo_wrapper.py
> @@ -0,0 +1,211 @@
> +#!/usr/bin/env python3
> +# Copyright (c) 2020 Red Hat, Inc.
> +# Copyright (c) 2023 Linaro Ltd.
> +#
> +# Authors:
> +# Manos Pitsidianakis<manos.pitsidianakis@linaro.org>
> +# Marc-André Lureau<marcandre.lureau@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +
> +import argparse
> +import configparser
> +import distutils.file_util
> +import json
> +import logging
> +import os
> +import os.path
> +import re
> +import subprocess
> +import sys
> +import pathlib
> +import shutil
> +import tomllib
Correct me if I'm wrong, but does this require python 3.11 for tomllib?
AFAIK, we're limited to assuming 3.9 from Debian 11 until Feb 2026, or
3.10 from Ubuntu 22.04 until Apr 2026.
I presume this package can be downloaded from pip, and therefore should be added to the
python venv that we create in configure?
r~
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-19 16:43 ` Paolo Bonzini
@ 2024-06-19 16:54 ` Daniel P. Berrangé
2024-06-19 17:23 ` Paolo Bonzini
0 siblings, 1 reply; 54+ messages in thread
From: Daniel P. Berrangé @ 2024-06-19 16:54 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Richard Henderson, Manos Pitsidianakis, qemu-devel
On Wed, Jun 19, 2024 at 06:43:01PM +0200, Paolo Bonzini wrote:
> On 6/19/24 07:34, Richard Henderson wrote:
> > First silly question: how much of this is boiler plate that gets moved
> > the moment that the second rust subdirectory is added?
>
> If my suggestion at https://lore.kernel.org/qemu-devel/CABgObfaP7DRD8dbSKNmUzhZNyxeHWO0MztaW3_EFYt=vf6SbzA@mail.gmail.com/
> works, we'd have only two directories that have a Cargo.toml in it. For
> example it could be rust/qemu/ (common code) and rust/hw/ (code that depends
> on Kconfig).
>
> I think we can also have a rust/Cargo.toml file as in
> https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace,
> and then the various configuration files under rust/ will be valid for all
> subpackages.
>
> > > +[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?
>
> -Crelocation-mode should be pie. But also, I am not sure it works because I
> think it's always going to be overridden by cargo_wrapper.py? See
> https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags.
>
> (I'm not sure what +crt-static is for).
>
> I think the generate_cfg_flags() mechanism of cargo_wrapper.py has to be
> rewritten from Python to Rust and moved to build.rs (using
> cargo::rustc-cfg). By doing this, the cfg flags are added to whatever is in
> .cargo/config.toml, rather than replaced.
>
> > > 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?
>
> If I understand correctly, Manos considered two possible ways to invoke
> cargo on the Rust code:
>
> - directly, in which case you need to copy the generated source file to
> rust/pl011/src/generated.rs.inc, because cargo does not know where the build
> tree
>
> - do everything through meson, which does the right thing because
> cargo_wrapper.py knows about the build tree and passes the information.
>
> To avoid this, the first workflow could be adjusted so that cargo can still
> be invoked directly, _but from the build tree_, not the source tree. For
> example configure could generate a .../build/.cargo/config.toml with
>
> [env]
> MESON_BUILD_ROOT = ".../build"
>
> (extra advantage: -Crelocation-model=pie can be added based on
> --enable-pie/--disable-pie). configure can also create symlinks in the
> build tree for the source tree's rust/, Cargo.toml and Cargo.lock.
>
> This allows rust/pl011/src/generated.rs (which probably will become
> something like rust/common/src/generated.rs) to be:
>
> include!(concat!(env!("MESON_BUILD_ROOT"), "/generated.rs.inc"));
>
> when cargo is invoked from the build tree.
>
> Putting everything together you'd have
>
> build/
> rust/
> .cargo/
> config.toml # generated by configure or meson.build
> Cargo.toml # workspace generated by configure or meson.build
> Cargo.lock # can be either linked to srctree or generated
> qemu/ # symlink to srctree/rust/qemu
> aarch64-softmmu-hw/
> Cargo.toml # generated by meson.build (*)
> src/ # symlink to srctree/rust/hw/
> i386-softmmu-hw/
> Cargo.toml # generated by meson.build
> src/ # symlink to srctree/rust/hw/
> generated/ # files generated by rust/generated/meson.build
If we're generating a build tree to invoke cargo on, can we then
avoid creating a completely separate dir hierarchy in the source
tree rooted at /rust, and just have rust source within our existing
hierarchy.
eg
aarch64-softmmu-hw/
Cargo.toml # generated by meson.build (*)
src/
pl011/ # symlink to srctree/hw/p1011/
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-19 16:54 ` Daniel P. Berrangé
@ 2024-06-19 17:23 ` Paolo Bonzini
0 siblings, 0 replies; 54+ messages in thread
From: Paolo Bonzini @ 2024-06-19 17:23 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Richard Henderson, Manos Pitsidianakis, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2170 bytes --]
Il mer 19 giu 2024, 18:54 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:
> > build/
> > rust/
> > .cargo/
> > config.toml # generated by configure or meson.build
> > Cargo.toml # workspace generated by configure or meson.build
> > Cargo.lock # can be either linked to srctree or generated
> > qemu/ # symlink to srctree/rust/qemu
> > aarch64-softmmu-hw/
> > Cargo.toml # generated by meson.build (*)
> > src/ # symlink to srctree/rust/hw/
> > i386-softmmu-hw/
> > Cargo.toml # generated by meson.build
> > src/ # symlink to srctree/rust/hw/
> > generated/ # files generated by rust/generated/meson.build
>
> If we're generating a build tree to invoke cargo on, can we then
> avoid creating a completely separate dir hierarchy in the source
> tree rooted at /rust, and just have rust source within our existing
> hierarchy.
>
Maybe... I hadn't even considered the possibility of having a single cargo
invocation (and thus a cargo workspace in the build tree) until Richard
pointed out the duplication in configuration files.
I suppose you could just link rust/aarch64-softmmu-hw to srctree/hw, and
have a srctree/hw/lib.rs file in there to prime the search for submodules.
I think the resulting hierarchy would feel a little foreign though. Without
seeing the code I can't judge but my impression is that, if we wanted to go
that way, I would also move all C code under src/. Perhaps we can consider
such a unification later, once we have more experience, but for now keep
Rust and C code separate?
Paolo
> eg
>
> aarch64-softmmu-hw/
> Cargo.toml # generated by meson.build (*)
> src/
> pl011/ # symlink to srctree/hw/p1011/
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o-
> https://www.instagram.com/dberrange :|
>
>
[-- Attachment #2: Type: text/html, Size: 3686 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 1/5] build-sys: Add rust feature option
2024-06-19 16:52 ` Richard Henderson
@ 2024-06-19 17:32 ` Manos Pitsidianakis
0 siblings, 0 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-19 17:32 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini, Peter Maydell,
Alex Benné e, Daniel P. Berrangé ,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé , Zhao Liu, Gustavo Romero,
Pierrick Bouvier, John Snow, Cleber Rosa
On Wed, 19 Jun 2024 19:52, Richard Henderson <richard.henderson@linaro.org> wrote:
>On 6/11/24 03:33, Manos Pitsidianakis wrote:
>> +++ b/scripts/cargo_wrapper.py
>> @@ -0,0 +1,211 @@
>> +#!/usr/bin/env python3
>> +# Copyright (c) 2020 Red Hat, Inc.
>> +# Copyright (c) 2023 Linaro Ltd.
>> +#
>> +# Authors:
>> +# Manos Pitsidianakis<manos.pitsidianakis@linaro.org>
>> +# Marc-André Lureau<marcandre.lureau@redhat.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later. See the COPYING file in the top-level directory.
>> +
>> +import argparse
>> +import configparser
>> +import distutils.file_util
>> +import json
>> +import logging
>> +import os
>> +import os.path
>> +import re
>> +import subprocess
>> +import sys
>> +import pathlib
>> +import shutil
>> +import tomllib
>
>Correct me if I'm wrong, but does this require python 3.11 for tomllib?
>AFAIK, we're limited to assuming 3.9 from Debian 11 until Feb 2026, or
>3.10 from Ubuntu 22.04 until Apr 2026.
>
>I presume this package can be downloaded from pip, and therefore should be added to the
>python venv that we create in configure?
>
>
>r~
That's absolutely correct. I will make it compatible with at least 3.9
in the next version, thanks!
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 0/5] Implement ARM PL011 in Rust
2024-06-19 3:31 ` Richard Henderson
@ 2024-06-19 17:36 ` Manos Pitsidianakis
0 siblings, 0 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-06-19 17:36 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
On Wed, 19 Jun 2024 06:31, Richard Henderson <richard.henderson@linaro.org> wrote:
>On 6/11/24 03:33, Manos Pitsidianakis wrote:
>> If `cargo` and `bindgen` is installed in your system, you should be able
>> to build qemu-system-aarch64 with configure flag --enable-rust and
>> launch an arm virt VM. One of the patches hardcodes the default UART of
>> the machine to the Rust one, so if something goes wrong you will see it
>> upon launching qemu-system-aarch64.
>
>What version is required?
>
>On my stock ubuntu 22.04 system, I get
>
>/usr/bin/bindgen aarch64-softmmu_wrapper.h ...
>error: Found argument '--formatter' which wasn't expected, or isn't valid in this context
>
>USAGE:
> bindgen [FLAGS] [OPTIONS] <header> -- <clang-args>...
>
>$ bindgen --version
>bindgen 0.59.1
I added version checks in the (yet unpublished) next version:
(Which we will also need to match with distro ones whenever possible)
+# FIXME: Latest stable versions, refine to actual minimum ones.
+msrv = {
+ 'rustc': '1.79.0',
+ 'cargo': '1.79.0',
+ 'bindgen': '0.69.4',
+}
+
+# rustup = find_program('rustup', required: false)
+foreach bin_dep: msrv.keys()
+ bin = find_program(bin_dep, required: true)
+ if bin.version() < msrv[bin_dep]
+ # TODO verify behavior
+ if bin == 'bindgen' and get_option('wrap_mode') != 'nodownload'
+ run_command(cargo, 'install', 'bindgen', capture: true, check: true)
+ bin = find_program(bin_dep, required: true)
+ if bin.version() >= msrv[bin_dep]
+ continue
+ endif
+ endif
+ message()
+ error(bin_dep + ' version ' + bin.version() + ' is unsupported: Please upgrade to at least ' + msrv[bin_dep])
+ endif
+endforeach
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-06-11 10:33 ` [RFC PATCH v2 3/5] rust: add PL011 device model Manos Pitsidianakis
` (2 preceding siblings ...)
2024-06-19 5:34 ` Richard Henderson
@ 2024-07-11 4:21 ` Zhao Liu
2024-07-11 5:35 ` Manos Pitsidianakis
3 siblings, 1 reply; 54+ messages in thread
From: Zhao Liu @ 2024-07-11 4:21 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Bennée, Daniel P. Berrangé,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé, Gustavo Romero, Pierrick Bouvier
Hi Manos and all,
On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote:
> 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
>
I find it's stiil necessary to strictly limit the width of the lines by
"error_on_line_overflow = true" [1].
Currently rustfmt defaults the width limitation with "max_width = 100",
but it has bugs and doesn't always work. For example, the line of
rust/qemu-api/src/device_class.rs:108 comes to 157 characters and is
ignored by rustfmt, which doesn't even fit in one line of my screen!
Of course I think it's feasible to manually review and fix similar cases,
but it's definitely better to have readily available tool that can help
us rigorously formatted...
[1]: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#error_on_line_overflow
-Zhao
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC PATCH v2 3/5] rust: add PL011 device model
2024-07-11 4:21 ` Zhao Liu
@ 2024-07-11 5:35 ` Manos Pitsidianakis
0 siblings, 0 replies; 54+ messages in thread
From: Manos Pitsidianakis @ 2024-07-11 5:35 UTC (permalink / raw)
To: Zhao Liu
Cc: qemu-devel, Stefan Hajnoczi, Mads Ynddal, Paolo Bonzini,
Peter Maydell, Alex Bennée, Daniel P. Berrangé,
Marc-André Lureau, Thomas Huth, Markus Armbruster,
Philippe Mathieu-Daudé, Gustavo Romero, Pierrick Bouvier
Hey Zhao,
On Thu, 11 Jul 2024 at 07:05, Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Hi Manos and all,
>
> On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote:
> > 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
> >
>
> I find it's stiil necessary to strictly limit the width of the lines by
> "error_on_line_overflow = true" [1].
>
> Currently rustfmt defaults the width limitation with "max_width = 100",
> but it has bugs and doesn't always work. For example, the line of
> rust/qemu-api/src/device_class.rs:108 comes to 157 characters and is
> ignored by rustfmt, which doesn't even fit in one line of my screen!
>
> Of course I think it's feasible to manually review and fix similar cases,
> but it's definitely better to have readily available tool that can help
> us rigorously formatted...
>
> [1]: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#error_on_line_overflow
>
> -Zhao
Thank you for pointing that out, I hadn't noticed it! The problem is
that macro definitions are also macros and macros aren't formatted
because they have to be expanded, IIUC. I agree that a hard error on
an overflow is necessary for readable code.
By the way, my usual go-to workaround for this bug is to format the
macro body outside the macro_rules! { } scope, (rustfmt works even if
the code does not compile) and then put it back in.
Manos
^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2024-07-11 5:37 UTC | newest]
Thread overview: 54+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-11 10:33 [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 1/5] build-sys: Add rust feature option Manos Pitsidianakis
2024-06-19 4:44 ` Richard Henderson
2024-06-19 16:52 ` Richard Henderson
2024-06-19 17:32 ` Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 2/5] rust: add bindgen step as a meson dependency Manos Pitsidianakis
2024-06-17 21:01 ` Paolo Bonzini
2024-06-11 10:33 ` [RFC PATCH v2 3/5] rust: add PL011 device model Manos Pitsidianakis
2024-06-12 12:29 ` Paolo Bonzini
2024-06-12 14:14 ` Manos Pitsidianakis
2024-06-12 15:20 ` Paolo Bonzini
2024-06-12 16:06 ` Daniel P. Berrangé
2024-06-12 20:57 ` Manos Pitsidianakis
2024-06-12 21:27 ` Paolo Bonzini
2024-06-13 5:09 ` Manos Pitsidianakis
2024-06-13 7:13 ` Daniel P. Berrangé
2024-06-13 7:56 ` Paolo Bonzini
2024-06-13 8:49 ` Manos Pitsidianakis
2024-06-13 9:16 ` Daniel P. Berrangé
2024-06-13 20:57 ` Paolo Bonzini
2024-06-14 6:38 ` Manos Pitsidianakis
2024-06-14 17:50 ` Paolo Bonzini
2024-06-17 8:45 ` Manos Pitsidianakis
2024-06-17 11:32 ` Paolo Bonzini
2024-06-17 13:54 ` Manos Pitsidianakis
2024-06-17 14:32 ` Paolo Bonzini
2024-06-17 21:04 ` Manos Pitsidianakis
2024-06-17 23:33 ` Pierrick Bouvier
2024-06-18 6:00 ` Paolo Bonzini
2024-06-18 6:00 ` Paolo Bonzini
2024-06-17 23:18 ` Pierrick Bouvier
2024-06-18 9:13 ` Daniel P. Berrangé
2024-06-18 9:29 ` Paolo Bonzini
2024-06-18 9:49 ` Peter Maydell
2024-06-13 16:20 ` Zhao Liu
2024-06-13 17:56 ` Paolo Bonzini
2024-06-13 8:30 ` Zhao Liu
2024-06-13 8:41 ` Manos Pitsidianakis
2024-06-13 8:53 ` Daniel P. Berrangé
2024-06-13 8:59 ` Manos Pitsidianakis
2024-06-13 9:20 ` Daniel P. Berrangé
2024-06-19 5:34 ` Richard Henderson
2024-06-19 16:43 ` Paolo Bonzini
2024-06-19 16:54 ` Daniel P. Berrangé
2024-06-19 17:23 ` Paolo Bonzini
2024-07-11 4:21 ` Zhao Liu
2024-07-11 5:35 ` Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 4/5] DO NOT MERGE: add rustdoc build for gitlab pages Manos Pitsidianakis
2024-06-11 10:33 ` [RFC PATCH v2 5/5] DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine Manos Pitsidianakis
2024-06-12 8:37 ` [RFC PATCH v2 0/5] Implement ARM PL011 in Rust Daniel P. Berrangé
2024-06-13 5:13 ` Manos Pitsidianakis
2024-06-13 7:56 ` Daniel P. Berrangé
2024-06-19 3:31 ` Richard Henderson
2024-06-19 17:36 ` Manos Pitsidianakis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).