| Message ID | 20241205060714.256270-6-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Reinvent the wheel for HPET timer in Rust | expand |
On 12/5/24 07:07, Zhao Liu wrote: > +pub fn deposit64(value: u64, start: usize, length: usize, fieldval: u64) -> u64 { > + /* FIXME: Implement a more elegant check with error handling support? */ > + assert!(length > 0 && length <= 64 - start); > + > + let mask = (u64::MAX >> (64 - length)) << start; > + (value & !mask) | ((fieldval << start) & mask) > +} This should be more generic and implemented as a trait that is implemented by u8/u16/u32/u64. It's okay to rewrite these utility functions in Rust instead of relying on bindgen, because the way you'd like to use them is likely different from C. Something like: pub trait IntegerExt { fn deposit(self, start: u32, length: u32, fieldval: U) -> Self; } impl IntegerExt for u64 { fn deposit(self, start: usize, length: usize, fieldval: u64) -> u64 { /* FIXME: Implement a more elegant check with error handling support? */ assert!(length > 0 && length <= 64 - start); let mask = (u64::MAX >> (64 - length)) << start; (value & !mask) | ((fieldval << start) & mask) } } And we can add a "prelude" module so that you can do use qemu_api::prelude::*; and get all these useful traits at once. I will send a patch after fleshing the idea out a bit more. Paolo
On Thu, Dec 05, 2024 at 05:09:42PM +0100, Paolo Bonzini wrote: > Date: Thu, 5 Dec 2024 17:09:42 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [RFC 05/13] rust: add a bit operation binding for deposit64 > > On 12/5/24 07:07, Zhao Liu wrote: > > +pub fn deposit64(value: u64, start: usize, length: usize, fieldval: u64) -> u64 { > > + /* FIXME: Implement a more elegant check with error handling support? */ > > + assert!(length > 0 && length <= 64 - start); > > + > > + let mask = (u64::MAX >> (64 - length)) << start; > > + (value & !mask) | ((fieldval << start) & mask) > > +} > > This should be more generic and implemented as a trait that is > implemented by u8/u16/u32/u64. Yes, I agree! > It's okay to rewrite these utility > functions in Rust instead of relying on bindgen, because the way > you'd like to use them is likely different from C. Something like: > > pub trait IntegerExt > { > fn deposit(self, start: u32, length: u32, fieldval: U) -> Self; > } > > impl IntegerExt for u64 > { > fn deposit(self, start: usize, length: usize, fieldval: u64) -> u64 { > /* FIXME: Implement a more elegant check with error handling support? */ > assert!(length > 0 && length <= 64 - start); > > let mask = (u64::MAX >> (64 - length)) << start; > (value & !mask) | ((fieldval << start) & mask) > } > } Then C and Rust would be using completely different bitops library, is it necessary to implement the C interface directly in Rust instead of keeping the C implementation (when Rust is enabled)? > And we can add a "prelude" module so that you can do > > use qemu_api::prelude::*; > > and get all these useful traits at once. I will send a patch after > fleshing the idea out a bit more. Thanks! Cross fingers. Regards, Zhao
Il sab 7 dic 2024, 16:43 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > impl IntegerExt for u64 > > { > > fn deposit(self, start: usize, length: usize, fieldval: u64) -> u64 { > > /* FIXME: Implement a more elegant check with error handling > support? */ > > assert!(length > 0 && length <= 64 - start); > > > > let mask = (u64::MAX >> (64 - length)) << start; > > (value & !mask) | ((fieldval << start) & mask) > > } > > } > > Then C and Rust would be using completely different bitops library, is > it necessary to implement the C interface directly in Rust instead of > keeping the C implementation (when Rust is enabled)? > If it's domain specific (related to emulation) then it's better to avoid duplication but In some cases it's unavoidable: for example very very simple inlines (e.g. clock_get_hz for an example) or simple forwarding APIs like the various timer_init variants. It's simpler to redo the forwarding in Rust and only write the callback translation once, than to repeat many times the code to translate the callbacks and forward each init variant to the corresponding C function. Paolo > And we can add a "prelude" module so that you can do > > > > use qemu_api::prelude::*; > > > > and get all these useful traits at once. I will send a patch after > > fleshing the idea out a bit more. > > Thanks! Cross fingers. > > Regards, > Zhao > > >
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 3ac69cbc76c4..00e86a679d8a 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/irq.rs', diff --git a/rust/qemu-api/src/bitops.rs b/rust/qemu-api/src/bitops.rs new file mode 100644 index 000000000000..a11a07fb8830 --- /dev/null +++ b/rust/qemu-api/src/bitops.rs @@ -0,0 +1,11 @@ +// Copyright (C) 2024 Intel Corporation. +// Author(s): Zhao Liu <zhai1.liu@intel.com> +// SPDX-License-Identifier: GPL-2.0-or-later + +pub fn deposit64(value: u64, start: usize, length: usize, fieldval: u64) -> u64 { + /* FIXME: Implement a more elegant check with error handling support? */ + assert!(length > 0 && length <= 64 - start); + + let mask = (u64::MAX >> (64 - length)) << start; + (value & !mask) | ((fieldval << start) & mask) +} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index 0d46b372c6bb..009906c907e7 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -7,6 +7,7 @@ #[rustfmt::skip] pub mod bindings; +pub mod bitops; pub mod c_str; pub mod cell; pub mod irq;
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). Manually implement a binding. Since it only involves bit operations, fortunately, the Rust version of deposit64() is almost identical to the original C version. Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- rust/qemu-api/meson.build | 1 + rust/qemu-api/src/bitops.rs | 11 +++++++++++ rust/qemu-api/src/lib.rs | 1 + 3 files changed, 13 insertions(+) create mode 100644 rust/qemu-api/src/bitops.rs