| Message ID | 20241209123717.99077-7-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: bundle of prerequisites for HPET implementation | expand |
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>
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> > > >
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> +// 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 + 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 + } + + #[inline] + #[must_use] + fn mask(start: u32, length: u32) -> Self + { + /* FIXME: Implement a more elegant check with error handling support? */ + 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); + + 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 0efbef47441..9e007e16354 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -12,6 +12,7 @@ #[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 dfaddbd062a..a39e228babf 100644 --- a/rust/qemu-api/src/prelude.rs +++ b/rust/qemu-api/src/prelude.rs @@ -2,5 +2,7 @@ // Author(s): Paolo Bonzini <pbonzini@redhat.com> // SPDX-License-Identifier: GPL-2.0-or-later +pub use crate::bitops::IntegerExt; + pub use crate::cell::BqlCell; pub use crate::cell::BqlRefCell;