Re: [PATCH 06/26] rust: add a bit operation module

[Zhao Liu]

On Mon, Dec 09, 2024 at 01:36:57PM +0100, Paolo Bonzini wrote:
> Date: Mon,  9 Dec 2024 13:36:57 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 06/26] rust: add a bit operation module
> X-Mailer: git-send-email 2.47.1
> 
> 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 <zhao1.liu@intel.com>
> Co-authored-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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 b927eb58c8e..adcee661150 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 00000000000..5acd6642d1a
> --- /dev/null
> +++ b/rust/qemu-api/src/bitops.rs
> @@ -0,0 +1,119 @@
> +// Copyright (C) 2024 Intel Corporation.
> +// Author(s): Zhao Liu <zhai1.liu@intel.com>

You deserve to be the author!

> +// 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<Self, Output = Self> + AddAssign<Self> +
> +    BitAnd<Self, Output = Self> + BitAndAssign<Self> +
> +    BitOr<Self, Output = Self> + BitOrAssign<Self> +
> +    BitXor<Self, Output = Self> + BitXorAssign<Self> +
> +    Copy +
> +    Div<Self, Output = Self> + DivAssign<Self> +
> +    Eq +
> +    Mul<Self, Output = Self> + MulAssign<Self> +
> +    Not<Output = Self> + Ord + PartialOrd +
> +    Rem<Self, Output = Self> + RemAssign<Self> +
> +    Shl<Self, Output = Self> + ShlAssign<Self> +
> +    Shl<u32, Output = Self> + ShlAssign<u32> + // add more as needed

Ah, I used to define shift bits as usize. I can change the bit shift
type back to u32 in HPET.

> +    Shr<Self, Output = Self> + ShrAssign<Self> +
> +    Shr<u32, Output = Self> + ShrAssign<u32> // 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
> +    {
> +        assert!(start <= Self::BITS);
> +
> +        Self::ONE << start
> +    }

I think with this helper, I can add activating_bit() and deactivating_bit()
bindings, as they are also commonly used operations.

I will rename them to activate_bit and deactivate_bit, if no one has any
objections.

> +    #[inline]
> +    #[must_use]
> +    fn mask(start: u32, length: u32) -> Self
> +    {
> +        /* FIXME: Implement a more elegant check with error handling 
> support? */

I think current design is elegant enough, and this FIXME is not needed.

> +        assert!(length > 0 && length <= Self::BITS - start);
> +
> +        (Self::MAX >> (Self::BITS - length)) << start
> +    }
> +
> +    #[inline]
> +    #[must_use]
> +    fn deposit<U: IntegerExt>(self, start: u32, length: u32,
> +                          fieldval: U) -> Self
> +        where Self: From<U>
> +    {
> +        debug_assert!(length <= U::BITS);

assert? as you've already replaced debug_assert with assert in BqlCell.

> +        let mask = Self::mask(start, length);
> +        (self & !mask) | ((Self::from(fieldval) << start) & mask)
> +    }
> +

Very useful for HPET! Thanks.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>