From 72d6ed19bf25e1289be7b05326d3ed3c9f0cabe9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 22 Oct 2024 07:24:32 +0200 Subject: [PATCH 01/49] ci: enable rust in the Debian and Ubuntu system build job We have fixed all incompatibilities with older versions of rustc and bindgen. Enable Rust on Debian to check that the minimum supported version of Rust is indeed 1.63.0, and 0.60.x for bindgen. Reviewed-by: Pierrick Bouvier Signed-off-by: Paolo Bonzini --- .gitlab-ci.d/buildtest.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml index 336223484d..4265a57783 100644 --- a/.gitlab-ci.d/buildtest.yml +++ b/.gitlab-ci.d/buildtest.yml @@ -40,7 +40,7 @@ build-system-ubuntu: job: amd64-ubuntu2204-container variables: IMAGE: ubuntu2204 - CONFIGURE_ARGS: --enable-docs + CONFIGURE_ARGS: --enable-docs --enable-rust TARGETS: alpha-softmmu microblazeel-softmmu mips64el-softmmu MAKE_CHECK_ARGS: check-build @@ -71,7 +71,7 @@ build-system-debian: job: amd64-debian-container variables: IMAGE: debian - CONFIGURE_ARGS: --with-coroutine=sigaltstack + CONFIGURE_ARGS: --with-coroutine=sigaltstack --enable-rust TARGETS: arm-softmmu i386-softmmu riscv64-softmmu sh4eb-softmmu sparc-softmmu xtensa-softmmu MAKE_CHECK_ARGS: check-build From 8c2866750319fac3f7b641d1a097f3779952631d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 12 Nov 2024 11:35:34 +0100 Subject: [PATCH 02/49] rust: apply --cfg MESON to all crates We might have more uses for --cfg MESON, even though right now it's only qemu-api that has generated files. Since we're going to add more flags to the add_project_arguments calls for Rust, it makes sense to also add --cfg MESON everywhere. Signed-off-by: Paolo Bonzini --- meson.build | 7 ++++--- rust/qemu-api/meson.build | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index 147097c652..c35ce64cd6 100644 --- a/meson.build +++ b/meson.build @@ -3422,10 +3422,11 @@ if have_rust # is safe; https://github.com/rust-lang/rust/pull/54675 says that # passing -nodefaultlibs to the linker "was more ideological to # start with than anything". - add_project_arguments(rustc_args + ['-C', 'default-linker-libraries'], + add_project_arguments(rustc_args + + ['--cfg', 'MESON', '-C', 'default-linker-libraries'], native: false, language: 'rust') - - add_project_arguments(rustc_args, native: true, language: 'rust') + add_project_arguments(rustc_args + ['--cfg', 'MESON'], + native: true, language: 'rust') endif hxtool = find_program('scripts/hxtool') diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 6f637af7b1..cad9ac4844 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -1,4 +1,4 @@ -_qemu_api_cfg = ['--cfg', 'MESON'] +_qemu_api_cfg = [] # _qemu_api_cfg += ['--cfg', 'feature="allocator"'] if rustc.version().version_compare('>=1.77.0') _qemu_api_cfg += ['--cfg', 'has_offset_of'] From cb7ada5409f171dae364f206a7fe3ff30fcba7cb Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 12 Nov 2024 11:52:23 +0100 Subject: [PATCH 03/49] rust: allow using build-root bindings.rs from cargo Right now, using cargo with QEMU requires copying by hand the bindings.rs to the source tree. Instead, we can use an include file to escape the cage of cargo's mandated source directory structure. By running cargo within meson's "devenv" and adding a MESON_BUILD_ROOT environment variable, it is easy for build.rs to find the file. However, the file must be symlinked into cargo's output directory for rust-analyzer to find it. Suggested-by: Junjie Mao Signed-off-by: Paolo Bonzini --- meson.build | 4 +++- rust/hw/char/pl011/.gitignore | 2 -- rust/qemu-api/.gitignore | 2 +- rust/qemu-api/README.md | 4 ++-- rust/qemu-api/build.rs | 34 +++++++++++++++++++++++++++------- rust/qemu-api/meson.build | 1 + rust/qemu-api/src/bindings.rs | 29 +++++++++++++++++++++++++++++ rust/qemu-api/src/lib.rs | 22 ---------------------- 8 files changed, 63 insertions(+), 35 deletions(-) delete mode 100644 rust/hw/char/pl011/.gitignore create mode 100644 rust/qemu-api/src/bindings.rs diff --git a/meson.build b/meson.build index c35ce64cd6..ea211c2dbb 100644 --- a/meson.build +++ b/meson.build @@ -3,6 +3,8 @@ project('qemu', ['c'], meson_version: '>=1.5.0', 'b_staticpic=false', 'stdsplit=false', 'optimization=2', 'b_pie=true'], version: files('VERSION')) +meson.add_devenv({ 'MESON_BUILD_ROOT' : meson.project_build_root() }) + add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true) add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow']) add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough']) @@ -4090,7 +4092,7 @@ if have_rust bindings_rs = rust.bindgen( input: 'rust/wrapper.h', dependencies: common_ss.all_dependencies(), - output: 'bindings.rs', + output: 'bindings.inc.rs', include_directories: include_directories('.', 'include'), bindgen_version: ['>=0.60.0'], args: bindgen_args, diff --git a/rust/hw/char/pl011/.gitignore b/rust/hw/char/pl011/.gitignore deleted file mode 100644 index 71eaff2035..0000000000 --- a/rust/hw/char/pl011/.gitignore +++ /dev/null @@ -1,2 +0,0 @@ -# Ignore generated bindings file overrides. -src/bindings.rs.inc diff --git a/rust/qemu-api/.gitignore b/rust/qemu-api/.gitignore index b9e7e004c8..df6c2163e0 100644 --- a/rust/qemu-api/.gitignore +++ b/rust/qemu-api/.gitignore @@ -1,2 +1,2 @@ # Ignore generated bindings file overrides. -src/bindings.rs +/src/bindings.inc.rs diff --git a/rust/qemu-api/README.md b/rust/qemu-api/README.md index 7588fa29ef..53810f4888 100644 --- a/rust/qemu-api/README.md +++ b/rust/qemu-api/README.md @@ -5,7 +5,7 @@ This library exports helper Rust types, Rust macros and C FFI bindings for inter The C bindings can be generated with `bindgen`, using this build target: ```console -$ ninja bindings.rs +$ ninja bindings.inc.rs ``` ## Generate Rust documentation @@ -13,5 +13,5 @@ $ ninja bindings.rs To generate docs for this crate, including private items: ```sh -cargo doc --no-deps --document-private-items +pyvenv/bin/meson devenv -w ../rust cargo doc --no-deps --document-private-items ``` diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs index 20f8f718b9..40644d5e4f 100644 --- a/rust/qemu-api/build.rs +++ b/rust/qemu-api/build.rs @@ -2,22 +2,42 @@ // Author(s): Manos Pitsidianakis // SPDX-License-Identifier: GPL-2.0-or-later -use std::path::Path; +#[cfg(unix)] +use std::os::unix::fs::symlink as symlink_file; +#[cfg(windows)] +use std::os::windows::fs::symlink_file; +use std::{env, fs::remove_file, io::Result, path::Path}; use version_check as rustc; -fn main() { - if !Path::new("src/bindings.rs").exists() { - panic!( - "No generated C bindings found! Either build them manually with bindgen or with meson \ - (`ninja bindings.rs`) and copy them to src/bindings.rs, or build through meson." - ); +fn main() -> Result<()> { + // Placing bindings.inc.rs in the source directory is supported + // but not documented or encouraged. + let path = env::var("MESON_BUILD_ROOT") + .unwrap_or_else(|_| format!("{}/src", env!("CARGO_MANIFEST_DIR"))); + + let file = format!("{}/bindings.inc.rs", path); + let file = Path::new(&file); + if !Path::new(&file).exists() { + panic!(concat!( + "No generated C bindings found! If you want to run `cargo`, start a subshell\n", + "with `meson devenv`, or point MESON_BUILD_ROOT to the top of the build tree." + )); } + let out_dir = env::var("OUT_DIR").unwrap(); + let dest_path = format!("{}/bindings.inc.rs", out_dir); + let dest_path = Path::new(&dest_path); + if dest_path.symlink_metadata().is_ok() { + remove_file(dest_path)?; + } + symlink_file(file, dest_path)?; + // Check for available rustc features if rustc::is_min_version("1.77.0").unwrap_or(false) { println!("cargo:rustc-cfg=has_offset_of"); } println!("cargo:rerun-if-changed=build.rs"); + Ok(()) } diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index cad9ac4844..3be7b7e5ce 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -9,6 +9,7 @@ _qemu_api_rs = static_library( structured_sources( [ 'src/lib.rs', + 'src/bindings.rs', 'src/c_str.rs', 'src/definitions.rs', 'src/device_class.rs', diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs new file mode 100644 index 0000000000..0b76ec58be --- /dev/null +++ b/rust/qemu-api/src/bindings.rs @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#![allow( + dead_code, + improper_ctypes_definitions, + improper_ctypes, + non_camel_case_types, + non_snake_case, + non_upper_case_globals, + unsafe_op_in_unsafe_fn, + clippy::missing_const_for_fn, + clippy::too_many_arguments, + clippy::approx_constant, + clippy::use_self, + clippy::useless_transmute, + clippy::missing_safety_doc +)] + +#[cfg(MESON)] +include!("bindings.inc.rs"); + +#[cfg(not(MESON))] +include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs")); + +unsafe impl Send for Property {} +unsafe impl Sync for Property {} +unsafe impl Sync for TypeInfo {} +unsafe impl Sync for VMStateDescription {} +unsafe impl Sync for VMStateField {} +unsafe impl Sync for VMStateInfo {} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index aa8d16ec94..440aff3817 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -4,31 +4,9 @@ #![cfg_attr(not(MESON), doc = include_str!("../README.md"))] -#[allow( - dead_code, - improper_ctypes_definitions, - improper_ctypes, - non_camel_case_types, - non_snake_case, - non_upper_case_globals, - unsafe_op_in_unsafe_fn, - clippy::missing_const_for_fn, - clippy::too_many_arguments, - clippy::approx_constant, - clippy::use_self, - clippy::useless_transmute, - clippy::missing_safety_doc, -)] #[rustfmt::skip] pub mod bindings; -unsafe impl Send for bindings::Property {} -unsafe impl Sync for bindings::Property {} -unsafe impl Sync for bindings::TypeInfo {} -unsafe impl Sync for bindings::VMStateDescription {} -unsafe impl Sync for bindings::VMStateField {} -unsafe impl Sync for bindings::VMStateInfo {} - pub mod c_str; pub mod definitions; pub mod device_class; From f3a6e9bc475504152e45e5fbf86c756157438d08 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 12 Nov 2024 11:54:11 +0100 Subject: [PATCH 04/49] rust: build: move rustc_args.py invocation to qemu-api crate Only qemu-api needs access to the symbols in config-host.h. Remove the temptation to use them elsewhere by limiting the --cfg arguments to the qemu-api crate. Per-crate invocation of the script will also be needed to add --check-cfg options for each crate's features (when more complex, build-time configurable devices are added in the future). Signed-off-by: Paolo Bonzini --- meson.build | 56 +++++++++++++++++---------------------- rust/qemu-api/meson.build | 5 +++- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/meson.build b/meson.build index ea211c2dbb..2f1f2aece4 100644 --- a/meson.build +++ b/meson.build @@ -120,7 +120,30 @@ if have_rust endif if have_rust + rustc_args = find_program('scripts/rust/rustc_args.py') rustfmt = find_program('rustfmt', required: false) + + # Prohibit code that is forbidden in Rust 2024 + rustc_lint_args = ['-D', 'unsafe_op_in_unsafe_fn'] + + # Occasionally, we may need to silence warnings and clippy lints that + # were only introduced in newer Rust compiler versions. Do not croak + # in that case; a CI job with rust_strict_lints == true ensures that + # we do not have misspelled allow() attributes. + if not get_option('strict_rust_lints') + rustc_lint_args += ['-A', 'unknown_lints'] + endif + + # Apart from procedural macros, our Rust executables will often link + # with C code, so include all the libraries that C code needs. This + # is safe; https://github.com/rust-lang/rust/pull/54675 says that + # passing -nodefaultlibs to the linker "was more ideological to + # start with than anything". + add_project_arguments(rustc_lint_args + + ['--cfg', 'MESON', '-C', 'default-linker-libraries'], + native: false, language: 'rust') + add_project_arguments(rustc_lint_args + ['--cfg', 'MESON'], + native: true, language: 'rust') endif dtrace = not_found @@ -3399,37 +3422,8 @@ endif # Generated sources # ##################### -genh += configure_file(output: 'config-host.h', configuration: config_host_data) - -if have_rust - rustc_args = run_command( - find_program('scripts/rust/rustc_args.py'), - '--config-headers', meson.project_build_root() / 'config-host.h', - capture : true, - check: true).stdout().strip().split() - - # Prohibit code that is forbidden in Rust 2024 - rustc_args += ['-D', 'unsafe_op_in_unsafe_fn'] - - # Occasionally, we may need to silence warnings and clippy lints that - # were only introduced in newer Rust compiler versions. Do not croak - # in that case; a CI job with rust_strict_lints == true ensures that - # we do not have misspelled allow() attributes. - if not get_option('strict_rust_lints') - rustc_args += ['-A', 'unknown_lints'] - endif - - # Apart from procedural macros, our Rust executables will often link - # with C code, so include all the libraries that C code needs. This - # is safe; https://github.com/rust-lang/rust/pull/54675 says that - # passing -nodefaultlibs to the linker "was more ideological to - # start with than anything". - add_project_arguments(rustc_args + - ['--cfg', 'MESON', '-C', 'default-linker-libraries'], - native: false, language: 'rust') - add_project_arguments(rustc_args + ['--cfg', 'MESON'], - native: true, language: 'rust') -endif +config_host_h = configure_file(output: 'config-host.h', configuration: config_host_data) +genh += config_host_h hxtool = find_program('scripts/hxtool') shaderinclude = find_program('scripts/shaderinclude.py') diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 3be7b7e5ce..5df6b35bf8 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -1,4 +1,7 @@ -_qemu_api_cfg = [] +_qemu_api_cfg = run_command(rustc_args, + '--config-headers', config_host_h, + capture: true, check: true).stdout().strip().split() + # _qemu_api_cfg += ['--cfg', 'feature="allocator"'] if rustc.version().version_compare('>=1.77.0') _qemu_api_cfg += ['--cfg', 'has_offset_of'] From 1de82059aa097340d0ffde9140d338cddef478e5 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 6 Nov 2024 11:25:55 +0100 Subject: [PATCH 05/49] rust: build: restrict --cfg generation to only required symbols Parse the Cargo.toml file, looking for the unexpected_cfgs configuration. When generating --cfg options from the config-host.h file, only use those that are included in the configuration. Signed-off-by: Paolo Bonzini --- rust/qemu-api/meson.build | 2 +- scripts/rust/rustc_args.py | 61 ++++++++++++++++++++++++++++---------- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 5df6b35bf8..2ff6d2ce3d 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -1,5 +1,5 @@ _qemu_api_cfg = run_command(rustc_args, - '--config-headers', config_host_h, + '--config-headers', config_host_h, files('Cargo.toml'), capture: true, check: true).stdout().strip().split() # _qemu_api_cfg += ['--cfg', 'feature="allocator"'] diff --git a/scripts/rust/rustc_args.py b/scripts/rust/rustc_args.py index e4cc9720e1..942dd2b2ba 100644 --- a/scripts/rust/rustc_args.py +++ b/scripts/rust/rustc_args.py @@ -26,30 +26,51 @@ along with this program. If not, see . import argparse import logging +from pathlib import Path +from typing import Any, Iterable, Mapping, Optional, Set -from typing import List +try: + import tomllib +except ImportError: + import tomli as tomllib -def generate_cfg_flags(header: str) -> List[str]: +class CargoTOML: + tomldata: Mapping[Any, Any] + check_cfg: Set[str] + + def __init__(self, path: str): + with open(path, 'rb') as f: + self.tomldata = tomllib.load(f) + + self.check_cfg = set(self.find_check_cfg()) + + def find_check_cfg(self) -> Iterable[str]: + toml_lints = self.lints + rust_lints = toml_lints.get("rust", {}) + cfg_lint = rust_lints.get("unexpected_cfgs", {}) + return cfg_lint.get("check-cfg", []) + + @property + def lints(self) -> Mapping[Any, Any]: + return self.get_table("lints") + + def get_table(self, key: str) -> Mapping[Any, Any]: + table = self.tomldata.get(key, {}) + + return table + + +def generate_cfg_flags(header: str, cargo_toml: CargoTOML) -> Iterable[str]: """Converts defines from config[..].h headers to rustc --cfg flags.""" - def cfg_name(name: str) -> str: - """Filter function for C #defines""" - if ( - name.startswith("CONFIG_") - or name.startswith("TARGET_") - or name.startswith("HAVE_") - ): - return name - return "" - 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: + name = cfg[0] + if f'cfg({name})' not in cargo_toml.check_cfg: continue if len(cfg) >= 2 and cfg[1] != "1": continue @@ -59,7 +80,6 @@ def generate_cfg_flags(header: str) -> List[str]: def main() -> None: - # pylint: disable=missing-function-docstring parser = argparse.ArgumentParser() parser.add_argument("-v", "--verbose", action="store_true") parser.add_argument( @@ -71,12 +91,21 @@ def main() -> None: required=False, default=[], ) + parser.add_argument( + metavar="TOML_FILE", + action="store", + dest="cargo_toml", + help="path to Cargo.toml file", + ) args = parser.parse_args() if args.verbose: logging.basicConfig(level=logging.DEBUG) logging.debug("args: %s", args) + + cargo_toml = CargoTOML(args.cargo_toml) + for header in args.config_headers: - for tok in generate_cfg_flags(header): + for tok in generate_cfg_flags(header, cargo_toml): print(tok) From 97ed1e9c8e2b0744508ef61cac8a23bb1e107820 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 7 Nov 2024 10:02:15 +0100 Subject: [PATCH 06/49] rust: build: generate lint flags from Cargo.toml Cargo.toml makes it possible to describe the desired lint level settings in a nice format. We can extend this to Meson-built crates, by teaching rustc_args.py to fetch lint and --check-cfg arguments from Cargo.toml. --check-cfg arguments come from the unexpected_cfgs lint as well as crate features Start with qemu-api, since it already has a [lints.rust] table and an invocation of rustc_args.py. Signed-off-by: Paolo Bonzini --- meson.build | 3 +- rust/qemu-api/meson.build | 4 +- scripts/rust/rustc_args.py | 83 +++++++++++++++++++++++++++++++++++++- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index 2f1f2aece4..1e1d8f5cd6 100644 --- a/meson.build +++ b/meson.build @@ -120,7 +120,8 @@ if have_rust endif if have_rust - rustc_args = find_program('scripts/rust/rustc_args.py') + rustc_args = [find_program('scripts/rust/rustc_args.py'), + '--rustc-version', rustc.version()] rustfmt = find_program('rustfmt', required: false) # Prohibit code that is forbidden in Rust 2024 diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 2ff6d2ce3d..1ed79672cc 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -1,6 +1,6 @@ _qemu_api_cfg = run_command(rustc_args, - '--config-headers', config_host_h, files('Cargo.toml'), - capture: true, check: true).stdout().strip().split() + '--config-headers', config_host_h, '--features', '--lints', files('Cargo.toml'), + capture: true, check: true).stdout().strip().splitlines() # _qemu_api_cfg += ['--cfg', 'feature="allocator"'] if rustc.version().version_compare('>=1.77.0') diff --git a/scripts/rust/rustc_args.py b/scripts/rust/rustc_args.py index 942dd2b2ba..9b9778a1ca 100644 --- a/scripts/rust/rustc_args.py +++ b/scripts/rust/rustc_args.py @@ -25,9 +25,10 @@ along with this program. If not, see . """ import argparse +from dataclasses import dataclass import logging from pathlib import Path -from typing import Any, Iterable, Mapping, Optional, Set +from typing import Any, Iterable, List, Mapping, Optional, Set try: import tomllib @@ -61,6 +62,45 @@ class CargoTOML: return table +@dataclass +class LintFlag: + flags: List[str] + priority: int + + +def generate_lint_flags(cargo_toml: CargoTOML) -> Iterable[str]: + """Converts Cargo.toml lints to rustc -A/-D/-F/-W flags.""" + + toml_lints = cargo_toml.lints + + lint_list = [] + for k, v in toml_lints.items(): + prefix = "" if k == "rust" else k + "::" + for lint, data in v.items(): + level = data if isinstance(data, str) else data["level"] + priority = 0 if isinstance(data, str) else data.get("priority", 0) + if level == "deny": + flag = "-D" + elif level == "allow": + flag = "-A" + elif level == "warn": + flag = "-W" + elif level == "forbid": + flag = "-F" + else: + raise Exception(f"invalid level {level} for {prefix}{lint}") + + # This may change if QEMU ever invokes clippy-driver or rustdoc by + # hand. For now, check the syntax but do not add non-rustc lints to + # the command line. + if k == "rust": + lint_list.append(LintFlag(flags=[flag, prefix + lint], priority=priority)) + + lint_list.sort(key=lambda x: x.priority) + for lint in lint_list: + yield from lint.flags + + def generate_cfg_flags(header: str, cargo_toml: CargoTOML) -> Iterable[str]: """Converts defines from config[..].h headers to rustc --cfg flags.""" @@ -97,13 +137,54 @@ def main() -> None: dest="cargo_toml", help="path to Cargo.toml file", ) + parser.add_argument( + "--features", + action="store_true", + dest="features", + help="generate --check-cfg arguments for features", + required=False, + default=None, + ) + parser.add_argument( + "--lints", + action="store_true", + dest="lints", + help="generate arguments from [lints] table", + required=False, + default=None, + ) + parser.add_argument( + "--rustc-version", + metavar="VERSION", + dest="rustc_version", + action="store", + help="version of rustc", + required=False, + default="1.0.0", + ) args = parser.parse_args() if args.verbose: logging.basicConfig(level=logging.DEBUG) logging.debug("args: %s", args) + rustc_version = tuple((int(x) for x in args.rustc_version.split('.')[0:2])) cargo_toml = CargoTOML(args.cargo_toml) + if args.lints: + for tok in generate_lint_flags(cargo_toml): + print(tok) + + if rustc_version >= (1, 80): + if args.lints: + for cfg in sorted(cargo_toml.check_cfg): + print("--check-cfg") + print(cfg) + if args.features: + for feature in cargo_toml.get_table("features"): + if feature != "default": + print("--check-cfg") + print(f'cfg(feature,values("{feature}"))') + for header in args.config_headers: for tok in generate_cfg_flags(header, cargo_toml): print(tok) From 90868c3dcec755f567426d1fad64e7611053778e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 6 Nov 2024 13:03:45 +0100 Subject: [PATCH 07/49] rust: cargo: store desired warning levels in workspace Cargo.toml An extra benefit of workspaces is that they allow to place lint level settings in a single Cargo.toml; the settings are then inherited by packages in the workspace. Correspondingly, teach rustc_args.py to get the unexpected_cfgs configuration from the workspace Cargo.toml. Note that it is still possible to allow or deny warnings per crate or module, via the #![] attribute syntax. The rust/qemu-api/src/bindings.rs file is an example. Signed-off-by: Paolo Bonzini --- meson.build | 7 +++--- rust/Cargo.toml | 8 +++++++ rust/hw/char/pl011/Cargo.toml | 3 +++ rust/qemu-api-macros/Cargo.toml | 3 +++ rust/qemu-api/Cargo.toml | 5 ++--- rust/qemu-api/meson.build | 2 +- scripts/rust/rustc_args.py | 38 +++++++++++++++++++++++++++------ 7 files changed, 53 insertions(+), 13 deletions(-) diff --git a/meson.build b/meson.build index 1e1d8f5cd6..218ae441e3 100644 --- a/meson.build +++ b/meson.build @@ -121,11 +121,12 @@ endif if have_rust rustc_args = [find_program('scripts/rust/rustc_args.py'), - '--rustc-version', rustc.version()] + '--rustc-version', rustc.version(), + '--workspace', meson.project_source_root() / 'rust'] rustfmt = find_program('rustfmt', required: false) - # Prohibit code that is forbidden in Rust 2024 - rustc_lint_args = ['-D', 'unsafe_op_in_unsafe_fn'] + rustc_lint_args = run_command(rustc_args, '--lints', + capture: true, check: true).stdout().strip().splitlines() # Occasionally, we may need to silence warnings and clippy lints that # were only introduced in newer Rust compiler versions. Do not croak diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 0c94d5037d..4bb52bf0bd 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -5,3 +5,11 @@ members = [ "qemu-api", "hw/char/pl011", ] + +[workspace.lints.rust] +unexpected_cfgs = { level = "deny", check-cfg = [ + 'cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)', + 'cfg(has_offset_of)'] } + +# Prohibit code that is forbidden in Rust 2024 +unsafe_op_in_unsafe_fn = "deny" diff --git a/rust/hw/char/pl011/Cargo.toml b/rust/hw/char/pl011/Cargo.toml index a373906b9f..58f3e859f7 100644 --- a/rust/hw/char/pl011/Cargo.toml +++ b/rust/hw/char/pl011/Cargo.toml @@ -21,3 +21,6 @@ bilge = { version = "0.2.0" } bilge-impl = { version = "0.2.0" } qemu_api = { path = "../../../qemu-api" } qemu_api_macros = { path = "../../../qemu-api-macros" } + +[lints] +workspace = true diff --git a/rust/qemu-api-macros/Cargo.toml b/rust/qemu-api-macros/Cargo.toml index a8f7377106..5a27b52ee6 100644 --- a/rust/qemu-api-macros/Cargo.toml +++ b/rust/qemu-api-macros/Cargo.toml @@ -20,3 +20,6 @@ proc-macro = true proc-macro2 = "1" quote = "1" syn = { version = "2", features = ["extra-traits"] } + +[lints] +workspace = true diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml index cc716d75d4..669f288d1c 100644 --- a/rust/qemu-api/Cargo.toml +++ b/rust/qemu-api/Cargo.toml @@ -23,6 +23,5 @@ version_check = "~0.9" default = [] allocator = [] -[lints.rust] -unexpected_cfgs = { level = "warn", check-cfg = ['cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)', - 'cfg(has_offset_of)'] } +[lints] +workspace = true diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 1ed79672cc..d719c13f46 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -1,5 +1,5 @@ _qemu_api_cfg = run_command(rustc_args, - '--config-headers', config_host_h, '--features', '--lints', files('Cargo.toml'), + '--config-headers', config_host_h, '--features', files('Cargo.toml'), capture: true, check: true).stdout().strip().splitlines() # _qemu_api_cfg += ['--cfg', 'feature="allocator"'] diff --git a/scripts/rust/rustc_args.py b/scripts/rust/rustc_args.py index 9b9778a1ca..9df131a02b 100644 --- a/scripts/rust/rustc_args.py +++ b/scripts/rust/rustc_args.py @@ -38,11 +38,21 @@ except ImportError: class CargoTOML: tomldata: Mapping[Any, Any] + workspace_data: Mapping[Any, Any] check_cfg: Set[str] - def __init__(self, path: str): - with open(path, 'rb') as f: - self.tomldata = tomllib.load(f) + def __init__(self, path: Optional[str], workspace: Optional[str]): + if path is not None: + with open(path, 'rb') as f: + self.tomldata = tomllib.load(f) + else: + self.tomldata = {"lints": {"workspace": True}} + + if workspace is not None: + with open(workspace, 'rb') as f: + self.workspace_data = tomllib.load(f) + if "workspace" not in self.workspace_data: + self.workspace_data["workspace"] = {} self.check_cfg = set(self.find_check_cfg()) @@ -54,10 +64,12 @@ class CargoTOML: @property def lints(self) -> Mapping[Any, Any]: - return self.get_table("lints") + return self.get_table("lints", True) - def get_table(self, key: str) -> Mapping[Any, Any]: + def get_table(self, key: str, can_be_workspace: bool = False) -> Mapping[Any, Any]: table = self.tomldata.get(key, {}) + if can_be_workspace and table.get("workspace", False) is True: + table = self.workspace_data["workspace"].get(key, {}) return table @@ -136,6 +148,16 @@ def main() -> None: action="store", dest="cargo_toml", help="path to Cargo.toml file", + nargs='?', + ) + parser.add_argument( + "--workspace", + metavar="DIR", + action="store", + dest="workspace", + help="path to root of the workspace", + required=False, + default=None, ) parser.add_argument( "--features", @@ -168,7 +190,11 @@ def main() -> None: logging.debug("args: %s", args) rustc_version = tuple((int(x) for x in args.rustc_version.split('.')[0:2])) - cargo_toml = CargoTOML(args.cargo_toml) + if args.workspace: + workspace_cargo_toml = Path(args.workspace, "Cargo.toml").resolve() + cargo_toml = CargoTOML(args.cargo_toml, str(workspace_cargo_toml)) + else: + cargo_toml = CargoTOML(args.cargo_toml, None) if args.lints: for tok in generate_lint_flags(cargo_toml): From de98c17593218e1c713c6e5d8ad7242c17d90e7e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 7 Nov 2024 10:14:49 +0100 Subject: [PATCH 08/49] rust: build: move strict lints handling to rustc_args.py Make Cargo use unknown_lints = "allow" as well. This is more future proof as we might add new lints to rust/Cargo.toml that are not supported by older versions of rustc or clippy. Signed-off-by: Paolo Bonzini --- meson.build | 12 ++++-------- rust/Cargo.toml | 6 ++++++ scripts/rust/rustc_args.py | 19 ++++++++++++++++--- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/meson.build b/meson.build index 218ae441e3..85f7485473 100644 --- a/meson.build +++ b/meson.build @@ -123,19 +123,15 @@ if have_rust rustc_args = [find_program('scripts/rust/rustc_args.py'), '--rustc-version', rustc.version(), '--workspace', meson.project_source_root() / 'rust'] + if get_option('strict_rust_lints') + rustc_args += ['--strict-lints'] + endif + rustfmt = find_program('rustfmt', required: false) rustc_lint_args = run_command(rustc_args, '--lints', capture: true, check: true).stdout().strip().splitlines() - # Occasionally, we may need to silence warnings and clippy lints that - # were only introduced in newer Rust compiler versions. Do not croak - # in that case; a CI job with rust_strict_lints == true ensures that - # we do not have misspelled allow() attributes. - if not get_option('strict_rust_lints') - rustc_lint_args += ['-A', 'unknown_lints'] - endif - # Apart from procedural macros, our Rust executables will often link # with C code, so include all the libraries that C code needs. This # is safe; https://github.com/rust-lang/rust/pull/54675 says that diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 4bb52bf0bd..358c517bc5 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -11,5 +11,11 @@ unexpected_cfgs = { level = "deny", check-cfg = [ 'cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)', 'cfg(has_offset_of)'] } +# Occasionally, we may need to silence warnings and clippy lints that +# were only introduced in newer Rust compiler versions. Do not croak +# in that case; a CI job with rust_strict_lints == true disables this +# and ensures that we do not have misspelled allow() attributes. +unknown_lints = "allow" + # Prohibit code that is forbidden in Rust 2024 unsafe_op_in_unsafe_fn = "deny" diff --git a/scripts/rust/rustc_args.py b/scripts/rust/rustc_args.py index 9df131a02b..5525b3886f 100644 --- a/scripts/rust/rustc_args.py +++ b/scripts/rust/rustc_args.py @@ -35,6 +35,8 @@ try: except ImportError: import tomli as tomllib +STRICT_LINTS = {"unknown_lints", "warnings"} + class CargoTOML: tomldata: Mapping[Any, Any] @@ -80,7 +82,7 @@ class LintFlag: priority: int -def generate_lint_flags(cargo_toml: CargoTOML) -> Iterable[str]: +def generate_lint_flags(cargo_toml: CargoTOML, strict_lints: bool) -> Iterable[str]: """Converts Cargo.toml lints to rustc -A/-D/-F/-W flags.""" toml_lints = cargo_toml.lints @@ -105,9 +107,13 @@ def generate_lint_flags(cargo_toml: CargoTOML) -> Iterable[str]: # This may change if QEMU ever invokes clippy-driver or rustdoc by # hand. For now, check the syntax but do not add non-rustc lints to # the command line. - if k == "rust": + if k == "rust" and not (strict_lints and lint in STRICT_LINTS): lint_list.append(LintFlag(flags=[flag, prefix + lint], priority=priority)) + if strict_lints: + for lint in STRICT_LINTS: + lint_list.append(LintFlag(flags=["-D", lint], priority=1000000)) + lint_list.sort(key=lambda x: x.priority) for lint in lint_list: yield from lint.flags @@ -184,6 +190,13 @@ def main() -> None: required=False, default="1.0.0", ) + parser.add_argument( + "--strict-lints", + action="store_true", + dest="strict_lints", + help="apply stricter checks (for nightly Rust)", + default=False, + ) args = parser.parse_args() if args.verbose: logging.basicConfig(level=logging.DEBUG) @@ -197,7 +210,7 @@ def main() -> None: cargo_toml = CargoTOML(args.cargo_toml, None) if args.lints: - for tok in generate_lint_flags(cargo_toml): + for tok in generate_lint_flags(cargo_toml, args.strict_lints): print(tok) if rustc_version >= (1, 80): From 7a35e2fb80df4c536cc808e498a259d96d0a804e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2024 18:03:12 +0100 Subject: [PATCH 09/49] rust: fix a couple style issues from clippy These are reported as clippy::semicolon_inside_block and clippy::as_ptr_cast_mut. clippy::semicolon_inside_block can be configured not to lint single-line blocks; just go with the default. Reviewed-by: Junjie Mao Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 8 +++++--- rust/hw/char/pl011/src/memory_ops.rs | 4 +++- rust/qemu-api/tests/tests.rs | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 476cacc844..317a9b3c5a 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -2,7 +2,7 @@ // Author(s): Manos Pitsidianakis // SPDX-License-Identifier: GPL-2.0-or-later -use core::ptr::{addr_of, addr_of_mut, NonNull}; +use core::ptr::{addr_of_mut, NonNull}; use std::{ ffi::CStr, os::raw::{c_int, c_uchar, c_uint, c_void}, @@ -499,7 +499,9 @@ impl PL011State { let flags = self.int_level & self.int_enabled; for (irq, i) in self.interrupts.iter().zip(IRQMASK) { // SAFETY: self.interrupts have been initialized in init(). - unsafe { qemu_set_irq(*irq, i32::from(flags & i != 0)) }; + unsafe { + qemu_set_irq(*irq, i32::from(flags & i != 0)); + } } } @@ -601,7 +603,7 @@ pub unsafe extern "C" fn pl011_create( let sysbus: *mut SysBusDevice = dev.cast::(); qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr); - sysbus_realize_and_unref(sysbus, addr_of!(error_fatal) as *mut *mut Error); + sysbus_realize_and_unref(sysbus, addr_of_mut!(error_fatal)); sysbus_mmio_map(sysbus, 0, addr); sysbus_connect_irq(sysbus, 0, irq); dev diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs index 169d485a4d..c4e8599ba4 100644 --- a/rust/hw/char/pl011/src/memory_ops.rs +++ b/rust/hw/char/pl011/src/memory_ops.rs @@ -33,7 +33,9 @@ unsafe extern "C" fn pl011_read(opaque: *mut c_void, addr: hwaddr, size: c_uint) // SAFETY: self.char_backend is a valid CharBackend instance after it's been // initialized in realize(). let cb_ptr = unsafe { core::ptr::addr_of_mut!(state.as_mut().char_backend) }; - unsafe { qemu_chr_fe_accept_input(cb_ptr) }; + unsafe { + qemu_chr_fe_accept_input(cb_ptr); + } val } diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index 43a4827de1..925f5a3c77 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -74,6 +74,6 @@ fn test_device_decl_macros() { unsafe { module_call_init(module_init_type::MODULE_INIT_QOM); - object_unref(object_new(DummyState::TYPE_NAME.as_ptr()) as *mut _); + object_unref(object_new(DummyState::TYPE_NAME.as_ptr()).cast()); } } From 2f9eec8f72673f97766eba1682a75f06f16302cb Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 6 Nov 2024 11:42:00 +0100 Subject: [PATCH 10/49] rust: build: establish a baseline of lints across all crates Many lints that default to allow can be helpful in detecting bugs or keeping the code style homogeneous. Add them liberally, though perhaps not as liberally as in hw/char/pl011/src/lib.rs. In particular, enabling entire groups can be problematic because of bitrot when new links are added in the future. For Clippy, this is actually a feature that is only present in Cargo 1.74.0 but, since we are not using Cargo to *build* QEMU, only developers will need a new-enough cargo and only to run tools such as clippy. The requirement does not apply to distros that are building QEMU. Reviewed-by: Junjie Mao Signed-off-by: Paolo Bonzini --- rust/Cargo.toml | 68 +++++++++++++++++++++++++++++++++++ rust/hw/char/pl011/src/lib.rs | 19 ++-------- rust/qemu-api/src/bindings.rs | 6 ++-- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 358c517bc5..6ec19b6729 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -19,3 +19,71 @@ unknown_lints = "allow" # Prohibit code that is forbidden in Rust 2024 unsafe_op_in_unsafe_fn = "deny" + +[workspace.lints.rustdoc] +private_intra_doc_links = "allow" + +broken_intra_doc_links = "deny" +invalid_html_tags = "deny" +invalid_rust_codeblocks = "deny" +bare_urls = "deny" +unescaped_backticks = "deny" +redundant_explicit_links = "deny" + +[workspace.lints.clippy] +# default-warn lints +result_unit_err = "allow" +should_implement_trait = "deny" +# can be for a reason, e.g. in callbacks +unused_self = "allow" + +# default-allow lints +as_underscore = "deny" +assertions_on_result_states = "deny" +bool_to_int_with_if = "deny" +borrow_as_ptr = "deny" +cast_lossless = "deny" +dbg_macro = "deny" +debug_assert_with_mut_call = "deny" +derive_partial_eq_without_eq = "deny" +doc_markdown = "deny" +empty_structs_with_brackets = "deny" +ignored_unit_patterns = "deny" +implicit_clone = "deny" +macro_use_imports = "deny" +missing_const_for_fn = "deny" +missing_safety_doc = "deny" +multiple_crate_versions = "deny" +mut_mut = "deny" +needless_bitwise_bool = "deny" +needless_pass_by_ref_mut = "deny" +no_effect_underscore_binding = "deny" +option_option = "deny" +or_fun_call = "deny" +ptr_as_ptr = "deny" +pub_underscore_fields = "deny" +redundant_clone = "deny" +redundant_closure_for_method_calls = "deny" +redundant_else = "deny" +redundant_pub_crate = "deny" +ref_binding_to_reference = "deny" +ref_option_ref = "deny" +return_self_not_must_use = "deny" +same_name_method = "deny" +semicolon_inside_block = "deny" +shadow_unrelated = "deny" +significant_drop_in_scrutinee = "deny" +significant_drop_tightening = "deny" +suspicious_operation_groupings = "deny" +transmute_ptr_to_ptr = "deny" +transmute_undefined_repr = "deny" +type_repetition_in_bounds = "deny" +used_underscore_binding = "deny" + +# nice to have, but cannot be enabled yet +#wildcard_imports = "deny" # still have many bindings::* imports +#ptr_cast_constness = "deny" # needs 1.65.0 for cast_mut()/cast_const() + +# these may have false positives +#option_if_let_else = "deny" +cognitive_complexity = "deny" diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs index cd0a49acb9..4dc0e8f345 100644 --- a/rust/hw/char/pl011/src/lib.rs +++ b/rust/hw/char/pl011/src/lib.rs @@ -14,28 +14,15 @@ //! the [`registers`] module for register types. #![deny( - rustdoc::broken_intra_doc_links, - rustdoc::redundant_explicit_links, clippy::correctness, clippy::suspicious, clippy::complexity, clippy::perf, clippy::cargo, clippy::nursery, - clippy::style, - // restriction group - clippy::dbg_macro, - clippy::as_underscore, - clippy::assertions_on_result_states, - // pedantic group - clippy::doc_markdown, - clippy::borrow_as_ptr, - clippy::cast_lossless, - clippy::option_if_let_else, - clippy::missing_const_for_fn, - clippy::cognitive_complexity, - clippy::missing_safety_doc, - )] + clippy::style +)] +#![allow(clippy::upper_case_acronyms)] #![allow(clippy::result_unit_err)] extern crate bilge; diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index 0b76ec58be..8a9b821bb9 100644 --- a/rust/qemu-api/src/bindings.rs +++ b/rust/qemu-api/src/bindings.rs @@ -7,10 +7,10 @@ non_snake_case, non_upper_case_globals, unsafe_op_in_unsafe_fn, + clippy::pedantic, + clippy::restriction, + clippy::style, clippy::missing_const_for_fn, - clippy::too_many_arguments, - clippy::approx_constant, - clippy::use_self, clippy::useless_transmute, clippy::missing_safety_doc )] From cab1d0bceb6d042674f6892216d1769021f73916 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 13 Nov 2024 09:11:48 +0100 Subject: [PATCH 11/49] rust: build: add "make clippy", "make rustfmt", "make rustdoc" Abstract common invocations of "cargo", that do not require copying the generated bindgen file or setting up MESON_BUILD_ROOT. In the future these could also do completely without cargo and invoke the underlying programs directly. Reviewed-by: Junjie Mao Signed-off-by: Paolo Bonzini --- rust/meson.build | 22 ++++++++++++++++++++++ rust/qemu-api/README.md | 10 ++++++---- rust/qemu-api/build.rs | 9 +++++++-- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/rust/meson.build b/rust/meson.build index def77389cd..91e52b8fb8 100644 --- a/rust/meson.build +++ b/rust/meson.build @@ -2,3 +2,25 @@ subdir('qemu-api-macros') subdir('qemu-api') subdir('hw') + +cargo = find_program('cargo', required: false) + +if cargo.found() + run_target('clippy', + command: [config_host['MESON'], 'devenv', + '--workdir', '@CURRENT_SOURCE_DIR@', + cargo, 'clippy', '--tests'], + depends: bindings_rs) + + run_target('rustfmt', + command: [config_host['MESON'], 'devenv', + '--workdir', '@CURRENT_SOURCE_DIR@', + cargo, 'fmt'], + depends: bindings_rs) + + run_target('rustdoc', + command: [config_host['MESON'], 'devenv', + '--workdir', '@CURRENT_SOURCE_DIR@', + cargo, 'doc', '--no-deps', '--document-private-items'], + depends: bindings_rs) +endif diff --git a/rust/qemu-api/README.md b/rust/qemu-api/README.md index 53810f4888..ed1b7ab263 100644 --- a/rust/qemu-api/README.md +++ b/rust/qemu-api/README.md @@ -5,13 +5,15 @@ This library exports helper Rust types, Rust macros and C FFI bindings for inter The C bindings can be generated with `bindgen`, using this build target: ```console -$ ninja bindings.inc.rs +$ make bindings.inc.rs ``` ## Generate Rust documentation -To generate docs for this crate, including private items: +Common Cargo tasks can be performed from the QEMU build directory -```sh -pyvenv/bin/meson devenv -w ../rust cargo doc --no-deps --document-private-items +```console +$ make clippy +$ make rustfmt +$ make rustdoc ``` diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs index 40644d5e4f..471e6c633d 100644 --- a/rust/qemu-api/build.rs +++ b/rust/qemu-api/build.rs @@ -20,8 +20,13 @@ fn main() -> Result<()> { let file = Path::new(&file); if !Path::new(&file).exists() { panic!(concat!( - "No generated C bindings found! If you want to run `cargo`, start a subshell\n", - "with `meson devenv`, or point MESON_BUILD_ROOT to the top of the build tree." + "\n", + " No generated C bindings found! Maybe you wanted one of\n", + " `make clippy`, `make rustfmt`, `make rustdoc`?\n", + "\n", + " For other uses of `cargo`, start a subshell with\n", + " `pyvenv/bin/meson devenv`, or point MESON_BUILD_ROOT to\n", + " the top of the build tree." )); } From 4e0a11d12fd4439b2f72e989a647fcf3a18b0f69 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 31 Oct 2024 15:10:29 +0100 Subject: [PATCH 12/49] rust: ci: add job that runs Rust tools Code checks, as well as documentation generation, are not yet tied to "make check" because they need new version of the Rust toolchain (even nightly in the case of "rustfmt"). Run them in CI using the existing nightly-Rust container. Signed-off-by: Paolo Bonzini --- .gitlab-ci.d/static_checks.yml | 24 +++++++++++++++++++ .../dockerfiles/fedora-rust-nightly.docker | 4 ++++ tests/lcitool/refresh | 4 ++++ 3 files changed, 32 insertions(+) diff --git a/.gitlab-ci.d/static_checks.yml b/.gitlab-ci.d/static_checks.yml index ad9f426a52..c0ba453382 100644 --- a/.gitlab-ci.d/static_checks.yml +++ b/.gitlab-ci.d/static_checks.yml @@ -46,3 +46,27 @@ check-python-tox: QEMU_JOB_OPTIONAL: 1 needs: job: python-container + +check-rust-tools-nightly: + extends: .base_job_template + stage: test + image: $CI_REGISTRY_IMAGE/qemu/fedora-rust-nightly:$QEMU_CI_CONTAINER_TAG + script: + - source scripts/ci/gitlab-ci-section + - section_start test "Running Rust code checks" + - cd build + - pyvenv/bin/meson devenv -w ../rust ${CARGO-cargo} fmt --check + - make clippy + - make rustdoc + - section_end test + variables: + GIT_DEPTH: 1 + allow_failure: true + needs: + - job: build-system-fedora-rust-nightly + artifacts: true + artifacts: + when: on_success + expire_in: 2 days + paths: + - rust/target/doc diff --git a/tests/docker/dockerfiles/fedora-rust-nightly.docker b/tests/docker/dockerfiles/fedora-rust-nightly.docker index 9180c8b522..a8e4fb279a 100644 --- a/tests/docker/dockerfiles/fedora-rust-nightly.docker +++ b/tests/docker/dockerfiles/fedora-rust-nightly.docker @@ -155,6 +155,7 @@ ENV PYTHON "/usr/bin/python3" RUN dnf install -y wget ENV RUSTUP_HOME=/usr/local/rustup CARGO_HOME=/usr/local/cargo ENV RUSTC=/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc +ENV CARGO=/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo RUN set -eux && \ rustArch='x86_64-unknown-linux-gnu' && \ rustupSha256='6aeece6993e902708983b209d04c0d1dbb14ebb405ddb87def578d41f920f56d' && \ @@ -165,10 +166,13 @@ RUN set -eux && \ ./rustup-init -y --no-modify-path --profile default --default-toolchain nightly --default-host ${rustArch} && \ chmod -R a+w $RUSTUP_HOME $CARGO_HOME && \ /usr/local/cargo/bin/rustup --version && \ + /usr/local/cargo/bin/rustup run nightly cargo --version && \ /usr/local/cargo/bin/rustup run nightly rustc --version && \ + test "$CARGO" = "$(/usr/local/cargo/bin/rustup +nightly which cargo)" && \ test "$RUSTC" = "$(/usr/local/cargo/bin/rustup +nightly which rustc)" ENV PATH=$CARGO_HOME/bin:$PATH RUN /usr/local/cargo/bin/rustup run nightly cargo install bindgen-cli +RUN $CARGO --list # As a final step configure the user (if env is defined) ARG USER ARG UID diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh index 51012783c0..6720516b94 100755 --- a/tests/lcitool/refresh +++ b/tests/lcitool/refresh @@ -121,6 +121,7 @@ fedora_rustup_nightly_extras = [ "RUN dnf install -y wget\n", "ENV RUSTUP_HOME=/usr/local/rustup CARGO_HOME=/usr/local/cargo\n", "ENV RUSTC=/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc\n", + "ENV CARGO=/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo\n", "RUN set -eux && \\\n", " rustArch='x86_64-unknown-linux-gnu' && \\\n", " rustupSha256='6aeece6993e902708983b209d04c0d1dbb14ebb405ddb87def578d41f920f56d' && \\\n", @@ -131,10 +132,13 @@ fedora_rustup_nightly_extras = [ " ./rustup-init -y --no-modify-path --profile default --default-toolchain nightly --default-host ${rustArch} && \\\n", " chmod -R a+w $RUSTUP_HOME $CARGO_HOME && \\\n", " /usr/local/cargo/bin/rustup --version && \\\n", + " /usr/local/cargo/bin/rustup run nightly cargo --version && \\\n", " /usr/local/cargo/bin/rustup run nightly rustc --version && \\\n", + ' test "$CARGO" = "$(/usr/local/cargo/bin/rustup +nightly which cargo)" && \\\n', ' test "$RUSTC" = "$(/usr/local/cargo/bin/rustup +nightly which rustc)"\n', 'ENV PATH=$CARGO_HOME/bin:$PATH\n', 'RUN /usr/local/cargo/bin/rustup run nightly cargo install bindgen-cli\n', + 'RUN $CARGO --list\n', ] ubuntu2204_bindgen_extras = [ From 8a88b55f698ad660a11d24925cedcff106053ff4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2024 18:44:56 +0100 Subject: [PATCH 13/49] rust: fix doc test syntax Allow "cargo test --doc" to pass. Reviewed-by: Junjie Mao Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/zeroable.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs index 13cdb2ccba..6125aeed8b 100644 --- a/rust/qemu-api/src/zeroable.rs +++ b/rust/qemu-api/src/zeroable.rs @@ -7,9 +7,9 @@ use std::ptr; /// behavior. This trait in principle could be implemented as just: /// /// ``` -/// const ZERO: Self = unsafe { -/// ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() -/// }, +/// pub unsafe trait Zeroable: Default { +/// const ZERO: Self = unsafe { ::core::mem::MaybeUninit::::zeroed().assume_init() }; +/// } /// ``` /// /// The need for a manual implementation is only because `zeroed()` cannot From a3057c52f45c280bffc22980f535799a12500d66 Mon Sep 17 00:00:00 2001 From: Junjie Mao Date: Thu, 17 Oct 2024 22:32:44 +0800 Subject: [PATCH 14/49] rust/qemu-api: Fix fragment-specifiers in define_property macro For the matcher of macro, "expr" is used for expressions, while "ident" is used for variable/function names, and "ty" matches types. In define_property macro, $field is a member name of type $state, so it should be defined as "ident", though offset_of! doesn't complain about this. $type is the type of $field, since it is not used in the macro, so that no type mismatch error is triggered either. Fix fragment-specifiers of $field and $type. Signed-off-by: Junjie Mao Co-developed-by: Zhao Liu Signed-off-by: Zhao Liu Link: https://lore.kernel.org/r/20241017143245.1248589-2-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/device_class.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs index 0ba798d3e3..922bbce1bb 100644 --- a/rust/qemu-api/src/device_class.rs +++ b/rust/qemu-api/src/device_class.rs @@ -27,7 +27,7 @@ macro_rules! device_class_init { #[macro_export] macro_rules! define_property { - ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr, default = $defval:expr$(,)*) => { + ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty, default = $defval:expr$(,)*) => { $crate::bindings::Property { // use associated function syntax for type checking name: ::std::ffi::CStr::as_ptr($name), @@ -38,7 +38,7 @@ macro_rules! define_property { ..$crate::zeroable::Zeroable::ZERO } }; - ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr$(,)*) => { + ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty$(,)*) => { $crate::bindings::Property { // use associated function syntax for type checking name: ::std::ffi::CStr::as_ptr($name), From 4cc055039fc1c1ab18f172fa791aa3d48d7d20aa Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 28 Nov 2024 13:21:13 +0100 Subject: [PATCH 15/49] clock: clear callback on unparent Signed-off-by: Paolo Bonzini --- hw/core/clock.c | 22 +++++++++++++++++----- hw/core/qdev-clock.c | 5 +---- include/hw/clock.h | 8 -------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/hw/core/clock.c b/hw/core/clock.c index cbe7b1bc46..391095eb4e 100644 --- a/hw/core/clock.c +++ b/hw/core/clock.c @@ -44,16 +44,12 @@ Clock *clock_new(Object *parent, const char *name) void clock_set_callback(Clock *clk, ClockCallback *cb, void *opaque, unsigned int events) { + assert(OBJECT(clk)->parent); clk->callback = cb; clk->callback_opaque = opaque; clk->callback_events = events; } -void clock_clear_callback(Clock *clk) -{ - clock_set_callback(clk, NULL, NULL, 0); -} - bool clock_set(Clock *clk, uint64_t period) { if (clk->period == period) { @@ -168,6 +164,16 @@ static void clock_period_prop_get(Object *obj, Visitor *v, const char *name, visit_type_uint64(v, name, &period, errp); } +static void clock_unparent(Object *obj) +{ + /* + * Callback are registered by the parent, which might die anytime after + * it's unparented the children. Avoid having a callback to a deleted + * object in case the clock is still referenced somewhere else (eg: by + * a clock output). + */ + clock_set_callback(CLOCK(obj), NULL, NULL, 0); +} static void clock_initfn(Object *obj) { @@ -200,11 +206,17 @@ static void clock_finalizefn(Object *obj) g_free(clk->canonical_path); } +static void clock_class_init(ObjectClass *klass, void *data) +{ + klass->unparent = clock_unparent; +} + static const TypeInfo clock_info = { .name = TYPE_CLOCK, .parent = TYPE_OBJECT, .instance_size = sizeof(Clock), .instance_init = clock_initfn, + .class_init = clock_class_init, .instance_finalize = clock_finalizefn, }; diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c index 82799577f3..ca65685c04 100644 --- a/hw/core/qdev-clock.c +++ b/hw/core/qdev-clock.c @@ -87,11 +87,8 @@ void qdev_finalize_clocklist(DeviceState *dev) if (!ncl->output && !ncl->alias) { /* * We kept a reference on the input clock to ensure it lives up to - * this point so we can safely remove the callback. - * It avoids having a callback to a deleted object if ncl->clock - * is still referenced somewhere else (eg: by a clock output). + * this point; it is used by the monitor to show the frequency. */ - clock_clear_callback(ncl->clock); object_unref(OBJECT(ncl->clock)); } g_free(ncl->name); diff --git a/include/hw/clock.h b/include/hw/clock.h index eb58599131..a279bd4ba5 100644 --- a/include/hw/clock.h +++ b/include/hw/clock.h @@ -141,14 +141,6 @@ Clock *clock_new(Object *parent, const char *name); void clock_set_callback(Clock *clk, ClockCallback *cb, void *opaque, unsigned int events); -/** - * clock_clear_callback: - * @clk: the clock to delete the callback from - * - * Unregister the callback registered with clock_set_callback. - */ -void clock_clear_callback(Clock *clk); - /** * clock_set_source: * @clk: the clock. From 118b46a461fb42ecbb13cbd0ca71167b128b050e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 29 Nov 2024 07:47:01 +0100 Subject: [PATCH 16/49] clock: treat outputs and inputs the same in NamedClockList Signed-off-by: Paolo Bonzini --- hw/core/qdev-clock.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c index ca65685c04..2f9d6cb757 100644 --- a/hw/core/qdev-clock.c +++ b/hw/core/qdev-clock.c @@ -48,14 +48,6 @@ static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, if (clk == NULL) { clk = CLOCK(object_new(TYPE_CLOCK)); object_property_add_child(OBJECT(dev), name, OBJECT(clk)); - if (output) { - /* - * Remove object_new()'s initial reference. - * Note that for inputs, the reference created by object_new() - * will be deleted in qdev_finalize_clocklist(). - */ - object_unref(OBJECT(clk)); - } } else { object_property_add_link(OBJECT(dev), name, object_get_typename(OBJECT(clk)), @@ -84,7 +76,7 @@ void qdev_finalize_clocklist(DeviceState *dev) QLIST_FOREACH_SAFE(ncl, &dev->clocks, node, ncl_next) { QLIST_REMOVE(ncl, node); - if (!ncl->output && !ncl->alias) { + if (!ncl->alias) { /* * We kept a reference on the input clock to ensure it lives up to * this point; it is used by the monitor to show the frequency. From 7d95502ced4220edb91efa4621054bbce237528a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 28 Nov 2024 18:30:39 +0100 Subject: [PATCH 17/49] clock: inline most of qdev_init_clocklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move object creation out of qdev_init_clocklist. The input/output cases are very simple, and the aliases are completely different. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Paolo Bonzini --- hw/core/qdev-clock.c | 71 +++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 44 deletions(-) diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c index 2f9d6cb757..dacafa4e03 100644 --- a/hw/core/qdev-clock.c +++ b/hw/core/qdev-clock.c @@ -22,7 +22,7 @@ * Add a new clock in a device */ static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, - bool output, Clock *clk) + bool alias, bool output, Clock *clk) { NamedClockList *ncl; @@ -38,31 +38,8 @@ static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, */ ncl = g_new0(NamedClockList, 1); ncl->name = g_strdup(name); + ncl->alias = alias; ncl->output = output; - ncl->alias = (clk != NULL); - - /* - * Trying to create a clock whose name clashes with some other - * clock or property is a bug in the caller and we will abort(). - */ - if (clk == NULL) { - clk = CLOCK(object_new(TYPE_CLOCK)); - object_property_add_child(OBJECT(dev), name, OBJECT(clk)); - } else { - object_property_add_link(OBJECT(dev), name, - object_get_typename(OBJECT(clk)), - (Object **) &ncl->clock, - NULL, OBJ_PROP_LINK_STRONG); - /* - * Since the link property has the OBJ_PROP_LINK_STRONG flag, the clk - * object reference count gets decremented on property deletion. - * However object_property_add_link does not increment it since it - * doesn't know the linked object. Increment it here to ensure the - * aliased clock stays alive during this device life-time. - */ - object_ref(OBJECT(clk)); - } - ncl->clock = clk; QLIST_INSERT_HEAD(&dev->clocks, ncl, node); @@ -90,29 +67,25 @@ void qdev_finalize_clocklist(DeviceState *dev) Clock *qdev_init_clock_out(DeviceState *dev, const char *name) { - NamedClockList *ncl; + Clock *clk = CLOCK(object_new(TYPE_CLOCK)); + object_property_add_child(OBJECT(dev), name, OBJECT(clk)); - assert(name); - - ncl = qdev_init_clocklist(dev, name, true, NULL); - - return ncl->clock; + qdev_init_clocklist(dev, name, false, true, clk); + return clk; } Clock *qdev_init_clock_in(DeviceState *dev, const char *name, ClockCallback *callback, void *opaque, unsigned int events) { - NamedClockList *ncl; - - assert(name); - - ncl = qdev_init_clocklist(dev, name, false, NULL); + Clock *clk = CLOCK(object_new(TYPE_CLOCK)); + object_property_add_child(OBJECT(dev), name, OBJECT(clk)); + qdev_init_clocklist(dev, name, false, false, clk); if (callback) { - clock_set_callback(ncl->clock, callback, opaque, events); + clock_set_callback(clk, callback, opaque, events); } - return ncl->clock; + return clk; } void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks) @@ -183,15 +156,25 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name) Clock *qdev_alias_clock(DeviceState *dev, const char *name, DeviceState *alias_dev, const char *alias_name) { - NamedClockList *ncl; + NamedClockList *ncl = qdev_get_clocklist(dev, name); + Clock *clk = ncl->clock; - assert(name && alias_name); + ncl = qdev_init_clocklist(alias_dev, alias_name, true, ncl->output, clk); - ncl = qdev_get_clocklist(dev, name); + object_property_add_link(OBJECT(alias_dev), alias_name, + TYPE_CLOCK, + (Object **) &ncl->clock, + NULL, OBJ_PROP_LINK_STRONG); + /* + * Since the link property has the OBJ_PROP_LINK_STRONG flag, the clk + * object reference count gets decremented on property deletion. + * However object_property_add_link does not increment it since it + * doesn't know the linked object. Increment it here to ensure the + * aliased clock stays alive during this device life-time. + */ + object_ref(OBJECT(clk)); - qdev_init_clocklist(alias_dev, alias_name, ncl->output, ncl->clock); - - return ncl->clock; + return clk; } void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source) From feccfa77bed975e2e60c90756f08b8a56df6daf6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 13 Nov 2024 19:07:10 +0100 Subject: [PATCH 18/49] kvm: remove unnecessary #ifdef Signed-off-by: Paolo Bonzini --- target/i386/kvm/kvm_i386.h | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h index 9de9c0d303..7edb154a16 100644 --- a/target/i386/kvm/kvm_i386.h +++ b/target/i386/kvm/kvm_i386.h @@ -13,8 +13,7 @@ #include "sysemu/kvm.h" -#ifdef CONFIG_KVM - +/* always false if !CONFIG_KVM */ #define kvm_pit_in_kernel() \ (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) #define kvm_pic_in_kernel() \ @@ -22,14 +21,6 @@ #define kvm_ioapic_in_kernel() \ (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) -#else - -#define kvm_pit_in_kernel() 0 -#define kvm_pic_in_kernel() 0 -#define kvm_ioapic_in_kernel() 0 - -#endif /* CONFIG_KVM */ - bool kvm_has_smm(void); bool kvm_enable_x2apic(void); bool kvm_hv_vpindex_settable(void); From f34562766867efe94e451e9ec3a2f19a70841e16 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:19 +0800 Subject: [PATCH 19/49] arm: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-2-zhao1.liu@intel.com --- hw/arm/armsse.c | 2 +- hw/arm/smmuv3.c | 4 ++-- target/arm/cpu.c | 2 +- target/arm/cpu64.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c index 255346a595..58ed504b2b 100644 --- a/hw/arm/armsse.c +++ b/hw/arm/armsse.c @@ -1731,7 +1731,7 @@ static void armsse_register_types(void) .class_init = armsse_class_init, .class_data = (void *)&armsse_variants[i], }; - type_register(&ti); + type_register_static(&ti); } } diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index 4c49b5a885..6e847e8773 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -2065,8 +2065,8 @@ static const TypeInfo smmuv3_iommu_memory_region_info = { static void smmuv3_register_types(void) { - type_register(&smmuv3_type_info); - type_register(&smmuv3_iommu_memory_region_info); + type_register_static(&smmuv3_type_info); + type_register_static(&smmuv3_iommu_memory_region_info); } type_init(smmuv3_register_types) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 6938161b95..17519b5e46 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -2755,7 +2755,7 @@ void arm_cpu_register(const ARMCPUInfo *info) }; type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name); - type_register(&type_info); + type_register_static(&type_info); g_free((void *)type_info.name); } diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 458d1cee01..c1cac912a0 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -841,7 +841,7 @@ void aarch64_cpu_register(const ARMCPUInfo *info) }; type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name); - type_register(&type_info); + type_register_static(&type_info); g_free((void *)type_info.name); } From 55398938b427aa430d70f576f8240fe5977a44a7 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:20 +0800 Subject: [PATCH 20/49] hw/block: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-3-zhao1.liu@intel.com --- hw/block/m25p80.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index e2e84f8b5f..748594524e 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -1894,7 +1894,7 @@ static void m25p80_register_types(void) .class_init = m25p80_class_init, .class_data = (void *)&known_devices[i], }; - type_register(&ti); + type_register_static(&ti); } } From 29ec04f08933702f7916ce2ffd03e87aee203fb9 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:21 +0800 Subject: [PATCH 21/49] hw/net: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-4-zhao1.liu@intel.com --- hw/net/e1000.c | 2 +- hw/net/eepro100.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 5012b96464..ab72236d18 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -1774,7 +1774,7 @@ static void e1000_register_types(void) type_info.class_data = (void *)info; type_info.class_init = e1000_class_init; - type_register(&type_info); + type_register_static(&type_info); } } diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index c8a88b9813..20b22d8e49 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -2102,7 +2102,7 @@ static void eepro100_register_types(void) { }, }; - type_register(&type_info); + type_register_static(&type_info); } } From 6e0a88837a74f6dc668c1838d2259a83deb7285b Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:22 +0800 Subject: [PATCH 22/49] ppc: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-5-zhao1.liu@intel.com --- hw/ppc/spapr.c | 2 +- target/ppc/kvm.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0d4efaa0c0..827e7d6b14 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -4723,7 +4723,7 @@ static void spapr_machine_latest_class_options(MachineClass *mc) static void MACHINE_VER_SYM(register, spapr, __VA_ARGS__)(void) \ { \ MACHINE_VER_DELETION(__VA_ARGS__); \ - type_register(&MACHINE_VER_SYM(info, spapr, __VA_ARGS__)); \ + type_register_static(&MACHINE_VER_SYM(info, spapr, __VA_ARGS__)); \ } \ type_init(MACHINE_VER_SYM(register, spapr, __VA_ARGS__)) diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index 3efc28f18b..0d464824db 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -2633,7 +2633,7 @@ static int kvm_ppc_register_host_cpu_type(void) return -1; } type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc)); - type_register(&type_info); + type_register_static(&type_info); /* override TCG default cpu type with 'host' cpu model */ object_class_foreach(pseries_machine_class_fixup, TYPE_SPAPR_MACHINE, false, NULL); From da2608b7b6370d49b15c348e2e66c54b0a9cc90f Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:23 +0800 Subject: [PATCH 23/49] hw/rtc: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-6-zhao1.liu@intel.com --- hw/rtc/m48t59-isa.c | 2 +- hw/rtc/m48t59.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/rtc/m48t59-isa.c b/hw/rtc/m48t59-isa.c index 6e9723fdf1..b642b82680 100644 --- a/hw/rtc/m48t59-isa.c +++ b/hw/rtc/m48t59-isa.c @@ -161,7 +161,7 @@ static void m48t59_isa_register_types(void) for (i = 0; i < ARRAY_SIZE(m48txx_isa_info); i++) { isa_type_info.name = m48txx_isa_info[i].bus_name; isa_type_info.class_data = &m48txx_isa_info[i]; - type_register(&isa_type_info); + type_register_static(&isa_type_info); } } diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c index 48846d8df4..90299ea56f 100644 --- a/hw/rtc/m48t59.c +++ b/hw/rtc/m48t59.c @@ -679,7 +679,7 @@ static void m48t59_register_types(void) for (i = 0; i < ARRAY_SIZE(m48txx_sysbus_info); i++) { sysbus_type_info.name = m48txx_sysbus_info[i].bus_name; sysbus_type_info.class_data = &m48txx_sysbus_info[i]; - type_register(&sysbus_type_info); + type_register_static(&sysbus_type_info); } } From dbeafa453baec78a9f77580f83177f4fdfad6852 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:24 +0800 Subject: [PATCH 24/49] hw/scsi: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because\ type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-7-zhao1.liu@intel.com --- hw/scsi/megasas.c | 2 +- hw/scsi/mptsas.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index b33229d71a..df58aeb995 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2576,7 +2576,7 @@ static void megasas_register_types(void) type_info.class_init = megasas_class_init; type_info.interfaces = info->interfaces; - type_register(&type_info); + type_register_static(&type_info); } } diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index 361b75e633..c6bc3479e9 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1450,7 +1450,7 @@ static const TypeInfo mptsas_info = { static void mptsas_register_types(void) { - type_register(&mptsas_info); + type_register_static(&mptsas_info); } type_init(mptsas_register_types) From bead9801750b8cd6da257cbf38ec184d40e02f47 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:25 +0800 Subject: [PATCH 25/49] hw/sensor: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-8-zhao1.liu@intel.com --- hw/sensor/tmp421.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/sensor/tmp421.c b/hw/sensor/tmp421.c index b6f0b62ab1..82e604279c 100644 --- a/hw/sensor/tmp421.c +++ b/hw/sensor/tmp421.c @@ -384,7 +384,7 @@ static void tmp421_register_types(void) .class_init = tmp421_class_init, .class_data = (void *) &devices[i], }; - type_register(&ti); + type_register_static(&ti); } } From 220a81eb2f08c33eeb91deab4fd43bd2853313b9 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:26 +0800 Subject: [PATCH 26/49] hw/usb: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-9-zhao1.liu@intel.com --- hw/usb/hcd-ehci-pci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c index c94fc9f6c5..dd06451e23 100644 --- a/hw/usb/hcd-ehci-pci.c +++ b/hw/usb/hcd-ehci-pci.c @@ -228,7 +228,7 @@ static void ehci_pci_register_types(void) for (i = 0; i < ARRAY_SIZE(ehci_pci_info); i++) { ehci_type_info.name = ehci_pci_info[i].name; ehci_type_info.class_data = ehci_pci_info + i; - type_register(&ehci_type_info); + type_register_static(&ehci_type_info); } } diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 3d0339af7b..65c1f93cc9 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -1362,7 +1362,7 @@ static void uhci_register_types(void) for (i = 0; i < ARRAY_SIZE(uhci_info); i++) { uhci_type_info.name = uhci_info[i].name; uhci_type_info.class_data = uhci_info + i; - type_register(&uhci_type_info); + type_register_static(&uhci_type_info); } } From 3b5f3070386e5b2a48e0e96ff96ef2251131eeb8 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:27 +0800 Subject: [PATCH 27/49] hw/virtio: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-10-zhao1.liu@intel.com --- hw/virtio/virtio-pci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 5a394821da..5c6c2019ce 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -2511,9 +2511,9 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t) base_type_info.class_data = (void *)t; } - type_register(&base_type_info); + type_register_static(&base_type_info); if (generic_type_info.name) { - type_register(&generic_type_info); + type_register_static(&generic_type_info); } if (t->non_transitional_name) { @@ -2527,7 +2527,7 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t) { } }, }; - type_register(&non_transitional_type_info); + type_register_static(&non_transitional_type_info); } if (t->transitional_name) { @@ -2544,7 +2544,7 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t) { } }, }; - type_register(&transitional_type_info); + type_register_static(&transitional_type_info); } g_free(base_name); } From 81b69b502e3f67d357766ad42ae9a3f8bd675ad2 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:28 +0800 Subject: [PATCH 28/49] i386: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-11-zhao1.liu@intel.com --- include/hw/i386/pc.h | 4 ++-- target/i386/cpu.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 14ee06287d..b3477ad6a2 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -316,7 +316,7 @@ extern const size_t pc_compat_2_3_len; }; \ static void pc_machine_init_##suffix(void) \ { \ - type_register(&pc_machine_type_##suffix); \ + type_register_static(&pc_machine_type_##suffix); \ } \ type_init(pc_machine_init_##suffix) @@ -344,7 +344,7 @@ extern const size_t pc_compat_2_3_len; static void MACHINE_VER_SYM(register, namesym, __VA_ARGS__)(void) \ { \ MACHINE_VER_DELETION(__VA_ARGS__); \ - type_register(&MACHINE_VER_SYM(info, namesym, __VA_ARGS__)); \ + type_register_static(&MACHINE_VER_SYM(info, namesym, __VA_ARGS__)); \ } \ type_init(MACHINE_VER_SYM(register, namesym, __VA_ARGS__)); diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 3725dbbc4b..305f2a41cf 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6429,7 +6429,7 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model) .class_data = model, }; - type_register(&ti); + type_register_static(&ti); } From 96a4616dfcd3caa956373469fc70ff882ab1af9f Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:29 +0800 Subject: [PATCH 29/49] target/mips: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-12-zhao1.liu@intel.com --- target/mips/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/mips/cpu.c b/target/mips/cpu.c index d0a43b6d5c..4feacc88c0 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -626,7 +626,7 @@ static void mips_register_cpudef_type(const struct mips_def_t *def) .class_data = (void *)def, }; - type_register(&ti); + type_register_static(&ti); g_free(typename); } From 2f02b71b5a51c0536280e56109a37e1360c52c50 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:30 +0800 Subject: [PATCH 30/49] target/sparc: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-13-zhao1.liu@intel.com --- target/sparc/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c index dd7af86de7..adba24af2c 100644 --- a/target/sparc/cpu.c +++ b/target/sparc/cpu.c @@ -1008,7 +1008,7 @@ static void sparc_register_cpudef_type(const struct sparc_def_t *def) .class_data = (void *)def, }; - type_register(&ti); + type_register_static(&ti); g_free(typename); } From 2780dd6acbed68b1abacc54b1ade0e883bbbb371 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:31 +0800 Subject: [PATCH 31/49] target/xtensa: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-14-zhao1.liu@intel.com --- target/xtensa/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c index ca214b948a..2978c471c1 100644 --- a/target/xtensa/helper.c +++ b/target/xtensa/helper.c @@ -198,7 +198,7 @@ void xtensa_register_core(XtensaConfigList *node) node->next = xtensa_cores; xtensa_cores = node; type.name = g_strdup_printf(XTENSA_CPU_TYPE_NAME("%s"), node->config->name); - type_register(&type); + type_register_static(&type); g_free((gpointer)type.name); } From e5d60d930cc328e773e14670f3b6c60f94b3e167 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:32 +0800 Subject: [PATCH 32/49] ui: Replace type_register() with type_register_static() Replace type_register() with type_register_static() because type_register() will be deprecated. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-15-zhao1.liu@intel.com --- ui/console-vc.c | 2 +- ui/dbus.c | 2 +- ui/gtk.c | 2 +- ui/spice-app.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ui/console-vc.c b/ui/console-vc.c index 53fcee88f4..fe20579832 100644 --- a/ui/console-vc.c +++ b/ui/console-vc.c @@ -1073,6 +1073,6 @@ void qemu_console_early_init(void) { /* set the default vc driver */ if (!object_class_by_name(TYPE_CHARDEV_VC)) { - type_register(&char_vc_type_info); + type_register_static(&char_vc_type_info); } } diff --git a/ui/dbus.c b/ui/dbus.c index 7ecd39e784..d60b59cc54 100644 --- a/ui/dbus.c +++ b/ui/dbus.c @@ -476,7 +476,7 @@ early_dbus_init(DisplayOptions *opts) #endif } - type_register(&dbus_vc_type_info); + type_register_static(&dbus_vc_type_info); } static void diff --git a/ui/gtk.c b/ui/gtk.c index bf9d3dd679..f9a53ea78e 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -2540,7 +2540,7 @@ static void early_gtk_display_init(DisplayOptions *opts) keycode_map = gd_get_keymap(&keycode_maplen); #if defined(CONFIG_VTE) - type_register(&char_gd_vc_type_info); + type_register_static(&char_gd_vc_type_info); #endif } diff --git a/ui/spice-app.c b/ui/spice-app.c index a10b4a58fe..2a93ae5918 100644 --- a/ui/spice-app.c +++ b/ui/spice-app.c @@ -173,7 +173,7 @@ static void spice_app_display_early_init(DisplayOptions *opts) exit(1); } - type_register(&char_vc_type_info); + type_register_static(&char_vc_type_info); sock_path = g_strjoin("", app_dir, "/", "spice.sock", NULL); qopts = qemu_opts_create(list, NULL, 0, &error_abort); From 4e40d50558432c0e2344ee1838e58e6299e63884 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:33 +0800 Subject: [PATCH 33/49] script/codeconverter/qom_type_info: Deprecate MakeTypeRegisterStatic and MakeTypeRegisterNotStatic Deprecate MakeTypeRegisterStatic and MakeTypeRegisterNotStatic because type_register() will be deprecated, then only type_register_static() is used. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-16-zhao1.liu@intel.com --- .../codeconverter/qom_type_info.py | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/scripts/codeconverter/codeconverter/qom_type_info.py b/scripts/codeconverter/codeconverter/qom_type_info.py index 255cb59923..f92c3a4730 100644 --- a/scripts/codeconverter/codeconverter/qom_type_info.py +++ b/scripts/codeconverter/codeconverter/qom_type_info.py @@ -901,26 +901,6 @@ class TypeRegisterCall(FileMatch): regexp = S(r'^[ \t]*', NAMED('func_name', 'type_register'), r'\s*\(&\s*', NAMED('name', RE_IDENTIFIER), r'\s*\);[ \t]*\n') -class MakeTypeRegisterStatic(TypeRegisterCall): - """Make type_register() call static if variable is static const""" - def gen_patches(self): - var = self.file.find_match(TypeInfoVar, self.name) - if var is None: - self.warn("can't find TypeInfo var declaration for %s", self.name) - return - if var.is_static() and var.is_const(): - yield self.group_match('func_name').make_patch('type_register_static') - -class MakeTypeRegisterNotStatic(TypeRegisterStaticCall): - """Make type_register() call static if variable is static const""" - def gen_patches(self): - var = self.file.find_match(TypeInfoVar, self.name) - if var is None: - self.warn("can't find TypeInfo var declaration for %s", self.name) - return - if not var.is_static() or not var.is_const(): - yield self.group_match('func_name').make_patch('type_register') - class TypeInfoMacro(FileMatch): """TYPE_INFO macro usage""" regexp = S(r'^[ \t]*TYPE_INFO\s*\(\s*', NAMED('name', RE_IDENTIFIER), r'\s*\)[ \t]*;?[ \t]*\n') From 281305d3e08ac7330dfe7cf7b3978c119a888bad Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Tue, 29 Oct 2024 16:59:34 +0800 Subject: [PATCH 34/49] qom/object: Remove type_register() At present, type_register() and type_register_static() are identical, although their documentation expects the *_static variant to accept the Typeinfo with the strings that have the static lifetime. However, the code implementation doesn't have any check or guarantee for static lifetime. In fact, this is unnecessary because type_new() duplicates all strings, thereby taking ownership of them. Therefore, type_register() and type_register_static() are redundant, so one of them should be removed. Since the changes required to remove type_register() were smaller, type_register() was replaced with type_register_static() throughout the code base. Drop its definition, and delete the requirement about string lifetime from the documentation. Signed-off-by: Zhao Liu Signed-off-by: Paolo Bonzini Link: https://lore.kernel.org/r/20241029085934.2799066-17-zhao1.liu@intel.com --- include/qom/object.h | 14 -------------- qom/object.c | 7 +------ 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index 43c135984a..a201c9712a 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -880,24 +880,10 @@ const char *object_get_typename(const Object *obj); * type_register_static: * @info: The #TypeInfo of the new type. * - * @info and all of the strings it points to should exist for the life time - * that the type is registered. - * * Returns: the new #Type. */ Type type_register_static(const TypeInfo *info); -/** - * type_register: - * @info: The #TypeInfo of the new type - * - * Unlike type_register_static(), this call does not require @info or its - * string members to continue to exist after the call returns. - * - * Returns: the new #Type. - */ -Type type_register(const TypeInfo *info); - /** * type_register_static_array: * @infos: The array of the new type #TypeInfo structures. diff --git a/qom/object.c b/qom/object.c index 9edc06d391..c7660f9a09 100644 --- a/qom/object.c +++ b/qom/object.c @@ -175,17 +175,12 @@ static TypeImpl *type_register_internal(const TypeInfo *info) return ti; } -TypeImpl *type_register(const TypeInfo *info) +TypeImpl *type_register_static(const TypeInfo *info) { assert(info->parent); return type_register_internal(info); } -TypeImpl *type_register_static(const TypeInfo *info) -{ - return type_register(info); -} - void type_register_static_array(const TypeInfo *infos, int nr_infos) { int i; From 37fb26601dd156369ebb84096c2ecfbe89f0a83b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 6 Nov 2024 08:59:07 +0100 Subject: [PATCH 35/49] bql: check that the BQL is not dropped within marked sections The Big QEMU Lock (BQL) is used to provide interior mutability to Rust code. While BqlCell performs indivisible accesses, an equivalent of RefCell will allow the borrower to hold to the interior content for a long time. If the BQL is dropped, another thread could come and mutate the data from C code (Rust code would panic on borrow_mut() instead). In order to prevent this, add a new BQL primitive that can mark BQL-atomic sections and aborts if the BQL is dropped within them. Signed-off-by: Paolo Bonzini --- include/qemu/main-loop.h | 15 +++++++++++++++ stubs/iothread-lock.c | 15 +++++++++++++++ system/cpus.c | 15 +++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 5764db157c..646306c272 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -262,6 +262,21 @@ AioContext *iohandler_get_aio_context(void); */ bool bql_locked(void); +/** + * bql_block: Allow/deny releasing the BQL + * + * The Big QEMU Lock (BQL) is used to provide interior mutability to + * Rust code, but this only works if other threads cannot run while + * the Rust code has an active borrow. This is because C code in + * other threads could come in and mutate data under the Rust code's + * feet. + * + * @increase: Whether to increase or decrease the blocking counter. + * Releasing the BQL while the counter is nonzero triggers + * an assertion failure. + */ +void bql_block_unlock(bool increase); + /** * qemu_in_main_thread: return whether it's possible to safely access * the global state of the block layer. diff --git a/stubs/iothread-lock.c b/stubs/iothread-lock.c index d7890e5581..5467659895 100644 --- a/stubs/iothread-lock.c +++ b/stubs/iothread-lock.c @@ -1,6 +1,8 @@ #include "qemu/osdep.h" #include "qemu/main-loop.h" +static uint32_t bql_unlock_blocked; + bool bql_locked(void) { return false; @@ -12,4 +14,17 @@ void bql_lock_impl(const char *file, int line) void bql_unlock(void) { + assert(!bql_unlock_blocked); +} + +void bql_block_unlock(bool increase) +{ + uint32_t new_value; + + assert(bql_locked()); + + /* check for overflow! */ + new_value = bql_unlock_blocked + increase - !increase; + assert((new_value > bql_unlock_blocked) == increase); + bql_unlock_blocked = new_value; } diff --git a/system/cpus.c b/system/cpus.c index 1c818ff682..ba633c7688 100644 --- a/system/cpus.c +++ b/system/cpus.c @@ -514,6 +514,20 @@ bool qemu_in_vcpu_thread(void) QEMU_DEFINE_STATIC_CO_TLS(bool, bql_locked) +static uint32_t bql_unlock_blocked; + +void bql_block_unlock(bool increase) +{ + uint32_t new_value; + + assert(bql_locked()); + + /* check for overflow! */ + new_value = bql_unlock_blocked + increase - !increase; + assert((new_value > bql_unlock_blocked) == increase); + bql_unlock_blocked = new_value; +} + bool bql_locked(void) { return get_bql_locked(); @@ -540,6 +554,7 @@ void bql_lock_impl(const char *file, int line) void bql_unlock(void) { g_assert(bql_locked()); + g_assert(!bql_unlock_blocked); set_bql_locked(false); qemu_mutex_unlock(&bql); } From 8e194c0ea5cdbae05b77125a582f9927678121ee Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 15 Nov 2024 13:26:58 +0100 Subject: [PATCH 36/49] rust: cell: add BQL-enforcing Cell variant QEMU objects usually have their pointer shared with the "outside world" very early in their lifetime, for example when they create their MemoryRegions. Because at this point it is not valid anymore to create a &mut reference to the device, individual parts of the device struct must be made mutable in a controlled manner. QEMU's Big Lock (BQL) effectively turns multi-threaded code into single-threaded code while device code runs, as long as the BQL is not released while the device is borrowed (because C code could sneak in and mutate the device). We can then introduce custom interior mutability primitives that are semantically similar to the standard library's (single-threaded) Cell and RefCell, but account for QEMU's threading model. Accessing the "BqlCell" or borrowing the "BqlRefCell" requires proving that the BQL is held, and attempting to access without the BQL is a runtime panic, similar to RefCell's already-borrowed panic. With respect to naming I also considered omitting the "Bql" prefix or moving it to the module, e.g. qemu_api::bql::{Cell, RefCell}. However, this could easily lead to mistakes and confusion; for example rustc could suggest the wrong import, leading to subtle bugs. As a start introduce the an equivalent of Cell. Almost all of the code was taken from Rust's standard library, while removing unstable features and probably-unnecessary functionality that constitute a large of the original code. A lot of what's left is documentation, as well as unit tests in the form of doctests. These are not yet integrated in "make check" but can be run with "cargo test --doc". Signed-off-by: Paolo Bonzini --- rust/qemu-api/meson.build | 1 + rust/qemu-api/src/cell.rs | 298 ++++++++++++++++++++++++++++++++++++++ rust/qemu-api/src/lib.rs | 1 + 3 files changed, 300 insertions(+) create mode 100644 rust/qemu-api/src/cell.rs diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index d719c13f46..edc21e1a3f 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -13,6 +13,7 @@ _qemu_api_rs = static_library( [ 'src/lib.rs', 'src/bindings.rs', + 'src/cell.rs', 'src/c_str.rs', 'src/definitions.rs', 'src/device_class.rs', diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs new file mode 100644 index 0000000000..2e4ea8d590 --- /dev/null +++ b/rust/qemu-api/src/cell.rs @@ -0,0 +1,298 @@ +// SPDX-License-Identifier: MIT +// +// This file is based on library/core/src/cell.rs from +// Rust 1.82.0. +// +// 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. + +//! BQL-protected mutable containers. +//! +//! Rust memory safety is based on this rule: Given an object `T`, it is only +//! possible to have one of the following: +//! +//! - Having several immutable references (`&T`) to the object (also known as +//! **aliasing**). +//! - Having one mutable reference (`&mut T`) to the object (also known as +//! **mutability**). +//! +//! This is enforced by the Rust compiler. However, there are situations where +//! this rule is not flexible enough. Sometimes it is required to have multiple +//! references to an object and yet mutate it. In particular, QEMU objects +//! usually have their pointer shared with the "outside world very early in +//! their lifetime", for example when they create their +//! [`MemoryRegion`s](crate::bindings::MemoryRegion). Therefore, individual +//! parts of a device must be made mutable in a controlled manner through the +//! use of cell types. +//! +//! This module provides a way to do so via the Big QEMU Lock. While +//! [`BqlCell`] is essentially the same single-threaded primitive that is +//! available in `std::cell`, the BQL allows it to be used from a multi-threaded +//! context and to share references across threads, while maintaining Rust's +//! safety guarantees. For this reason, unlike its `std::cell` counterpart, +//! `BqlCell` implements the `Sync` trait. +//! +//! BQL checks are performed in debug builds but can be optimized away in +//! release builds, providing runtime safety during development with no overhead +//! in production. +//! +//! Warning: While `BqlCell` is similar to its `std::cell` counterpart, the two +//! are not interchangeable. Using `std::cell` types in QEMU device +//! implementations is usually incorrect and can lead to thread-safety issues. +//! +//! ## `BqlCell` +//! +//! [`BqlCell`] implements interior mutability by moving values in and out of +//! the cell. That is, an `&mut T` to the inner value can never be obtained as +//! long as the cell is shared. The value itself cannot be directly obtained +//! without copying it, cloning it, or replacing it with something else. This +//! type provides the following methods, all of which can be called only while +//! the BQL is held: +//! +//! - For types that implement [`Copy`], the [`get`](BqlCell::get) method +//! retrieves the current interior value by duplicating it. +//! - For types that implement [`Default`], the [`take`](BqlCell::take) method +//! replaces the current interior value with [`Default::default()`] and +//! returns the replaced value. +//! - All types have: +//! - [`replace`](BqlCell::replace): replaces the current interior value and +//! returns the replaced value. +//! - [`set`](BqlCell::set): this method replaces the interior value, +//! dropping the replaced value. + +use std::{cell::UnsafeCell, cmp::Ordering, fmt, mem}; + +use crate::bindings; + +// TODO: When building doctests do not include the actual BQL, because cargo +// does not know how to link them to libqemuutil. This can be fixed by +// running rustdoc from "meson test" instead of relying on cargo. +pub fn bql_locked() -> bool { + // SAFETY: the function does nothing but return a thread-local bool + !cfg!(MESON) || unsafe { bindings::bql_locked() } +} + +/// A mutable memory location that is protected by the Big QEMU Lock. +/// +/// # Memory layout +/// +/// `BqlCell` has the same in-memory representation as its inner type `T`. +#[repr(transparent)] +pub struct BqlCell { + value: UnsafeCell, +} + +// SAFETY: Same as for std::sync::Mutex. In the end this *is* a Mutex, +// except it is stored out-of-line +unsafe impl Send for BqlCell {} +unsafe impl Sync for BqlCell {} + +impl Clone for BqlCell { + #[inline] + fn clone(&self) -> BqlCell { + BqlCell::new(self.get()) + } +} + +impl Default for BqlCell { + /// Creates a `BqlCell`, with the `Default` value for T. + #[inline] + fn default() -> BqlCell { + BqlCell::new(Default::default()) + } +} + +impl PartialEq for BqlCell { + #[inline] + fn eq(&self, other: &BqlCell) -> bool { + self.get() == other.get() + } +} + +impl Eq for BqlCell {} + +impl PartialOrd for BqlCell { + #[inline] + fn partial_cmp(&self, other: &BqlCell) -> Option { + self.get().partial_cmp(&other.get()) + } +} + +impl Ord for BqlCell { + #[inline] + fn cmp(&self, other: &BqlCell) -> Ordering { + self.get().cmp(&other.get()) + } +} + +impl From for BqlCell { + /// Creates a new `BqlCell` containing the given value. + fn from(t: T) -> BqlCell { + BqlCell::new(t) + } +} + +impl fmt::Debug for BqlCell { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.get().fmt(f) + } +} + +impl fmt::Display for BqlCell { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.get().fmt(f) + } +} + +impl BqlCell { + /// Creates a new `BqlCell` containing the given value. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlCell; + /// + /// let c = BqlCell::new(5); + /// ``` + #[inline] + pub const fn new(value: T) -> BqlCell { + BqlCell { + value: UnsafeCell::new(value), + } + } + + /// Sets the contained value. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlCell; + /// + /// let c = BqlCell::new(5); + /// + /// c.set(10); + /// ``` + #[inline] + pub fn set(&self, val: T) { + self.replace(val); + } + + /// Replaces the contained value with `val`, and returns the old contained + /// value. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlCell; + /// + /// let cell = BqlCell::new(5); + /// assert_eq!(cell.get(), 5); + /// assert_eq!(cell.replace(10), 5); + /// assert_eq!(cell.get(), 10); + /// ``` + #[inline] + pub fn replace(&self, val: T) -> T { + assert!(bql_locked()); + // SAFETY: This can cause data races if called from multiple threads, + // but it won't happen as long as C code accesses the value + // under BQL protection only. + mem::replace(unsafe { &mut *self.value.get() }, val) + } + + /// Unwraps the value, consuming the cell. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlCell; + /// + /// let c = BqlCell::new(5); + /// let five = c.into_inner(); + /// + /// assert_eq!(five, 5); + /// ``` + pub fn into_inner(self) -> T { + assert!(bql_locked()); + self.value.into_inner() + } +} + +impl BqlCell { + /// Returns a copy of the contained value. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlCell; + /// + /// let c = BqlCell::new(5); + /// + /// let five = c.get(); + /// ``` + #[inline] + pub fn get(&self) -> T { + assert!(bql_locked()); + // SAFETY: This can cause data races if called from multiple threads, + // but it won't happen as long as C code accesses the value + // under BQL protection only. + unsafe { *self.value.get() } + } +} + +impl BqlCell { + /// Returns a raw pointer to the underlying data in this cell. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlCell; + /// + /// let c = BqlCell::new(5); + /// + /// let ptr = c.as_ptr(); + /// ``` + #[inline] + pub const fn as_ptr(&self) -> *mut T { + self.value.get() + } +} + +impl BqlCell { + /// Takes the value of the cell, leaving `Default::default()` in its place. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlCell; + /// + /// let c = BqlCell::new(5); + /// let five = c.take(); + /// + /// assert_eq!(five, 5); + /// assert_eq!(c.into_inner(), 0); + /// ``` + pub fn take(&self) -> T { + self.replace(Default::default()) + } +} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index 440aff3817..b04d110b3f 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -8,6 +8,7 @@ pub mod bindings; pub mod c_str; +pub mod cell; pub mod definitions; pub mod device_class; pub mod offset_of; From c596199f639cdf4cc021c9bc076e4a1e64e9d50f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 15 Nov 2024 12:20:35 +0100 Subject: [PATCH 37/49] rust: cell: add BQL-enforcing RefCell variant Similar to the existing BqlCell, introduce a custom interior mutability primitive that resembles RefCell but accounts for QEMU's threading model. Borrowing the RefCell requires proving that the BQL is held, and attempting to access without the BQL is a runtime panic. Almost all of the code was taken from Rust's standard library, while removing unstable features and probably-unnecessary functionality that amounts to 60% of the original code. A lot of what's left is documentation, as well as unit tests in the form of doctests. These are not yet integrated in "make check" but can be run with "cargo test --doc". Signed-off-by: Paolo Bonzini --- rust/qemu-api/Cargo.toml | 3 +- rust/qemu-api/meson.build | 3 + rust/qemu-api/src/cell.rs | 544 +++++++++++++++++++++++++++++++++++++- 3 files changed, 539 insertions(+), 11 deletions(-) diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml index 669f288d1c..4aa22f3198 100644 --- a/rust/qemu-api/Cargo.toml +++ b/rust/qemu-api/Cargo.toml @@ -20,8 +20,9 @@ qemu_api_macros = { path = "../qemu-api-macros" } version_check = "~0.9" [features] -default = [] +default = ["debug_cell"] allocator = [] +debug_cell = [] [lints] workspace = true diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index edc21e1a3f..cacb112c5c 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -6,6 +6,9 @@ _qemu_api_cfg = run_command(rustc_args, if rustc.version().version_compare('>=1.77.0') _qemu_api_cfg += ['--cfg', 'has_offset_of'] endif +if get_option('debug_mutex') + _qemu_api_cfg += ['--feature', 'debug_cell'] +endif _qemu_api_rs = static_library( 'qemu_api', diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs index 2e4ea8d590..28349de291 100644 --- a/rust/qemu-api/src/cell.rs +++ b/rust/qemu-api/src/cell.rs @@ -46,20 +46,30 @@ //! parts of a device must be made mutable in a controlled manner through the //! use of cell types. //! -//! This module provides a way to do so via the Big QEMU Lock. While -//! [`BqlCell`] is essentially the same single-threaded primitive that is -//! available in `std::cell`, the BQL allows it to be used from a multi-threaded -//! context and to share references across threads, while maintaining Rust's -//! safety guarantees. For this reason, unlike its `std::cell` counterpart, -//! `BqlCell` implements the `Sync` trait. +//! [`BqlCell`] and [`BqlRefCell`] allow doing this via the Big QEMU Lock. +//! While they are essentially the same single-threaded primitives that are +//! available in `std::cell`, the BQL allows them to be used from a +//! multi-threaded context and to share references across threads, while +//! maintaining Rust's safety guarantees. For this reason, unlike +//! their `std::cell` counterparts, `BqlCell` and `BqlRefCell` implement the +//! `Sync` trait. //! //! BQL checks are performed in debug builds but can be optimized away in //! release builds, providing runtime safety during development with no overhead //! in production. //! -//! Warning: While `BqlCell` is similar to its `std::cell` counterpart, the two -//! are not interchangeable. Using `std::cell` types in QEMU device -//! implementations is usually incorrect and can lead to thread-safety issues. +//! The two provide different ways of handling interior mutability. +//! `BqlRefCell` is best suited for data that is primarily accessed by the +//! device's own methods, where multiple reads and writes can be grouped within +//! a single borrow and a mutable reference can be passed around. Instead, +//! [`BqlCell`] is a better choice when sharing small pieces of data with +//! external code (especially C code), because it provides simple get/set +//! operations that can be used one at a time. +//! +//! Warning: While `BqlCell` and `BqlRefCell` are similar to their `std::cell` +//! counterparts, they are not interchangeable. Using `std::cell` types in +//! QEMU device implementations is usually incorrect and can lead to +//! thread-safety issues. //! //! ## `BqlCell` //! @@ -80,8 +90,37 @@ //! returns the replaced value. //! - [`set`](BqlCell::set): this method replaces the interior value, //! dropping the replaced value. +//! +//! ## `BqlRefCell` +//! +//! [`BqlRefCell`] uses Rust's lifetimes to implement "dynamic borrowing", a +//! process whereby one can claim temporary, exclusive, mutable access to the +//! inner value: +//! +//! ```ignore +//! fn clear_interrupts(&self, val: u32) { +//! // A mutable borrow gives read-write access to the registers +//! let mut regs = self.registers.borrow_mut(); +//! let old = regs.interrupt_status(); +//! regs.update_interrupt_status(old & !val); +//! } +//! ``` +//! +//! Borrows for `BqlRefCell`s are tracked at _runtime_, unlike Rust's native +//! reference types which are entirely tracked statically, at compile time. +//! Multiple immutable borrows are allowed via [`borrow`](BqlRefCell::borrow), +//! or a single mutable borrow via [`borrow_mut`](BqlRefCell::borrow_mut). The +//! thread will panic if these rules are violated or if the BQL is not held. -use std::{cell::UnsafeCell, cmp::Ordering, fmt, mem}; +use std::{ + cell::{Cell, UnsafeCell}, + cmp::Ordering, + fmt, + marker::PhantomData, + mem, + ops::{Deref, DerefMut}, + ptr::NonNull, +}; use crate::bindings; @@ -93,6 +132,15 @@ pub fn bql_locked() -> bool { !cfg!(MESON) || unsafe { bindings::bql_locked() } } +fn bql_block_unlock(increase: bool) { + if cfg!(MESON) { + // SAFETY: this only adjusts a counter + unsafe { + bindings::bql_block_unlock(increase); + } + } +} + /// A mutable memory location that is protected by the Big QEMU Lock. /// /// # Memory layout @@ -296,3 +344,479 @@ impl BqlCell { self.replace(Default::default()) } } + +/// A mutable memory location with dynamically checked borrow rules, +/// protected by the Big QEMU Lock. +/// +/// See the [module-level documentation](self) for more. +/// +/// # Memory layout +/// +/// `BqlRefCell` starts with the same in-memory representation as its +/// inner type `T`. +#[repr(C)] +pub struct BqlRefCell { + // It is important that this is the first field (which is not the case + // for std::cell::BqlRefCell), so that we can use offset_of! on it. + // UnsafeCell and repr(C) both prevent usage of niches. + value: UnsafeCell, + borrow: Cell, + // Stores the location of the earliest currently active borrow. + // This gets updated whenever we go from having zero borrows + // to having a single borrow. When a borrow occurs, this gets included + // in the panic message + #[cfg(feature = "debug_cell")] + borrowed_at: Cell>>, +} + +// Positive values represent the number of `BqlRef` active. Negative values +// represent the number of `BqlRefMut` active. Right now QEMU's implementation +// does not allow to create `BqlRefMut`s that refer to distinct, nonoverlapping +// components of a `BqlRefCell` (e.g., different ranges of a slice). +// +// `BqlRef` and `BqlRefMut` are both two words in size, and so there will likely +// never be enough `BqlRef`s or `BqlRefMut`s in existence to overflow half of +// the `usize` range. Thus, a `BorrowFlag` will probably never overflow or +// underflow. However, this is not a guarantee, as a pathological program could +// repeatedly create and then mem::forget `BqlRef`s or `BqlRefMut`s. Thus, all +// code must explicitly check for overflow and underflow in order to avoid +// unsafety, or at least behave correctly in the event that overflow or +// underflow happens (e.g., see BorrowRef::new). +type BorrowFlag = isize; +const UNUSED: BorrowFlag = 0; + +#[inline(always)] +const fn is_writing(x: BorrowFlag) -> bool { + x < UNUSED +} + +#[inline(always)] +const fn is_reading(x: BorrowFlag) -> bool { + x > UNUSED +} + +impl BqlRefCell { + /// Creates a new `BqlRefCell` containing `value`. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlRefCell; + /// + /// let c = BqlRefCell::new(5); + /// ``` + #[inline] + pub const fn new(value: T) -> BqlRefCell { + BqlRefCell { + value: UnsafeCell::new(value), + borrow: Cell::new(UNUSED), + #[cfg(feature = "debug_cell")] + borrowed_at: Cell::new(None), + } + } +} + +// This ensures the panicking code is outlined from `borrow_mut` for +// `BqlRefCell`. +#[inline(never)] +#[cold] +#[cfg(feature = "debug_cell")] +fn panic_already_borrowed(source: &Cell>>) -> ! { + // If a borrow occurred, then we must already have an outstanding borrow, + // so `borrowed_at` will be `Some` + panic!("already borrowed at {:?}", source.take().unwrap()) +} + +#[inline(never)] +#[cold] +#[cfg(not(feature = "debug_cell"))] +fn panic_already_borrowed() -> ! { + panic!("already borrowed") +} + +impl BqlRefCell { + #[inline] + #[allow(clippy::unused_self)] + fn panic_already_borrowed(&self) -> ! { + #[cfg(feature = "debug_cell")] + { + panic_already_borrowed(&self.borrowed_at) + } + #[cfg(not(feature = "debug_cell"))] + { + panic_already_borrowed() + } + } + + /// Immutably borrows the wrapped value. + /// + /// The borrow lasts until the returned `BqlRef` exits scope. Multiple + /// immutable borrows can be taken out at the same time. + /// + /// # Panics + /// + /// Panics if the value is currently mutably borrowed. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlRefCell; + /// + /// let c = BqlRefCell::new(5); + /// + /// let borrowed_five = c.borrow(); + /// let borrowed_five2 = c.borrow(); + /// ``` + /// + /// An example of panic: + /// + /// ```should_panic + /// use qemu_api::cell::BqlRefCell; + /// + /// let c = BqlRefCell::new(5); + /// + /// let m = c.borrow_mut(); + /// let b = c.borrow(); // this causes a panic + /// ``` + #[inline] + #[track_caller] + pub fn borrow(&self) -> BqlRef<'_, T> { + if let Some(b) = BorrowRef::new(&self.borrow) { + // `borrowed_at` is always the *first* active borrow + if b.borrow.get() == 1 { + #[cfg(feature = "debug_cell")] + self.borrowed_at.set(Some(std::panic::Location::caller())); + } + + bql_block_unlock(true); + + // SAFETY: `BorrowRef` ensures that there is only immutable access + // to the value while borrowed. + let value = unsafe { NonNull::new_unchecked(self.value.get()) }; + BqlRef { value, borrow: b } + } else { + self.panic_already_borrowed() + } + } + + /// Mutably borrows the wrapped value. + /// + /// The borrow lasts until the returned `BqlRefMut` or all `BqlRefMut`s + /// derived from it exit scope. The value cannot be borrowed while this + /// borrow is active. + /// + /// # Panics + /// + /// Panics if the value is currently borrowed. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlRefCell; + /// + /// let c = BqlRefCell::new("hello".to_owned()); + /// + /// *c.borrow_mut() = "bonjour".to_owned(); + /// + /// assert_eq!(&*c.borrow(), "bonjour"); + /// ``` + /// + /// An example of panic: + /// + /// ```should_panic + /// use qemu_api::cell::BqlRefCell; + /// + /// let c = BqlRefCell::new(5); + /// let m = c.borrow(); + /// + /// let b = c.borrow_mut(); // this causes a panic + /// ``` + #[inline] + #[track_caller] + pub fn borrow_mut(&self) -> BqlRefMut<'_, T> { + if let Some(b) = BorrowRefMut::new(&self.borrow) { + #[cfg(feature = "debug_cell")] + { + self.borrowed_at.set(Some(std::panic::Location::caller())); + } + + // SAFETY: this only adjusts a counter + bql_block_unlock(true); + + // SAFETY: `BorrowRefMut` guarantees unique access. + let value = unsafe { NonNull::new_unchecked(self.value.get()) }; + BqlRefMut { + value, + _borrow: b, + marker: PhantomData, + } + } else { + self.panic_already_borrowed() + } + } + + /// Returns a raw pointer to the underlying data in this cell. + /// + /// # Examples + /// + /// ``` + /// use qemu_api::cell::BqlRefCell; + /// + /// let c = BqlRefCell::new(5); + /// + /// let ptr = c.as_ptr(); + /// ``` + #[inline] + pub const fn as_ptr(&self) -> *mut T { + self.value.get() + } +} + +// SAFETY: Same as for std::sync::Mutex. In the end this is a Mutex that is +// stored out-of-line. Even though BqlRefCell includes Cells, they are +// themselves protected by the Big QEMU Lock. Furtheremore, the Big QEMU +// Lock cannot be released while any borrows is active. +unsafe impl Send for BqlRefCell where T: Send {} +unsafe impl Sync for BqlRefCell {} + +impl Clone for BqlRefCell { + /// # Panics + /// + /// Panics if the value is currently mutably borrowed. + #[inline] + #[track_caller] + fn clone(&self) -> BqlRefCell { + BqlRefCell::new(self.borrow().clone()) + } + + /// # Panics + /// + /// Panics if `source` is currently mutably borrowed. + #[inline] + #[track_caller] + fn clone_from(&mut self, source: &Self) { + self.value.get_mut().clone_from(&source.borrow()) + } +} + +impl Default for BqlRefCell { + /// Creates a `BqlRefCell`, with the `Default` value for T. + #[inline] + fn default() -> BqlRefCell { + BqlRefCell::new(Default::default()) + } +} + +impl PartialEq for BqlRefCell { + /// # Panics + /// + /// Panics if the value in either `BqlRefCell` is currently mutably + /// borrowed. + #[inline] + fn eq(&self, other: &BqlRefCell) -> bool { + *self.borrow() == *other.borrow() + } +} + +impl Eq for BqlRefCell {} + +impl PartialOrd for BqlRefCell { + /// # Panics + /// + /// Panics if the value in either `BqlRefCell` is currently mutably + /// borrowed. + #[inline] + fn partial_cmp(&self, other: &BqlRefCell) -> Option { + self.borrow().partial_cmp(&*other.borrow()) + } +} + +impl Ord for BqlRefCell { + /// # Panics + /// + /// Panics if the value in either `BqlRefCell` is currently mutably + /// borrowed. + #[inline] + fn cmp(&self, other: &BqlRefCell) -> Ordering { + self.borrow().cmp(&*other.borrow()) + } +} + +impl From for BqlRefCell { + /// Creates a new `BqlRefCell` containing the given value. + fn from(t: T) -> BqlRefCell { + BqlRefCell::new(t) + } +} + +struct BorrowRef<'b> { + borrow: &'b Cell, +} + +impl<'b> BorrowRef<'b> { + #[inline] + fn new(borrow: &'b Cell) -> Option> { + let b = borrow.get().wrapping_add(1); + if !is_reading(b) { + // Incrementing borrow can result in a non-reading value (<= 0) in these cases: + // 1. It was < 0, i.e. there are writing borrows, so we can't allow a read + // borrow due to Rust's reference aliasing rules + // 2. It was isize::MAX (the max amount of reading borrows) and it overflowed + // into isize::MIN (the max amount of writing borrows) so we can't allow an + // additional read borrow because isize can't represent so many read borrows + // (this can only happen if you mem::forget more than a small constant amount + // of `BqlRef`s, which is not good practice) + None + } else { + // Incrementing borrow can result in a reading value (> 0) in these cases: + // 1. It was = 0, i.e. it wasn't borrowed, and we are taking the first read + // borrow + // 2. It was > 0 and < isize::MAX, i.e. there were read borrows, and isize is + // large enough to represent having one more read borrow + borrow.set(b); + Some(BorrowRef { borrow }) + } + } +} + +impl Drop for BorrowRef<'_> { + #[inline] + fn drop(&mut self) { + let borrow = self.borrow.get(); + debug_assert!(is_reading(borrow)); + self.borrow.set(borrow - 1); + bql_block_unlock(false) + } +} + +impl Clone for BorrowRef<'_> { + #[inline] + fn clone(&self) -> Self { + BorrowRef::new(self.borrow).unwrap() + } +} + +/// Wraps a borrowed reference to a value in a `BqlRefCell` box. +/// A wrapper type for an immutably borrowed value from a `BqlRefCell`. +/// +/// See the [module-level documentation](self) for more. +pub struct BqlRef<'b, T: 'b> { + // NB: we use a pointer instead of `&'b T` to avoid `noalias` violations, because a + // `BqlRef` argument doesn't hold immutability for its whole scope, only until it drops. + // `NonNull` is also covariant over `T`, just like we would have with `&T`. + value: NonNull, + borrow: BorrowRef<'b>, +} + +impl Deref for BqlRef<'_, T> { + type Target = T; + + #[inline] + fn deref(&self) -> &T { + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_ref() } + } +} + +impl<'b, T> BqlRef<'b, T> { + /// Copies a `BqlRef`. + /// + /// The `BqlRefCell` is already immutably borrowed, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `BqlRef::clone(...)`. A `Clone` implementation or a method would + /// interfere with the widespread use of `r.borrow().clone()` to clone + /// the contents of a `BqlRefCell`. + #[must_use] + #[inline] + #[allow(clippy::should_implement_trait)] + pub fn clone(orig: &BqlRef<'b, T>) -> BqlRef<'b, T> { + BqlRef { + value: orig.value, + borrow: orig.borrow.clone(), + } + } +} + +impl fmt::Debug for BqlRef<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + +impl fmt::Display for BqlRef<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + +struct BorrowRefMut<'b> { + borrow: &'b Cell, +} + +impl<'b> BorrowRefMut<'b> { + #[inline] + fn new(borrow: &'b Cell) -> Option> { + // There must currently be no existing references when borrow_mut() is + // called, so we explicitly only allow going from UNUSED to UNUSED - 1. + match borrow.get() { + UNUSED => { + borrow.set(UNUSED - 1); + Some(BorrowRefMut { borrow }) + } + _ => None, + } + } +} + +impl Drop for BorrowRefMut<'_> { + #[inline] + fn drop(&mut self) { + let borrow = self.borrow.get(); + debug_assert!(is_writing(borrow)); + self.borrow.set(borrow + 1); + bql_block_unlock(false) + } +} + +/// A wrapper type for a mutably borrowed value from a `BqlRefCell`. +/// +/// See the [module-level documentation](self) for more. +pub struct BqlRefMut<'b, T: 'b> { + // NB: we use a pointer instead of `&'b mut T` to avoid `noalias` violations, because a + // `BqlRefMut` argument doesn't hold exclusivity for its whole scope, only until it drops. + value: NonNull, + _borrow: BorrowRefMut<'b>, + // `NonNull` is covariant over `T`, so we need to reintroduce invariance. + marker: PhantomData<&'b mut T>, +} + +impl Deref for BqlRefMut<'_, T> { + type Target = T; + + #[inline] + fn deref(&self) -> &T { + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_ref() } + } +} + +impl DerefMut for BqlRefMut<'_, T> { + #[inline] + fn deref_mut(&mut self) -> &mut T { + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_mut() } + } +} + +impl fmt::Debug for BqlRefMut<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + +impl fmt::Display for BqlRefMut<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} From 28d0ad3d425a956a1257256c46ef44581f6678c5 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 5 Dec 2024 13:42:33 +0100 Subject: [PATCH 38/49] rust: define prelude Add a module that will contain frequently used traits and occasionally structs. They can be included quickly with "use qemu_api::prelude::*". Signed-off-by: Paolo Bonzini --- rust/qemu-api/meson.build | 1 + rust/qemu-api/src/lib.rs | 5 +++++ rust/qemu-api/src/prelude.rs | 6 ++++++ 3 files changed, 12 insertions(+) create mode 100644 rust/qemu-api/src/prelude.rs diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index cacb112c5c..f8b4cd39a2 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -21,6 +21,7 @@ _qemu_api_rs = static_library( 'src/definitions.rs', 'src/device_class.rs', 'src/offset_of.rs', + 'src/prelude.rs', 'src/vmstate.rs', 'src/zeroable.rs', ], diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index b04d110b3f..e5956cd5eb 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -7,6 +7,11 @@ #[rustfmt::skip] pub mod bindings; +// preserve one-item-per-"use" syntax, it is clearer +// for prelude-like modules +#[rustfmt::skip] +pub mod prelude; + pub mod c_str; pub mod cell; pub mod definitions; diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs new file mode 100644 index 0000000000..dfaddbd062 --- /dev/null +++ b/rust/qemu-api/src/prelude.rs @@ -0,0 +1,6 @@ +// Copyright 2024 Red Hat, Inc. +// Author(s): Paolo Bonzini +// SPDX-License-Identifier: GPL-2.0-or-later + +pub use crate::cell::BqlCell; +pub use crate::cell::BqlRefCell; From 4ed4da164c957a4475b9d075206f33113a69abda Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 31 Oct 2024 11:29:42 +0100 Subject: [PATCH 39/49] rust: add bindings for interrupt sources The InterruptSource bindings let us call qemu_set_irq() and sysbus_init_irq() as safe code. Interrupt sources, qemu_irq in C code, are pointers to IRQState objects. They are QOM link properties and can be written to outside the control of the device (i.e. from a shared reference); therefore they must be interior-mutable in Rust. Since thread-safety is provided by the BQL, what we want here is the newly-introduced BqlCell. A pointer to the contents of the BqlCell (an IRQState**, or equivalently qemu_irq*) is then passed to the C sysbus_init_irq function. Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 22 ++++---- rust/qemu-api/meson.build | 2 + rust/qemu-api/src/irq.rs | 91 ++++++++++++++++++++++++++++++++ rust/qemu-api/src/lib.rs | 2 + rust/qemu-api/src/sysbus.rs | 27 ++++++++++ 5 files changed, 134 insertions(+), 10 deletions(-) create mode 100644 rust/qemu-api/src/irq.rs create mode 100644 rust/qemu-api/src/sysbus.rs diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 317a9b3c5a..c5c8c463d3 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -13,6 +13,7 @@ use qemu_api::{ c_str, definitions::ObjectImpl, device_class::TYPE_SYS_BUS_DEVICE, + irq::InterruptSource, }; use crate::{ @@ -94,7 +95,7 @@ pub struct PL011State { /// * sysbus IRQ 5: `UARTEINTR` (error interrupt line) /// ``` #[doc(alias = "irq")] - pub interrupts: [qemu_irq; 6usize], + pub interrupts: [InterruptSource; IRQMASK.len()], #[doc(alias = "clk")] pub clock: NonNull, #[doc(alias = "migrate_clk")] @@ -139,7 +140,8 @@ impl PL011State { unsafe fn init(&mut self) { const CLK_NAME: &CStr = c_str!("clk"); - let dev = addr_of_mut!(*self).cast::(); + let sbd = unsafe { &mut *(addr_of_mut!(*self).cast::()) }; + // SAFETY: // // self and self.iomem are guaranteed to be valid at this point since callers @@ -153,12 +155,15 @@ impl PL011State { Self::TYPE_INFO.name, 0x1000, ); - let sbd = addr_of_mut!(*self).cast::(); sysbus_init_mmio(sbd, addr_of_mut!(self.iomem)); - for irq in self.interrupts.iter_mut() { - sysbus_init_irq(sbd, irq); - } } + + for irq in self.interrupts.iter() { + sbd.init_irq(irq); + } + + let dev = addr_of_mut!(*self).cast::(); + // SAFETY: // // self.clock is not initialized at this point; but since `NonNull<_>` is Copy, @@ -498,10 +503,7 @@ impl PL011State { pub fn update(&self) { let flags = self.int_level & self.int_enabled; for (irq, i) in self.interrupts.iter().zip(IRQMASK) { - // SAFETY: self.interrupts have been initialized in init(). - unsafe { - qemu_set_irq(*irq, i32::from(flags & i != 0)); - } + irq.set(flags & i != 0); } } diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index f8b4cd39a2..b927eb58c8 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -20,8 +20,10 @@ _qemu_api_rs = static_library( 'src/c_str.rs', 'src/definitions.rs', 'src/device_class.rs', + 'src/irq.rs', 'src/offset_of.rs', 'src/prelude.rs', + 'src/sysbus.rs', 'src/vmstate.rs', 'src/zeroable.rs', ], diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs new file mode 100644 index 0000000000..6258141bdf --- /dev/null +++ b/rust/qemu-api/src/irq.rs @@ -0,0 +1,91 @@ +// Copyright 2024 Red Hat, Inc. +// Author(s): Paolo Bonzini +// SPDX-License-Identifier: GPL-2.0-or-later + +//! Bindings for interrupt sources + +use core::ptr; +use std::{marker::PhantomData, os::raw::c_int}; + +use crate::{ + bindings::{qemu_set_irq, IRQState}, + prelude::*, +}; + +/// Interrupt sources are used by devices to pass changes to a value (typically +/// a boolean). The interrupt sink is usually an interrupt controller or +/// GPIO controller. +/// +/// As far as devices are concerned, interrupt sources are always active-high: +/// for example, `InterruptSource`'s [`raise`](InterruptSource::raise) +/// method sends a `true` value to the sink. If the guest has to see a +/// different polarity, that change is performed by the board between the +/// device and the interrupt controller. +/// +/// Interrupts are implemented as a pointer to the interrupt "sink", which has +/// type [`IRQState`]. A device exposes its source as a QOM link property using +/// a function such as +/// [`SysBusDevice::init_irq`](crate::sysbus::SysBusDevice::init_irq), and +/// initially leaves the pointer to a NULL value, representing an unconnected +/// interrupt. To connect it, whoever creates the device fills the pointer with +/// the sink's `IRQState *`, for example using `sysbus_connect_irq`. Because +/// devices are generally shared objects, interrupt sources are an example of +/// the interior mutability pattern. +/// +/// Interrupt sources can only be triggered under the Big QEMU Lock; `BqlCell` +/// allows access from whatever thread has it. +#[derive(Debug)] +#[repr(transparent)] +pub struct InterruptSource +where + c_int: From, +{ + cell: BqlCell<*mut IRQState>, + _marker: PhantomData, +} + +impl InterruptSource { + /// Send a low (`false`) value to the interrupt sink. + pub fn lower(&self) { + self.set(false); + } + + /// Send a high-low pulse to the interrupt sink. + pub fn pulse(&self) { + self.set(true); + self.set(false); + } + + /// Send a high (`true`) value to the interrupt sink. + pub fn raise(&self) { + self.set(true); + } +} + +impl InterruptSource +where + c_int: From, +{ + /// Send `level` to the interrupt sink. + pub fn set(&self, level: T) { + let ptr = self.cell.get(); + // SAFETY: the pointer is retrieved under the BQL and remains valid + // until the BQL is released, which is after qemu_set_irq() is entered. + unsafe { + qemu_set_irq(ptr, level.into()); + } + } + + pub(crate) const fn as_ptr(&self) -> *mut *mut IRQState { + self.cell.as_ptr() + } +} + +impl Default for InterruptSource { + fn default() -> Self { + InterruptSource { + cell: BqlCell::new(ptr::null_mut()), + _marker: PhantomData, + } + } +} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index e5956cd5eb..0efbef4744 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -16,7 +16,9 @@ pub mod c_str; pub mod cell; pub mod definitions; pub mod device_class; +pub mod irq; pub mod offset_of; +pub mod sysbus; pub mod vmstate; pub mod zeroable; diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs new file mode 100644 index 0000000000..4e192c7589 --- /dev/null +++ b/rust/qemu-api/src/sysbus.rs @@ -0,0 +1,27 @@ +// Copyright 2024 Red Hat, Inc. +// Author(s): Paolo Bonzini +// SPDX-License-Identifier: GPL-2.0-or-later + +use std::ptr::addr_of; + +pub use bindings::{SysBusDevice, SysBusDeviceClass}; + +use crate::{bindings, cell::bql_locked, irq::InterruptSource}; + +impl SysBusDevice { + /// Return `self` cast to a mutable pointer, for use in calls to C code. + const fn as_mut_ptr(&self) -> *mut SysBusDevice { + addr_of!(*self) as *mut _ + } + + /// Expose an interrupt source outside the device as a qdev GPIO output. + /// Note that the ordering of calls to `init_irq` is important, since + /// whoever creates the sysbus device will refer to the interrupts with + /// a number that corresponds to the order of calls to `init_irq`. + pub fn init_irq(&self, irq: &InterruptSource) { + assert!(bql_locked()); + unsafe { + bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr()); + } + } +} From ab870fa106e0e3f48db2c5ef0507d107b1b41a21 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 5 Dec 2024 14:29:13 +0100 Subject: [PATCH 40/49] rust: add a bit operation module The bindgen supports `static inline` function binding since v0.64.0 as an experimental feature (`--wrap-static-fns`), and stabilizes it after v0.70.0. But the oldest version of bindgen supported by QEMU is v0.60.1, so there's no way to generate the binding for deposit64() which is `static inline` (in include/qemu/bitops.h). Instead, implement it by hand in Rust and make it available for all unsigned types through an IntegerExt trait. Since it only involves bit operations, the Rust version of the code is almost identical to the original C version, but it applies to more types than just u64. Signed-off-by: Zhao Liu Co-authored-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/meson.build | 1 + rust/qemu-api/src/bitops.rs | 119 +++++++++++++++++++++++++++++++++++ rust/qemu-api/src/lib.rs | 1 + rust/qemu-api/src/prelude.rs | 2 + 4 files changed, 123 insertions(+) create mode 100644 rust/qemu-api/src/bitops.rs diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index b927eb58c8..adcee66115 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -16,6 +16,7 @@ _qemu_api_rs = static_library( [ 'src/lib.rs', 'src/bindings.rs', + 'src/bitops.rs', 'src/cell.rs', 'src/c_str.rs', 'src/definitions.rs', diff --git a/rust/qemu-api/src/bitops.rs b/rust/qemu-api/src/bitops.rs new file mode 100644 index 0000000000..023ec1a998 --- /dev/null +++ b/rust/qemu-api/src/bitops.rs @@ -0,0 +1,119 @@ +// Copyright (C) 2024 Intel Corporation. +// Author(s): Zhao Liu +// SPDX-License-Identifier: GPL-2.0-or-later + +//! This module provides bit operation extensions to integer types. +//! It is usually included via the `qemu_api` prelude. + +use std::ops::{ + Add, AddAssign, BitAnd, BitAndAssign, BitOr, BitOrAssign, BitXor, BitXorAssign, Div, DivAssign, + Mul, MulAssign, Not, Rem, RemAssign, Shl, ShlAssign, Shr, ShrAssign, +}; + +/// Trait for extensions to integer types +pub trait IntegerExt: + Add + AddAssign + + BitAnd + BitAndAssign + + BitOr + BitOrAssign + + BitXor + BitXorAssign + + Copy + + Div + DivAssign + + Eq + + Mul + MulAssign + + Not + Ord + PartialOrd + + Rem + RemAssign + + Shl + ShlAssign + + Shl + ShlAssign + // add more as needed + Shr + ShrAssign + + Shr + ShrAssign // add more as needed +{ + const BITS: u32; + const MAX: Self; + const MIN: Self; + const ONE: Self; + const ZERO: Self; + + #[inline] + #[must_use] + fn bit(start: u32) -> Self + { + debug_assert!(start < Self::BITS); + + Self::ONE << start + } + + #[inline] + #[must_use] + fn mask(start: u32, length: u32) -> Self + { + /* FIXME: Implement a more elegant check with error handling support? */ + debug_assert!(start < Self::BITS && length > 0 && length <= Self::BITS - start); + + (Self::MAX >> (Self::BITS - length)) << start + } + + #[inline] + #[must_use] + fn deposit(self, start: u32, length: u32, + fieldval: U) -> Self + where Self: From + { + debug_assert!(length <= U::BITS); + + let mask = Self::mask(start, length); + (self & !mask) | ((Self::from(fieldval) << start) & mask) + } + + #[inline] + #[must_use] + fn extract(self, start: u32, length: u32) -> Self + { + let mask = Self::mask(start, length); + (self & mask) >> start + } +} + +macro_rules! impl_num_ext { + ($type:ty) => { + impl IntegerExt for $type { + const BITS: u32 = <$type>::BITS; + const MAX: Self = <$type>::MAX; + const MIN: Self = <$type>::MIN; + const ONE: Self = 1; + const ZERO: Self = 0; + } + }; +} + +impl_num_ext!(u8); +impl_num_ext!(u16); +impl_num_ext!(u32); +impl_num_ext!(u64); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_deposit() { + assert_eq!(15u32.deposit(8, 8, 1u32), 256 + 15); + assert_eq!(15u32.deposit(8, 1, 255u8), 256 + 15); + } + + #[test] + fn test_extract() { + assert_eq!(15u32.extract(2, 4), 3); + } + + #[test] + fn test_bit() { + assert_eq!(u8::bit(7), 128); + assert_eq!(u32::bit(16), 0x10000); + } + + #[test] + fn test_mask() { + assert_eq!(u8::mask(7, 1), 128); + assert_eq!(u32::mask(8, 8), 0xff00); + } +} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index 0efbef4744..9e007e1635 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -12,6 +12,7 @@ pub mod bindings; #[rustfmt::skip] pub mod prelude; +pub mod bitops; pub mod c_str; pub mod cell; pub mod definitions; diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs index dfaddbd062..a39e228bab 100644 --- a/rust/qemu-api/src/prelude.rs +++ b/rust/qemu-api/src/prelude.rs @@ -2,5 +2,7 @@ // Author(s): Paolo Bonzini // SPDX-License-Identifier: GPL-2.0-or-later +pub use crate::bitops::IntegerExt; + pub use crate::cell::BqlCell; pub use crate::cell::BqlRefCell; From b2a4854508a02fc8a585890e0272c8ae5fbad5c1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 28 Oct 2024 11:28:23 +0100 Subject: [PATCH 41/49] rust: qom: add default definitions for ObjectImpl Remove a bunch of duplicate const definitions. Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 6 ------ rust/qemu-api/src/definitions.rs | 8 ++++---- rust/qemu-api/tests/tests.rs | 4 ---- 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index c5c8c463d3..3d173ae816 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -109,10 +109,7 @@ impl ObjectImpl for PL011State { const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self }; const TYPE_NAME: &'static CStr = crate::TYPE_PL011; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); - const ABSTRACT: bool = false; const INSTANCE_INIT: Option = Some(pl011_init); - const INSTANCE_POST_INIT: Option = None; - const INSTANCE_FINALIZE: Option = None; } #[repr(C)] @@ -666,8 +663,5 @@ impl ObjectImpl for PL011Luminary { const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self }; const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011); - const ABSTRACT: bool = false; const INSTANCE_INIT: Option = Some(pl011_luminary_init); - const INSTANCE_POST_INIT: Option = None; - const INSTANCE_FINALIZE: Option = None; } diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index 26597934bb..92b3c6f911 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -14,10 +14,10 @@ pub trait ObjectImpl { const TYPE_INFO: TypeInfo; const TYPE_NAME: &'static CStr; const PARENT_TYPE_NAME: Option<&'static CStr>; - const ABSTRACT: bool; - const INSTANCE_INIT: Option; - const INSTANCE_POST_INIT: Option; - const INSTANCE_FINALIZE: Option; + const ABSTRACT: bool = false; + const INSTANCE_INIT: Option = None; + const INSTANCE_POST_INIT: Option = None; + const INSTANCE_FINALIZE: Option = None; } pub trait Class { diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index 925f5a3c77..f793ff26e5 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -58,10 +58,6 @@ fn test_device_decl_macros() { const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self }; const TYPE_NAME: &'static CStr = c_str!("dummy"); const PARENT_TYPE_NAME: Option<&'static CStr> = Some(device_class::TYPE_DEVICE); - const ABSTRACT: bool = false; - const INSTANCE_INIT: Option = None; - const INSTANCE_POST_INIT: Option = None; - const INSTANCE_FINALIZE: Option = None; } impl Class for DummyClass { From 93ea0896eaa97adfcc664fa65b5b70e555a652ff Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 28 Oct 2024 13:05:43 +0100 Subject: [PATCH 42/49] rust: qom: rename Class trait to ClassInitImpl While at it, document it. Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 4 ++-- rust/qemu-api/src/definitions.rs | 25 ++++++++++++++++++++++--- rust/qemu-api/tests/tests.rs | 4 ++-- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 3d173ae816..bd12067aaf 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -117,7 +117,7 @@ pub struct PL011Class { _inner: [u8; 0], } -impl qemu_api::definitions::Class for PL011Class { +impl qemu_api::definitions::ClassInitImpl for PL011Class { const CLASS_INIT: Option = Some(crate::device_class::pl011_class_init); const CLASS_BASE_INIT: Option< @@ -650,7 +650,7 @@ pub unsafe extern "C" fn pl011_luminary_init(obj: *mut Object) { } } -impl qemu_api::definitions::Class for PL011LuminaryClass { +impl qemu_api::definitions::ClassInitImpl for PL011LuminaryClass { const CLASS_INIT: Option = None; const CLASS_BASE_INIT: Option< diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index 92b3c6f911..3291f4242c 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -20,8 +20,27 @@ pub trait ObjectImpl { const INSTANCE_FINALIZE: Option = None; } -pub trait Class { +/// Trait used to fill in a class struct. +/// +/// Each QOM class that has virtual methods describes them in a +/// _class struct_. Class structs include a parent field corresponding +/// to the vtable of the parent class, all the way up to [`ObjectClass`]. +/// Each QOM type has one such class struct. +/// +/// The Rust implementation of methods will usually come from a trait +/// like [`ObjectImpl`]. +pub trait ClassInitImpl { + /// Function that is called after all parent class initialization + /// has occurred. On entry, the virtual method pointers are set to + /// the default values coming from the parent classes; the function + /// can change them to override virtual methods of a parent class. const CLASS_INIT: Option; + + /// Called on descendent classes after all parent class initialization + /// has occurred, but before the class itself is initialized. This + /// is only useful if a class is not a leaf, and can be used to undo + /// the effects of copying the contents of the parent's class struct + /// to the descendants. const CLASS_BASE_INIT: Option< unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), >; @@ -82,8 +101,8 @@ macro_rules! type_info { instance_finalize: <$t as $crate::definitions::ObjectImpl>::INSTANCE_FINALIZE, abstract_: <$t as $crate::definitions::ObjectImpl>::ABSTRACT, class_size: ::core::mem::size_of::<<$t as $crate::definitions::ObjectImpl>::Class>(), - class_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::Class>::CLASS_INIT, - class_base_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::Class>::CLASS_BASE_INIT, + class_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::ClassInitImpl>::CLASS_INIT, + class_base_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::ClassInitImpl>::CLASS_BASE_INIT, class_data: ::core::ptr::null_mut(), interfaces: ::core::ptr::null_mut(), }; diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index f793ff26e5..704c63c846 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -7,7 +7,7 @@ use std::{ffi::CStr, os::raw::c_void}; use qemu_api::{ bindings::*, c_str, declare_properties, define_property, - definitions::{Class, ObjectImpl}, + definitions::{ClassInitImpl, ObjectImpl}, device_class, device_class_init, zeroable::Zeroable, }; @@ -60,7 +60,7 @@ fn test_device_decl_macros() { const PARENT_TYPE_NAME: Option<&'static CStr> = Some(device_class::TYPE_DEVICE); } - impl Class for DummyClass { + impl ClassInitImpl for DummyClass { const CLASS_INIT: Option = Some(dummy_class_init); const CLASS_BASE_INIT: Option< From 3701fb22dfd438993e76e158beb97683359d1dd9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 28 Oct 2024 11:47:12 +0100 Subject: [PATCH 43/49] rust: qom: convert type_info! macro to an associated const type_info! is only used in the definition of ObjectImpl::TYPE_INFO, and in fact in all of them. Pull type_info!'s definition into the ObjectImpl trait, thus simplifying the external interface of qemu_api::definitions. Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 6 ++-- rust/qemu-api/src/definitions.rs | 50 ++++++++++++++------------------ rust/qemu-api/tests/tests.rs | 1 - 3 files changed, 24 insertions(+), 33 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index bd12067aaf..bcb146c24d 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -106,7 +106,6 @@ pub struct PL011State { impl ObjectImpl for PL011State { type Class = PL011Class; - const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self }; const TYPE_NAME: &'static CStr = crate::TYPE_PL011; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); const INSTANCE_INIT: Option = Some(pl011_init); @@ -149,7 +148,7 @@ impl PL011State { addr_of_mut!(*self).cast::(), &PL011_OPS, addr_of_mut!(*self).cast::(), - Self::TYPE_INFO.name, + Self::TYPE_NAME.as_ptr(), 0x1000, ); sysbus_init_mmio(sbd, addr_of_mut!(self.iomem)); @@ -598,7 +597,7 @@ pub unsafe extern "C" fn pl011_create( chr: *mut Chardev, ) -> *mut DeviceState { unsafe { - let dev: *mut DeviceState = qdev_new(PL011State::TYPE_INFO.name); + let dev: *mut DeviceState = qdev_new(PL011State::TYPE_NAME.as_ptr()); let sysbus: *mut SysBusDevice = dev.cast::(); qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr); @@ -660,7 +659,6 @@ impl qemu_api::definitions::ClassInitImpl for PL011LuminaryClass { impl ObjectImpl for PL011Luminary { type Class = PL011LuminaryClass; - const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self }; const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011); const INSTANCE_INIT: Option = Some(pl011_luminary_init); diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index 3291f4242c..6ecfaf51b0 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -9,15 +9,34 @@ use std::{ffi::CStr, os::raw::c_void}; use crate::bindings::{Object, ObjectClass, TypeInfo}; /// Trait a type must implement to be registered with QEMU. -pub trait ObjectImpl { - type Class; - const TYPE_INFO: TypeInfo; +pub trait ObjectImpl: Sized { + type Class: ClassInitImpl; const TYPE_NAME: &'static CStr; const PARENT_TYPE_NAME: Option<&'static CStr>; const ABSTRACT: bool = false; const INSTANCE_INIT: Option = None; const INSTANCE_POST_INIT: Option = None; const INSTANCE_FINALIZE: Option = None; + + const TYPE_INFO: TypeInfo = TypeInfo { + name: Self::TYPE_NAME.as_ptr(), + parent: if let Some(pname) = Self::PARENT_TYPE_NAME { + pname.as_ptr() + } else { + core::ptr::null_mut() + }, + instance_size: core::mem::size_of::(), + instance_align: core::mem::align_of::(), + instance_init: Self::INSTANCE_INIT, + instance_post_init: Self::INSTANCE_POST_INIT, + instance_finalize: Self::INSTANCE_FINALIZE, + abstract_: Self::ABSTRACT, + class_size: core::mem::size_of::(), + class_init: ::CLASS_INIT, + class_base_init: ::CLASS_BASE_INIT, + class_data: core::ptr::null_mut(), + interfaces: core::ptr::null_mut(), + }; } /// Trait used to fill in a class struct. @@ -83,28 +102,3 @@ macro_rules! module_init { } }; } - -#[macro_export] -macro_rules! type_info { - ($t:ty) => { - $crate::bindings::TypeInfo { - name: <$t as $crate::definitions::ObjectImpl>::TYPE_NAME.as_ptr(), - parent: if let Some(pname) = <$t as $crate::definitions::ObjectImpl>::PARENT_TYPE_NAME { - pname.as_ptr() - } else { - ::core::ptr::null_mut() - }, - instance_size: ::core::mem::size_of::<$t>(), - instance_align: ::core::mem::align_of::<$t>(), - instance_init: <$t as $crate::definitions::ObjectImpl>::INSTANCE_INIT, - instance_post_init: <$t as $crate::definitions::ObjectImpl>::INSTANCE_POST_INIT, - instance_finalize: <$t as $crate::definitions::ObjectImpl>::INSTANCE_FINALIZE, - abstract_: <$t as $crate::definitions::ObjectImpl>::ABSTRACT, - class_size: ::core::mem::size_of::<<$t as $crate::definitions::ObjectImpl>::Class>(), - class_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::ClassInitImpl>::CLASS_INIT, - class_base_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::ClassInitImpl>::CLASS_BASE_INIT, - class_data: ::core::ptr::null_mut(), - interfaces: ::core::ptr::null_mut(), - }; - } -} diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index 704c63c846..7f9df348b0 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -55,7 +55,6 @@ fn test_device_decl_macros() { impl ObjectImpl for DummyState { type Class = DummyClass; - const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self }; const TYPE_NAME: &'static CStr = c_str!("dummy"); const PARENT_TYPE_NAME: Option<&'static CStr> = Some(device_class::TYPE_DEVICE); } From c6c4f3e0d90990c642523c087482eac3f42566c1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 28 Oct 2024 14:42:23 +0100 Subject: [PATCH 44/49] rust: qom: move ClassInitImpl to the instance side Put all traits on the instance struct, which makes it possible to reuse class structs if no new virtual methods or class fields are added. This is almost always the case for devices (because they are leaf classes), which is the primary use case for Rust. This is also simpler: soon we will find the implemented methods without macros, and this removes the need to go from the class struct to the instance struct to find the implementation of the *Impl traits. Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 4 ++-- rust/qemu-api/src/definitions.rs | 8 ++++---- rust/qemu-api/tests/tests.rs | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index bcb146c24d..2384d4bcb9 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -116,7 +116,7 @@ pub struct PL011Class { _inner: [u8; 0], } -impl qemu_api::definitions::ClassInitImpl for PL011Class { +impl qemu_api::definitions::ClassInitImpl for PL011State { const CLASS_INIT: Option = Some(crate::device_class::pl011_class_init); const CLASS_BASE_INIT: Option< @@ -649,7 +649,7 @@ pub unsafe extern "C" fn pl011_luminary_init(obj: *mut Object) { } } -impl qemu_api::definitions::ClassInitImpl for PL011LuminaryClass { +impl qemu_api::definitions::ClassInitImpl for PL011Luminary { const CLASS_INIT: Option = None; const CLASS_BASE_INIT: Option< diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index 6ecfaf51b0..487712611f 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -9,8 +9,8 @@ use std::{ffi::CStr, os::raw::c_void}; use crate::bindings::{Object, ObjectClass, TypeInfo}; /// Trait a type must implement to be registered with QEMU. -pub trait ObjectImpl: Sized { - type Class: ClassInitImpl; +pub trait ObjectImpl: ClassInitImpl + Sized { + type Class; const TYPE_NAME: &'static CStr; const PARENT_TYPE_NAME: Option<&'static CStr>; const ABSTRACT: bool = false; @@ -32,8 +32,8 @@ pub trait ObjectImpl: Sized { instance_finalize: Self::INSTANCE_FINALIZE, abstract_: Self::ABSTRACT, class_size: core::mem::size_of::(), - class_init: ::CLASS_INIT, - class_base_init: ::CLASS_BASE_INIT, + class_init: ::CLASS_INIT, + class_base_init: ::CLASS_BASE_INIT, class_data: core::ptr::null_mut(), interfaces: core::ptr::null_mut(), }; diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index 7f9df348b0..fd0c979121 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -59,7 +59,7 @@ fn test_device_decl_macros() { const PARENT_TYPE_NAME: Option<&'static CStr> = Some(device_class::TYPE_DEVICE); } - impl ClassInitImpl for DummyClass { + impl ClassInitImpl for DummyState { const CLASS_INIT: Option = Some(dummy_class_init); const CLASS_BASE_INIT: Option< From 8c80c472da6342c5924bc4ea7e87c77ca61477b8 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 28 Oct 2024 10:29:27 +0100 Subject: [PATCH 45/49] rust: qdev: move device_class_init! body to generic function, ClassInitImpl implementation to macro Use a trait to access the former parameters to device_class_init!. This allows hiding the details of the class_init implementation behind a generic function and makes higher-level functionality available from qemu_api. The implementation of ClassInitImpl is then the same for all devices and is easily macroized. Later on, we can remove the need to implement ClassInitImpl by hand for all device types, and stop making rust_device_class_init<>() public. While at it, document the members of DeviceImpl. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 34 +++++----- rust/hw/char/pl011/src/device_class.rs | 8 --- rust/qemu-api/src/device_class.rs | 87 +++++++++++++++++++++----- rust/qemu-api/tests/tests.rs | 30 ++++----- 4 files changed, 103 insertions(+), 56 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 2384d4bcb9..28b1924337 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -12,11 +12,13 @@ use qemu_api::{ bindings::{self, *}, c_str, definitions::ObjectImpl, - device_class::TYPE_SYS_BUS_DEVICE, + device_class::{DeviceImpl, TYPE_SYS_BUS_DEVICE}, + impl_device_class, irq::InterruptSource, }; use crate::{ + device_class, memory_ops::PL011_OPS, registers::{self, Interrupt}, RegisterOffset, @@ -116,14 +118,20 @@ pub struct PL011Class { _inner: [u8; 0], } -impl qemu_api::definitions::ClassInitImpl for PL011State { - const CLASS_INIT: Option = - Some(crate::device_class::pl011_class_init); - const CLASS_BASE_INIT: Option< - unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), - > = None; +impl DeviceImpl for PL011State { + fn properties() -> &'static [Property] { + &device_class::PL011_PROPERTIES + } + fn vmsd() -> Option<&'static VMStateDescription> { + Some(&device_class::VMSTATE_PL011) + } + const REALIZE: Option = + Some(device_class::pl011_realize); + const RESET: Option = Some(device_class::pl011_reset); } +impl_device_class!(PL011State); + impl PL011State { /// Initializes a pre-allocated, unitialized instance of `PL011State`. /// @@ -649,17 +657,13 @@ pub unsafe extern "C" fn pl011_luminary_init(obj: *mut Object) { } } -impl qemu_api::definitions::ClassInitImpl for PL011Luminary { - const CLASS_INIT: Option = - None; - const CLASS_BASE_INIT: Option< - unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), - > = None; -} - impl ObjectImpl for PL011Luminary { type Class = PL011LuminaryClass; const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011); const INSTANCE_INIT: Option = Some(pl011_luminary_init); } + +impl DeviceImpl for PL011Luminary {} + +impl_device_class!(PL011Luminary); diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs index a707fde138..c61b6bb025 100644 --- a/rust/hw/char/pl011/src/device_class.rs +++ b/rust/hw/char/pl011/src/device_class.rs @@ -93,14 +93,6 @@ qemu_api::declare_properties! { ), } -qemu_api::device_class_init! { - pl011_class_init, - props => PL011_PROPERTIES, - realize_fn => Some(pl011_realize), - legacy_reset_fn => Some(pl011_reset), - vmsd => VMSTATE_PL011, -} - /// # Safety /// /// We expect the FFI user of this function to pass a valid pointer, that has diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs index 922bbce1bb..f683f94f2a 100644 --- a/rust/qemu-api/src/device_class.rs +++ b/rust/qemu-api/src/device_class.rs @@ -2,25 +2,80 @@ // Author(s): Manos Pitsidianakis // SPDX-License-Identifier: GPL-2.0-or-later -use std::ffi::CStr; +use std::{ffi::CStr, os::raw::c_void}; -use crate::bindings; +use crate::{ + bindings::{self, DeviceClass, DeviceState, Error, ObjectClass, Property, VMStateDescription}, + zeroable::Zeroable, +}; + +/// Trait providing the contents of [`DeviceClass`]. +pub trait DeviceImpl { + /// _Realization_ is the second stage of device creation. It contains + /// all operations that depend on device properties and can fail (note: + /// this is not yet supported for Rust devices). + /// + /// If not `None`, the parent class's `realize` method is overridden + /// with the function pointed to by `REALIZE`. + const REALIZE: Option = None; + + /// If not `None`, the parent class's `reset` method is overridden + /// with the function pointed to by `RESET`. + /// + /// Rust does not yet support the three-phase reset protocol; this is + /// usually okay for leaf classes. + const RESET: Option = None; + + /// An array providing the properties that the user can set on the + /// device. Not a `const` because referencing statics in constants + /// is unstable until Rust 1.83.0. + fn properties() -> &'static [Property] { + &[Zeroable::ZERO; 1] + } + + /// A `VMStateDescription` providing the migration format for the device + /// Not a `const` because referencing statics in constants is unstable + /// until Rust 1.83.0. + fn vmsd() -> Option<&'static VMStateDescription> { + None + } +} + +/// # Safety +/// +/// We expect the FFI user of this function to pass a valid pointer that +/// can be downcasted to type `DeviceClass`, because `T` implements +/// `DeviceImpl`. +pub unsafe extern "C" fn rust_device_class_init( + klass: *mut ObjectClass, + _: *mut c_void, +) { + let mut dc = ::core::ptr::NonNull::new(klass.cast::()).unwrap(); + unsafe { + let dc = dc.as_mut(); + if let Some(realize_fn) = ::REALIZE { + dc.realize = Some(realize_fn); + } + if let Some(reset_fn) = ::RESET { + bindings::device_class_set_legacy_reset(dc, Some(reset_fn)); + } + if let Some(vmsd) = ::vmsd() { + dc.vmsd = vmsd; + } + bindings::device_class_set_props(dc, ::properties().as_ptr()); + } +} #[macro_export] -macro_rules! device_class_init { - ($func:ident, props => $props:ident, realize_fn => $realize_fn:expr, legacy_reset_fn => $legacy_reset_fn:expr, vmsd => $vmsd:ident$(,)*) => { - pub unsafe extern "C" fn $func( - klass: *mut $crate::bindings::ObjectClass, - _: *mut ::std::os::raw::c_void, - ) { - let mut dc = - ::core::ptr::NonNull::new(klass.cast::<$crate::bindings::DeviceClass>()).unwrap(); - unsafe { - dc.as_mut().realize = $realize_fn; - dc.as_mut().vmsd = &$vmsd; - $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn); - $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_ptr()); - } +macro_rules! impl_device_class { + ($type:ty) => { + impl $crate::definitions::ClassInitImpl for $type { + const CLASS_INIT: Option< + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut ::std::os::raw::c_void), + > = Some($crate::device_class::rust_device_class_init::<$type>); + const CLASS_BASE_INIT: Option< + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut ::std::os::raw::c_void), + > = None; } }; } diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index fd0c979121..b8b12a4042 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -2,13 +2,14 @@ // Author(s): Manos Pitsidianakis // SPDX-License-Identifier: GPL-2.0-or-later -use std::{ffi::CStr, os::raw::c_void}; +use std::ffi::CStr; use qemu_api::{ bindings::*, c_str, declare_properties, define_property, - definitions::{ClassInitImpl, ObjectImpl}, - device_class, device_class_init, + definitions::ObjectImpl, + device_class::{self, DeviceImpl}, + impl_device_class, zeroable::Zeroable, }; @@ -45,28 +46,23 @@ fn test_device_decl_macros() { ), } - device_class_init! { - dummy_class_init, - props => DUMMY_PROPERTIES, - realize_fn => None, - legacy_reset_fn => None, - vmsd => VMSTATE, - } - impl ObjectImpl for DummyState { type Class = DummyClass; const TYPE_NAME: &'static CStr = c_str!("dummy"); const PARENT_TYPE_NAME: Option<&'static CStr> = Some(device_class::TYPE_DEVICE); } - impl ClassInitImpl for DummyState { - const CLASS_INIT: Option = - Some(dummy_class_init); - const CLASS_BASE_INIT: Option< - unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), - > = None; + impl DeviceImpl for DummyState { + fn properties() -> &'static [Property] { + &DUMMY_PROPERTIES + } + fn vmsd() -> Option<&'static VMStateDescription> { + Some(&VMSTATE) + } } + impl_device_class!(DummyState); + unsafe { module_call_init(module_init_type::MODULE_INIT_QOM); object_unref(object_new(DummyState::TYPE_NAME.as_ptr()).cast()); From f75fb90ff2af75cd4405fe4c6ba0c0c38a120590 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 12 Nov 2024 17:08:07 +0100 Subject: [PATCH 46/49] rust: qdev: move bridge for realize and reset functions out of pl011 Allow the DeviceImpl trait to expose safe Rust functions. rust_device_class_init<> adds thunks around the functions in DeviceImpl. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 5 ++-- rust/hw/char/pl011/src/device_class.rs | 26 ------------------- rust/qemu-api/src/definitions.rs | 2 +- rust/qemu-api/src/device_class.rs | 36 +++++++++++++++++++++----- 4 files changed, 33 insertions(+), 36 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 28b1924337..56403c3660 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -125,9 +125,8 @@ impl DeviceImpl for PL011State { fn vmsd() -> Option<&'static VMStateDescription> { Some(&device_class::VMSTATE_PL011) } - const REALIZE: Option = - Some(device_class::pl011_realize); - const RESET: Option = Some(device_class::pl011_reset); + const REALIZE: Option = Some(Self::realize); + const RESET: Option = Some(Self::reset); } impl_device_class!(PL011State); diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs index c61b6bb025..975c3d42be 100644 --- a/rust/hw/char/pl011/src/device_class.rs +++ b/rust/hw/char/pl011/src/device_class.rs @@ -92,29 +92,3 @@ qemu_api::declare_properties! { default = true ), } - -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -pub unsafe extern "C" fn pl011_realize(dev: *mut DeviceState, _errp: *mut *mut Error) { - unsafe { - assert!(!dev.is_null()); - let mut state = NonNull::new_unchecked(dev.cast::()); - state.as_mut().realize(); - } -} - -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -pub unsafe extern "C" fn pl011_reset(dev: *mut DeviceState) { - unsafe { - assert!(!dev.is_null()); - let mut state = NonNull::new_unchecked(dev.cast::()); - state.as_mut().reset(); - } -} diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index 487712611f..0467e6290e 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -47,7 +47,7 @@ pub trait ObjectImpl: ClassInitImpl + Sized { /// Each QOM type has one such class struct. /// /// The Rust implementation of methods will usually come from a trait -/// like [`ObjectImpl`]. +/// like [`ObjectImpl`] or [`DeviceImpl`](crate::device_class::DeviceImpl). pub trait ClassInitImpl { /// Function that is called after all parent class initialization /// has occurred. On entry, the virtual method pointers are set to diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs index f683f94f2a..f25904be4f 100644 --- a/rust/qemu-api/src/device_class.rs +++ b/rust/qemu-api/src/device_class.rs @@ -17,14 +17,14 @@ pub trait DeviceImpl { /// /// If not `None`, the parent class's `realize` method is overridden /// with the function pointed to by `REALIZE`. - const REALIZE: Option = None; + const REALIZE: Option = None; /// If not `None`, the parent class's `reset` method is overridden /// with the function pointed to by `RESET`. /// /// Rust does not yet support the three-phase reset protocol; this is /// usually okay for leaf classes. - const RESET: Option = None; + const RESET: Option = None; /// An array providing the properties that the user can set on the /// device. Not a `const` because referencing statics in constants @@ -41,6 +41,30 @@ pub trait DeviceImpl { } } +/// # Safety +/// +/// This function is only called through the QOM machinery and +/// the `impl_device_class!` macro. +/// We expect the FFI user of this function to pass a valid pointer that +/// can be downcasted to type `T`. We also expect the device is +/// readable/writeable from one thread at any time. +unsafe extern "C" fn rust_realize_fn(dev: *mut DeviceState, _errp: *mut *mut Error) { + assert!(!dev.is_null()); + let state = dev.cast::(); + T::REALIZE.unwrap()(unsafe { &mut *state }); +} + +/// # Safety +/// +/// We expect the FFI user of this function to pass a valid pointer that +/// can be downcasted to type `T`. We also expect the device is +/// readable/writeable from one thread at any time. +unsafe extern "C" fn rust_reset_fn(dev: *mut DeviceState) { + assert!(!dev.is_null()); + let state = dev.cast::(); + T::RESET.unwrap()(unsafe { &mut *state }); +} + /// # Safety /// /// We expect the FFI user of this function to pass a valid pointer that @@ -53,11 +77,11 @@ pub unsafe extern "C" fn rust_device_class_init( let mut dc = ::core::ptr::NonNull::new(klass.cast::()).unwrap(); unsafe { let dc = dc.as_mut(); - if let Some(realize_fn) = ::REALIZE { - dc.realize = Some(realize_fn); + if ::REALIZE.is_some() { + dc.realize = Some(rust_realize_fn::); } - if let Some(reset_fn) = ::RESET { - bindings::device_class_set_legacy_reset(dc, Some(reset_fn)); + if ::RESET.is_some() { + bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::)); } if let Some(vmsd) = ::vmsd() { dc.vmsd = vmsd; From 1f9d52c9388d14c3f5a605543a8ef53dceaad5bb Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 28 Oct 2024 10:45:55 +0100 Subject: [PATCH 47/49] rust: qom: move bridge for TypeInfo functions out of pl011 Allow the ObjectImpl trait to expose Rust functions that avoid raw pointers (though INSTANCE_INIT for example is still unsafe). ObjectImpl::TYPE_INFO adds thunks around the functions in ObjectImpl. While at it, document `TypeInfo`. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 40 +++++++-------------- rust/qemu-api/src/definitions.rs | 61 +++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 32 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 56403c3660..b9f8fb134b 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -110,7 +110,7 @@ impl ObjectImpl for PL011State { type Class = PL011Class; const TYPE_NAME: &'static CStr = crate::TYPE_PL011; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); - const INSTANCE_INIT: Option = Some(pl011_init); + const INSTANCE_INIT: Option = Some(Self::init); } #[repr(C)] @@ -615,19 +615,6 @@ pub unsafe extern "C" fn pl011_create( } } -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -pub unsafe extern "C" fn pl011_init(obj: *mut Object) { - unsafe { - debug_assert!(!obj.is_null()); - let mut state = NonNull::new_unchecked(obj.cast::()); - state.as_mut().init(); - } -} - #[repr(C)] #[derive(Debug, qemu_api_macros::Object)] /// PL011 Luminary device model. @@ -640,19 +627,16 @@ pub struct PL011LuminaryClass { _inner: [u8; 0], } -/// Initializes a pre-allocated, unitialized instance of `PL011Luminary`. -/// -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011Luminary`]. We also expect the device is -/// readable/writeable from one thread at any time. -pub unsafe extern "C" fn pl011_luminary_init(obj: *mut Object) { - unsafe { - debug_assert!(!obj.is_null()); - let mut state = NonNull::new_unchecked(obj.cast::()); - let state = state.as_mut(); - state.parent_obj.device_id = DeviceId::Luminary; +impl PL011Luminary { + /// Initializes a pre-allocated, unitialized instance of `PL011Luminary`. + /// + /// # Safety + /// + /// We expect the FFI user of this function to pass a valid pointer, that + /// has the same size as [`PL011Luminary`]. We also expect the device is + /// readable/writeable from one thread at any time. + unsafe fn init(&mut self) { + self.parent_obj.device_id = DeviceId::Luminary; } } @@ -660,7 +644,7 @@ impl ObjectImpl for PL011Luminary { type Class = PL011LuminaryClass; const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011); - const INSTANCE_INIT: Option = Some(pl011_luminary_init); + const INSTANCE_INIT: Option = Some(Self::init); } impl DeviceImpl for PL011Luminary {} diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index 0467e6290e..f297075898 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -8,16 +8,63 @@ use std::{ffi::CStr, os::raw::c_void}; use crate::bindings::{Object, ObjectClass, TypeInfo}; +unsafe extern "C" fn rust_instance_init(obj: *mut Object) { + // SAFETY: obj is an instance of T, since rust_instance_init + // is called from QOM core as the instance_init function + // for class T + unsafe { T::INSTANCE_INIT.unwrap()(&mut *obj.cast::()) } +} + +unsafe extern "C" fn rust_instance_post_init(obj: *mut Object) { + // SAFETY: obj is an instance of T, since rust_instance_post_init + // is called from QOM core as the instance_post_init function + // for class T + // + // FIXME: it's not really guaranteed that there are no backpointers to + // obj; it's quite possible that they have been created by instance_init(). + // The receiver should be &self, not &mut self. + T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::() }) +} + /// Trait a type must implement to be registered with QEMU. +/// +/// # Safety +/// +/// - the struct must be `#[repr(C)]` +/// +/// - `Class` and `TYPE` must match the data in the `TypeInfo` (this is +/// automatic if the class is defined via `ObjectImpl`). +/// +/// - the first field of the struct must be of the instance struct corresponding +/// to the superclass declared as `PARENT_TYPE_NAME` pub trait ObjectImpl: ClassInitImpl + Sized { + /// The QOM class object corresponding to this struct. Not used yet. type Class; + + /// The name of the type, which can be passed to `object_new()` to + /// generate an instance of this type. const TYPE_NAME: &'static CStr; + + /// The parent of the type. This should match the first field of + /// the struct that implements `ObjectImpl`: const PARENT_TYPE_NAME: Option<&'static CStr>; + + /// Whether the object can be instantiated const ABSTRACT: bool = false; - const INSTANCE_INIT: Option = None; - const INSTANCE_POST_INIT: Option = None; const INSTANCE_FINALIZE: Option = None; + /// Function that is called to initialize an object. The parent class will + /// have already been initialized so the type is only responsible for + /// initializing its own members. + /// + /// FIXME: The argument is not really a valid reference. `&mut + /// MaybeUninit` would be a better description. + const INSTANCE_INIT: Option = None; + + /// Function that is called to finish initialization of an object, once + /// `INSTANCE_INIT` functions have been called. + const INSTANCE_POST_INIT: Option = None; + const TYPE_INFO: TypeInfo = TypeInfo { name: Self::TYPE_NAME.as_ptr(), parent: if let Some(pname) = Self::PARENT_TYPE_NAME { @@ -27,8 +74,14 @@ pub trait ObjectImpl: ClassInitImpl + Sized { }, instance_size: core::mem::size_of::(), instance_align: core::mem::align_of::(), - instance_init: Self::INSTANCE_INIT, - instance_post_init: Self::INSTANCE_POST_INIT, + instance_init: match Self::INSTANCE_INIT { + None => None, + Some(_) => Some(rust_instance_init::), + }, + instance_post_init: match Self::INSTANCE_POST_INIT { + None => None, + Some(_) => Some(rust_instance_post_init::), + }, instance_finalize: Self::INSTANCE_FINALIZE, abstract_: Self::ABSTRACT, class_size: core::mem::size_of::(), From 7bd8e3ef63330e870cf4644d21c285cce35c703d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 31 Oct 2024 09:56:15 +0100 Subject: [PATCH 48/49] rust: qom: split ObjectType from ObjectImpl trait Define a separate trait for fields that also applies to classes that are defined by C code. This makes it possible to add metadata to core classes, which has multiple uses: - it makes it possible to access the parent struct's TYPE_* for types that are defined in Rust code, and to avoid repeating it in every subclass - implementors of ObjectType will be allowed to implement the IsA<> trait and therefore to perform typesafe casts from one class to another. - in the future, an ObjectType could be created with Foo::new() in a type-safe manner, without having to pass a TYPE_* constant. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 17 ++++++++++++----- rust/qemu-api/src/definitions.rs | 27 +++++++++++++++++++++------ rust/qemu-api/src/device_class.rs | 11 ++++++----- rust/qemu-api/src/prelude.rs | 2 ++ rust/qemu-api/src/sysbus.rs | 10 ++++++++-- rust/qemu-api/tests/tests.rs | 17 +++++++++-------- 6 files changed, 58 insertions(+), 26 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index b9f8fb134b..0ab825b1ca 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -12,9 +12,10 @@ use qemu_api::{ bindings::{self, *}, c_str, definitions::ObjectImpl, - device_class::{DeviceImpl, TYPE_SYS_BUS_DEVICE}, + device_class::DeviceImpl, impl_device_class, irq::InterruptSource, + prelude::*, }; use crate::{ @@ -106,10 +107,13 @@ pub struct PL011State { device_id: DeviceId, } -impl ObjectImpl for PL011State { +unsafe impl ObjectType for PL011State { type Class = PL011Class; const TYPE_NAME: &'static CStr = crate::TYPE_PL011; - const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); +} + +impl ObjectImpl for PL011State { + const PARENT_TYPE_NAME: Option<&'static CStr> = Some(::TYPE_NAME); const INSTANCE_INIT: Option = Some(Self::init); } @@ -640,10 +644,13 @@ impl PL011Luminary { } } -impl ObjectImpl for PL011Luminary { +unsafe impl ObjectType for PL011Luminary { type Class = PL011LuminaryClass; const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY; - const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011); +} + +impl ObjectImpl for PL011Luminary { + const PARENT_TYPE_NAME: Option<&'static CStr> = Some(::TYPE_NAME); const INSTANCE_INIT: Option = Some(Self::init); } diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index f297075898..b98a692678 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -26,25 +26,40 @@ unsafe extern "C" fn rust_instance_post_init(obj: *mut Object) { T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::() }) } -/// Trait a type must implement to be registered with QEMU. +/// Trait exposed by all structs corresponding to QOM objects. /// /// # Safety /// -/// - the struct must be `#[repr(C)]` +/// For classes declared in C: /// -/// - `Class` and `TYPE` must match the data in the `TypeInfo` (this is -/// automatic if the class is defined via `ObjectImpl`). +/// - `Class` and `TYPE` must match the data in the `TypeInfo`; +/// +/// - the first field of the struct must be of the instance type corresponding +/// to the superclass, as declared in the `TypeInfo` +/// +/// - likewise, the first field of the `Class` struct must be of the class type +/// corresponding to the superclass +/// +/// For classes declared in Rust and implementing [`ObjectImpl`]: +/// +/// - the struct must be `#[repr(C)]`; /// /// - the first field of the struct must be of the instance struct corresponding -/// to the superclass declared as `PARENT_TYPE_NAME` -pub trait ObjectImpl: ClassInitImpl + Sized { +/// to the superclass, as declared in `ObjectImpl::PARENT_TYPE_NAME` +/// +/// - likewise, the first field of the `Class` must be of the class struct +/// corresponding to the superclass +pub unsafe trait ObjectType: Sized { /// The QOM class object corresponding to this struct. Not used yet. type Class; /// The name of the type, which can be passed to `object_new()` to /// generate an instance of this type. const TYPE_NAME: &'static CStr; +} +/// Trait a type must implement to be registered with QEMU. +pub trait ObjectImpl: ObjectType + ClassInitImpl { /// The parent of the type. This should match the first field of /// the struct that implements `ObjectImpl`: const PARENT_TYPE_NAME: Option<&'static CStr>; diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs index f25904be4f..03d03feee8 100644 --- a/rust/qemu-api/src/device_class.rs +++ b/rust/qemu-api/src/device_class.rs @@ -6,6 +6,7 @@ use std::{ffi::CStr, os::raw::c_void}; use crate::{ bindings::{self, DeviceClass, DeviceState, Error, ObjectClass, Property, VMStateDescription}, + prelude::*, zeroable::Zeroable, }; @@ -146,8 +147,8 @@ macro_rules! declare_properties { }; } -// workaround until we can use --generate-cstr in bindgen. -pub const TYPE_DEVICE: &CStr = - unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) }; -pub const TYPE_SYS_BUS_DEVICE: &CStr = - unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_SYS_BUS_DEVICE) }; +unsafe impl ObjectType for bindings::DeviceState { + type Class = bindings::DeviceClass; + const TYPE_NAME: &'static CStr = + unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) }; +} diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs index a39e228bab..1b8677b2d9 100644 --- a/rust/qemu-api/src/prelude.rs +++ b/rust/qemu-api/src/prelude.rs @@ -6,3 +6,5 @@ pub use crate::bitops::IntegerExt; pub use crate::cell::BqlCell; pub use crate::cell::BqlRefCell; + +pub use crate::definitions::ObjectType; diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs index 4e192c7589..5ee068541c 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -2,11 +2,17 @@ // Author(s): Paolo Bonzini // SPDX-License-Identifier: GPL-2.0-or-later -use std::ptr::addr_of; +use std::{ffi::CStr, ptr::addr_of}; pub use bindings::{SysBusDevice, SysBusDeviceClass}; -use crate::{bindings, cell::bql_locked, irq::InterruptSource}; +use crate::{bindings, cell::bql_locked, irq::InterruptSource, prelude::*}; + +unsafe impl ObjectType for SysBusDevice { + type Class = SysBusDeviceClass; + const TYPE_NAME: &'static CStr = + unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_SYS_BUS_DEVICE) }; +} impl SysBusDevice { /// Return `self` cast to a mutable pointer, for use in calls to C code. diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index b8b12a4042..1d027dd652 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -5,12 +5,8 @@ use std::ffi::CStr; use qemu_api::{ - bindings::*, - c_str, declare_properties, define_property, - definitions::ObjectImpl, - device_class::{self, DeviceImpl}, - impl_device_class, - zeroable::Zeroable, + bindings::*, c_str, declare_properties, define_property, definitions::ObjectImpl, + device_class::DeviceImpl, impl_device_class, prelude::*, zeroable::Zeroable, }; #[test] @@ -46,10 +42,15 @@ fn test_device_decl_macros() { ), } - impl ObjectImpl for DummyState { + unsafe impl ObjectType for DummyState { type Class = DummyClass; const TYPE_NAME: &'static CStr = c_str!("dummy"); - const PARENT_TYPE_NAME: Option<&'static CStr> = Some(device_class::TYPE_DEVICE); + } + + impl ObjectImpl for DummyState { + const PARENT_TYPE_NAME: Option<&'static CStr> = + Some(::TYPE_NAME); + const ABSTRACT: bool = false; } impl DeviceImpl for DummyState { From 166e8a1fd15bfa527b25fc15ca315e572c0556d2 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sun, 24 Nov 2024 18:51:34 +0100 Subject: [PATCH 49/49] rust: qom: change the parent type to an associated type Avoid duplicated code to retrieve the QOM type strings from the Rust type. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 6 ++++-- rust/qemu-api/src/definitions.rs | 12 ++++-------- rust/qemu-api/tests/tests.rs | 3 +-- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 0ab825b1ca..3e29442a62 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -113,7 +113,8 @@ unsafe impl ObjectType for PL011State { } impl ObjectImpl for PL011State { - const PARENT_TYPE_NAME: Option<&'static CStr> = Some(::TYPE_NAME); + type ParentType = SysBusDevice; + const INSTANCE_INIT: Option = Some(Self::init); } @@ -650,7 +651,8 @@ unsafe impl ObjectType for PL011Luminary { } impl ObjectImpl for PL011Luminary { - const PARENT_TYPE_NAME: Option<&'static CStr> = Some(::TYPE_NAME); + type ParentType = PL011State; + const INSTANCE_INIT: Option = Some(Self::init); } diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index b98a692678..df91a2e31a 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -45,10 +45,10 @@ unsafe extern "C" fn rust_instance_post_init(obj: *mut Object) { /// - the struct must be `#[repr(C)]`; /// /// - the first field of the struct must be of the instance struct corresponding -/// to the superclass, as declared in `ObjectImpl::PARENT_TYPE_NAME` +/// to the superclass, which is `ObjectImpl::ParentType` /// /// - likewise, the first field of the `Class` must be of the class struct -/// corresponding to the superclass +/// corresponding to the superclass, which is `ObjectImpl::ParentType::Class`. pub unsafe trait ObjectType: Sized { /// The QOM class object corresponding to this struct. Not used yet. type Class; @@ -62,7 +62,7 @@ pub unsafe trait ObjectType: Sized { pub trait ObjectImpl: ObjectType + ClassInitImpl { /// The parent of the type. This should match the first field of /// the struct that implements `ObjectImpl`: - const PARENT_TYPE_NAME: Option<&'static CStr>; + type ParentType: ObjectType; /// Whether the object can be instantiated const ABSTRACT: bool = false; @@ -82,11 +82,7 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl { const TYPE_INFO: TypeInfo = TypeInfo { name: Self::TYPE_NAME.as_ptr(), - parent: if let Some(pname) = Self::PARENT_TYPE_NAME { - pname.as_ptr() - } else { - core::ptr::null_mut() - }, + parent: Self::ParentType::TYPE_NAME.as_ptr(), instance_size: core::mem::size_of::(), instance_align: core::mem::align_of::(), instance_init: match Self::INSTANCE_INIT { diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs index 1d027dd652..278efe967f 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -48,8 +48,7 @@ fn test_device_decl_macros() { } impl ObjectImpl for DummyState { - const PARENT_TYPE_NAME: Option<&'static CStr> = - Some(::TYPE_NAME); + type ParentType = DeviceState; const ABSTRACT: bool = false; }