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

[Paolo Bonzini]

On 12/10/24 09:13, Zhao Liu wrote:
> > 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!

I'll add both of us.  At this stage even identifying a need is an 
important step.

> > +// 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.

I used u32 because BITS is u32 in the standard library, so probably 
better to change it 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.

I think "-ing" is correct because it looks at a change between two 
values.  Or another possibility is putting the mask as "self":

    fn bits_activated(from: Self, to: Self) -> bool {
        (to & !from & self) != 0
    }

    fn bits_deactivated(from: Self, to: Self) -> bool {
        (from & !to & self) != 0
    }

Anyhow feel free to pick something you like and add it to IntegerExt.

> > +    #[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.

No, not this time :) Rust has checks for overflow in debug mode but not 
in release mode, so (independent of how Meson handles debug assertions) 
debug_assert is the right one here.

Paolo

> > +        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>