From f26137893b98c6e1fd6819d5f13cb74fafcdcff9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 27 Jan 2025 11:01:09 +0100 Subject: [PATCH 1/9] rust: remove unnecessary Cargo.toml metadata Some items of Cargo.toml (readme, homepage, repository) are only present because of clippy::cargo warnings being enabled in rust/hw/char/pl011/src/lib.rs. But these items are not particularly useful and would be all the same for all Cargo.toml files in the QEMU workspace. Clean them up. Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/Cargo.toml | 3 --- rust/hw/char/pl011/README.md | 31 ------------------------------- rust/hw/char/pl011/src/lib.rs | 14 ++++++-------- rust/qemu-api-macros/Cargo.toml | 3 --- rust/qemu-api-macros/README.md | 1 - 5 files changed, 6 insertions(+), 46 deletions(-) delete mode 100644 rust/hw/char/pl011/README.md delete mode 100644 rust/qemu-api-macros/README.md diff --git a/rust/hw/char/pl011/Cargo.toml b/rust/hw/char/pl011/Cargo.toml index 58f3e859f7..2b4097864d 100644 --- a/rust/hw/char/pl011/Cargo.toml +++ b/rust/hw/char/pl011/Cargo.toml @@ -4,10 +4,7 @@ version = "0.1.0" edition = "2021" authors = ["Manos Pitsidianakis "] license = "GPL-2.0-or-later" -readme = "README.md" -homepage = "https://www.qemu.org" description = "pl011 device model for QEMU" -repository = "https://gitlab.com/epilys/rust-for-qemu" resolver = "2" publish = false keywords = [] diff --git a/rust/hw/char/pl011/README.md b/rust/hw/char/pl011/README.md deleted file mode 100644 index cd7dea3163..0000000000 --- a/rust/hw/char/pl011/README.md +++ /dev/null @@ -1,31 +0,0 @@ -# PL011 QEMU Device Model - -This library implements a device model for the PrimeCell® UART (PL011) -device in QEMU. - -## Build static lib - -Host build target must be explicitly specified: - -```sh -cargo build --target x86_64-unknown-linux-gnu -``` - -Replace host target triplet if necessary. - -## Generate Rust documentation - -To generate docs for this crate, including private items: - -```sh -cargo doc --no-deps --document-private-items --target x86_64-unknown-linux-gnu -``` - -To include direct dependencies like `bilge` (bitmaps for register types): - -```sh -cargo tree --depth 1 -e normal --prefix none \ - | cut -d' ' -f1 \ - | xargs printf -- '-p %s\n' \ - | xargs cargo doc --no-deps --document-private-items --target x86_64-unknown-linux-gnu -``` diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs index e2df4586bc..e704daf6e3 100644 --- a/rust/hw/char/pl011/src/lib.rs +++ b/rust/hw/char/pl011/src/lib.rs @@ -1,13 +1,12 @@ // Copyright 2024, Linaro Limited // Author(s): Manos Pitsidianakis // SPDX-License-Identifier: GPL-2.0-or-later -// -// PL011 QEMU Device Model -// -// This library implements a device model for the PrimeCell® UART (PL011) -// device in QEMU. -// -#![doc = include_str!("../README.md")] + +//! PL011 QEMU Device Model +//! +//! This library implements a device model for the PrimeCell® UART (PL011) +//! device in QEMU. +//! //! # Library crate //! //! See [`PL011State`](crate::device::PL011State) for the device model type and @@ -18,7 +17,6 @@ clippy::suspicious, clippy::complexity, clippy::perf, - clippy::cargo, clippy::nursery, clippy::style )] diff --git a/rust/qemu-api-macros/Cargo.toml b/rust/qemu-api-macros/Cargo.toml index 5a27b52ee6..b9b4baecdd 100644 --- a/rust/qemu-api-macros/Cargo.toml +++ b/rust/qemu-api-macros/Cargo.toml @@ -4,10 +4,7 @@ version = "0.1.0" edition = "2021" authors = ["Manos Pitsidianakis "] license = "GPL-2.0-or-later" -readme = "README.md" -homepage = "https://www.qemu.org" description = "Rust bindings for QEMU - Utility macros" -repository = "https://gitlab.com/qemu-project/qemu/" resolver = "2" publish = false keywords = [] diff --git a/rust/qemu-api-macros/README.md b/rust/qemu-api-macros/README.md deleted file mode 100644 index f60f54ac4b..0000000000 --- a/rust/qemu-api-macros/README.md +++ /dev/null @@ -1 +0,0 @@ -# `qemu-api-macros` - Utility macros for defining QEMU devices From 669fab6a1f3ee23966dc4ec0a92711ae6e93597e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 27 Jan 2025 11:12:38 +0100 Subject: [PATCH 2/9] rust: include rust_version in Cargo.toml Tell clippy the minimum supported Rust version for QEMU. Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/Cargo.toml | 1 + rust/hw/char/pl011/src/device_class.rs | 1 - rust/qemu-api-macros/Cargo.toml | 1 + rust/qemu-api/Cargo.toml | 1 + 4 files changed, 3 insertions(+), 1 deletion(-) diff --git a/rust/hw/char/pl011/Cargo.toml b/rust/hw/char/pl011/Cargo.toml index 2b4097864d..f2296cad58 100644 --- a/rust/hw/char/pl011/Cargo.toml +++ b/rust/hw/char/pl011/Cargo.toml @@ -9,6 +9,7 @@ resolver = "2" publish = false keywords = [] categories = [] +rust-version = "1.63.0" [lib] crate-type = ["staticlib"] diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs index 8a157a663f..dbef93f6cb 100644 --- a/rust/hw/char/pl011/src/device_class.rs +++ b/rust/hw/char/pl011/src/device_class.rs @@ -12,7 +12,6 @@ use qemu_api::{ use crate::device::{PL011Registers, PL011State}; -#[allow(clippy::missing_const_for_fn)] extern "C" fn pl011_clock_needed(opaque: *mut c_void) -> bool { let state = NonNull::new(opaque).unwrap().cast::(); unsafe { state.as_ref().migrate_clock } diff --git a/rust/qemu-api-macros/Cargo.toml b/rust/qemu-api-macros/Cargo.toml index b9b4baecdd..89dee1cfb3 100644 --- a/rust/qemu-api-macros/Cargo.toml +++ b/rust/qemu-api-macros/Cargo.toml @@ -9,6 +9,7 @@ resolver = "2" publish = false keywords = [] categories = [] +rust-version = "1.63.0" [lib] proc-macro = true diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml index 4aa22f3198..a51dd14285 100644 --- a/rust/qemu-api/Cargo.toml +++ b/rust/qemu-api/Cargo.toml @@ -12,6 +12,7 @@ resolver = "2" publish = false keywords = [] categories = [] +rust-version = "1.63.0" [dependencies] qemu_api_macros = { path = "../qemu-api-macros" } From 2590c02a5fc2d7e48b03f9ec46c302d5512b7526 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sat, 2 Nov 2024 12:03:31 +0100 Subject: [PATCH 3/9] rust: add docs Signed-off-by: Paolo Bonzini --- docs/devel/index-process.rst | 1 + docs/devel/rust.rst | 430 +++++++++++++++++++++++++++++++++++ 2 files changed, 431 insertions(+) create mode 100644 docs/devel/rust.rst diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst index 362f97ee30..cb7c6640fd 100644 --- a/docs/devel/index-process.rst +++ b/docs/devel/index-process.rst @@ -17,3 +17,4 @@ Notes about how to interact with the community and how and where to submit patch stable-process submitting-a-pull-request secure-coding-practices + rust diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst new file mode 100644 index 0000000000..390aae4386 --- /dev/null +++ b/docs/devel/rust.rst @@ -0,0 +1,430 @@ +.. |msrv| replace:: 1.63.0 + +Rust in QEMU +============ + +Rust in QEMU is a project to enable using the Rust programming language +to add new functionality to QEMU. + +Right now, the focus is on making it possible to write devices that inherit +from ``SysBusDevice`` in `*safe*`__ Rust. Later, it may become possible +to write other kinds of devices (e.g. PCI devices that can do DMA), +complete boards, or backends (e.g. block device formats). + +__ https://doc.rust-lang.org/nomicon/meet-safe-and-unsafe.html + +Building the Rust in QEMU code +------------------------------ + +The Rust in QEMU code is included in the emulators via Meson. Meson +invokes rustc directly, building static libraries that are then linked +together with the C code. This is completely automatic when you run +``make`` or ``ninja``. + +However, QEMU's build system also tries to be easy to use for people who +are accustomed to the more "normal" Cargo-based development workflow. +In particular: + +* the set of warnings and lints that are used to build QEMU always + comes from the ``rust/Cargo.toml`` workspace file + +* it is also possible to use ``cargo`` for common Rust-specific coding + tasks, in particular to invoke ``clippy``, ``rustfmt`` and ``rustdoc``. + +To this end, QEMU includes a ``build.rs`` build script that picks up +generated sources from QEMU's build directory and puts it in Cargo's +output directory (typically ``rust/target/``). A vanilla invocation +of Cargo will complain that it cannot find the generated sources, +which can be fixed in different ways: + +* by using special shorthand targets in the QEMU build directory:: + + make clippy + make rustfmt + make rustdoc + +* by invoking ``cargo`` through the Meson `development environment`__ + feature:: + + pyvenv/bin/meson devenv -w ../rust cargo clippy --tests + pyvenv/bin/meson devenv -w ../rust cargo fmt + + If you are going to use ``cargo`` repeatedly, ``pyvenv/bin/meson devenv`` + will enter a shell where commands like ``cargo clippy`` just work. + +__ https://mesonbuild.com/Commands.html#devenv + +* by pointing the ``MESON_BUILD_ROOT`` to the top of your QEMU build + tree. This third method is useful if you are using ``rust-analyzer``; + you can set the environment variable through the + ``rust-analyzer.cargo.extraEnv`` setting. + +As shown above, you can use the ``--tests`` option as usual to operate on test +code. Note however that you cannot *build* or run tests via ``cargo``, because +they need support C code from QEMU that Cargo does not know about. Tests can +be run via ``meson test`` or ``make``:: + + make check-rust + +Building Rust code with ``--enable-modules`` is not supported yet. + +Supported tools +''''''''''''''' + +QEMU supports rustc version 1.63.0 and newer. Notably, the following features +are missing: + +* ``core::ffi`` (1.64.0). Use ``std::os::raw`` and ``std::ffi`` instead. + +* ``cast_mut()``/``cast_const()`` (1.65.0). Use ``as`` instead. + +* "let ... else" (1.65.0). Use ``if let`` instead. This is currently patched + in QEMU's vendored copy of the bilge crate. + +* Generic Associated Types (1.65.0) + +* ``CStr::from_bytes_with_nul()`` as a ``const`` function (1.72.0). + +* "Return position ``impl Trait`` in Traits" (1.75.0, blocker for including + the pinned-init create). + +* ``MaybeUninit::zeroed()`` as a ``const`` function (1.75.0). QEMU's + ``Zeroable`` trait can be implemented without ``MaybeUninit::zeroed()``, + so this would be just a cleanup. + +* ``c"" literals`` (stable in 1.77.0). QEMU provides a ``c_str!()`` macro + to define ``CStr`` constants easily + +* ``offset_of!`` (stable in 1.77.0). QEMU uses ``offset_of!()`` heavily; it + provides a replacement in the ``qemu_api`` crate, but it does not support + lifetime parameters and therefore ``&'a Something`` fields in the struct + may have to be replaced by ``NonNull``. *Nested* ``offset_of!`` + was only stabilized in Rust 1.82.0, but it is not used. + +* inline const expression (stable in 1.79.0), currently worked around with + associated constants in the ``FnCall`` trait. + +* associated constants have to be explicitly marked ``'static`` (`changed in + 1.81.0`__) + +* ``&raw`` (stable in 1.82.0). Use ``addr_of!`` and ``addr_of_mut!`` instead, + though hopefully the need for raw pointers will go down over time. + +* ``new_uninit`` (stable in 1.82.0). This is used internally by the ``pinned_init`` + crate, which is planned for inclusion in QEMU, but it can be easily patched + out. + +* referencing statics in constants (stable in 1.83.0). For now use a const + function; this is an important limitation for QEMU's migration stream + architecture (VMState). Right now, VMState lacks type safety because + it is hard to place the ``VMStateField`` definitions in traits. + +* associated const equality would be nice to have for some users of + ``callbacks::FnCall``, but is still experimental. ``ASSERT_IS_SOME`` + replaces it. + +__ https://github.com/rust-lang/rust/pull/125258 + +It is expected that QEMU will advance its minimum supported version of +rustc to 1.77.0 as soon as possible; as of January 2025, blockers +for that right now are Debian bookworm and 32-bit MIPS processors. +This unfortunately means that references to statics in constants will +remain an issue. + +QEMU also supports version 0.60.x of bindgen, which is missing option +``--generate-cstr``. This option requires version 0.66.x and will +be adopted as soon as supporting these older versions is not necessary +anymore. + +Writing Rust code in QEMU +------------------------- + +Right now QEMU includes three crates: + +* ``qemu_api`` for bindings to C code and useful functionality + +* ``qemu_api_macros`` defines several procedural macros that are useful when + writing C code + +* ``pl011`` (under ``rust/hw/char/pl011``) is the sample device that is being + used to further develop ``qemu_api`` and ``qemu_api_macros``. It is a functional + replacement for the ``hw/char/pl011.c`` file. + +This section explains how to work with them. + +Status +'''''' + +Modules of ``qemu_api`` can be defined as: + +- *complete*: ready for use in new devices; if applicable, the API supports the + full functionality available in C + +- *stable*: ready for production use, the API is safe and should not undergo + major changes + +- *proof of concept*: the API is subject to change but allows working with safe + Rust + +- *initial*: the API is in its initial stages; it requires large amount of + unsafe code; it might have soundness or type-safety issues + +The status of the modules is as follows: + +================ ====================== +module status +================ ====================== +``assertions`` stable +``bitops`` complete +``callbacks`` complete +``cell`` stable +``c_str`` complete +``irq`` complete +``module`` complete +``offset_of`` stable +``qdev`` stable +``qom`` stable +``sysbus`` stable +``vmstate`` proof of concept +``zeroable`` stable +================ ====================== + +.. note:: + API stability is not a promise, if anything because the C APIs are not a stable + interface either. Also, ``unsafe`` interfaces may be replaced by safe interfaces + later. + +Common pitfalls +''''''''''''''' + +Rust has very strict rules with respect to how you get an exclusive (``&mut``) +reference; failure to respect those rules is a source of undefined behavior. +In particular, even if a value is loaded from a raw mutable pointer (``*mut``), +it *cannot* be casted to ``&mut`` unless the value was stored to the ``*mut`` +from a mutable reference. Furthermore, it is undefined behavior if any +shared reference was created between the store to the ``*mut`` and the load:: + + let mut p: u32 = 42; + let p_mut = &mut p; // 1 + let p_raw = p_mut as *mut u32; // 2 + + // p_raw keeps the mutable reference "alive" + + let p_shared = &p; // 3 + println!("access from &u32: {}", *p_shared); + + // Bring back the mutable reference, its lifetime overlaps + // with that of a shared reference. + let p_mut = unsafe { &mut *p_raw }; // 4 + println!("access from &mut 32: {}", *p_mut); + + println!("access from &u32: {}", *p_shared); // 5 + +These rules can be tested with `MIRI`__, for example. + +__ https://github.com/rust-lang/miri + +Almost all Rust code in QEMU will involve QOM objects, and pointers to these +objects are *shared*, for example because they are part of the QOM composition +tree. This creates exactly the above scenario: + +1. a QOM object is created + +2. a ``*mut`` is created, for example as the opaque value for a ``MemoryRegion`` + +3. the QOM object is placed in the composition tree + +4. a memory access dereferences the opaque value to a ``&mut`` + +5. but the shared reference is still present in the composition tree + +Because of this, QOM objects should almost always use ``&self`` instead +of ``&mut self``; access to internal fields must use *interior mutability* +to go from a shared reference to a ``&mut``. + +Whenever C code provides you with an opaque ``void *``, avoid converting it +to a Rust mutable reference, and use a shared reference instead. Rust code +will then have to use QEMU's ``BqlRefCell`` and ``BqlCell`` type, which +enforce that locking rules for the "Big QEMU Lock" are respected. These cell +types are also known to the ``vmstate`` crate, which is able to "look inside" +them when building an in-memory representation of a ``struct``s layout. +Note that the same is not true of a ``RefCell`` or ``Mutex``. + +In the future, similar cell types might also be provided for ``AioContext``-based +locking as well. + +Writing bindings to C code +'''''''''''''''''''''''''' + +Here are some things to keep in mind when working on the ``qemu_api`` crate. + +**Look at existing code** + Very often, similar idioms in C code correspond to similar tricks in + Rust bindings. If the C code uses ``offsetof``, look at qdev properties + or ``vmstate``. If the C code has a complex const struct, look at + ``MemoryRegion``. Reuse existing patterns for handling lifetimes; + for example use ``&T`` for QOM objects that do not need a reference + count (including those that can be embedded in other objects) and + ``Owned`` for those that need it. + +**Use the type system** + Bindings often will need access information that is specific to a type + (either a builtin one or a user-defined one) in order to pass it to C + functions. Put them in a trait and access it through generic parameters. + The ``vmstate`` module has examples of how to retrieve type information + for the fields of a Rust ``struct``. + +**Prefer unsafe traits to unsafe functions** + Unsafe traits are much easier to prove correct than unsafe functions. + They are an excellent place to store metadata that can later be accessed + by generic functions. C code usually places metadata in global variables; + in Rust, they can be stored in traits and then turned into ``static`` + variables. Often, unsafe traits can be generated by procedural macros. + +**Document limitations due to old Rust versions** + If you need to settle for an inferior solution because of the currently + supported set of Rust versions, document it in the source and in this + file. This ensures that it can be fixed when the minimum supported + version is bumped. + +**Keep locking in mind**. + When marking a type ``Sync``, be careful of whether it needs the big + QEMU lock. Use ``BqlCell`` and ``BqlRefCell`` for interior data, + or assert ``bql_locked()``. + +**Don't be afraid of complexity, but document and isolate it** + It's okay to be tricky; device code is written more often than bindings + code and it's important that it is idiomatic. However, you should strive + to isolate any tricks in a place (for example a ``struct``, a trait + or a macro) where it can be documented and tested. If needed, include + toy versions of the code in the documentation. + +Writing procedural macros +''''''''''''''''''''''''' + +By conventions, procedural macros are split in two functions, one +returning ``Result` with the body of +the procedural macro, and the second returning ``proc_macro::TokenStream`` +which is the actual procedural macro. The former's name is the same as +the latter with the ``_or_error`` suffix. The code for the latter is more +or less fixed; it follows the following template, which is fixed apart +from the type after ``as`` in the invocation of ``parse_macro_input!``:: + + #[proc_macro_derive(Object)] + pub fn derive_object(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + let expanded = derive_object_or_error(input).unwrap_or_else(Into::into); + + TokenStream::from(expanded) + } + +The ``qemu_api_macros`` crate has utility functions to examine a +``DeriveInput`` and perform common checks (e.g. looking for a struct +with named fields). These functions return ``Result<..., MacroError>`` +and can be used easily in the procedural macro function:: + + fn derive_object_or_error(input: DeriveInput) -> + Result + { + is_c_repr(&input, "#[derive(Object)]")?; + + let name = &input.ident; + let parent = &get_fields(&input, "#[derive(Object)]")?[0].ident; + ... + } + +Use procedural macros with care. They are mostly useful for two purposes: + +* Performing consistency checks; for example ``#[derive(Object)]`` checks + that the structure has ``#[repr[C])`` and that the type of the first field + is consistent with the ``ObjectType`` declaration. + +* Extracting information from Rust source code into traits, typically based + on types and attributes. For example, ``#[derive(TryInto)]`` builds an + implementation of ``TryFrom``, and it uses the ``#[repr(...)]`` attribute + as the ``TryFrom`` source and error types. + +Procedural macros can be hard to debug and test; if the code generation +exceeds a few lines of code, it may be worthwhile to delegate work to +"regular" declarative (``macro_rules!``) macros and write unit tests for +those instead. + + +Coding style +'''''''''''' + +Code should pass clippy and be formatted with rustfmt. + +Right now, only the nightly version of ``rustfmt`` is supported. This +might change in the future. While CI checks for correct formatting via +``cargo fmt --check``, maintainers can fix this for you when applying patches. + +It is expected that ``qemu_api`` provides full ``rustdoc`` documentation for +bindings that are in their final shape or close. + +Adding dependencies +------------------- + +Generally, the set of dependent crates is kept small. Think twice before +adding a new external crate, especially if it comes with a large set of +dependencies itself. Sometimes QEMU only needs a small subset of the +functionality; see for example QEMU's ``assertions`` or ``c_str`` modules. + +On top of this recommendation, adding external crates to QEMU is a +slightly complicated process, mostly due to the need to teach Meson how +to build them. While Meson has initial support for parsing ``Cargo.lock`` +files, it is still highly experimental and is therefore not used. + +Therefore, external crates must be added as subprojects for Meson to +learn how to build them, as well as to the relevant ``Cargo.toml`` files. +The versions specified in ``rust/Cargo.lock`` must be the same as the +subprojects; note that the ``rust/`` directory forms a Cargo `workspace`__, +and therefore there is a single lock file for the whole build. + +__ https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace + +Choose a version of the crate that works with QEMU's minimum supported +Rust version (|msrv|). + +Second, a new ``wrap`` file must be added to teach Meson how to download the +crate. The wrap file must be named ``NAME-SEMVER-rs.wrap``, where ``NAME`` +is the name of the crate and ``SEMVER`` is the version up to and including the +first non-zero number. For example, a crate with version ``0.2.3`` will use +``0.2`` for its ``SEMVER``, while a crate with version ``1.0.84`` will use ``1``. + +Third, the Meson rules to build the crate must be added at +``subprojects/NAME-SEMVER-rs/meson.build``. Generally this includes: + +* ``subproject`` and ``dependency`` lines for all dependent crates + +* a ``static_library`` or ``rust.proc_macro`` line to perform the actual build + +* ``declare_dependency`` and a ``meson.override_dependency`` lines to expose + the result to QEMU and to other subprojects + +Remember to add ``native: true`` to ``dependency``, ``static_library`` and +``meson.override_dependency`` for dependencies of procedural macros. +If a crate is needed in both procedural macros and QEMU binaries, everything +apart from ``subproject`` must be duplicated to build both native and +non-native versions of the crate. + +It's important to specify the right compiler options. These include: + +* the language edition (which can be found in the ``Cargo.toml`` file) + +* the ``--cfg`` (which have to be "reverse engineered" from the ``build.rs`` + file of the crate). + +* usually, a ``--cap-lints allow`` argument to hide warnings from rustc + or clippy. + +After every change to the ``meson.build`` file you have to update the patched +version with ``meson subprojects update --reset ``NAME-SEMVER-rs``. This might +be automated in the future. + +Also, after every change to the ``meson.build`` file it is strongly suggested to +do a dummy change to the ``.wrap`` file (for example adding a comment like +``# version 2``), which will help Meson notice that the subproject is out of date. + +As a last step, add the new subproject to ``scripts/archive-source.sh``, +``scripts/make-release`` and ``subprojects/.gitignore``. From 6b03c038067f2171110425593f722df8f4d19d17 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 6 Feb 2025 12:26:12 +0100 Subject: [PATCH 4/9] rust: add clippy configuration file Configure the minimum supported Rust version (though strictly speaking that's redundant with Cargo.toml), and the list of CamelCase identifiers that are not Rust types. Signed-off-by: Paolo Bonzini --- rust/clippy.toml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 rust/clippy.toml diff --git a/rust/clippy.toml b/rust/clippy.toml new file mode 100644 index 0000000000..5d190f91de --- /dev/null +++ b/rust/clippy.toml @@ -0,0 +1,2 @@ +doc-valid-idents = ["PrimeCell", ".."] +msrv = "1.63.0" From 2b3bfbb21b08bfd9aa49b665b66851e9ebf62802 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Thu, 16 Jan 2025 11:25:35 +0000 Subject: [PATCH 5/9] target/i386: Do not raise Invalid for 0 * Inf + QNaN In commit 8adcff4ae7 ("fpu: handle raising Invalid for infzero in pick_nan_muladd") we changed the handling of 0 * Inf + QNaN to always raise the Invalid exception regardless of target architecture. (This was a change affecting hppa, i386, sh4 and tricore.) However, this was incorrect for i386, which documents in the SDM section 14.5.2 that for the 0 * Inf + NaN case that it will only raise the Invalid exception when the input is an SNaN. (This is permitted by the IEEE 754-2008 specification, which documents that whether we raise Invalid for 0 * Inf + QNaN is implementation defined.) Adjust the softfloat pick_nan_muladd code to allow the target to suppress the raising of Invalid for the inf * zero + NaN case (as an extra flag orthogonal to its choice for when to use the default NaN), and enable that for x86. We do not revert here the behaviour change for hppa, sh4 or tricore: * The sh4 manual is clear that it should signal Invalid * The tricore manual is a bit vague but doesn't say it shouldn't * The hppa manual doesn't talk about fused multiply-add corner cases at all Cc: qemu-stable@nongnu.org Fixes: 8adcff4ae7 (""fpu: handle raising Invalid for infzero in pick_nan_muladd") Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Link: https://lore.kernel.org/r/20250116112536.4117889-2-peter.maydell@linaro.org Signed-off-by: Paolo Bonzini --- fpu/softfloat-parts.c.inc | 5 +++-- include/fpu/softfloat-types.h | 16 +++++++++++++--- target/i386/tcg/fpu_helper.c | 5 ++++- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc index fee05d0a86..73621f4a97 100644 --- a/fpu/softfloat-parts.c.inc +++ b/fpu/softfloat-parts.c.inc @@ -126,7 +126,8 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b, float_raise(float_flag_invalid | float_flag_invalid_snan, s); } - if (infzero) { + if (infzero && + !(s->float_infzeronan_rule & float_infzeronan_suppress_invalid)) { /* This is (0 * inf) + NaN or (inf * 0) + NaN */ float_raise(float_flag_invalid | float_flag_invalid_imz, s); } @@ -144,7 +145,7 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b, * Inf * 0 + NaN -- some implementations return the * default NaN here, and some return the input NaN. */ - switch (s->float_infzeronan_rule) { + switch (s->float_infzeronan_rule & ~float_infzeronan_suppress_invalid) { case float_infzeronan_dnan_never: break; case float_infzeronan_dnan_always: diff --git a/include/fpu/softfloat-types.h b/include/fpu/softfloat-types.h index 616c290145..2e43d1dd9e 100644 --- a/include/fpu/softfloat-types.h +++ b/include/fpu/softfloat-types.h @@ -280,11 +280,21 @@ typedef enum __attribute__((__packed__)) { /* No propagation rule specified */ float_infzeronan_none = 0, /* Result is never the default NaN (so always the input NaN) */ - float_infzeronan_dnan_never, + float_infzeronan_dnan_never = 1, /* Result is always the default NaN */ - float_infzeronan_dnan_always, + float_infzeronan_dnan_always = 2, /* Result is the default NaN if the input NaN is quiet */ - float_infzeronan_dnan_if_qnan, + float_infzeronan_dnan_if_qnan = 3, + /* + * Don't raise Invalid for 0 * Inf + NaN. Default is to raise. + * IEEE 754-2008 section 7.2 makes it implementation defined whether + * 0 * Inf + QNaN raises Invalid or not. Note that 0 * Inf + SNaN will + * raise the Invalid flag for the SNaN anyway. + * + * This is a flag which can be ORed in with any of the above + * DNaN behaviour options. + */ + float_infzeronan_suppress_invalid = (1 << 7), } FloatInfZeroNaNRule; /* diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c index 3d764bc138..de6d0b252e 100644 --- a/target/i386/tcg/fpu_helper.c +++ b/target/i386/tcg/fpu_helper.c @@ -178,8 +178,11 @@ void cpu_init_fp_statuses(CPUX86State *env) * "Fused-Multiply-ADD (FMA) Numeric Behavior" the NaN handling is * specified -- for 0 * inf + NaN the input NaN is selected, and if * there are multiple input NaNs they are selected in the order a, b, c. + * We also do not raise Invalid for the 0 * inf + (Q)NaN case. */ - set_float_infzeronan_rule(float_infzeronan_dnan_never, &env->sse_status); + set_float_infzeronan_rule(float_infzeronan_dnan_never | + float_infzeronan_suppress_invalid, + &env->sse_status); set_float_3nan_prop_rule(float_3nan_prop_abc, &env->sse_status); /* Default NaN: sign bit set, most significant frac bit set */ set_float_default_nan_pattern(0b11000000, &env->fp_status); From 059fad25fda352e3b8d79d0c7ad681d08516d8d5 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Thu, 16 Jan 2025 11:25:36 +0000 Subject: [PATCH 6/9] tests/tcg/x86_64/fma: Test some x86 fused-multiply-add cases Add a test case which tests some corner case behaviour of fused-multiply-add on x86: * 0 * Inf + SNaN should raise Invalid * 0 * Inf + QNaN shouldh not raise Invalid * tininess should be detected after rounding There is also one currently-disabled test case: * flush-to-zero should be done after rounding This is disabled because QEMU's emulation currently does this incorrectly (and so would fail the test). The test case is kept in but disabled, as the justification for why the test running harness has support for testing both with and without FTZ set. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Link: https://lore.kernel.org/r/20250116112536.4117889-3-peter.maydell@linaro.org Signed-off-by: Paolo Bonzini --- tests/tcg/x86_64/Makefile.target | 1 + tests/tcg/x86_64/fma.c | 109 +++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 tests/tcg/x86_64/fma.c diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target index d6dff559c7..be20fc64e8 100644 --- a/tests/tcg/x86_64/Makefile.target +++ b/tests/tcg/x86_64/Makefile.target @@ -18,6 +18,7 @@ X86_64_TESTS += adox X86_64_TESTS += test-1648 X86_64_TESTS += test-2175 X86_64_TESTS += cross-modifying-code +X86_64_TESTS += fma TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64 else TESTS=$(MULTIARCH_TESTS) diff --git a/tests/tcg/x86_64/fma.c b/tests/tcg/x86_64/fma.c new file mode 100644 index 0000000000..09c622ebc0 --- /dev/null +++ b/tests/tcg/x86_64/fma.c @@ -0,0 +1,109 @@ +/* + * Test some fused multiply add corner cases. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ +#include +#include +#include +#include + +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) + +/* + * Perform one "n * m + a" operation using the vfmadd insn and return + * the result; on return *mxcsr_p is set to the bottom 6 bits of MXCSR + * (the Flag bits). If ftz is true then we set MXCSR.FTZ while doing + * the operation. + * We print the operation and its results to stdout. + */ +static uint64_t do_fmadd(uint64_t n, uint64_t m, uint64_t a, + bool ftz, uint32_t *mxcsr_p) +{ + uint64_t r; + uint32_t mxcsr = 0; + uint32_t ftz_bit = ftz ? (1 << 15) : 0; + uint32_t saved_mxcsr = 0; + + asm volatile("stmxcsr %[saved_mxcsr]\n" + "stmxcsr %[mxcsr]\n" + "andl $0xffff7fc0, %[mxcsr]\n" + "orl %[ftz_bit], %[mxcsr]\n" + "ldmxcsr %[mxcsr]\n" + "movq %[a], %%xmm0\n" + "movq %[m], %%xmm1\n" + "movq %[n], %%xmm2\n" + /* xmm0 = xmm0 + xmm2 * xmm1 */ + "vfmadd231sd %%xmm1, %%xmm2, %%xmm0\n" + "movq %%xmm0, %[r]\n" + "stmxcsr %[mxcsr]\n" + "ldmxcsr %[saved_mxcsr]\n" + : [r] "=r" (r), [mxcsr] "=m" (mxcsr), + [saved_mxcsr] "=m" (saved_mxcsr) + : [n] "r" (n), [m] "r" (m), [a] "r" (a), + [ftz_bit] "r" (ftz_bit) + : "xmm0", "xmm1", "xmm2"); + *mxcsr_p = mxcsr & 0x3f; + printf("vfmadd132sd 0x%" PRIx64 " 0x%" PRIx64 " 0x%" PRIx64 + " = 0x%" PRIx64 " MXCSR flags 0x%" PRIx32 "\n", + n, m, a, r, *mxcsr_p); + return r; +} + +typedef struct testdata { + /* Input n, m, a */ + uint64_t n; + uint64_t m; + uint64_t a; + bool ftz; + /* Expected result */ + uint64_t expected_r; + /* Expected low 6 bits of MXCSR (the Flag bits) */ + uint32_t expected_mxcsr; +} testdata; + +static testdata tests[] = { + { 0, 0x7ff0000000000000, 0x7ff000000000aaaa, false, /* 0 * Inf + SNaN */ + 0x7ff800000000aaaa, 1 }, /* Should be QNaN and does raise Invalid */ + { 0, 0x7ff0000000000000, 0x7ff800000000aaaa, false, /* 0 * Inf + QNaN */ + 0x7ff800000000aaaa, 0 }, /* Should be QNaN and does *not* raise Invalid */ + /* + * These inputs give a result which is tiny before rounding but which + * becomes non-tiny after rounding. x86 is a "detect tininess after + * rounding" architecture, so it should give a non-denormal result and + * not set the Underflow flag (only the Precision flag for an inexact + * result). + */ + { 0x3fdfffffffffffff, 0x001fffffffffffff, 0x801fffffffffffff, false, + 0x8010000000000000, 0x20 }, + /* + * Flushing of denormal outputs to zero should also happen after + * rounding, so setting FTZ should not affect the result or the flags. + * QEMU currently does not emulate this correctly because we do the + * flush-to-zero check before rounding, so we incorrectly produce a + * zero result and set Underflow as well as Precision. + */ +#ifdef ENABLE_FAILING_TESTS + { 0x3fdfffffffffffff, 0x001fffffffffffff, 0x801fffffffffffff, true, + 0x8010000000000000, 0x20 }, /* Enabling FTZ shouldn't change flags */ +#endif +}; + +int main(void) +{ + bool passed = true; + for (int i = 0; i < ARRAY_SIZE(tests); i++) { + uint32_t mxcsr; + uint64_t r = do_fmadd(tests[i].n, tests[i].m, tests[i].a, + tests[i].ftz, &mxcsr); + if (r != tests[i].expected_r) { + printf("expected result 0x%" PRIx64 "\n", tests[i].expected_r); + passed = false; + } + if (mxcsr != tests[i].expected_mxcsr) { + printf("expected MXCSR flags 0x%x\n", tests[i].expected_mxcsr); + passed = false; + } + } + return passed ? 0 : 1; +} From 35020629914b4d3af6195d33e1a68e8b1c8c2fa3 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 22 Jan 2024 10:48:11 +0100 Subject: [PATCH 7/9] tcg/optimize: optimize TSTNE using smask and zmask Generalize the existing optimization of "TSTNE x,sign" and "TSTNE x,-1". This can be useful for example in the i386 frontend, which will generate tests of zero-extended registers against 0xffffffff. Ironically, on x86 hosts this is a very slight pessimization in the very case it's meant to optimize because brcond_i64 cc_dst,$0xffffffff,tsteq,$L1 (test %ebx, %ebx) is 1 byte smaller than brcond_i64 cc_dst,$0x0,eq,$L1 (test %rbx, %rbx). However, in general it is an improvement, especially if it avoids placing a large immediate in the constant pool. Signed-off-by: Paolo Bonzini --- tcg/optimize.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tcg/optimize.c b/tcg/optimize.c index 8c6303e3af..bca11cc427 100644 --- a/tcg/optimize.c +++ b/tcg/optimize.c @@ -766,6 +766,7 @@ static int do_constant_folding_cond1(OptContext *ctx, TCGOp *op, TCGArg dest, TCGArg *p1, TCGArg *p2, TCGArg *pcond) { TCGCond cond; + TempOptInfo *i1; bool swap; int r; @@ -783,19 +784,21 @@ static int do_constant_folding_cond1(OptContext *ctx, TCGOp *op, TCGArg dest, return -1; } + i1 = arg_info(*p1); + /* * TSTNE x,x -> NE x,0 - * TSTNE x,-1 -> NE x,0 + * TSTNE x,i -> NE x,0 if i includes all nonzero bits of x */ - if (args_are_copies(*p1, *p2) || arg_is_const_val(*p2, -1)) { + if (args_are_copies(*p1, *p2) || + (arg_is_const(*p2) && (i1->z_mask & ~arg_info(*p2)->val) == 0)) { *p2 = arg_new_constant(ctx, 0); *pcond = tcg_tst_eqne_cond(cond); return -1; } - /* TSTNE x,sign -> LT x,0 */ - if (arg_is_const_val(*p2, (ctx->type == TCG_TYPE_I32 - ? INT32_MIN : INT64_MIN))) { + /* TSTNE x,i -> LT x,0 if i only includes sign bit copies */ + if (arg_is_const(*p2) && (arg_info(*p2)->val & ~i1->s_mask) == 0) { *p2 = arg_new_constant(ctx, 0); *pcond = tcg_tst_ltge_cond(cond); return -1; From 9955093b302b318cd2c8a34dc96913531abd3df9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Feb 2025 15:34:16 +0100 Subject: [PATCH 8/9] rust: pl011: use default set of lints Being the first crate added to QEMU, pl011 has a rather restrictive Clippy setup. This can be sometimes a bit too heavy on its suggestions, for example error: this could be a `const fn` --> hw/char/pl011/src/device.rs:382:5 | 382 | / fn set_read_trigger(&mut self) { 383 | | self.read_trigger = 1; 384 | | } | |_____^ Just use the standard set that is present in rust/Cargo.toml, with just a small adjustment to allow upper case acronyms which are used for register names. Reported-by: Stefan Hajnoczi Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/lib.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs index e704daf6e3..3c72f1221f 100644 --- a/rust/hw/char/pl011/src/lib.rs +++ b/rust/hw/char/pl011/src/lib.rs @@ -12,16 +12,7 @@ //! See [`PL011State`](crate::device::PL011State) for the device model type and //! the [`registers`] module for register types. -#![deny( - clippy::correctness, - clippy::suspicious, - clippy::complexity, - clippy::perf, - clippy::nursery, - clippy::style -)] #![allow(clippy::upper_case_acronyms)] -#![allow(clippy::result_unit_err)] use qemu_api::c_str; From 476d6e4c9c4965734d6f47ee299ac9f84440a9b3 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 7 Feb 2025 15:38:40 +0100 Subject: [PATCH 9/9] rust: restrict missing_const_for_fn to qemu_api crate missing_const_for_fn is not necessarily useful or good. For example in a private API you can always add const later, and in a public API it can be unnecessarily restrictive to annotate everything with const (blocking further improvements to the API). Nevertheless, QEMU turns it on because qemu_api uses const quite aggressively and therefore it can be handy to have as much as possible annotated with const. Outside qemu_api though, not so much: devices are self contained consumers and if there is nothing that could use their functions in const contexts that were not anticipated. Since missing_const_for_fn can be a bit noisy and trigger on trivial functions that no one would ever call in const context, do not turn it on everywhere and only keep it in qemu_api as a special case. Signed-off-by: Paolo Bonzini --- rust/Cargo.toml | 1 - rust/qemu-api/src/lib.rs | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 5b6b6ca438..5b0cb55928 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -52,7 +52,6 @@ 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" diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index 83c6a987c0..3cf9371cff 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -3,6 +3,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #![cfg_attr(not(MESON), doc = include_str!("../README.md"))] +#![deny(clippy::missing_const_for_fn)] #[rustfmt::skip] pub mod bindings;