Re: [RFC PATCH 1/6] rust: add "bits", a custom bitflags implementation

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, May 21, 2025 at 10:18:40AM +0200, Paolo Bonzini wrote:
> One common thing that device emulation does is manipulate bitmasks, for example
> to check whether two bitmaps have common bits.  One example in the pl011 crate
> is the checks for pending interrupts, where an interrupt cause corresponds to
> at least one interrupt source from a fixed set.
> 
> Unfortunately, this is one case where Rust *can* provide some kind of
> abstraction but it does so with a rather Perl-ish There Is More Way To
> Do It.  It is not something where a crate like "bilge" helps, because
> it only covers the packing of bits in a structure; operations like "are
> all bits of Y set in X" almost never make sense for bit-packed structs;
> you need something else, there are several crates that do it and of course
> we're going to roll our own.
> 
> In particular I examined three:
> 
> - bitmask (https://docs.rs/bitmask/0.5.0/bitmask/) does not support const
>   at all.  This is a showstopper because one of the ugly things in the
>   current pl011 code is the ugliness of code that defines interrupt masks
>   at compile time:
> 
>     pub const E: Self = Self(Self::OE.0 | Self::BE.0 | Self::PE.0 | Self::FE.0);
> 
>   or even worse:
> 
>     const IRQMASK: [u32; 6] = [
>       Interrupt::E.0 | Interrupt::MS.0 | Interrupt::RT.0 | Interrupt::TX.0 | Interrupt::RX.0,
>       ...
>     }
> 
>   You would have to use roughly the same code---"bitmask" only helps with
>   defining the struct.
> 
> - bitmask_enum (https://docs.rs/bitmask-enum/2.2.5/bitmask_enum/) does not
>   have a good separation of "valid" and "invalid" bits, so for example "!x"
>   will invert all 16 bits if you choose u16 as the representation -- even if
>   you only defined 10 bits.  This makes it easier to introduce subtle bugs
>   in comparisons.
> 
> - bitflags (https://docs.rs/bitflags/2.6.0/bitflags/) is generally the most
>   used such crate and is the one that I took most inspiration from with
>   respect to the syntax.  It's a pretty sophisticated implementation,
>   with a lot of bells and whistles such as an implementation of "Iter"
>   that returns the bits one at a time.
> 
> The main thing that all of them lack, however, is a way to simplify the
> ugly definitions like the above.  "bitflags" includes const methods that
> perform AND/OR/XOR of masks (these are necessary because Rust operator
> overloading does not support const yet, and therefore overloaded operators
> cannot be used in the definition of a "static" variable), but they become
> even more verbose and unmanageable, like
> 
>   Interrupt::E.union(Interrupt::MS).union(Interrupt::RT).union(Interrupt::TX).union(Interrupt::RX)
> 
> This was the main reason to create "bits", which allows something like
> 
>   bits!(Interrupt: E | MS | RT | TX | RX)
> 
> and expands it 1) add "Interrupt::" in front of all identifiers 2) convert
> operators to the wordy const functions like "union".  It supports boolean
> operators "&", "|", "^", "!" and parentheses, with a relatively simple
> recursive descent parser that's implemented in qemu_api_macros.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/Cargo.lock                  |   7 +
>  rust/Cargo.toml                  |   1 +
>  rust/bits/Cargo.toml             |  19 ++
>  rust/bits/meson.build            |  12 +
>  rust/bits/src/lib.rs             | 441 +++++++++++++++++++++++++++++++
>  rust/meson.build                 |   1 +
>  rust/qemu-api-macros/src/bits.rs | 227 ++++++++++++++++
>  rust/qemu-api-macros/src/lib.rs  |  12 +
>  8 files changed, 720 insertions(+)
>  create mode 100644 rust/bits/Cargo.toml
>  create mode 100644 rust/bits/meson.build
>  create mode 100644 rust/bits/src/lib.rs
>  create mode 100644 rust/qemu-api-macros/src/bits.rs

> diff --git a/rust/bits/src/lib.rs b/rust/bits/src/lib.rs
> new file mode 100644
> index 00000000000..d80a6263f1e
> --- /dev/null
> +++ b/rust/bits/src/lib.rs
> @@ -0,0 +1,441 @@

This (and other new .rs files) needs SPDX-License-Identifier

> +/// # Definition entry point
> +///
> +/// Define a struct with a single field of type $type.  Include public constants
> +/// for each element listed in braces.
> +///
> +/// The unnamed element at the end, if present, can be used to enlarge the set
> +/// of valid bits.  Bits that are valid but not listed are treated normally for
> +/// the purpose of arithmetic operations, and are printed with their hexadecimal
> +/// value.
> +///
> +/// The struct implements the following traits: [`BitAnd`](std::ops::BitAnd),
> +/// [`BitOr`](std::ops::BitOr), [`BitXor`](std::ops::BitXor),
> +/// [`Not`](std::ops::Not), [`Sub`](std::ops::Sub); [`Debug`](std::fmt::Debug),
> +/// [`Display`](std::fmt::Display), [`Binary`](std::fmt::Binary),
> +/// [`Octal`](std::fmt::Octal), [`LowerHex`](std::fmt::LowerHex),
> +/// [`UpperHex`](std::fmt::UpperHex); [`From`]`<type>`/[`Into`]`<type>` where
> +/// type is the type specified in the definition.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|