* [RFC 01/13] bql: check that the BQL is not dropped within marked sections
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 6:07 ` [RFC 02/13] rust: cell: add BQL-enforcing RefCell variant Zhao Liu
` (13 subsequent siblings)
14 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
From: Paolo Bonzini <pbonzini@redhat.com>
The Big QEMU Lock (BQL) is used to provide interior mutability to Rust
code. While BqlCell performs indivisible accesses, an equivalent of
RefCell will allow the borrower to hold to the interior content for a
long time. If the BQL is dropped, another thread could come and mutate
the data from C code (Rust code would panic on borrow_mut() instead).
In order to prevent this, add a new BQL primitive that can mark
BQL-atomic sections and aborts if the BQL is dropped within them.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
include/qemu/main-loop.h | 15 +++++++++++++++
stubs/iothread-lock.c | 15 +++++++++++++++
system/cpus.c | 15 +++++++++++++++
3 files changed, 45 insertions(+)
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 5764db157c97..646306c272f7 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -262,6 +262,21 @@ AioContext *iohandler_get_aio_context(void);
*/
bool bql_locked(void);
+/**
+ * bql_block: Allow/deny releasing the BQL
+ *
+ * The Big QEMU Lock (BQL) is used to provide interior mutability to
+ * Rust code, but this only works if other threads cannot run while
+ * the Rust code has an active borrow. This is because C code in
+ * other threads could come in and mutate data under the Rust code's
+ * feet.
+ *
+ * @increase: Whether to increase or decrease the blocking counter.
+ * Releasing the BQL while the counter is nonzero triggers
+ * an assertion failure.
+ */
+void bql_block_unlock(bool increase);
+
/**
* qemu_in_main_thread: return whether it's possible to safely access
* the global state of the block layer.
diff --git a/stubs/iothread-lock.c b/stubs/iothread-lock.c
index d7890e5581c5..54676598950f 100644
--- a/stubs/iothread-lock.c
+++ b/stubs/iothread-lock.c
@@ -1,6 +1,8 @@
#include "qemu/osdep.h"
#include "qemu/main-loop.h"
+static uint32_t bql_unlock_blocked;
+
bool bql_locked(void)
{
return false;
@@ -12,4 +14,17 @@ void bql_lock_impl(const char *file, int line)
void bql_unlock(void)
{
+ assert(!bql_unlock_blocked);
+}
+
+void bql_block_unlock(bool increase)
+{
+ uint32_t new_value;
+
+ assert(bql_locked());
+
+ /* check for overflow! */
+ new_value = bql_unlock_blocked + increase - !increase;
+ assert((new_value > bql_unlock_blocked) == increase);
+ bql_unlock_blocked = new_value;
}
diff --git a/system/cpus.c b/system/cpus.c
index a1b46f68476a..793c4698c7ad 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -514,6 +514,20 @@ bool qemu_in_vcpu_thread(void)
QEMU_DEFINE_STATIC_CO_TLS(bool, bql_locked)
+static uint32_t bql_unlock_blocked;
+
+void bql_block_unlock(bool increase)
+{
+ uint32_t new_value;
+
+ assert(bql_locked());
+
+ /* check for overflow! */
+ new_value = bql_unlock_blocked + increase - !increase;
+ assert((new_value > bql_unlock_blocked) == increase);
+ bql_unlock_blocked = new_value;
+}
+
bool bql_locked(void)
{
return get_bql_locked();
@@ -540,6 +554,7 @@ void bql_lock_impl(const char *file, int line)
void bql_unlock(void)
{
g_assert(bql_locked());
+ g_assert(!bql_unlock_blocked);
set_bql_locked(false);
qemu_mutex_unlock(&bql);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 02/13] rust: cell: add BQL-enforcing RefCell variant
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
2024-12-05 6:07 ` [RFC 01/13] bql: check that the BQL is not dropped within marked sections Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 6:07 ` [RFC 03/13] rust/cell: add get_mut() method for BqlCell Zhao Liu
` (12 subsequent siblings)
14 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
From: Paolo Bonzini <pbonzini@redhat.com>
Similar to the existing BqlCell, introduce a custom interior mutability
primitive that resembles RefCell but accounts for QEMU's threading model.
Borrowing the RefCell requires proving that the BQL is held, and
attempting to access without the BQL is a runtime panic.
Almost all of the code was taken from Rust's standard library, while
removing unstable features and probably-unnecessary functionality that
amounts to 60% of the original code. A lot of what's left is documentation,
as well as unit tests in the form of doctests. These are not yet integrated
in "make check" but can be run with "cargo test --doc".
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes Before RFC v1:
* Changed debug_assert to assert like what Paolo did for BqlCell.
* Added #[derive(Debug)] for BqlRefCell since it looks like BqlRefCell
will often be embedded in the structure with Debug, e.g., HPETState.
---
rust/qemu-api/Cargo.toml | 3 +-
rust/qemu-api/meson.build | 3 +
rust/qemu-api/src/cell.rs | 546 +++++++++++++++++++++++++++++++++++++-
3 files changed, 541 insertions(+), 11 deletions(-)
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index 669f288d1cb5..4aa22f319860 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -20,8 +20,9 @@ qemu_api_macros = { path = "../qemu-api-macros" }
version_check = "~0.9"
[features]
-default = []
+default = ["debug_cell"]
allocator = []
+debug_cell = []
[lints]
workspace = true
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index d727ccf18354..3ac69cbc76c4 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -6,6 +6,9 @@ _qemu_api_cfg = run_command(rustc_args,
if rustc.version().version_compare('>=1.77.0')
_qemu_api_cfg += ['--cfg', 'has_offset_of']
endif
+if get_option('debug_mutex')
+ _qemu_api_cfg += ['--feature', 'debug_cell']
+endif
_qemu_api_rs = static_library(
'qemu_api',
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index 2e4ea8d590d5..07b636f26266 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -46,20 +46,30 @@
//! parts of a device must be made mutable in a controlled manner through the
//! use of cell types.
//!
-//! This module provides a way to do so via the Big QEMU Lock. While
-//! [`BqlCell<T>`] is essentially the same single-threaded primitive that is
-//! available in `std::cell`, the BQL allows it to be used from a multi-threaded
-//! context and to share references across threads, while maintaining Rust's
-//! safety guarantees. For this reason, unlike its `std::cell` counterpart,
-//! `BqlCell` implements the `Sync` trait.
+//! [`BqlCell<T>`] and [`BqlRefCell<T>`] allow doing this via the Big QEMU Lock.
+//! While they are essentially the same single-threaded primitives that are
+//! available in `std::cell`, the BQL allows them to be used from a
+//! multi-threaded context and to share references across threads, while
+//! maintaining Rust's safety guarantees. For this reason, unlike
+//! their `std::cell` counterparts, `BqlCell` and `BqlRefCell` implement the
+//! `Sync` trait.
//!
//! BQL checks are performed in debug builds but can be optimized away in
//! release builds, providing runtime safety during development with no overhead
//! in production.
//!
-//! Warning: While `BqlCell` is similar to its `std::cell` counterpart, the two
-//! are not interchangeable. Using `std::cell` types in QEMU device
-//! implementations is usually incorrect and can lead to thread-safety issues.
+//! The two provide different ways of handling interior mutability.
+//! `BqlRefCell` is best suited for data that is primarily accessed by the
+//! device's own methods, where multiple reads and writes can be grouped within
+//! a single borrow and a mutable reference can be passed around. Instead,
+//! [`BqlCell`] is a better choice when sharing small pieces of data with
+//! external code (especially C code), because it provides simple get/set
+//! operations that can be used one at a time.
+//!
+//! Warning: While `BqlCell` and `BqlRefCell` are similar to their `std::cell`
+//! counterparts, they are not interchangeable. Using `std::cell` types in
+//! QEMU device implementations is usually incorrect and can lead to
+//! thread-safety safety issues.
//!
//! ## `BqlCell<T>`
//!
@@ -80,8 +90,37 @@
//! returns the replaced value.
//! - [`set`](BqlCell::set): this method replaces the interior value,
//! dropping the replaced value.
+//!
+//! ## `BqlRefCell<T>`
+//!
+//! [`BqlRefCell<T>`] uses Rust's lifetimes to implement "dynamic borrowing", a
+//! process whereby one can claim temporary, exclusive, mutable access to the
+//! inner value:
+//!
+//! ```ignore
+//! fn clear_interrupts(&self, val: u32) {
+//! // A mutable borrow gives read-write access to the registers
+//! let mut regs = self.registers.borrow_mut();
+//! let old = regs.interrupt_status();
+//! regs.update_interrupt_status(old & !val);
+//! }
+//! ```
+//!
+//! Borrows for `BqlRefCell<T>`s are tracked at _runtime_, unlike Rust's native
+//! reference types which are entirely tracked statically, at compile time.
+//! Multiple immutable borrows are allowed via [`borrow`](BqlRefCell::borrow),
+//! or a single mutable borrow via [`borrow_mut`](BqlRefCell::borrow_mut). The
+//! thread will panic if these rules are violated or if the BQL is not held.
-use std::{cell::UnsafeCell, cmp::Ordering, fmt, mem};
+use std::{
+ cell::{Cell, UnsafeCell},
+ cmp::Ordering,
+ fmt,
+ marker::PhantomData,
+ mem,
+ ops::{Deref, DerefMut},
+ ptr::NonNull,
+};
use crate::bindings;
@@ -93,6 +132,15 @@ pub fn bql_locked() -> bool {
!cfg!(MESON) || unsafe { bindings::bql_locked() }
}
+fn bql_block_unlock(increase: bool) {
+ if cfg!(MESON) {
+ // SAFETY: this only adjusts a counter
+ unsafe {
+ bindings::bql_block_unlock(increase);
+ }
+ }
+}
+
/// A mutable memory location that is protected by the Big QEMU Lock.
///
/// # Memory layout
@@ -296,3 +344,481 @@ pub fn take(&self) -> T {
self.replace(Default::default())
}
}
+
+/// A mutable memory location with dynamically checked borrow rules,
+/// protected by the Big QEMU Lock.
+///
+/// See the [module-level documentation](self) for more.
+///
+/// # Memory layout
+///
+/// `BqlRefCell<T>` starts with the same in-memory representation as its
+/// inner type `T`.
+#[repr(C)]
+#[derive(Debug)]
+pub struct BqlRefCell<T> {
+ // It is important that this is the first field (which is not the case
+ // for std::cell::BqlRefCell), so that we can use offset_of! on it.
+ // UnsafeCell and repr(C) both prevent usage of niches.
+ value: UnsafeCell<T>,
+ borrow: Cell<BorrowFlag>,
+ // Stores the location of the earliest currently active borrow.
+ // This gets updated whenever we go from having zero borrows
+ // to having a single borrow. When a borrow occurs, this gets included
+ // in the panic message
+ #[cfg(feature = "debug_cell")]
+ borrowed_at: Cell<Option<&'static std::panic::Location<'static>>>,
+}
+
+// Positive values represent the number of `BqlRef` active. Negative values
+// represent the number of `BqlRefMut` active. Right now QEMU's implementation
+// does not allow to create `BqlRefMut`s that refer to distinct, nonoverlapping
+// components of a `BqlRefCell` (e.g., different ranges of a slice).
+//
+// `BqlRef` and `BqlRefMut` are both two words in size, and so there will likely
+// never be enough `BqlRef`s or `BqlRefMut`s in existence to overflow half of
+// the `usize` range. Thus, a `BorrowFlag` will probably never overflow or
+// underflow. However, this is not a guarantee, as a pathological program could
+// repeatedly create and then mem::forget `BqlRef`s or `BqlRefMut`s. Thus, all
+// code must explicitly check for overflow and underflow in order to avoid
+// unsafety, or at least behave correctly in the event that overflow or
+// underflow happens (e.g., see BorrowRef::new).
+type BorrowFlag = isize;
+const UNUSED: BorrowFlag = 0;
+
+#[inline(always)]
+const fn is_writing(x: BorrowFlag) -> bool {
+ x < UNUSED
+}
+
+#[inline(always)]
+const fn is_reading(x: BorrowFlag) -> bool {
+ x > UNUSED
+}
+
+impl<T> BqlRefCell<T> {
+ /// Creates a new `BqlRefCell` containing `value`.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use qemu_api::cell::BqlRefCell;
+ ///
+ /// let c = BqlRefCell::new(5);
+ /// ```
+ #[inline]
+ pub const fn new(value: T) -> BqlRefCell<T> {
+ BqlRefCell {
+ value: UnsafeCell::new(value),
+ borrow: Cell::new(UNUSED),
+ #[cfg(feature = "debug_cell")]
+ borrowed_at: Cell::new(None),
+ }
+ }
+}
+
+// This ensures the panicking code is outlined from `borrow_mut` for
+// `BqlRefCell`.
+#[inline(never)]
+#[cold]
+#[cfg(feature = "debug_cell")]
+fn panic_already_borrowed(source: &Cell<Option<&'static std::panic::Location<'static>>>) -> ! {
+ // If a borrow occurred, then we must already have an outstanding borrow,
+ // so `borrowed_at` will be `Some`
+ panic!("already borrowed at {:?}", source.take().unwrap())
+}
+
+#[inline(never)]
+#[cold]
+#[cfg(not(feature = "debug_cell"))]
+fn panic_already_borrowed() -> ! {
+ panic!("already borrowed")
+}
+
+impl<T> BqlRefCell<T> {
+ #[inline]
+ #[allow(clippy::unused_self)]
+ fn panic_already_borrowed(&self) -> ! {
+ #[cfg(feature = "debug_cell")]
+ {
+ panic_already_borrowed(&self.borrowed_at)
+ }
+ #[cfg(not(feature = "debug_cell"))]
+ {
+ panic_already_borrowed()
+ }
+ }
+
+ /// Immutably borrows the wrapped value.
+ ///
+ /// The borrow lasts until the returned `BqlRef` exits scope. Multiple
+ /// immutable borrows can be taken out at the same time.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the value is currently mutably borrowed.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use qemu_api::cell::BqlRefCell;
+ ///
+ /// let c = BqlRefCell::new(5);
+ ///
+ /// let borrowed_five = c.borrow();
+ /// let borrowed_five2 = c.borrow();
+ /// ```
+ ///
+ /// An example of panic:
+ ///
+ /// ```should_panic
+ /// use qemu_api::cell::BqlRefCell;
+ ///
+ /// let c = BqlRefCell::new(5);
+ ///
+ /// let m = c.borrow_mut();
+ /// let b = c.borrow(); // this causes a panic
+ /// ```
+ #[inline]
+ #[track_caller]
+ pub fn borrow(&self) -> BqlRef<'_, T> {
+ assert!(bql_locked());
+ if let Some(b) = BorrowRef::new(&self.borrow) {
+ // `borrowed_at` is always the *first* active borrow
+ if b.borrow.get() == 1 {
+ #[cfg(feature = "debug_cell")]
+ self.borrowed_at.set(Some(std::panic::Location::caller()));
+ }
+
+ bql_block_unlock(true);
+
+ // SAFETY: `BorrowRef` ensures that there is only immutable access
+ // to the value while borrowed.
+ let value = unsafe { NonNull::new_unchecked(self.value.get()) };
+ BqlRef { value, borrow: b }
+ } else {
+ self.panic_already_borrowed()
+ }
+ }
+
+ /// Mutably borrows the wrapped value.
+ ///
+ /// The borrow lasts until the returned `BqlRefMut` or all `BqlRefMut`s
+ /// derived from it exit scope. The value cannot be borrowed while this
+ /// borrow is active.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the value is currently borrowed.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use qemu_api::cell::BqlRefCell;
+ ///
+ /// let c = BqlRefCell::new("hello".to_owned());
+ ///
+ /// *c.borrow_mut() = "bonjour".to_owned();
+ ///
+ /// assert_eq!(&*c.borrow(), "bonjour");
+ /// ```
+ ///
+ /// An example of panic:
+ ///
+ /// ```should_panic
+ /// use qemu_api::cell::BqlRefCell;
+ ///
+ /// let c = BqlRefCell::new(5);
+ /// let m = c.borrow();
+ ///
+ /// let b = c.borrow_mut(); // this causes a panic
+ /// ```
+ #[inline]
+ #[track_caller]
+ pub fn borrow_mut(&self) -> BqlRefMut<'_, T> {
+ if let Some(b) = BorrowRefMut::new(&self.borrow) {
+ #[cfg(feature = "debug_cell")]
+ {
+ self.borrowed_at.set(Some(std::panic::Location::caller()));
+ }
+
+ // SAFETY: this only adjusts a counter
+ bql_block_unlock(true);
+
+ // SAFETY: `BorrowRefMut` guarantees unique access.
+ let value = unsafe { NonNull::new_unchecked(self.value.get()) };
+ BqlRefMut {
+ value,
+ _borrow: b,
+ marker: PhantomData,
+ }
+ } else {
+ self.panic_already_borrowed()
+ }
+ }
+
+ /// Returns a raw pointer to the underlying data in this cell.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use qemu_api::cell::BqlRefCell;
+ ///
+ /// let c = BqlRefCell::new(5);
+ ///
+ /// let ptr = c.as_ptr();
+ /// ```
+ #[inline]
+ pub const fn as_ptr(&self) -> *mut T {
+ self.value.get()
+ }
+}
+
+// SAFETY: Same as for std::sync::Mutex. In the end this is a Mutex that is
+// stored out-of-line. Even though BqlRefCell includes Cells, they are
+// themselves protected by the Big QEMU Lock. Furtheremore, the Big QEMU
+// Lock cannot be released while any borrows is active.
+unsafe impl<T> Send for BqlRefCell<T> where T: Send {}
+unsafe impl<T> Sync for BqlRefCell<T> {}
+
+impl<T: Clone> Clone for BqlRefCell<T> {
+ /// # Panics
+ ///
+ /// Panics if the value is currently mutably borrowed.
+ #[inline]
+ #[track_caller]
+ fn clone(&self) -> BqlRefCell<T> {
+ BqlRefCell::new(self.borrow().clone())
+ }
+
+ /// # Panics
+ ///
+ /// Panics if `source` is currently mutably borrowed.
+ #[inline]
+ #[track_caller]
+ fn clone_from(&mut self, source: &Self) {
+ self.value.get_mut().clone_from(&source.borrow())
+ }
+}
+
+impl<T: Default> Default for BqlRefCell<T> {
+ /// Creates a `BqlRefCell<T>`, with the `Default` value for T.
+ #[inline]
+ fn default() -> BqlRefCell<T> {
+ BqlRefCell::new(Default::default())
+ }
+}
+
+impl<T: PartialEq> PartialEq for BqlRefCell<T> {
+ /// # Panics
+ ///
+ /// Panics if the value in either `BqlRefCell` is currently mutably
+ /// borrowed.
+ #[inline]
+ fn eq(&self, other: &BqlRefCell<T>) -> bool {
+ *self.borrow() == *other.borrow()
+ }
+}
+
+impl<T: Eq> Eq for BqlRefCell<T> {}
+
+impl<T: PartialOrd> PartialOrd for BqlRefCell<T> {
+ /// # Panics
+ ///
+ /// Panics if the value in either `BqlRefCell` is currently mutably
+ /// borrowed.
+ #[inline]
+ fn partial_cmp(&self, other: &BqlRefCell<T>) -> Option<Ordering> {
+ self.borrow().partial_cmp(&*other.borrow())
+ }
+}
+
+impl<T: Ord> Ord for BqlRefCell<T> {
+ /// # Panics
+ ///
+ /// Panics if the value in either `BqlRefCell` is currently mutably
+ /// borrowed.
+ #[inline]
+ fn cmp(&self, other: &BqlRefCell<T>) -> Ordering {
+ self.borrow().cmp(&*other.borrow())
+ }
+}
+
+impl<T> From<T> for BqlRefCell<T> {
+ /// Creates a new `BqlRefCell<T>` containing the given value.
+ fn from(t: T) -> BqlRefCell<T> {
+ BqlRefCell::new(t)
+ }
+}
+
+struct BorrowRef<'b> {
+ borrow: &'b Cell<BorrowFlag>,
+}
+
+impl<'b> BorrowRef<'b> {
+ #[inline]
+ fn new(borrow: &'b Cell<BorrowFlag>) -> Option<BorrowRef<'b>> {
+ let b = borrow.get().wrapping_add(1);
+ if !is_reading(b) {
+ // Incrementing borrow can result in a non-reading value (<= 0) in these cases:
+ // 1. It was < 0, i.e. there are writing borrows, so we can't allow a read
+ // borrow due to Rust's reference aliasing rules
+ // 2. It was isize::MAX (the max amount of reading borrows) and it overflowed
+ // into isize::MIN (the max amount of writing borrows) so we can't allow an
+ // additional read borrow because isize can't represent so many read borrows
+ // (this can only happen if you mem::forget more than a small constant amount
+ // of `BqlRef`s, which is not good practice)
+ None
+ } else {
+ // Incrementing borrow can result in a reading value (> 0) in these cases:
+ // 1. It was = 0, i.e. it wasn't borrowed, and we are taking the first read
+ // borrow
+ // 2. It was > 0 and < isize::MAX, i.e. there were read borrows, and isize is
+ // large enough to represent having one more read borrow
+ borrow.set(b);
+ Some(BorrowRef { borrow })
+ }
+ }
+}
+
+impl Drop for BorrowRef<'_> {
+ #[inline]
+ fn drop(&mut self) {
+ let borrow = self.borrow.get();
+ assert!(is_reading(borrow));
+ self.borrow.set(borrow - 1);
+ bql_block_unlock(false)
+ }
+}
+
+impl Clone for BorrowRef<'_> {
+ #[inline]
+ fn clone(&self) -> Self {
+ BorrowRef::new(self.borrow).unwrap()
+ }
+}
+
+/// Wraps a borrowed reference to a value in a `BqlRefCell` box.
+/// A wrapper type for an immutably borrowed value from a `BqlRefCell<T>`.
+///
+/// See the [module-level documentation](self) for more.
+pub struct BqlRef<'b, T: 'b> {
+ // NB: we use a pointer instead of `&'b T` to avoid `noalias` violations, because a
+ // `BqlRef` argument doesn't hold immutability for its whole scope, only until it drops.
+ // `NonNull` is also covariant over `T`, just like we would have with `&T`.
+ value: NonNull<T>,
+ borrow: BorrowRef<'b>,
+}
+
+impl<T> Deref for BqlRef<'_, T> {
+ type Target = T;
+
+ #[inline]
+ fn deref(&self) -> &T {
+ // SAFETY: the value is accessible as long as we hold our borrow.
+ unsafe { self.value.as_ref() }
+ }
+}
+
+impl<'b, T> BqlRef<'b, T> {
+ /// Copies a `BqlRef`.
+ ///
+ /// The `BqlRefCell` is already immutably borrowed, so this cannot fail.
+ ///
+ /// This is an associated function that needs to be used as
+ /// `BqlRef::clone(...)`. A `Clone` implementation or a method would
+ /// interfere with the widespread use of `r.borrow().clone()` to clone
+ /// the contents of a `BqlRefCell`.
+ #[must_use]
+ #[inline]
+ #[allow(clippy::should_implement_trait)]
+ pub fn clone(orig: &BqlRef<'b, T>) -> BqlRef<'b, T> {
+ BqlRef {
+ value: orig.value,
+ borrow: orig.borrow.clone(),
+ }
+ }
+}
+
+impl<T: fmt::Debug> fmt::Debug for BqlRef<'_, T> {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ (**self).fmt(f)
+ }
+}
+
+impl<T: fmt::Display> fmt::Display for BqlRef<'_, T> {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ (**self).fmt(f)
+ }
+}
+
+struct BorrowRefMut<'b> {
+ borrow: &'b Cell<BorrowFlag>,
+}
+
+impl<'b> BorrowRefMut<'b> {
+ #[inline]
+ fn new(borrow: &'b Cell<BorrowFlag>) -> Option<BorrowRefMut<'b>> {
+ // There must currently be no existing references when borrow_mut() is
+ // called, so we explicitly only allow going from UNUSED to UNUSED - 1.
+ match borrow.get() {
+ UNUSED => {
+ borrow.set(UNUSED - 1);
+ Some(BorrowRefMut { borrow })
+ }
+ _ => None,
+ }
+ }
+}
+
+impl Drop for BorrowRefMut<'_> {
+ #[inline]
+ fn drop(&mut self) {
+ let borrow = self.borrow.get();
+ assert!(is_writing(borrow));
+ self.borrow.set(borrow + 1);
+ bql_block_unlock(false)
+ }
+}
+
+/// A wrapper type for a mutably borrowed value from a `BqlRefCell<T>`.
+///
+/// See the [module-level documentation](self) for more.
+pub struct BqlRefMut<'b, T: 'b> {
+ // NB: we use a pointer instead of `&'b mut T` to avoid `noalias` violations, because a
+ // `BqlRefMut` argument doesn't hold exclusivity for its whole scope, only until it drops.
+ value: NonNull<T>,
+ _borrow: BorrowRefMut<'b>,
+ // `NonNull` is covariant over `T`, so we need to reintroduce invariance.
+ marker: PhantomData<&'b mut T>,
+}
+
+impl<T> Deref for BqlRefMut<'_, T> {
+ type Target = T;
+
+ #[inline]
+ fn deref(&self) -> &T {
+ // SAFETY: the value is accessible as long as we hold our borrow.
+ unsafe { self.value.as_ref() }
+ }
+}
+
+impl<T> DerefMut for BqlRefMut<'_, T> {
+ #[inline]
+ fn deref_mut(&mut self) -> &mut T {
+ // SAFETY: the value is accessible as long as we hold our borrow.
+ unsafe { self.value.as_mut() }
+ }
+}
+
+impl<T: fmt::Debug> fmt::Debug for BqlRefMut<'_, T> {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ (**self).fmt(f)
+ }
+}
+
+impl<T: fmt::Display> fmt::Display for BqlRefMut<'_, T> {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ (**self).fmt(f)
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 03/13] rust/cell: add get_mut() method for BqlCell
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
2024-12-05 6:07 ` [RFC 01/13] bql: check that the BQL is not dropped within marked sections Zhao Liu
2024-12-05 6:07 ` [RFC 02/13] rust: cell: add BQL-enforcing RefCell variant Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 15:55 ` Paolo Bonzini
2024-12-05 6:07 ` [RFC 04/13] rust: add bindings for gpio_{in|out} initialization Zhao Liu
` (11 subsequent siblings)
14 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
The get_mut() is useful when doing compound assignment operations, e.g.,
*c.get_mut() += 1.
Implement get_mut() for BqlCell by referring to Cell.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/src/cell.rs | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index 07b636f26266..95f1cc0b3eb5 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -324,6 +324,31 @@ impl<T> BqlCell<T> {
pub const fn as_ptr(&self) -> *mut T {
self.value.get()
}
+
+ /// Returns a mutable reference to the underlying data.
+ ///
+ /// This call borrows `BqlCell` mutably (at compile-time) which guarantees
+ /// that we possess the only reference.
+ ///
+ /// However be cautious: this method expects `self` to be mutable, which is
+ /// generally not the case when using a `BqlCell`. If you require interior
+ /// mutability by reference, consider using `BqlRefCell` which provides
+ /// run-time checked mutable borrows through its [`borrow_mut`] method.
+ ///
+ /// [`borrow_mut`]: BqlRefCell::borrow_mut()
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use qemu_api::cell::BqlCell;;
+ ///
+ /// let mut c = BqlCell::new(5);
+ /// *c.get_mut() += 1;
+ ///
+ /// assert_eq!(c.get(), 6);
+ pub fn get_mut(&mut self) -> &mut T {
+ self.value.get_mut()
+ }
}
impl<T: Default> BqlCell<T> {
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 03/13] rust/cell: add get_mut() method for BqlCell
2024-12-05 6:07 ` [RFC 03/13] rust/cell: add get_mut() method for BqlCell Zhao Liu
@ 2024-12-05 15:55 ` Paolo Bonzini
2024-12-07 15:56 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 15:55 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> The get_mut() is useful when doing compound assignment operations, e.g.,
> *c.get_mut() += 1.
>
> Implement get_mut() for BqlCell by referring to Cell.
I think you can't do this because the BQL might be released while the owner has a &mut. Like:
let mtx = Mutex<BqlCell<u32>>::new();
let guard = mtx.lock();
let cell = &mut *guard;
let inner = cell.get_mut(cell);
// anything that releases bql_lock
*inner += 1;
On the other hand I don't think you need it. You have just two uses.
First, this one:
+ if set && self.is_int_level_triggered() {
+ // If Timer N Interrupt Enable bit is 0, "the timer will
+ // still operate and generate appropriate status bits, but
+ // will not cause an interrupt"
+ *self.get_state_mut().int_status.get_mut() |= mask;
+ } else {
+ *self.get_state_mut().int_status.get_mut() &= !mask;
+ }
Where you can just write
self.get_state_ref().update_int_status(self.index,
set && self.is_int_level_triggered())
and the HPETState can do something like
fn update_int_status(&self, index: u32, level: bool) {
self.int_status.set(deposit64(self.int_status.get(), bit, 1, level as u64));
}
For hpet_fw_cfg you have unsafe in the device and it's better if you do:
- self.hpet_id.set(unsafe { hpet_fw_cfg.assign_hpet_id() });
+ self.hpet_id.set(fw_cfg_config::assign_hpet_id());
with methods like this that do the unsafe access:
impl fw_cfg_config {
pub(crate) fn assign_hpet_id() -> usize {
assert!(bql_locked());
// SAFETY: all accesses go through these methods, which guarantee
// that the accesses are protected by the BQL.
let fw_cfg = unsafe { &mut *hpet_fw_cfg };
if self.count == u8::MAX {
// first instance
fw_cfg.count = 0;
}
if fw_cfg.count == 8 {
// TODO: Add error binding: error_setg()
panic!("Only 8 instances of HPET is allowed");
}
let id: usize = fw_cfg.count.into();
fw_cfg.count += 1;
id
}
}
and you can assert bql_locked by hand instead of using the BqlCell.
Paolo
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> rust/qemu-api/src/cell.rs | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
> index 07b636f26266..95f1cc0b3eb5 100644
> --- a/rust/qemu-api/src/cell.rs
> +++ b/rust/qemu-api/src/cell.rs
> @@ -324,6 +324,31 @@ impl<T> BqlCell<T> {
> pub const fn as_ptr(&self) -> *mut T {
> self.value.get()
> }
> +
> + /// Returns a mutable reference to the underlying data.
> + ///
> + /// This call borrows `BqlCell` mutably (at compile-time) which guarantees
> + /// that we possess the only reference.
> + ///
> + /// However be cautious: this method expects `self` to be mutable, which is
> + /// generally not the case when using a `BqlCell`. If you require interior
> + /// mutability by reference, consider using `BqlRefCell` which provides
> + /// run-time checked mutable borrows through its [`borrow_mut`] method.
> + ///
> + /// [`borrow_mut`]: BqlRefCell::borrow_mut()
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use qemu_api::cell::BqlCell;;
> + ///
> + /// let mut c = BqlCell::new(5);
> + /// *c.get_mut() += 1;
> + ///
> + /// assert_eq!(c.get(), 6);
> + pub fn get_mut(&mut self) -> &mut T {
> + self.value.get_mut()
> + }
> }
>
> impl<T: Default> BqlCell<T> {
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 03/13] rust/cell: add get_mut() method for BqlCell
2024-12-05 15:55 ` Paolo Bonzini
@ 2024-12-07 15:56 ` Zhao Liu
2024-12-07 19:49 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-07 15:56 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Thu, Dec 05, 2024 at 04:55:47PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 16:55:47 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 03/13] rust/cell: add get_mut() method for BqlCell
>
> On 12/5/24 07:07, Zhao Liu wrote:
> > The get_mut() is useful when doing compound assignment operations, e.g.,
> > *c.get_mut() += 1.
> >
> > Implement get_mut() for BqlCell by referring to Cell.
>
> I think you can't do this because the BQL might be released while the owner has a &mut. Like:
Thanks for pointing that out, I really didn't think of that, I
understand how that would break the atomicity of the BQL lock, right?
> let mtx = Mutex<BqlCell<u32>>::new();
> let guard = mtx.lock();
> let cell = &mut *guard;
> let inner = cell.get_mut(cell);
> // anything that releases bql_lock
> *inner += 1;
>
> On the other hand I don't think you need it. You have just two uses.
>
> First, this one:
>
> + if set && self.is_int_level_triggered() {
> + // If Timer N Interrupt Enable bit is 0, "the timer will
> + // still operate and generate appropriate status bits, but
> + // will not cause an interrupt"
> + *self.get_state_mut().int_status.get_mut() |= mask;
> + } else {
> + *self.get_state_mut().int_status.get_mut() &= !mask;
> + }
>
> Where you can just write
>
> self.get_state_ref().update_int_status(self.index,
> set && self.is_int_level_triggered())
>
> and the HPETState can do something like
>
> fn update_int_status(&self, index: u32, level: bool) {
> self.int_status.set(deposit64(self.int_status.get(), bit, 1, level as u64));
> }
Yes, it's clearer!
> For hpet_fw_cfg you have unsafe in the device and it's better if you do:
>
> - self.hpet_id.set(unsafe { hpet_fw_cfg.assign_hpet_id() });
> + self.hpet_id.set(fw_cfg_config::assign_hpet_id());
>
> with methods like this that do the unsafe access:
>
> impl fw_cfg_config {
> pub(crate) fn assign_hpet_id() -> usize {
> assert!(bql_locked());
> // SAFETY: all accesses go through these methods, which guarantee
> // that the accesses are protected by the BQL.
> let fw_cfg = unsafe { &mut *hpet_fw_cfg };
Nice idea!
> if self.count == u8::MAX {
> // first instance
> fw_cfg.count = 0;
> }
Will something like “anything that releases bql_lock” happen here?
There seems to be no atomicity guarantee here.
> if fw_cfg.count == 8 {
> // TODO: Add error binding: error_setg()
> panic!("Only 8 instances of HPET is allowed");
> }
>
> let id: usize = fw_cfg.count.into();
> fw_cfg.count += 1;
> id
> }
> }
>
> and you can assert bql_locked by hand instead of using the BqlCell.
Thanks! I can also add a line of doc for bql_locked that it can be used
directly without BqlCell if necessary.
And if you also agree the Phillipe's idea, I also need to add BqlCell
for fw_cfg field in HPETClass :-).
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 03/13] rust/cell: add get_mut() method for BqlCell
2024-12-07 15:56 ` Zhao Liu
@ 2024-12-07 19:49 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-07 19:49 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]
Il sab 7 dic 2024, 16:38 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> Thanks for pointing that out, I really didn't think of that, I
> understand how that would break the atomicity of the BQL lock, right?
>
Yes, but also the function seems unnecessary.
> impl fw_cfg_config {
> > pub(crate) fn assign_hpet_id() -> usize {
> > assert!(bql_locked());
> > // SAFETY: all accesses go through these methods, which guarantee
> // that the accesses are protected by the BQL.
> > let fw_cfg = unsafe { &mut *hpet_fw_cfg };
>
> Nice idea!
>
> > if self.count == u8::MAX {
> > // first instance
> > fw_cfg.count = 0;
> > }
>
> Will something like “anything that releases bql_lock” happen here?
No, there are no function calls even.
There seems to be no atomicity guarantee here.
>
It's not beautiful but it's guaranteed to be atomic. For the rare case of
static mut, which is unsafe anyway, it makes sense.
>
> > if fw_cfg.count == 8 {
> > // TODO: Add error binding: error_setg()
> > panic!("Only 8 instances of HPET is allowed");
> > }
> >
> > let id: usize = fw_cfg.count.into();
> > fw_cfg.count += 1;
> > id
> > }
> > }
> >
> > and you can assert bql_locked by hand instead of using the BqlCell.
>
> Thanks! I can also add a line of doc for bql_locked that it can be used
> directly without BqlCell if necessary.
>
Good idea!
And if you also agree the Phillipe's idea, I also need to add BqlCell
> for fw_cfg field in HPETClass :-).
>
No, that also breaks compilation with CONFIG_HPET=n. The idea is nice but
it doesn't work. ¯\_(ツ)_/¯
Paolo
> Regards,
> Zhao
>
>
>
[-- Attachment #2: Type: text/html, Size: 3880 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (2 preceding siblings ...)
2024-12-05 6:07 ` [RFC 03/13] rust/cell: add get_mut() method for BqlCell Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 18:53 ` Paolo Bonzini
2024-12-05 6:07 ` [RFC 05/13] rust: add a bit operation binding for deposit64 Zhao Liu
` (10 subsequent siblings)
14 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to
wrap them as DeviceState's methods in Rust API, which could eliminate
unsafe cases in the device lib.
Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl.
In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust
callback into a C-style callback qemu_irq_handler, add a handler pointer
member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it
needs to define a handler. And for device which just wants to initialize
GPIO out, it can leave the GPIO_IRQ_HANDLER as None.
Then device could use init_gpio_in() and init_gpio_out() to initialize
GPIO in and out, like C code.
Note, for qemu_irq_handler, assume the opaque parameter refers to the
self DeviceState, and this is enough as for now, as it's the most common
case in QEMU.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/src/qdev.rs | 55 +++++++++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 23a06b377b2c..5e6580b6f261 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -4,12 +4,17 @@
//! Bindings to create devices and access device functionality from Rust.
-use std::ffi::CStr;
+use std::{
+ ffi::CStr,
+ os::raw::{c_int, c_void},
+ ptr::{addr_of, NonNull},
+};
pub use bindings::{DeviceClass, DeviceState, Property};
use crate::{
- bindings::{self, Error},
+ bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error},
+ irq::InterruptSource,
qom::{ClassInitImpl, Object, ObjectClass, ObjectType},
qom_isa,
vmstate::VMStateDescription,
@@ -144,3 +149,49 @@ unsafe impl ObjectType for DeviceState {
unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) };
}
qom_isa!(DeviceState: Object);
+
+/// # Safety
+///
+/// We expect the FFI user of this function to pass a valid pointer that
+/// can be downcasted to type `T`. We also expect the device is
+/// readable/writeable from one thread at any time.
+///
+/// Note: Always assume opaque is referred to the self DeviceState, and
+/// this is also the most common case in QEMU.
+unsafe extern "C" fn rust_irq_handler<T: DeviceGPIOImpl>(
+ opaque: *mut c_void,
+ lines_num: c_int,
+ level: c_int,
+) {
+ // SAFETY:
+ // the pointer is convertible to a reference
+ let state = unsafe { NonNull::new(opaque.cast::<T>()).unwrap().as_mut() };
+
+ T::GPIO_IRQ_HANDLER.unwrap()(state, lines_num as u32, level as u32);
+}
+
+/// Trait that defines the irq handler for GPIO in.
+pub trait DeviceGPIOImpl {
+ const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None;
+
+ fn init_gpio_in(&self, lines_num: u32)
+ where
+ Self: Sized,
+ {
+ assert!(Self::GPIO_IRQ_HANDLER.is_some());
+
+ unsafe {
+ qdev_init_gpio_in(
+ addr_of!(*self) as *mut _,
+ Some(rust_irq_handler::<Self>),
+ lines_num as c_int,
+ );
+ }
+ }
+
+ fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) {
+ unsafe {
+ qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int);
+ }
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2024-12-05 6:07 ` [RFC 04/13] rust: add bindings for gpio_{in|out} initialization Zhao Liu
@ 2024-12-05 18:53 ` Paolo Bonzini
2024-12-08 16:27 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 18:53 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to
> wrap them as DeviceState's methods in Rust API, which could eliminate
> unsafe cases in the device lib.
>
> Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl.
>
> In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust
> callback into a C-style callback qemu_irq_handler, add a handler pointer
> member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it
> needs to define a handler. And for device which just wants to initialize
> GPIO out, it can leave the GPIO_IRQ_HANDLER as None.
This has the same issue as timers, in that you could have (especially
once someone adds named GPIOs) multiple handlers. So we need the same
kind of Fn-based thing here too.
> +/// Trait that defines the irq handler for GPIO in.
> +pub trait DeviceGPIOImpl {
> + const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None;
Ah, I see that you're placing the qdev_init_gpio_in here so that you
only make that accessible for devices that did implement DeviceGPIOImpl.
However you are not guaranteeing that this _is_ a DeviceState.
If the handler can be passed as a function, the problem of getting the
GPIO_INT_HANDLER does not exist anymore. So with the code in rust-next
you can add these to a trait like
/// Trait for methods of [`DeviceState`] and its subclasses.
pub trait DeviceMethods: ObjectDeref
where
Self::Target: IsA<DeviceState>,
{
fn init_gpio_in<F: ...)(&self, lines_num: u32, f: &F) {
}
}
impl<R: ObjectDeref> DeviceMethods for R where R::Target:
IsA<DeviceState> {}
> + fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) {
> + unsafe {
> + qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int);
> + }
> + }
> +}
Pass a slice &[InterruptSource], and get the "len" from the length of
the slice.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2024-12-05 18:53 ` Paolo Bonzini
@ 2024-12-08 16:27 ` Zhao Liu
2024-12-09 11:08 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-08 16:27 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Thu, Dec 05, 2024 at 07:53:42PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 19:53:42 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
>
> On 12/5/24 07:07, Zhao Liu wrote:
> > The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to
> > wrap them as DeviceState's methods in Rust API, which could eliminate
> > unsafe cases in the device lib.
> >
> > Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl.
> >
> > In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust
> > callback into a C-style callback qemu_irq_handler, add a handler pointer
> > member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it
> > needs to define a handler. And for device which just wants to initialize
> > GPIO out, it can leave the GPIO_IRQ_HANDLER as None.
>
> This has the same issue as timers, in that you could have (especially once
> someone adds named GPIOs) multiple handlers. So we need the same kind of
> Fn-based thing here too.
I will refer to the timer callback prototype you suggested and try that
way. Will you rework the timer binding soon? (I'm sorry for bringing such
burden to you).
> > +/// Trait that defines the irq handler for GPIO in.
> > +pub trait DeviceGPIOImpl {
> > + const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None;
>
> Ah, I see that you're placing the qdev_init_gpio_in here so that you
> only make that accessible for devices that did implement DeviceGPIOImpl.
> However you are not guaranteeing that this _is_ a DeviceState.
Thank you, I really couldn't think of a good way to implement the
DeviceState method...One reason is that DeviceImpl is a bit confusing to
me, and please see the comment below.
> If the handler can be passed as a function, the problem of getting the
> GPIO_INT_HANDLER does not exist anymore. So with the code in rust-next you
> can add these to a trait like
>
> /// Trait for methods of [`DeviceState`] and its subclasses.
> pub trait DeviceMethods: ObjectDeref
> where
> Self::Target: IsA<DeviceState>,
> {
> fn init_gpio_in<F: ...)(&self, lines_num: u32, f: &F) {
> }
> }
>
> impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState>
> {}
>
Thank you for your idea! This is a true implementation of the DeviceState
method. I'll try this way!
Additionally, the current DeviceImpl trait is quite special. Although in
Rust, DeviceImpl traits are implemented for device states, DeviceImpl is
actually used for device classes.
Semantically, it might be more natural for DeviceImpl to be a trait for
device classes. However, parameters of its methods are DeviceState, so
it makes sense as a trait for states in Rust.
This seems to be a different design before C and Rust Qdev.
> > + fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) {
> > + unsafe {
> > + qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int);
> > + }
> > + }
> > +}
>
> Pass a slice &[InterruptSource], and get the "len" from the length of the
> slice.
Thanks! Will change this.
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2024-12-08 16:27 ` Zhao Liu
@ 2024-12-09 11:08 ` Paolo Bonzini
2025-01-16 3:04 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-09 11:08 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Sun, Dec 8, 2024 at 5:09 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > This has the same issue as timers, in that you could have (especially once
> > someone adds named GPIOs) multiple handlers. So we need the same kind of
> > Fn-based thing here too.
>
> I will refer to the timer callback prototype you suggested and try that
> way. Will you rework the timer binding soon? (I'm sorry for bringing such
> burden to you).
No, I have written a utility that can be used for all callbacks but
I'll leave it to you to use it for timers. Both because you have
already started the work, and because it helps if one person writes
the code and one uses it.
> Additionally, the current DeviceImpl trait is quite special. Although in
> Rust, DeviceImpl traits are implemented for device states, DeviceImpl is
> actually used for device classes.
>
> Semantically, it might be more natural for DeviceImpl to be a trait for
> device classes. However, parameters of its methods are DeviceState, so
> it makes sense as a trait for states in Rust.
>
> This seems to be a different design before C and Rust Qdev.
I agree that there are differences in how you write the code, due to
the fact that Rust supports associating functions and traits to a
struct, while C only has a global namespace. Also, functions in a
trait can look like both "normal" and static methods, so it's easier
to place all functions in DeviceState. The DeviceClass part is mostly
automatic.
So if Xyz is a struct corresponding to a QOM type, it will:
- include a field of type Abc corresponding to the direct superclass
- implement virtual methods for all superclasses through traits such
as AbcImpl or DefImpl, up to ObjectImpl
- expose its virtual methods to C thorough a blanket implementation of
ClassInitImpl<AbcClass> or ClassInitImpl<DefClass>
- invoke methods through blanket implementations of AbcMethods,
AbcClassMethods etc. for all superclasses
and the structure of all the blanket implementation is always the same:
pub trait DeviceClassMethods: IsA<DeviceState> {...}
impl<T> DeviceClassMethods for T where T: IsA<DeviceState> {}
pub trait DeviceMethods: ObjectDeref
where
Self::Target: IsA<DeviceState>,
{...}
impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
impl<T> ClassInitImpl<DeviceClass> for T
where
T: ClassInitImpl<ObjectClass> + DeviceImpl
{...}
In the future, developers will not need to worry much about these, but
for some time every new device will probably need a few new functions
or even modules in qemu_api.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2024-12-09 11:08 ` Paolo Bonzini
@ 2025-01-16 3:04 ` Zhao Liu
2025-01-17 9:40 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2025-01-16 3:04 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
> and the structure of all the blanket implementation is always the same:
>
> pub trait DeviceClassMethods: IsA<DeviceState> {...}
> impl<T> DeviceClassMethods for T where T: IsA<DeviceState> {}
>
> pub trait DeviceMethods: ObjectDeref
> where
> Self::Target: IsA<DeviceState>,
> {...}
> impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
>
> impl<T> ClassInitImpl<DeviceClass> for T
> where
> T: ClassInitImpl<ObjectClass> + DeviceImpl
> {...}
>
Similiar to timer, now I've also worked out the gpio bindings (but one
thing that's different is that it uses `self` as an parameter) like:
* gpio_in and gpio_out:
/// Trait for methods of [`DeviceState`] and its subclasses.
pub trait DeviceMethods: ObjectDeref
where
Self::Target: IsA<DeviceState>,
{
fn init_gpio_in<F>(&self, lines_num: u32, _f: F)
where
F: for<'a> FnCall<(&'a Self::Target, u32, u32)>,
{
unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
opaque: *mut c_void,
lines_num: c_int,
level: c_int,
) {
// SAFETY: the opaque was passed as a reference to `T`
F::call((
unsafe { &*(opaque.cast::<T>()) },
lines_num as u32,
level as u32,
))
}
let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) =
rust_irq_handler::<Self::Target, F>;
unsafe {
qdev_init_gpio_in(
self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
Some(gpio_in_cb),
lines_num as c_int,
);
}
}
fn init_gpio_out(&self, pins: &[InterruptSource]) {
assert!(pins.len() > 0);
unsafe {
qdev_init_gpio_out(
self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
pins[0].as_ptr(),
pins.len() as c_int,
);
}
}
}
impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
* Use case in HPET:
impl HPETState {
...
fn handle_legacy_irq(&self, irq: u32, level: u32) {
if irq == HPET_LEGACY_PIT_INT {
if !self.is_legacy_mode() {
self.irqs[0].set(level != 0);
}
} else {
self.rtc_irq_level.set(level as u8);
if !self.is_legacy_mode() {
self.irqs[RTC_ISA_IRQ].set(level != 0);
}
}
}
...
fn realize(&self) {
...
self.init_gpio_in(2, HPETState::handle_legacy_irq);
self.init_gpio_out(from_ref(&self.pit_enabled));
}
}
---
I made the handler accept the inmuttable reference, but init_gpio_in()
is called in realize(), which now accepts the `&mut self`.
I think init_gpio_in() should be called in realize() so that realize()
needs to become safe in advance (before your plan).
The safe realize() mainly affects pl011, and before you formally
introduce char binding, if you don't mind, I can make this change to
pl011:
- pub fn realize(&mut self) {
+ pub fn realize(&self) {
// SAFETY: self.char_backend has the correct size and alignment for a
// CharBackend object, and its callbacks are of the correct types.
unsafe {
qemu_chr_fe_set_handlers(
- addr_of_mut!(self.char_backend),
+ addr_of!(self.char_backend) as *mut CharBackend,
Some(pl011_can_receive),
Some(pl011_receive),
Some(pl011_event),
None,
- addr_of_mut!(*self).cast::<c_void>(),
+ addr_of!(*self).cast::<c_void>() as *mut c_void,
core::ptr::null_mut(),
true,
);
Thanks,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2025-01-16 3:04 ` Zhao Liu
@ 2025-01-17 9:40 ` Paolo Bonzini
2025-01-17 11:14 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:40 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
Like timer there are just a couple nits here.
On Thu, Jan 16, 2025 at 3:45 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> * gpio_in and gpio_out:
>
> /// Trait for methods of [`DeviceState`] and its subclasses.
> pub trait DeviceMethods: ObjectDeref
> where
> Self::Target: IsA<DeviceState>,
> {
> fn init_gpio_in<F>(&self, lines_num: u32, _f: F)
num_lines :)
> where
> F: for<'a> FnCall<(&'a Self::Target, u32, u32)>,
> {
> unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
> opaque: *mut c_void,
> lines_num: c_int,
"line" instead of lines_num.
> unsafe {
> qdev_init_gpio_in(
> self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
I think you can use self.as_mut_ptr::<DeviceState>() or something like that.
> assert!(pins.len() > 0);
!pins.is_empty(). But I am not sure it's needed...
>
> unsafe {
> qdev_init_gpio_out(
> self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
> pins[0].as_ptr(),
> pins.len() as c_int,
... if you use instead pins.as_ptr() without the initial dereference.
> impl HPETState {
> ...
>
> fn handle_legacy_irq(&self, irq: u32, level: u32) {
> if irq == HPET_LEGACY_PIT_INT {
> if !self.is_legacy_mode() {
> self.irqs[0].set(level != 0);
> }
> } else {
> self.rtc_irq_level.set(level as u8);
Any reason why you defined rtc_irq_level as InterruptSource<u8>
instead of InterruptSource<u32>?
> fn realize(&self) {
> ...
> self.init_gpio_in(2, HPETState::handle_legacy_irq);
> self.init_gpio_out(from_ref(&self.pit_enabled));
> }
> }
>
> ---
>
> I made the handler accept the inmuttable reference, but init_gpio_in()
> is called in realize(), which now accepts the `&mut self`.
>
> I think init_gpio_in() should be called in realize() so that realize()
> needs to become safe in advance (before your plan).
>
> The safe realize() mainly affects pl011, and before you formally
> introduce char binding, if you don't mind, I can make this change to
> pl011:
>
> - pub fn realize(&mut self) {
> + pub fn realize(&self) {
> // SAFETY: self.char_backend has the correct size and alignment for a
> // CharBackend object, and its callbacks are of the correct types.
> unsafe {
> qemu_chr_fe_set_handlers(
> - addr_of_mut!(self.char_backend),
> + addr_of!(self.char_backend) as *mut CharBackend,
> Some(pl011_can_receive),
> Some(pl011_receive),
> Some(pl011_event),
> None,
> - addr_of_mut!(*self).cast::<c_void>(),
> + addr_of!(*self).cast::<c_void>() as *mut c_void,
> core::ptr::null_mut(),
> true,
> );
That's fine, yes.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2025-01-17 9:40 ` Paolo Bonzini
@ 2025-01-17 11:14 ` Zhao Liu
2025-01-17 12:47 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2025-01-17 11:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
> > unsafe {
> > qdev_init_gpio_in(
> > self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
>
> I think you can use self.as_mut_ptr::<DeviceState>() or something like that.
Yes, thank you!
>
> > assert!(pins.len() > 0);
>
> !pins.is_empty().
Yes.
> But I am not sure it's needed...
> >
> > unsafe {
> > qdev_init_gpio_out(
> > self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
> > pins[0].as_ptr(),
> > pins.len() as c_int,
>
> ... if you use instead pins.as_ptr() without the initial dereference.
Emm, pins.as_ptr() returns `*const InterruptSource`, which can't be
converted to `*mut *mut IRQState` with InterruptSource::as_ptr().
So I haven't thought of a better way yet...
> > impl HPETState {
> > ...
> >
> > fn handle_legacy_irq(&self, irq: u32, level: u32) {
> > if irq == HPET_LEGACY_PIT_INT {
> > if !self.is_legacy_mode() {
> > self.irqs[0].set(level != 0);
> > }
> > } else {
> > self.rtc_irq_level.set(level as u8);
>
> Any reason why you defined rtc_irq_level as InterruptSource<u8>
> instead of InterruptSource<u32>?
Thanks! I missed to clean up this, having previously used u8.
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
2025-01-17 11:14 ` Zhao Liu
@ 2025-01-17 12:47 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2025-01-17 12:47 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]
Il ven 17 gen 2025, 11:55 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> > > assert!(pins.len() > 0);
> >
> > !pins.is_empty().
>
> Yes.
>
> > But I am not sure it's needed...
> > >
> > > unsafe {
> > > qdev_init_gpio_out(
> > > self.upcast::<DeviceState>() as *const DeviceState as
> *mut DeviceState,
> > > pins[0].as_ptr(),
> > > pins.len() as c_int,
> >
> > ... if you use instead pins.as_ptr() without the initial dereference.
>
> Emm, pins.as_ptr() returns `*const InterruptSource`, which can't be
> converted to `*mut *mut IRQState` with InterruptSource::as_ptr().
>
It can be cast, since an InterruptSource is essentially a *mut IRQState,
but I agree it's not pretty.
Maybe add a "pub(crate) fn as_slice_of_qemu_irq(slice: [Self]) -> [*mut
IRQState)" to InterruptSource? It would just do a transmute and build the
new slice with from_raw_parts. And then "let pins =
InterruptSource::as_slice_of_qemu_irq(pins)" lets you use pins.as_ptr().
Paolo
So I haven't thought of a better way yet...
>
> > > impl HPETState {
> > > ...
> > >
> > > fn handle_legacy_irq(&self, irq: u32, level: u32) {
> > > if irq == HPET_LEGACY_PIT_INT {
> > > if !self.is_legacy_mode() {
> > > self.irqs[0].set(level != 0);
> > > }
> > > } else {
> > > self.rtc_irq_level.set(level as u8);
> >
> > Any reason why you defined rtc_irq_level as InterruptSource<u8>
> > instead of InterruptSource<u32>?
>
> Thanks! I missed to clean up this, having previously used u8.
>
> Regards,
> Zhao
>
>
[-- Attachment #2: Type: text/html, Size: 2718 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 05/13] rust: add a bit operation binding for deposit64
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (3 preceding siblings ...)
2024-12-05 6:07 ` [RFC 04/13] rust: add bindings for gpio_{in|out} initialization Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 16:09 ` Paolo Bonzini
2024-12-05 6:07 ` [RFC 06/13] rust: add bindings for memattrs Zhao Liu
` (9 subsequent siblings)
14 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
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
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;
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 05/13] rust: add a bit operation binding for deposit64
2024-12-05 6:07 ` [RFC 05/13] rust: add a bit operation binding for deposit64 Zhao Liu
@ 2024-12-05 16:09 ` Paolo Bonzini
2024-12-07 16:01 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 16:09 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
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
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 05/13] rust: add a bit operation binding for deposit64
2024-12-05 16:09 ` Paolo Bonzini
@ 2024-12-07 16:01 ` Zhao Liu
2024-12-07 19:44 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-07 16:01 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
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
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 05/13] rust: add a bit operation binding for deposit64
2024-12-07 16:01 ` Zhao Liu
@ 2024-12-07 19:44 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-07 19:44 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]
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
>
>
>
[-- Attachment #2: Type: text/html, Size: 2236 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 06/13] rust: add bindings for memattrs
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (4 preceding siblings ...)
2024-12-05 6:07 ` [RFC 05/13] rust: add a bit operation binding for deposit64 Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 18:15 ` Richard Henderson
2024-12-05 6:07 ` [RFC 07/13] rust: add bindings for timer Zhao Liu
` (8 subsequent siblings)
14 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
The MemTxAttrs structure is composed of bitfield members, and bindgen is
unable to generate an equivalent macro definition for
MEMTXATTRS_UNSPECIFIED.
Therefore, we have to manually define a global constant variable
MEMTXATTRS_UNSPECIFIED to support calls from Rust code.
However, the binding methods of MemTxAttrs are non-const, so we cannot
directly use them when defining MEMTXATTRS_UNSPECIFIED. As a result,
add the third-party crate once_cell to use its Lazy to help define
MEMTXATTRS_UNSPECIFIED.
Note, lazy_static has been deprecated and LazyCell (in std) became
stable since v1.80. When the minimum supported rustc version is bumped
to v1.80 in the future, LazyCell can be used to replace the current
once_cell.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/Cargo.lock | 7 ++++++
rust/qemu-api/Cargo.toml | 1 +
rust/qemu-api/meson.build | 9 ++++++--
rust/qemu-api/src/lib.rs | 1 +
rust/qemu-api/src/memattrs.rs | 21 +++++++++++++++++
rust/wrapper.h | 1 +
scripts/archive-source.sh | 2 +-
scripts/make-release | 2 +-
subprojects/.gitignore | 1 +
subprojects/once_cell-1.20-rs.wrap | 7 ++++++
.../once_cell-1.20-rs/meson.build | 23 +++++++++++++++++++
11 files changed, 71 insertions(+), 4 deletions(-)
create mode 100644 rust/qemu-api/src/memattrs.rs
create mode 100644 subprojects/once_cell-1.20-rs.wrap
create mode 100644 subprojects/packagefiles/once_cell-1.20-rs/meson.build
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index c0c6069247a8..6b19553b6d10 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -46,6 +46,12 @@ dependencies = [
"either",
]
+[[package]]
+name = "once_cell"
+version = "1.20.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1261fe7e33c73b354eab43b1273a57c8f967d0391e80353e51f764ac02cf6775"
+
[[package]]
name = "pl011"
version = "0.1.0"
@@ -92,6 +98,7 @@ dependencies = [
name = "qemu_api"
version = "0.1.0"
dependencies = [
+ "once_cell",
"qemu_api_macros",
"version_check",
]
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index 4aa22f319860..265e00f97176 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -14,6 +14,7 @@ keywords = []
categories = []
[dependencies]
+once_cell = { version = "1.20.2" }
qemu_api_macros = { path = "../qemu-api-macros" }
[build-dependencies]
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 00e86a679d8a..508986948883 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -10,6 +10,9 @@ if get_option('debug_mutex')
_qemu_api_cfg += ['--feature', 'debug_cell']
endif
+subproject('once_cell-1.20-rs', required: true)
+once_cell_dep = dependency('once_cell-1.20-rs')
+
_qemu_api_rs = static_library(
'qemu_api',
structured_sources(
@@ -20,6 +23,7 @@ _qemu_api_rs = static_library(
'src/cell.rs',
'src/c_str.rs',
'src/irq.rs',
+ 'src/memattrs.rs',
'src/module.rs',
'src/offset_of.rs',
'src/qdev.rs',
@@ -33,6 +37,7 @@ _qemu_api_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: _qemu_api_cfg,
+ dependencies: once_cell_dep,
)
rust.test('rust-qemu-api-tests', _qemu_api_rs,
@@ -40,7 +45,7 @@ rust.test('rust-qemu-api-tests', _qemu_api_rs,
qemu_api = declare_dependency(
link_with: _qemu_api_rs,
- dependencies: qemu_api_macros,
+ dependencies: [qemu_api_macros, once_cell_dep],
)
# Rust executables do not support objects, so add an intermediate step.
@@ -56,7 +61,7 @@ test('rust-qemu-api-integration',
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_args: ['--test'],
install: false,
- dependencies: [qemu_api, qemu_api_macros],
+ dependencies: [qemu_api, qemu_api_macros, once_cell_dep],
link_whole: [rust_qemu_api_objs, libqemuutil]),
args: [
'--test',
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 009906c907e7..e60c9ac16409 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -11,6 +11,7 @@
pub mod c_str;
pub mod cell;
pub mod irq;
+pub mod memattrs;
pub mod module;
pub mod offset_of;
pub mod qdev;
diff --git a/rust/qemu-api/src/memattrs.rs b/rust/qemu-api/src/memattrs.rs
new file mode 100644
index 000000000000..7cc8aea4b7b7
--- /dev/null
+++ b/rust/qemu-api/src/memattrs.rs
@@ -0,0 +1,21 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use once_cell::sync::Lazy;
+
+use crate::bindings::MemTxAttrs;
+
+impl MemTxAttrs {
+ fn memtxattrs_unspecified() -> Self {
+ let mut attrs = MemTxAttrs::default();
+ attrs.set_unspecified(1);
+ attrs
+ }
+}
+
+/// Bus masters which don't specify any attributes will get this,
+/// which has all attribute bits clear except the topmost one
+/// (so that we can distinguish "all attributes deliberately clear"
+/// from "didn't specify" if necessary).
+pub static MEMTXATTRS_UNSPECIFIED: Lazy<MemTxAttrs> = Lazy::new(MemTxAttrs::memtxattrs_unspecified);
diff --git a/rust/wrapper.h b/rust/wrapper.h
index 285d0eb6ad01..033f3e9cf32c 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -62,3 +62,4 @@ typedef enum memory_order {
#include "qapi/error.h"
#include "migration/vmstate.h"
#include "chardev/char-serial.h"
+#include "exec/memattrs.h"
diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
index 30677c3ec903..59e72d92498a 100755
--- a/scripts/archive-source.sh
+++ b/scripts/archive-source.sh
@@ -30,7 +30,7 @@ subprojects="keycodemapdb libvfio-user berkeley-softfloat-3
berkeley-testfloat-3 arbitrary-int-1-rs bilge-0.2-rs
bilge-impl-0.2-rs either-1-rs itertools-0.11-rs proc-macro2-1-rs
proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs
- syn-2-rs unicode-ident-1-rs"
+ syn-2-rs unicode-ident-1-rs once_cell-1.20-rs"
sub_deinit=""
function cleanup() {
diff --git a/scripts/make-release b/scripts/make-release
index 8dc939124c4f..b6b48bdf6f08 100755
--- a/scripts/make-release
+++ b/scripts/make-release
@@ -21,7 +21,7 @@ SUBPROJECTS="libvfio-user keycodemapdb berkeley-softfloat-3
berkeley-testfloat-3 arbitrary-int-1-rs bilge-0.2-rs
bilge-impl-0.2-rs either-1-rs itertools-0.11-rs proc-macro2-1-rs
proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs
- syn-2-rs unicode-ident-1-rs"
+ syn-2-rs unicode-ident-1-rs once_cell-1.20-rs"
src="$1"
version="$2"
diff --git a/subprojects/.gitignore b/subprojects/.gitignore
index 50f173f90dbe..dba8ea74b823 100644
--- a/subprojects/.gitignore
+++ b/subprojects/.gitignore
@@ -11,6 +11,7 @@
/bilge-impl-0.2.0
/either-1.12.0
/itertools-0.11.0
+/once_cell-1.20.2
/proc-macro-error-1.0.4
/proc-macro-error-attr-1.0.4
/proc-macro2-1.0.84
diff --git a/subprojects/once_cell-1.20-rs.wrap b/subprojects/once_cell-1.20-rs.wrap
new file mode 100644
index 000000000000..b13ba81a22b1
--- /dev/null
+++ b/subprojects/once_cell-1.20-rs.wrap
@@ -0,0 +1,7 @@
+[wrap-file]
+directory = once_cell-1.20.2
+source_url = https://crates.io/api/v1/crates/once_cell/1.20.2/download
+source_filename = once_cell-1.20.2.tar.gz
+source_hash = 1261fe7e33c73b354eab43b1273a57c8f967d0391e80353e51f764ac02cf6775
+#method = cargo
+patch_directory = once_cell-1.20-rs
diff --git a/subprojects/packagefiles/once_cell-1.20-rs/meson.build b/subprojects/packagefiles/once_cell-1.20-rs/meson.build
new file mode 100644
index 000000000000..0b81f8e76250
--- /dev/null
+++ b/subprojects/packagefiles/once_cell-1.20-rs/meson.build
@@ -0,0 +1,23 @@
+project('once_cell-1.20-rs', 'rust',
+ version: '1.20.2',
+ license: 'MIT OR Apache-2.0',
+ default_options: [])
+
+_once_cell_rs = static_library(
+ 'once_cell',
+ files('src/lib.rs'),
+ gnu_symbol_visibility: 'hidden',
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'rust',
+ rust_args: [
+ '--cfg', 'feature="std"'
+ ],
+ dependencies: [],
+ native: true,
+)
+
+once_cell_dep = declare_dependency(
+ link_with: _once_cell_rs,
+)
+
+meson.override_dependency('once_cell-1.20-rs', once_cell_dep, native: true)
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-05 6:07 ` [RFC 06/13] rust: add bindings for memattrs Zhao Liu
@ 2024-12-05 18:15 ` Richard Henderson
2024-12-05 18:30 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Richard Henderson @ 2024-12-05 18:15 UTC (permalink / raw)
To: Zhao Liu, Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 00:07, Zhao Liu wrote:
> The MemTxAttrs structure is composed of bitfield members, and bindgen is
> unable to generate an equivalent macro definition for
> MEMTXATTRS_UNSPECIFIED.
I'm happy to move away from bit fields to uint32_t or suchlike to enable
MEMTXATTRS_UNSPECIFIED be a compile-time constant.
r~
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-05 18:15 ` Richard Henderson
@ 2024-12-05 18:30 ` Paolo Bonzini
2024-12-05 23:51 ` Richard Henderson
2024-12-06 10:59 ` Peter Maydell
0 siblings, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 18:30 UTC (permalink / raw)
To: Richard Henderson, Zhao Liu, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 19:15, Richard Henderson wrote:
> On 12/5/24 00:07, Zhao Liu wrote:
>> The MemTxAttrs structure is composed of bitfield members, and bindgen is
>> unable to generate an equivalent macro definition for
>> MEMTXATTRS_UNSPECIFIED.
>
> I'm happy to move away from bit fields to uint32_t or suchlike to enable
> MEMTXATTRS_UNSPECIFIED be a compile-time constant.
Yeah, if we go from
typedef struct MemTxAttrs {
unsigned int unspecified:1;
unsigned int secure:1;
unsigned int space:2;
unsigned int user:1;
unsigned int memory:1;
unsigned int requester_id:16;
unsigned int pid:8;
} MemTxAttrs;
to
typedef struct MemTxAttrs {
uint8_t unspecified;
uint8_t secure;
uint8_t space;
uint8_t user;
uint8_t memory;
uint8_t pid;
uint16_t requester_id;
} MemTxAttrs;
is still decently packed and simplifies things a lot. Zhao, can you
submit that as an independent patch?
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-05 18:30 ` Paolo Bonzini
@ 2024-12-05 23:51 ` Richard Henderson
2024-12-06 8:41 ` Zhao Liu
2024-12-06 10:59 ` Peter Maydell
1 sibling, 1 reply; 72+ messages in thread
From: Richard Henderson @ 2024-12-05 23:51 UTC (permalink / raw)
To: Paolo Bonzini, Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 12:30, Paolo Bonzini wrote:
> On 12/5/24 19:15, Richard Henderson wrote:
>> On 12/5/24 00:07, Zhao Liu wrote:
>>> The MemTxAttrs structure is composed of bitfield members, and bindgen is
>>> unable to generate an equivalent macro definition for
>>> MEMTXATTRS_UNSPECIFIED.
>>
>> I'm happy to move away from bit fields to uint32_t or suchlike to enable
>> MEMTXATTRS_UNSPECIFIED be a compile-time constant.
>
> Yeah, if we go from
>
> typedef struct MemTxAttrs {
> unsigned int unspecified:1;
> unsigned int secure:1;
> unsigned int space:2;
> unsigned int user:1;
> unsigned int memory:1;
> unsigned int requester_id:16;
> unsigned int pid:8;
> } MemTxAttrs;
>
> to
>
> typedef struct MemTxAttrs {
> uint8_t unspecified;
> uint8_t secure;
> uint8_t space;
> uint8_t user;
> uint8_t memory;
> uint8_t pid;
> uint16_t requester_id;
> } MemTxAttrs;
>
> is still decently packed and simplifies things a lot. Zhao, can you submit that as an
> independent patch?
Hmm. I'd been thinking of uint32_t and hw/registerfields.h, but I have not scoped the
size of that conversion.
But short of that, please use 'bool' not 'uint8_t' for those single-bit flags.
r~
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-05 23:51 ` Richard Henderson
@ 2024-12-06 8:41 ` Zhao Liu
2024-12-06 14:07 ` Richard Henderson
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-06 8:41 UTC (permalink / raw)
To: Richard Henderson
Cc: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell, qemu-devel, qemu-rust, Zhao Liu
On Thu, Dec 05, 2024 at 05:51:42PM -0600, Richard Henderson wrote:
> Date: Thu, 5 Dec 2024 17:51:42 -0600
> From: Richard Henderson <richard.henderson@linaro.org>
> Subject: Re: [RFC 06/13] rust: add bindings for memattrs
>
> On 12/5/24 12:30, Paolo Bonzini wrote:
> > On 12/5/24 19:15, Richard Henderson wrote:
> > > On 12/5/24 00:07, Zhao Liu wrote:
> > > > The MemTxAttrs structure is composed of bitfield members, and bindgen is
> > > > unable to generate an equivalent macro definition for
> > > > MEMTXATTRS_UNSPECIFIED.
> > >
> > > I'm happy to move away from bit fields to uint32_t or suchlike to
> > > enable MEMTXATTRS_UNSPECIFIED be a compile-time constant.
> >
> > Yeah, if we go from
> >
> > typedef struct MemTxAttrs {
> > unsigned int unspecified:1;
> > unsigned int secure:1;
> > unsigned int space:2;
> > unsigned int user:1;
> > unsigned int memory:1;
> > unsigned int requester_id:16;
> > unsigned int pid:8;
> > } MemTxAttrs;
> >
> > to
> >
> > typedef struct MemTxAttrs {
> > uint8_t unspecified;
> > uint8_t secure;
> > uint8_t space;
> > uint8_t user;
> > uint8_t memory;
> > uint8_t pid;
> > uint16_t requester_id;
> > } MemTxAttrs;
> >
> > is still decently packed and simplifies things a lot. Zhao, can you
> > submit that as an independent patch?
> Hmm. I'd been thinking of uint32_t and hw/registerfields.h, but I have not
> scoped the size of that conversion.
>
> But short of that, please use 'bool' not 'uint8_t' for those single-bit flags.
>
Sure! Thank u both!
BTW, may I ask why you favor bool over uint8_t, and is it because they
are indeed just 0/1 as well? :)
Thanks,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-06 8:41 ` Zhao Liu
@ 2024-12-06 14:07 ` Richard Henderson
0 siblings, 0 replies; 72+ messages in thread
From: Richard Henderson @ 2024-12-06 14:07 UTC (permalink / raw)
To: Zhao Liu
Cc: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell, qemu-devel, qemu-rust
On 12/6/24 02:41, Zhao Liu wrote:
> BTW, may I ask why you favor bool over uint8_t, and is it because they
> are indeed just 0/1 as well? :)
Yes, exactly. :-)
r~
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-05 18:30 ` Paolo Bonzini
2024-12-05 23:51 ` Richard Henderson
@ 2024-12-06 10:59 ` Peter Maydell
2024-12-06 14:28 ` Paolo Bonzini
2024-12-07 9:21 ` Philippe Mathieu-Daudé
1 sibling, 2 replies; 72+ messages in thread
From: Peter Maydell @ 2024-12-06 10:59 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Richard Henderson, Zhao Liu, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
On Thu, 5 Dec 2024 at 18:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 12/5/24 19:15, Richard Henderson wrote:
> > On 12/5/24 00:07, Zhao Liu wrote:
> >> The MemTxAttrs structure is composed of bitfield members, and bindgen is
> >> unable to generate an equivalent macro definition for
> >> MEMTXATTRS_UNSPECIFIED.
> >
> > I'm happy to move away from bit fields to uint32_t or suchlike to enable
> > MEMTXATTRS_UNSPECIFIED be a compile-time constant.
>
> Yeah, if we go from
>
> typedef struct MemTxAttrs {
> unsigned int unspecified:1;
> unsigned int secure:1;
> unsigned int space:2;
> unsigned int user:1;
> unsigned int memory:1;
> unsigned int requester_id:16;
> unsigned int pid:8;
> } MemTxAttrs;
>
> to
>
> typedef struct MemTxAttrs {
> uint8_t unspecified;
> uint8_t secure;
> uint8_t space;
> uint8_t user;
> uint8_t memory;
> uint8_t pid;
> uint16_t requester_id;
> } MemTxAttrs;
>
> is still decently packed and simplifies things a lot.
The old struct is 4 bytes, and the new one is 8 bytes. We do
a lot of directly passing 'struct MemTxAttrs' arguments around
as arguments to functions (i.e. not passing pointers to them),
so increasing it in size is not completely free.
thanks
-- PMM
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-06 10:59 ` Peter Maydell
@ 2024-12-06 14:28 ` Paolo Bonzini
2024-12-06 14:42 ` Peter Maydell
2024-12-07 9:21 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-06 14:28 UTC (permalink / raw)
To: Peter Maydell
Cc: Richard Henderson, Zhao Liu, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 554 bytes --]
Il ven 6 dic 2024, 05:59 Peter Maydell <peter.maydell@linaro.org> ha
scritto:
> > is still decently packed and simplifies things a lot.
>
> The old struct is 4 bytes, and the new one is 8 bytes.
Yes, hence "decently" packed. But I think in both cases it's passed in
registers, and for 64-bit machine that shouldn't change anything.
Paolo
We do
> a lot of directly passing 'struct MemTxAttrs' arguments around
> as arguments to functions (i.e. not passing pointers to them),
> so increasing it in size is not completely free.
>
> thanks
> -- PMM
>
>
[-- Attachment #2: Type: text/html, Size: 1251 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-06 14:28 ` Paolo Bonzini
@ 2024-12-06 14:42 ` Peter Maydell
2024-12-06 19:13 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Peter Maydell @ 2024-12-06 14:42 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Richard Henderson, Zhao Liu, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> Il ven 6 dic 2024, 05:59 Peter Maydell <peter.maydell@linaro.org> ha scritto:
>>
>> > is still decently packed and simplifies things a lot.
>>
>> The old struct is 4 bytes, and the new one is 8 bytes.
>
>
> Yes, hence "decently" packed. But I think in both cases it's passed in registers, and for 64-bit machine that shouldn't change anything.
True. Though it does mean we go from "space to add new fields
without making it overflow from one register to two" to
"completely full and no space for expanding it".
-- PMM
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-06 14:42 ` Peter Maydell
@ 2024-12-06 19:13 ` Paolo Bonzini
2025-01-20 16:52 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-06 19:13 UTC (permalink / raw)
To: Peter Maydell
Cc: Richard Henderson, Zhao Liu, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 775 bytes --]
Il ven 6 dic 2024, 09:42 Peter Maydell <peter.maydell@linaro.org> ha
scritto:
> On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Yes, hence "decently" packed. But I think in both cases it's passed in
> registers, and for 64-bit machine that shouldn't change anything.
>
> True. Though it does mean we go from "space to add new fields
> without making it overflow from one register to two" to
> "completely full and no space for expanding it".
>
I guess it's enough to make unspecified the only non-bitfield. Then you can
declare MEMTXATTRS_UNSPECIFIED as { unspecified: true, ..Zeroable::ZERO }
("start with Zeroable::ZERO and change unspecified to true"). For the rest
it is not important to make them available in a "const".
Paolo
-- PMM
>
>
[-- Attachment #2: Type: text/html, Size: 1546 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-06 19:13 ` Paolo Bonzini
@ 2025-01-20 16:52 ` Zhao Liu
2025-01-20 17:19 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2025-01-20 16:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, Richard Henderson, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
On Fri, Dec 06, 2024 at 02:13:57PM -0500, Paolo Bonzini wrote:
> Date: Fri, 6 Dec 2024 14:13:57 -0500
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 06/13] rust: add bindings for memattrs
>
> Il ven 6 dic 2024, 09:42 Peter Maydell <peter.maydell@linaro.org> ha
> scritto:
>
> > On Fri, 6 Dec 2024 at 14:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > Yes, hence "decently" packed. But I think in both cases it's passed in
> > registers, and for 64-bit machine that shouldn't change anything.
> >
> > True. Though it does mean we go from "space to add new fields
> > without making it overflow from one register to two" to
> > "completely full and no space for expanding it".
> >
>
> I guess it's enough to make unspecified the only non-bitfield. Then you can
> declare MEMTXATTRS_UNSPECIFIED as { unspecified: true, ..Zeroable::ZERO }
> ("start with Zeroable::ZERO and change unspecified to true"). For the rest
> it is not important to make them available in a "const".
>
Sorry I missed this comment before...
Now I have a MemTxAttrs like,
typedef struct MemTxAttrs {
unsigned int secure:1;
unsigned int space:2;
unsigned int user:1;
unsigned int memory:1;
unsigned int requester_id:16;
unsigned int pid:8;
bool unspecified;
uint8_t _reserved1;
uint16_t _reserved2;
} MemTxAttrs;
and its binding is,
#[repr(C)]
#[repr(align(4))]
#[derive(Debug, Default, Copy, Clone)]
pub struct MemTxAttrs {
pub _bitfield_align_1: [u16; 0],
pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize]>,
pub unspecified: bool,
pub _reserved1: u8,
pub _reserved2: u16,
}
unfortunately, Zeroable can't be applied to __BindgenBitfieldUnit since
event its member (`storage`) is private :-(.
But there's a solution to force (and at the same time unsafely) ZERO the
entire structure in const:
* const_zero macro: https://docs.rs/const-zero/latest/const_zero/
With const_zero, we can implement Zeroable for MemTxAttrs:
unsafe impl Zeroable for MemTxAttrs {
const ZERO: Self = unsafe {const_zero!(MemTxAttrs)};
}
pub static MEMTXATTRS_UNSPECIFIED: MemTxAttrs = MemTxAttrs {
unspecified: true,
..Zeroable::ZERO
};
So do you like this idea? If so, I can be a mover to introduce
const_zero macro.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2025-01-20 16:52 ` Zhao Liu
@ 2025-01-20 17:19 ` Paolo Bonzini
2025-01-23 15:10 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2025-01-20 17:19 UTC (permalink / raw)
To: Zhao Liu
Cc: Peter Maydell, Richard Henderson, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
On 1/20/25 17:52, Zhao Liu wrote:
> Sorry I missed this comment before...
>
> Now I have a MemTxAttrs like,
>
> typedef struct MemTxAttrs {
> unsigned int secure:1;
> unsigned int space:2;
> unsigned int user:1;
> unsigned int memory:1;
> unsigned int requester_id:16;
> unsigned int pid:8;
> bool unspecified;
> uint8_t _reserved1;
> uint16_t _reserved2;
> } MemTxAttrs;
>
> and its binding is,
>
> #[repr(C)]
> #[repr(align(4))]
> #[derive(Debug, Default, Copy, Clone)]
> pub struct MemTxAttrs {
> pub _bitfield_align_1: [u16; 0],
> pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize]>,
> pub unspecified: bool,
> pub _reserved1: u8,
> pub _reserved2: u16,
> }
>
> unfortunately, Zeroable can't be applied to __BindgenBitfieldUnit since
> event its member (`storage`) is private :-(.
>
> But there's a solution to force (and at the same time unsafely) ZERO the
> entire structure in const:
>
> * const_zero macro: https://docs.rs/const-zero/latest/const_zero/
>
> With const_zero, we can implement Zeroable for MemTxAttrs:
>
> unsafe impl Zeroable for MemTxAttrs {
> const ZERO: Self = unsafe {const_zero!(MemTxAttrs)};
> }
Another solution would be to implement Zeroable for
__BindgenBitfieldUnit in bindings.rs, but this is much nicer! It works
even with old Rust versions and, even though it needs manual
implementation of the trait each type, it doesn't require enumerating
the fields one by one. So it's better than the current version of
Zeroable and, if you wish, you can also replace existing implementations
of Zeroable with const_zero.
I wouldn't bother adding a subproject; just include the macro in
zeroable.rs, with attribution and MIT license, and document that it
might go away once we require a newer rustc.
Thanks very much!
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2025-01-20 17:19 ` Paolo Bonzini
@ 2025-01-23 15:10 ` Zhao Liu
2025-01-23 15:33 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2025-01-23 15:10 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, Richard Henderson, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
> Another solution would be to implement Zeroable for __BindgenBitfieldUnit in
> bindings.rs, but this is much nicer! It works even with old Rust versions
> and, even though it needs manual implementation of the trait each type, it
> doesn't require enumerating the fields one by one. So it's better than the
> current version of Zeroable and, if you wish, you can also replace existing
> implementations of Zeroable with const_zero.
I'm working on this, and it's just a simple patch.
But I'm not sure why Zeroable needs a Default constraint. I think Sized
seems to be enough, doesn't it?
Thanks,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 06/13] rust: add bindings for memattrs
2025-01-23 15:10 ` Zhao Liu
@ 2025-01-23 15:33 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2025-01-23 15:33 UTC (permalink / raw)
To: Zhao Liu
Cc: Peter Maydell, Richard Henderson, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, qemu-devel, qemu-rust
On 1/23/25 16:10, Zhao Liu wrote:
>> Another solution would be to implement Zeroable for __BindgenBitfieldUnit in
>> bindings.rs, but this is much nicer! It works even with old Rust versions
>> and, even though it needs manual implementation of the trait each type, it
>> doesn't require enumerating the fields one by one. So it's better than the
>> current version of Zeroable and, if you wish, you can also replace existing
>> implementations of Zeroable with const_zero.
>
> I'm working on this, and it's just a simple patch.
>
> But I'm not sure why Zeroable needs a Default constraint. I think Sized
> seems to be enough, doesn't it?
It's not needed but I think it makes sense semantically:
1) the idea was that Default::default() would always return the same
value as Zeroable::ZERO, apart possibly from padding. That is, for
something that's Zeroable it should always make sense to use an all-zero
value for Default::default(). And since there are standard library
methods that use Default, the bound makes it possible to use them and
know that they fill-with-zeroes.
2) I wasn't sure in which cases bindgen doesn't generate Default, if
any; and with Default as a supertrait we'd have to double-check the
first occurrence of a non-Default but zeroable struct.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-06 10:59 ` Peter Maydell
2024-12-06 14:28 ` Paolo Bonzini
@ 2024-12-07 9:21 ` Philippe Mathieu-Daudé
2024-12-08 9:30 ` Paolo Bonzini
1 sibling, 1 reply; 72+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-12-07 9:21 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini
Cc: Richard Henderson, Zhao Liu, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée, qemu-devel,
qemu-rust
On 6/12/24 11:59, Peter Maydell wrote:
> On Thu, 5 Dec 2024 at 18:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 12/5/24 19:15, Richard Henderson wrote:
>>> On 12/5/24 00:07, Zhao Liu wrote:
>>>> The MemTxAttrs structure is composed of bitfield members, and bindgen is
>>>> unable to generate an equivalent macro definition for
>>>> MEMTXATTRS_UNSPECIFIED.
>>>
>>> I'm happy to move away from bit fields to uint32_t or suchlike to enable
>>> MEMTXATTRS_UNSPECIFIED be a compile-time constant.
>>
>> Yeah, if we go from
>>
>> typedef struct MemTxAttrs {
>> unsigned int unspecified:1;
>> unsigned int secure:1;
>> unsigned int space:2;
>> unsigned int user:1;
>> unsigned int memory:1;
>> unsigned int requester_id:16;
>> unsigned int pid:8;
>> } MemTxAttrs;
>>
>> to
>>
>> typedef struct MemTxAttrs {
>> uint8_t unspecified;
>> uint8_t secure;
>> uint8_t space;
>> uint8_t user;
>> uint8_t memory;
>> uint8_t pid;
>> uint16_t requester_id;
>> } MemTxAttrs;
>>
>> is still decently packed and simplifies things a lot.
>
> The old struct is 4 bytes, and the new one is 8 bytes. We do
> a lot of directly passing 'struct MemTxAttrs' arguments around
> as arguments to functions (i.e. not passing pointers to them),
> so increasing it in size is not completely free.
Should we add a check to not pass 8 bytes?
QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) != sizeof(uint64_t));
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-07 9:21 ` Philippe Mathieu-Daudé
@ 2024-12-08 9:30 ` Paolo Bonzini
2024-12-08 15:51 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-08 9:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Richard Henderson, Zhao Liu, Michael S . Tsirkin,
Manos Pitsidianakis, Junjie Mao, Alex Bennée, qemu-devel,
qemu-rust
[-- Attachment #1: Type: text/plain, Size: 559 bytes --]
Il sab 7 dic 2024, 10:21 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:
> >> is still decently packed and simplifies things a lot.
> >
> > The old struct is 4 bytes, and the new one is 8 bytes. We do
> > a lot of directly passing 'struct MemTxAttrs' arguments around
> > as arguments to functions (i.e. not passing pointers to them),
> > so increasing it in size is not completely free.
>
> Should we add a check to not pass 8 bytes?
>
> QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) != sizeof(uint64_t));
>
Yes, why not.
Paolo
>
[-- Attachment #2: Type: text/html, Size: 1202 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 06/13] rust: add bindings for memattrs
2024-12-08 9:30 ` Paolo Bonzini
@ 2024-12-08 15:51 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-08 15:51 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Philippe Mathieu-Daudé, Peter Maydell, Richard Henderson,
Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, qemu-devel, qemu-rust
On Sun, Dec 08, 2024 at 10:30:34AM +0100, Paolo Bonzini wrote:
> Date: Sun, 8 Dec 2024 10:30:34 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 06/13] rust: add bindings for memattrs
>
> Il sab 7 dic 2024, 10:21 Philippe Mathieu-Daudé <philmd@linaro.org> ha
> scritto:
>
> > >> is still decently packed and simplifies things a lot.
> > >
> > > The old struct is 4 bytes, and the new one is 8 bytes. We do
> > > a lot of directly passing 'struct MemTxAttrs' arguments around
> > > as arguments to functions (i.e. not passing pointers to them),
> > > so increasing it in size is not completely free.
> >
> > Should we add a check to not pass 8 bytes?
> >
> > QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) != sizeof(uint64_t));
> >
>
> Yes, why not.
>
Thank you all! Will also include this in the followup clean-up patches.
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 07/13] rust: add bindings for timer
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (5 preceding siblings ...)
2024-12-05 6:07 ` [RFC 06/13] rust: add bindings for memattrs Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 18:18 ` Richard Henderson
2024-12-05 18:46 ` Paolo Bonzini
2024-12-05 6:07 ` [RFC 08/13] rust/qdev: add the macro to define bit property Zhao Liu
` (7 subsequent siblings)
14 siblings, 2 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
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 bindings for timer_new() and its variants
which are `static inline` (in include/qemu/timer.h).
Manually implement bindings to help create new timers in Rust.
Additionally, wrap timer_mod(), timer_del() and
qemu_clock_get_virtual_ns() as safe functions to make timer interfaces
more Rust-idiomatic.
In addition, for timer_new() and its variants, to convert the idiomatic
Rust callback into a C-style callback QEMUTimerCB, introduce a trait
QEMUTimerImpl. For any object needs to initialize a new timer, it needs
to implement QEMUTimerImpl trait and define a handler.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/meson.build | 1 +
rust/qemu-api/src/lib.rs | 1 +
rust/qemu-api/src/timer.rs | 123 +++++++++++++++++++++++++++++++++++++
rust/wrapper.h | 1 +
4 files changed, 126 insertions(+)
create mode 100644 rust/qemu-api/src/timer.rs
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 508986948883..5bf3c3dfab67 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -29,6 +29,7 @@ _qemu_api_rs = static_library(
'src/qdev.rs',
'src/qom.rs',
'src/sysbus.rs',
+ 'src/timer.rs',
'src/vmstate.rs',
'src/zeroable.rs',
],
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index e60c9ac16409..495261976dbc 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -17,6 +17,7 @@
pub mod qdev;
pub mod qom;
pub mod sysbus;
+pub mod timer;
pub mod vmstate;
pub mod zeroable;
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
new file mode 100644
index 000000000000..4f9e8c9277c6
--- /dev/null
+++ b/rust/qemu-api/src/timer.rs
@@ -0,0 +1,123 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use std::{
+ borrow::BorrowMut,
+ boxed::Box,
+ os::raw::{c_int, c_void},
+ ptr::NonNull,
+};
+
+pub use bindings::QEMUTimer;
+
+use crate::bindings::{self, *};
+
+impl QEMUTimer {
+ fn new() -> Self {
+ QEMUTimer {
+ expire_time: 0,
+ timer_list: ::core::ptr::null_mut(),
+ cb: None,
+ opaque: ::core::ptr::null_mut(),
+ next: ::core::ptr::null_mut(),
+ attributes: 0,
+ scale: 0,
+ }
+ }
+
+ // TODO: Consider how to avoid passing in C style callbacks directly.
+ fn timer_new_full<T: QEMUTimerImpl>(
+ timer_list_group: Option<&mut QEMUTimerListGroup>,
+ clk_type: QEMUClockType,
+ scale: u32,
+ attributes: u32,
+ opaque: &mut T::Opaque,
+ ) -> Self {
+ let mut ts: Box<QEMUTimer> = Box::new(QEMUTimer::new());
+ let group_ptr = if let Some(g) = timer_list_group {
+ g
+ } else {
+ ::core::ptr::null_mut()
+ };
+
+ // Safety:
+ // ts is a valid Box object which can borrow a valid mutable
+ // pointer, and opaque is converted from a reference so it's
+ // also valid.
+ unsafe {
+ timer_init_full(
+ ts.borrow_mut(),
+ group_ptr,
+ clk_type,
+ scale as c_int,
+ attributes as c_int,
+ Some(rust_timer_handler::<T>),
+ (opaque as *mut T::Opaque).cast::<c_void>(),
+ )
+ };
+
+ *ts
+ }
+
+ pub fn timer_mod(&mut self, expire_time: u64) {
+ unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
+ }
+
+ pub fn timer_del(&mut self) {
+ unsafe { timer_del(self as *mut QEMUTimer) };
+ }
+}
+
+/// timer expiration callback
+unsafe extern "C" fn rust_timer_handler<T: QEMUTimerImpl>(opaque: *mut c_void) {
+ // SAFETY:
+ // the pointer is convertible to a reference
+ let para = unsafe { NonNull::new(opaque.cast::<T::Opaque>()).unwrap().as_mut() };
+
+ T::QEMU_TIMER_CB.unwrap()(para);
+}
+
+pub trait QEMUTimerImpl {
+ type Opaque;
+
+ // To be more general, opaque is mutable here. But it still should
+ // be protected by BqlCell/BqlRefCell.
+ //
+ // FIXME: limit opaque to immutable?
+ const QEMU_TIMER_CB: Option<fn(opaque: &mut Self::Opaque)> = None;
+
+ fn timer_new(clk_type: QEMUClockType, scale: u32, opaque: &mut Self::Opaque) -> QEMUTimer
+ where
+ Self: Sized,
+ {
+ QEMUTimer::timer_new_full::<Self>(None, clk_type, scale, 0, opaque)
+ }
+
+ fn timer_new_ns(clk_type: QEMUClockType, opaque: &mut Self::Opaque) -> QEMUTimer
+ where
+ Self: Sized,
+ {
+ Self::timer_new(clk_type, SCALE_NS, opaque)
+ }
+
+ fn timer_new_us(clk_type: QEMUClockType, opaque: &mut Self::Opaque) -> QEMUTimer
+ where
+ Self: Sized,
+ {
+ Self::timer_new(clk_type, SCALE_US, opaque)
+ }
+
+ fn timer_new_ms(clk_type: QEMUClockType, opaque: &mut Self::Opaque) -> QEMUTimer
+ where
+ Self: Sized,
+ {
+ Self::timer_new(clk_type, SCALE_MS, opaque)
+ }
+}
+
+pub fn qemu_clock_get_virtual_ns() -> u64 {
+ // SAFETY:
+ // Valid parameter.
+ (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
+}
diff --git a/rust/wrapper.h b/rust/wrapper.h
index 033f3e9cf32c..0da42e84933a 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -63,3 +63,4 @@ typedef enum memory_order {
#include "migration/vmstate.h"
#include "chardev/char-serial.h"
#include "exec/memattrs.h"
+#include "qemu/timer.h"
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 07/13] rust: add bindings for timer
2024-12-05 6:07 ` [RFC 07/13] rust: add bindings for timer Zhao Liu
@ 2024-12-05 18:18 ` Richard Henderson
2024-12-07 17:18 ` Zhao Liu
2024-12-05 18:46 ` Paolo Bonzini
1 sibling, 1 reply; 72+ messages in thread
From: Richard Henderson @ 2024-12-05 18:18 UTC (permalink / raw)
To: Zhao Liu, Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 00:07, Zhao Liu wrote:
> 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 bindings for timer_new() and its variants
> which are `static inline` (in include/qemu/timer.h).
>
> Manually implement bindings to help create new timers in Rust.
> Additionally, wrap timer_mod(), timer_del() and
> qemu_clock_get_virtual_ns() as safe functions to make timer interfaces
> more Rust-idiomatic.
>
> In addition, for timer_new() and its variants, to convert the idiomatic
> Rust callback into a C-style callback QEMUTimerCB, introduce a trait
> QEMUTimerImpl. For any object needs to initialize a new timer, it needs
> to implement QEMUTimerImpl trait and define a handler.
It might be cleaner to move all of those static inline into timer.c.
I don't see anything performance sensitive there...
r~
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 07/13] rust: add bindings for timer
2024-12-05 18:18 ` Richard Henderson
@ 2024-12-07 17:18 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-07 17:18 UTC (permalink / raw)
To: Richard Henderson
Cc: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell, qemu-devel, qemu-rust
> It might be cleaner to move all of those static inline into timer.c.
> I don't see anything performance sensitive there...
I agree, thanks! I'll submit a patch to clean up this.
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 07/13] rust: add bindings for timer
2024-12-05 6:07 ` [RFC 07/13] rust: add bindings for timer Zhao Liu
2024-12-05 18:18 ` Richard Henderson
@ 2024-12-05 18:46 ` Paolo Bonzini
2024-12-07 16:54 ` Zhao Liu
2025-01-14 15:36 ` Zhao Liu
1 sibling, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 18:46 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> +impl QEMUTimer {
> + fn new() -> Self {
> + QEMUTimer {
> + expire_time: 0,
> + timer_list: ::core::ptr::null_mut(),
> + cb: None,
> + opaque: ::core::ptr::null_mut(),
> + next: ::core::ptr::null_mut(),
> + attributes: 0,
> + scale: 0,
> + }
> + }
Please implement Zeroable instead and make this just Self::ZERO.
> + // TODO: Consider how to avoid passing in C style callbacks directly.
> + fn timer_new_full<T: QEMUTimerImpl>(
> + timer_list_group: Option<&mut QEMUTimerListGroup>,
> + clk_type: QEMUClockType,
> + scale: u32,
> + attributes: u32,
> + opaque: &mut T::Opaque,
This gets tricky when you have more than one timer per device. With the right
infrastructure we can make this something like
fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>(
&'a mut self,
timer_list_group: Option<&mut QEMUTimerListGroup>,
clk_type: QEMUClockType,
scale: u32,
attributes: u32,
f: &F,
opaque: &'b T) {
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(...)
}
}
So I guess that's another thing I have to extract and post. :)
BTW don't bother with timer_new, only do the non-pointer timer_init.
> + pub fn timer_del(&mut self) {
> + unsafe { timer_del(self as *mut QEMUTimer) };
> + }
> +}
Please also add a Drop implementation that calls timer_del.
> +pub fn qemu_clock_get_virtual_ns() -> u64 {
> + // SAFETY:
> + // Valid parameter.
> + (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
> +}
Maybe something like
pub struct Clock {
id: QEMUClockType
}
impl Clock {
pub fn get_ns() -> u64 {
// SAFETY: cannot be created outside this module, therefore id
// is valid
(unsafe { qemu_clock_get_ns(self.id) }) as u64
}
}
pub static CLOCK_VIRTUAL: Clock = Clock { id: QEMUClockType::QEMU_CLOCK_VIRTUAL };
// etc.
and then the user can do timer::CLOCK_VIRTUAL::get_ns().
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 07/13] rust: add bindings for timer
2024-12-05 18:46 ` Paolo Bonzini
@ 2024-12-07 16:54 ` Zhao Liu
2025-01-14 15:36 ` Zhao Liu
1 sibling, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-07 16:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Thu, Dec 05, 2024 at 07:46:52PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 19:46:52 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 07/13] rust: add bindings for timer
>
> On 12/5/24 07:07, Zhao Liu wrote:
> > +impl QEMUTimer {
> > + fn new() -> Self {
> > + QEMUTimer {
> > + expire_time: 0,
> > + timer_list: ::core::ptr::null_mut(),
> > + cb: None,
> > + opaque: ::core::ptr::null_mut(),
> > + next: ::core::ptr::null_mut(),
> > + attributes: 0,
> > + scale: 0,
> > + }
> > + }
>
> Please implement Zeroable instead and make this just Self::ZERO.
Sure, will do.
> > + // TODO: Consider how to avoid passing in C style callbacks directly.
> > + fn timer_new_full<T: QEMUTimerImpl>(
> > + timer_list_group: Option<&mut QEMUTimerListGroup>,
> > + clk_type: QEMUClockType,
> > + scale: u32,
> > + attributes: u32,
> > + opaque: &mut T::Opaque,
>
> This gets tricky when you have more than one timer per device. With the right
> infrastructure we can make this something like
>
> fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>(
> &'a mut self,
> timer_list_group: Option<&mut QEMUTimerListGroup>,
> clk_type: QEMUClockType,
> scale: u32,
> attributes: u32,
> f: &F,
> opaque: &'b T) {
> // SAFETY: the opaque outlives the timer
> unsafe {
> timer_init_full(...)
> }
> }
>
> So I guess that's another thing I have to extract and post. :)
Thank you so much for your help! I'm happy to help test and review.
(if you feel you don't have enough time, I'm also willing to try this
way.. and if there's any issue, I'll let you know...)
whichever way you think is solid, I'm both happy with it.
> BTW don't bother with timer_new, only do the non-pointer timer_init.
Do you mean timer_init is enough, and timer_new and its variants are not
necessary?
> > + pub fn timer_del(&mut self) {
> > + unsafe { timer_del(self as *mut QEMUTimer) };
> > + }
> > +}
>
> Please also add a Drop implementation that calls timer_del.
Sure!
> > +pub fn qemu_clock_get_virtual_ns() -> u64 {
> > + // SAFETY:
> > + // Valid parameter.
> > + (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
> > +}
>
> Maybe something like
>
> pub struct Clock {
> id: QEMUClockType
> }
>
> impl Clock {
> pub fn get_ns() -> u64 {
> // SAFETY: cannot be created outside this module, therefore id
> // is valid
> (unsafe { qemu_clock_get_ns(self.id) }) as u64
> }
> }
>
> pub static CLOCK_VIRTUAL: Clock = Clock { id: QEMUClockType::QEMU_CLOCK_VIRTUAL };
> // etc.
>
> and then the user can do timer::CLOCK_VIRTUAL::get_ns().
More idiomatic, thank you! (I feel like I need to switch my mindset even
more from current C style.)
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 07/13] rust: add bindings for timer
2024-12-05 18:46 ` Paolo Bonzini
2024-12-07 16:54 ` Zhao Liu
@ 2025-01-14 15:36 ` Zhao Liu
2025-01-14 15:42 ` Zhao Liu
2025-01-14 16:14 ` Paolo Bonzini
1 sibling, 2 replies; 72+ messages in thread
From: Zhao Liu @ 2025-01-14 15:36 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
Hi Paolo,
Thanks for your FnCall and the guidance below...
> This gets tricky when you have more than one timer per device. With the right
> infrastructure we can make this something like
>
> fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>(
> &'a mut self,
> timer_list_group: Option<&mut QEMUTimerListGroup>,
> clk_type: QEMUClockType,
> scale: u32,
> attributes: u32,
> f: &F,
> opaque: &'b T) {
> // SAFETY: the opaque outlives the timer
> unsafe {
> timer_init_full(...)
> }
> }
...
> pub struct Clock {
> id: QEMUClockType
> }
>
> impl Clock {
> pub fn get_ns() -> u64 {
> // SAFETY: cannot be created outside this module, therefore id
> // is valid
> (unsafe { qemu_clock_get_ns(self.id) }) as u64
> }
> }
>
> pub static CLOCK_VIRTUAL: Clock = Clock { id: QEMUClockType::QEMU_CLOCK_VIRTUAL };
> // etc.
>
> and then the user can do timer::CLOCK_VIRTUAL::get_ns().
>
...Now I have a draft for timer binding:
* timer binding:
impl QEMUTimer {
pub fn new() -> Self {
Zeroable::ZERO
}
pub fn timer_init_full<'a, 'b: 'a, T, F>(
&'a mut self,
timer_list_group: Option<&mut QEMUTimerListGroup>,
clk_type: QEMUClockType,
scale: u32,
attributes: u32,
_f: &F,
opaque: &'b T,
) where
F: for<'c> FnCall<(&'c T,)> + 'b,
{
let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::<T, F>;
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(
self,
if let Some(g) = timer_list_group {
g
} else {
::core::ptr::null_mut()
},
clk_type,
scale as c_int,
attributes as c_int,
Some(timer_cb),
opaque as *const T as *const c_void as *mut c_void,
)
}
}
pub fn timer_mod(&mut self, expire_time: u64) {
unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
}
}
* use of timer binding:
fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
timer_cell.borrow_mut().callback()
}
impl HPETTimer {
...
fn init_timer_with_state(&mut self) {
let index = self.index;
let cb = |cell: &BqlRefCell<HPETTimer>| {
timer_handler(cell);
};
self.qemu_timer = Some(Box::new({
let mut t = QEMUTimer::new();
t.timer_init_full(
None,
CLOCK_VIRTUAL.id,
SCALE_NS,
0,
&cb,
&self.get_state().timer[index],
);
t
}));
}
...
}
---
Is this timer binding as you expected? Hope I am on the right track. :-)
Thanks,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 07/13] rust: add bindings for timer
2025-01-14 15:36 ` Zhao Liu
@ 2025-01-14 15:42 ` Zhao Liu
2025-01-14 16:14 ` Paolo Bonzini
1 sibling, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2025-01-14 15:42 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Benn�e, Philippe Mathieu-Daud�, Peter Maydell,
qemu-devel, qemu-rust
On Tue, Jan 14, 2025 at 11:36:48PM +0800, Zhao Liu wrote:
> Date: Tue, 14 Jan 2025 23:36:48 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: Re: [RFC 07/13] rust: add bindings for timer
>
> Hi Paolo,
>
> Thanks for your FnCall and the guidance below...
>
> > This gets tricky when you have more than one timer per device. With the right
> > infrastructure we can make this something like
> >
> > fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>(
> > &'a mut self,
> > timer_list_group: Option<&mut QEMUTimerListGroup>,
> > clk_type: QEMUClockType,
> > scale: u32,
> > attributes: u32,
> > f: &F,
> > opaque: &'b T) {
> > // SAFETY: the opaque outlives the timer
> > unsafe {
> > timer_init_full(...)
> > }
> > }
>
> ...
>
> > pub struct Clock {
> > id: QEMUClockType
> > }
> >
> > impl Clock {
> > pub fn get_ns() -> u64 {
> > // SAFETY: cannot be created outside this module, therefore id
> > // is valid
> > (unsafe { qemu_clock_get_ns(self.id) }) as u64
> > }
> > }
> >
> > pub static CLOCK_VIRTUAL: Clock = Clock { id: QEMUClockType::QEMU_CLOCK_VIRTUAL };
> > // etc.
> >
> > and then the user can do timer::CLOCK_VIRTUAL::get_ns().
> >
>
> ...Now I have a draft for timer binding:
>
> * timer binding:
Missed rust_timer_handler, which follows the good example of FnCall doc:
unsafe extern "C" fn rust_timer_handler<T, F: for<'a> FnCall<(&'a T,)>>(opaque: *mut c_void) {
// SAFETY: the opaque was passed as a reference to `T`.
F::call((unsafe { &*(opaque.cast::<T>()) },))
}
> impl QEMUTimer {
> pub fn new() -> Self {
> Zeroable::ZERO
> }
>
> pub fn timer_init_full<'a, 'b: 'a, T, F>(
> &'a mut self,
> timer_list_group: Option<&mut QEMUTimerListGroup>,
> clk_type: QEMUClockType,
> scale: u32,
> attributes: u32,
> _f: &F,
> opaque: &'b T,
> ) where
> F: for<'c> FnCall<(&'c T,)> + 'b,
> {
> let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::<T, F>;
>
> // SAFETY: the opaque outlives the timer
> unsafe {
> timer_init_full(
> self,
> if let Some(g) = timer_list_group {
> g
> } else {
> ::core::ptr::null_mut()
> },
> clk_type,
> scale as c_int,
> attributes as c_int,
> Some(timer_cb),
> opaque as *const T as *const c_void as *mut c_void,
> )
> }
> }
>
> pub fn timer_mod(&mut self, expire_time: u64) {
> unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
> }
> }
>
>
> * use of timer binding:
>
> fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
> timer_cell.borrow_mut().callback()
> }
>
> impl HPETTimer {
> ...
>
> fn init_timer_with_state(&mut self) {
> let index = self.index;
> let cb = |cell: &BqlRefCell<HPETTimer>| {
> timer_handler(cell);
> };
>
> self.qemu_timer = Some(Box::new({
> let mut t = QEMUTimer::new();
> t.timer_init_full(
> None,
> CLOCK_VIRTUAL.id,
> SCALE_NS,
> 0,
> &cb,
> &self.get_state().timer[index],
> );
> t
> }));
> }
> ...
> }
>
> ---
>
> Is this timer binding as you expected? Hope I am on the right track. :-)
>
> Thanks,
> Zhao
>
>
>
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 07/13] rust: add bindings for timer
2025-01-14 15:36 ` Zhao Liu
2025-01-14 15:42 ` Zhao Liu
@ 2025-01-14 16:14 ` Paolo Bonzini
2025-01-15 7:09 ` Zhao Liu
1 sibling, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2025-01-14 16:14 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Tue, Jan 14, 2025 at 4:18 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> ...Now I have a draft for timer binding:
>
> * timer binding:
>
> impl QEMUTimer {
> pub fn new() -> Self {
> Zeroable::ZERO
> }
Maybe Default too (not sure if you even need new())?
> pub fn timer_init_full<'a, 'b: 'a, T, F>(
It's better to use longer names like 'timer and 'opaque. But it's also
always possible to pass a longer lifetime, so 'opaque: 'timer is
strictly speaking not needed: you can just use &'timer T which in turn
means that lifetime elision applies. That said, I think I like the
idea of using 'timer and 'opaque lifetimes here, for clarity.
> &'a mut self,
> timer_list_group: Option<&mut QEMUTimerListGroup>,
I think QEMUTimerListGroup can (should) be shared because it's thread safe.
> clk_type: QEMUClockType,
> scale: u32,
> attributes: u32,
> _f: &F,
Better: "_f: &'static F", or even "_f: F" if it works.
> opaque: &'b T,
> ) where
> F: for<'c> FnCall<(&'c T,)> + 'b,
'b ('opaque) is not needed here because the opaque is passed _into_
the function (thus its lifetime is 'c). 'timer would make sense, but
in fact the function itself is always 'static (see FnCall declaration)
so it is unnecessary to add a lifetime to FnCall.
> fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
> timer_cell.borrow_mut().callback()
> }
>
> impl HPETTimer {
> ...
>
> fn init_timer_with_state(&mut self) {
> let index = self.index;
> let cb = |cell: &BqlRefCell<HPETTimer>| {
> timer_handler(cell);
> };
Why is the anonymous function needed? Can you just pass "timer_handler"?
> Is this timer binding as you expected? Hope I am on the right track. :-)
If the only correction is to the function declaration, that's as good
as it can be for Rust! ;)
Thanks,
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 07/13] rust: add bindings for timer
2025-01-14 16:14 ` Paolo Bonzini
@ 2025-01-15 7:09 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2025-01-15 7:09 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Tue, Jan 14, 2025 at 05:14:48PM +0100, Paolo Bonzini wrote:
> Date: Tue, 14 Jan 2025 17:14:48 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 07/13] rust: add bindings for timer
>
> On Tue, Jan 14, 2025 at 4:18 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > ...Now I have a draft for timer binding:
> >
> > * timer binding:
> >
> > impl QEMUTimer {
> > pub fn new() -> Self {
> > Zeroable::ZERO
> > }
>
> Maybe Default too (not sure if you even need new())?
Yes, I find bindgen has already implemented Default for QEMUTimer:
#[repr(C)]
#[derive(Debug)]
pub struct QEMUTimer {
pub expire_time: i64,
pub timer_list: *mut QEMUTimerList,
pub cb: QEMUTimerCB,
pub opaque: *mut std::os::raw::c_void,
pub next: *mut QEMUTimer,
pub attributes: std::os::raw::c_int,
pub scale: std::os::raw::c_int,
}
impl Default for QEMUTimer {
fn default() -> Self {
let mut s = ::std::mem::MaybeUninit::<Self>::uninit();
unsafe {
::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1);
s.assume_init()
}
}
}
HPETTimer just has a pointer to a QEMUTimer, so I need the new() in
init_timer_with_state() to create QEMUTimer within a Box<>. I'll use
Default instead in new().
> > pub fn timer_init_full<'a, 'b: 'a, T, F>(
>
> It's better to use longer names like 'timer and 'opaque. But it's also
> always possible to pass a longer lifetime, so 'opaque: 'timer is
> strictly speaking not needed: you can just use &'timer T which in turn
> means that lifetime elision applies. That said, I think I like the
> idea of using 'timer and 'opaque lifetimes here, for clarity.
Thanks, I'll change the lifetime names.
> > &'a mut self,
> > timer_list_group: Option<&mut QEMUTimerListGroup>,
>
> I think QEMUTimerListGroup can (should) be shared because it's thread safe.
I understand here I can pass the immutable reference like (and that's
the updated timer_init_full()):
pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>(
&'timer mut self,
timer_list_group: Option<&QEMUTimerListGroup>,
clk_type: QEMUClockType,
scale: u32,
attributes: u32,
_f: F,
opaque: &'opaque T,
) where
F: for<'a> FnCall<(&'a T,)>,
{
let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::<T, F>;
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(
self,
if let Some(g) = timer_list_group {
g as *const QEMUTimerListGroup as *mut QEMUTimerListGroup
} else {
::core::ptr::null_mut()
},
clk_type,
scale as c_int,
attributes as c_int,
Some(timer_cb),
opaque as *const T as *const c_void as *mut c_void,
)
}
}
> > clk_type: QEMUClockType,
> > scale: u32,
> > attributes: u32,
> > _f: &F,
>
> Better: "_f: &'static F", or even "_f: F" if it works.
"_f: F" can work since I passed a function reference (&cb, not a
function pointer).
With "_f: F", passing timer_handler directly is better.
> > opaque: &'b T,
> > ) where
> > F: for<'c> FnCall<(&'c T,)> + 'b,
>
> 'b ('opaque) is not needed here because the opaque is passed _into_
> the function (thus its lifetime is 'c). 'timer would make sense, but
> in fact the function itself is always 'static (see FnCall declaration)
> so it is unnecessary to add a lifetime to FnCall.
I see! Thank you for clarification.
> > fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
> > timer_cell.borrow_mut().callback()
> > }
> >
> > impl HPETTimer {
> > ...
> >
> > fn init_timer_with_state(&mut self) {
> > let index = self.index;
> > let cb = |cell: &BqlRefCell<HPETTimer>| {
> > timer_handler(cell);
> > };
>
> Why is the anonymous function needed? Can you just pass "timer_handler"?
Yes, I should clean up this closure...
> > Is this timer binding as you expected? Hope I am on the right track. :-)
>
> If the only correction is to the function declaration, that's as good
> as it can be for Rust! ;)
>
Thank you! Now I have a updated timer_init_full() (like I pasted above).
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 08/13] rust/qdev: add the macro to define bit property
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (6 preceding siblings ...)
2024-12-05 6:07 ` [RFC 07/13] rust: add bindings for timer Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 6:07 ` [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c Zhao Liu
` (6 subsequent siblings)
14 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
HPET device (Rust device) needs to define the bit type property.
Add a variant of define_property macro to define bit type property.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/src/qdev.rs | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 5e6580b6f261..66810803ec9d 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -103,6 +103,18 @@ fn class_init(dc: &mut DeviceClass) {
#[macro_export]
macro_rules! define_property {
+ ($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr, bit = $bitnr:expr, default = $defval:expr$(,)*) => {
+ $crate::bindings::Property {
+ // use associated function syntax for type checking
+ name: ::std::ffi::CStr::as_ptr($name),
+ info: $prop,
+ offset: $crate::offset_of!($state, $field) as isize,
+ bitnr: $bitnr,
+ set_default: true,
+ defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval as u64 },
+ ..$crate::zeroable::Zeroable::ZERO
+ }
+ };
($name:expr, $state:ty, $field:ident, $prop:expr, $type:expr, default = $defval:expr$(,)*) => {
$crate::bindings::Property {
// use associated function syntax for type checking
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (7 preceding siblings ...)
2024-12-05 6:07 ` [RFC 08/13] rust/qdev: add the macro to define bit property Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 12:04 ` Philippe Mathieu-Daudé
2024-12-05 15:30 ` Paolo Bonzini
2024-12-05 6:07 ` [RFC 10/13] rust/timer/hpet: define hpet_fw_cfg Zhao Liu
` (5 subsequent siblings)
14 siblings, 2 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
HPET device needs to access and update hpet_cfg variable, but now it is
defined in hw/i386/fw_cfg.c and Rust code can't access it.
Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
allows Rust HPET device implements its own global hpet_fw_cfg variable,
and will further reduce the use of unsafe C code access and calls in the
Rust HPET implementation.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
hw/i386/fw_cfg.c | 4 +---
hw/timer/hpet.c | 16 +++++++++-------
include/hw/timer/hpet.h | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 0e4494627c21..965e6306838a 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -26,8 +26,6 @@
#include CONFIG_DEVICES
#include "target/i386/cpu.h"
-struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
-
const char *fw_cfg_arch_key_name(uint16_t key)
{
static const struct {
@@ -149,7 +147,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
#endif
fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
- fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
+ fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_fw_cfg, sizeof(hpet_fw_cfg));
/* allocate memory for the NUMA channel: one (64bit) word for the number
* of nodes, one word for each VCPU->node and one word for each node to
* hold the amount of memory.
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 5399f1b2a3f7..d8bd51b7e202 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -40,6 +40,8 @@
#include "qom/object.h"
#include "trace.h"
+struct hpet_fw_config hpet_fw_cfg = {.count = UINT8_MAX};
+
#define HPET_MSI_SUPPORT 0
OBJECT_DECLARE_SIMPLE_TYPE(HPETState, HPET)
@@ -278,7 +280,7 @@ static int hpet_post_load(void *opaque, int version_id)
/* Push number of timers into capability returned via HPET_ID */
s->capability &= ~HPET_ID_NUM_TIM_MASK;
s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
- hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
+ hpet_fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
/* Derive HPET_MSI_SUPPORT from the capability of the first timer. */
s->flags &= ~(1 << HPET_MSI_SUPPORT);
@@ -665,8 +667,8 @@ static void hpet_reset(DeviceState *d)
s->hpet_counter = 0ULL;
s->hpet_offset = 0ULL;
s->config = 0ULL;
- hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
- hpet_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
+ hpet_fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
+ hpet_fw_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
/* to document that the RTC lowers its output on reset as well */
s->rtc_irq_level = 0;
@@ -708,17 +710,17 @@ static void hpet_realize(DeviceState *dev, Error **errp)
if (!s->intcap) {
warn_report("Hpet's intcap not initialized");
}
- if (hpet_cfg.count == UINT8_MAX) {
+ if (hpet_fw_cfg.count == UINT8_MAX) {
/* first instance */
- hpet_cfg.count = 0;
+ hpet_fw_cfg.count = 0;
}
- if (hpet_cfg.count == 8) {
+ if (hpet_fw_cfg.count == 8) {
error_setg(errp, "Only 8 instances of HPET is allowed");
return;
}
- s->hpet_id = hpet_cfg.count++;
+ s->hpet_id = hpet_fw_cfg.count++;
for (i = 0; i < HPET_NUM_IRQ_ROUTES; i++) {
sysbus_init_irq(sbd, &s->irqs[i]);
diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
index d17a8d43199e..dbf709251a8f 100644
--- a/include/hw/timer/hpet.h
+++ b/include/hw/timer/hpet.h
@@ -74,7 +74,7 @@ struct hpet_fw_config
struct hpet_fw_entry hpet[8];
} QEMU_PACKED;
-extern struct hpet_fw_config hpet_cfg;
+extern struct hpet_fw_config hpet_fw_cfg;
#define TYPE_HPET "hpet"
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 6:07 ` [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c Zhao Liu
@ 2024-12-05 12:04 ` Philippe Mathieu-Daudé
2024-12-05 12:46 ` Zhao Liu
2024-12-05 15:30 ` Paolo Bonzini
1 sibling, 1 reply; 72+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-12-05 12:04 UTC (permalink / raw)
To: Zhao Liu, Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Peter Maydell
Cc: qemu-devel, qemu-rust
On 5/12/24 07:07, Zhao Liu wrote:
> HPET device needs to access and update hpet_cfg variable, but now it is
> defined in hw/i386/fw_cfg.c and Rust code can't access it.
>
> Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
> allows Rust HPET device implements its own global hpet_fw_cfg variable,
> and will further reduce the use of unsafe C code access and calls in the
> Rust HPET implementation.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> hw/i386/fw_cfg.c | 4 +---
> hw/timer/hpet.c | 16 +++++++++-------
> include/hw/timer/hpet.h | 2 +-
> 3 files changed, 11 insertions(+), 11 deletions(-)
> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> index d17a8d43199e..dbf709251a8f 100644
> --- a/include/hw/timer/hpet.h
> +++ b/include/hw/timer/hpet.h
> @@ -74,7 +74,7 @@ struct hpet_fw_config
> struct hpet_fw_entry hpet[8];
> } QEMU_PACKED;
>
> -extern struct hpet_fw_config hpet_cfg;
> +extern struct hpet_fw_config hpet_fw_cfg;
Could this field belong to the (yet unexisting) HPETClass?
>
> #define TYPE_HPET "hpet"
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 12:04 ` Philippe Mathieu-Daudé
@ 2024-12-05 12:46 ` Zhao Liu
2024-12-05 21:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 12:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Peter Maydell, qemu-devel,
qemu-rust
Hi Philippe,
On Thu, Dec 05, 2024 at 01:04:58PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Thu, 5 Dec 2024 13:04:58 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
>
> On 5/12/24 07:07, Zhao Liu wrote:
> > HPET device needs to access and update hpet_cfg variable, but now it is
> > defined in hw/i386/fw_cfg.c and Rust code can't access it.
> >
> > Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
> > allows Rust HPET device implements its own global hpet_fw_cfg variable,
> > and will further reduce the use of unsafe C code access and calls in the
> > Rust HPET implementation.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> > hw/i386/fw_cfg.c | 4 +---
> > hw/timer/hpet.c | 16 +++++++++-------
> > include/hw/timer/hpet.h | 2 +-
> > 3 files changed, 11 insertions(+), 11 deletions(-)
>
>
> > diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> > index d17a8d43199e..dbf709251a8f 100644
> > --- a/include/hw/timer/hpet.h
> > +++ b/include/hw/timer/hpet.h
> > @@ -74,7 +74,7 @@ struct hpet_fw_config
> > struct hpet_fw_entry hpet[8];
> > } QEMU_PACKED;
> > -extern struct hpet_fw_config hpet_cfg;
> > +extern struct hpet_fw_config hpet_fw_cfg;
>
> Could this field belong to the (yet unexisting) HPETClass?
Several instances would share the same class, so HPETClass could manage
multiple HPETState info.
But in fw_cfg.c, do you have idea about how to get the HPETClass?
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 12:46 ` Zhao Liu
@ 2024-12-05 21:17 ` Philippe Mathieu-Daudé
2024-12-05 21:19 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-12-05 21:17 UTC (permalink / raw)
To: Zhao Liu
Cc: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Peter Maydell, qemu-devel,
qemu-rust
On 5/12/24 13:46, Zhao Liu wrote:
> Hi Philippe,
>
> On Thu, Dec 05, 2024 at 01:04:58PM +0100, Philippe Mathieu-Daudé wrote:
>> Date: Thu, 5 Dec 2024 13:04:58 +0100
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
>>
>> On 5/12/24 07:07, Zhao Liu wrote:
>>> HPET device needs to access and update hpet_cfg variable, but now it is
>>> defined in hw/i386/fw_cfg.c and Rust code can't access it.
>>>
>>> Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
>>> allows Rust HPET device implements its own global hpet_fw_cfg variable,
>>> and will further reduce the use of unsafe C code access and calls in the
>>> Rust HPET implementation.
>>>
>>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>>> ---
>>> hw/i386/fw_cfg.c | 4 +---
>>> hw/timer/hpet.c | 16 +++++++++-------
>>> include/hw/timer/hpet.h | 2 +-
>>> 3 files changed, 11 insertions(+), 11 deletions(-)
>>
>>
>>> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
>>> index d17a8d43199e..dbf709251a8f 100644
>>> --- a/include/hw/timer/hpet.h
>>> +++ b/include/hw/timer/hpet.h
>>> @@ -74,7 +74,7 @@ struct hpet_fw_config
>>> struct hpet_fw_entry hpet[8];
>>> } QEMU_PACKED;
>>> -extern struct hpet_fw_config hpet_cfg;
>>> +extern struct hpet_fw_config hpet_fw_cfg;
>>
>> Could this field belong to the (yet unexisting) HPETClass?
>
> Several instances would share the same class, so HPETClass could manage
> multiple HPETState info.
>
> But in fw_cfg.c, do you have idea about how to get the HPETClass?
Have hpet_find() return an Object and call object_get_class()?
> Regards,
> Zhao
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 21:17 ` Philippe Mathieu-Daudé
@ 2024-12-05 21:19 ` Paolo Bonzini
2024-12-07 9:16 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 21:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Peter Maydell, qemu-devel, qemu-rust
On Thu, Dec 5, 2024 at 10:18 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 5/12/24 13:46, Zhao Liu wrote:
> > Hi Philippe,
> >
> > On Thu, Dec 05, 2024 at 01:04:58PM +0100, Philippe Mathieu-Daudé wrote:
> >> Date: Thu, 5 Dec 2024 13:04:58 +0100
> >> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
> >>
> >> On 5/12/24 07:07, Zhao Liu wrote:
> >>> HPET device needs to access and update hpet_cfg variable, but now it is
> >>> defined in hw/i386/fw_cfg.c and Rust code can't access it.
> >>>
> >>> Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
> >>> allows Rust HPET device implements its own global hpet_fw_cfg variable,
> >>> and will further reduce the use of unsafe C code access and calls in the
> >>> Rust HPET implementation.
> >>>
> >>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >>> ---
> >>> hw/i386/fw_cfg.c | 4 +---
> >>> hw/timer/hpet.c | 16 +++++++++-------
> >>> include/hw/timer/hpet.h | 2 +-
> >>> 3 files changed, 11 insertions(+), 11 deletions(-)
> >>
> >>
> >>> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> >>> index d17a8d43199e..dbf709251a8f 100644
> >>> --- a/include/hw/timer/hpet.h
> >>> +++ b/include/hw/timer/hpet.h
> >>> @@ -74,7 +74,7 @@ struct hpet_fw_config
> >>> struct hpet_fw_entry hpet[8];
> >>> } QEMU_PACKED;
> >>> -extern struct hpet_fw_config hpet_cfg;
> >>> +extern struct hpet_fw_config hpet_fw_cfg;
> >>
> >> Could this field belong to the (yet unexisting) HPETClass?
> >
> > Several instances would share the same class, so HPETClass could manage
> > multiple HPETState info.
> >
> > But in fw_cfg.c, do you have idea about how to get the HPETClass?
>
> Have hpet_find() return an Object and call object_get_class()?
That would work, but anyhow this patch breaks compilation without HPET
so the question is a bit moot. :)
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 21:19 ` Paolo Bonzini
@ 2024-12-07 9:16 ` Philippe Mathieu-Daudé
2024-12-07 15:36 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-12-07 9:16 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Peter Maydell, qemu-devel, qemu-rust
On 5/12/24 22:19, Paolo Bonzini wrote:
> On Thu, Dec 5, 2024 at 10:18 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 5/12/24 13:46, Zhao Liu wrote:
>>> Hi Philippe,
>>>
>>> On Thu, Dec 05, 2024 at 01:04:58PM +0100, Philippe Mathieu-Daudé wrote:
>>>> Date: Thu, 5 Dec 2024 13:04:58 +0100
>>>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
>>>>
>>>> On 5/12/24 07:07, Zhao Liu wrote:
>>>>> HPET device needs to access and update hpet_cfg variable, but now it is
>>>>> defined in hw/i386/fw_cfg.c and Rust code can't access it.
>>>>>
>>>>> Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
>>>>> allows Rust HPET device implements its own global hpet_fw_cfg variable,
>>>>> and will further reduce the use of unsafe C code access and calls in the
>>>>> Rust HPET implementation.
>>>>>
>>>>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>>>>> ---
>>>>> hw/i386/fw_cfg.c | 4 +---
>>>>> hw/timer/hpet.c | 16 +++++++++-------
>>>>> include/hw/timer/hpet.h | 2 +-
>>>>> 3 files changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>>
>>>>> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
>>>>> index d17a8d43199e..dbf709251a8f 100644
>>>>> --- a/include/hw/timer/hpet.h
>>>>> +++ b/include/hw/timer/hpet.h
>>>>> @@ -74,7 +74,7 @@ struct hpet_fw_config
>>>>> struct hpet_fw_entry hpet[8];
>>>>> } QEMU_PACKED;
>>>>> -extern struct hpet_fw_config hpet_cfg;
>>>>> +extern struct hpet_fw_config hpet_fw_cfg;
>>>>
>>>> Could this field belong to the (yet unexisting) HPETClass?
>>>
>>> Several instances would share the same class, so HPETClass could manage
>>> multiple HPETState info.
>>>
>>> But in fw_cfg.c, do you have idea about how to get the HPETClass?
>>
>> Have hpet_find() return an Object and call object_get_class()?
Implemented as
https://lore.kernel.org/qemu-devel/20241206191124.9195-1-philmd@linaro.org/,
hoping it simplifies the Rust model integration.
>
> That would work, but anyhow this patch breaks compilation without HPET
> so the question is a bit moot. :)
>
> Paolo
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-07 9:16 ` Philippe Mathieu-Daudé
@ 2024-12-07 15:36 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-07 15:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Peter Maydell, qemu-devel,
qemu-rust
On Sat, Dec 07, 2024 at 10:16:03AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Sat, 7 Dec 2024 10:16:03 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
>
> On 5/12/24 22:19, Paolo Bonzini wrote:
> > On Thu, Dec 5, 2024 at 10:18 PM Philippe Mathieu-Daudé
> > <philmd@linaro.org> wrote:
> > >
> > > On 5/12/24 13:46, Zhao Liu wrote:
> > > > Hi Philippe,
> > > >
> > > > On Thu, Dec 05, 2024 at 01:04:58PM +0100, Philippe Mathieu-Daudé wrote:
> > > > > Date: Thu, 5 Dec 2024 13:04:58 +0100
> > > > > From: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > > Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
> > > > >
> > > > > On 5/12/24 07:07, Zhao Liu wrote:
> > > > > > HPET device needs to access and update hpet_cfg variable, but now it is
> > > > > > defined in hw/i386/fw_cfg.c and Rust code can't access it.
> > > > > >
> > > > > > Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
> > > > > > allows Rust HPET device implements its own global hpet_fw_cfg variable,
> > > > > > and will further reduce the use of unsafe C code access and calls in the
> > > > > > Rust HPET implementation.
> > > > > >
> > > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > > > > ---
> > > > > > hw/i386/fw_cfg.c | 4 +---
> > > > > > hw/timer/hpet.c | 16 +++++++++-------
> > > > > > include/hw/timer/hpet.h | 2 +-
> > > > > > 3 files changed, 11 insertions(+), 11 deletions(-)
> > > > >
> > > > >
> > > > > > diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> > > > > > index d17a8d43199e..dbf709251a8f 100644
> > > > > > --- a/include/hw/timer/hpet.h
> > > > > > +++ b/include/hw/timer/hpet.h
> > > > > > @@ -74,7 +74,7 @@ struct hpet_fw_config
> > > > > > struct hpet_fw_entry hpet[8];
> > > > > > } QEMU_PACKED;
> > > > > > -extern struct hpet_fw_config hpet_cfg;
> > > > > > +extern struct hpet_fw_config hpet_fw_cfg;
> > > > >
> > > > > Could this field belong to the (yet unexisting) HPETClass?
> > > >
> > > > Several instances would share the same class, so HPETClass could manage
> > > > multiple HPETState info.
> > > >
> > > > But in fw_cfg.c, do you have idea about how to get the HPETClass?
> > >
> > > Have hpet_find() return an Object and call object_get_class()?
>
> Implemented as
> https://lore.kernel.org/qemu-devel/20241206191124.9195-1-philmd@linaro.org/,
> hoping it simplifies the Rust model integration.
>
Hi Philli, thank you very much!
This approach is very helpful for more “rich” QOM class attempts, and if
Paolo feels the same way, I'll move to your approach for the next
version!
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 6:07 ` [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c Zhao Liu
2024-12-05 12:04 ` Philippe Mathieu-Daudé
@ 2024-12-05 15:30 ` Paolo Bonzini
2024-12-07 15:28 ` Zhao Liu
2025-01-17 10:31 ` Zhao Liu
1 sibling, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 15:30 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> HPET device needs to access and update hpet_cfg variable, but now it is
> defined in hw/i386/fw_cfg.c and Rust code can't access it.
>
> Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
> allows Rust HPET device implements its own global hpet_fw_cfg variable,
> and will further reduce the use of unsafe C code access and calls in the
> Rust HPET implementation.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> hw/i386/fw_cfg.c | 4 +---
> hw/timer/hpet.c | 16 +++++++++-------
> include/hw/timer/hpet.h | 2 +-
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 0e4494627c21..965e6306838a 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -26,8 +26,6 @@
> #include CONFIG_DEVICES
> #include "target/i386/cpu.h"
>
> -struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
This breaks if you disable HPET, which is why fw_cfg.c defines it.
You can do something like
diff --git a/include/hw/timer/hpet-fw-cfg.h b/include/hw/timer/hpet-fw-cfg.h
new file mode 100644
index 00000000000..234a49fc92e
--- /dev/null
+++ b/include/hw/timer/hpet-fw-cfg.h
@@ -0,0 +1,16 @@
+struct hpet_fw_entry
+{
+ uint32_t event_timer_block_id;
+ uint64_t address;
+ uint16_t min_tick;
+ uint8_t page_prot;
+} QEMU_PACKED;
+
+struct hpet_fw_config
+{
+ uint8_t count;
+ struct hpet_fw_entry hpet[8];
+} QEMU_PACKED;
+
+extern struct hpet_fw_config hpet_fw_cfg;
+
diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
index d17a8d43199..6f7fcbc3c60 100644
--- a/include/hw/timer/hpet.h
+++ b/include/hw/timer/hpet.h
@@ -60,26 +60,12 @@
#define HPET_TN_INT_ROUTE_CAP_SHIFT 32
#define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
-struct hpet_fw_entry
-{
- uint32_t event_timer_block_id;
- uint64_t address;
- uint16_t min_tick;
- uint8_t page_prot;
-} QEMU_PACKED;
-
-struct hpet_fw_config
-{
- uint8_t count;
- struct hpet_fw_entry hpet[8];
-} QEMU_PACKED;
-
-extern struct hpet_fw_config hpet_cfg;
-
#define TYPE_HPET "hpet"
#define HPET_INTCAP "hpet-intcap"
+#include "hw/timer/hpet-fw-cfg.h"
+
static inline bool hpet_find(void)
{
return object_resolve_path_type("", TYPE_HPET, NULL);
diff --git a/rust/wrapper.h b/rust/wrapper.h
index 285d0eb6ad0..82381e43472 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -62,3 +62,4 @@ typedef enum memory_order {
#include "qapi/error.h"
#include "migration/vmstate.h"
#include "chardev/char-serial.h"
+#include "hw/timer/hpet-fw-cfg.h"
but you will have to use unsafe to access it since it's a "static mut".
Paolo
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 15:30 ` Paolo Bonzini
@ 2024-12-07 15:28 ` Zhao Liu
2025-01-17 10:31 ` Zhao Liu
1 sibling, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-07 15:28 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
Hi Paolo,
Sorry for late reply,
On Thu, Dec 05, 2024 at 04:30:15PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 16:30:15 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
>
> On 12/5/24 07:07, Zhao Liu wrote:
> > HPET device needs to access and update hpet_cfg variable, but now it is
> > defined in hw/i386/fw_cfg.c and Rust code can't access it.
> >
> > Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
> > allows Rust HPET device implements its own global hpet_fw_cfg variable,
> > and will further reduce the use of unsafe C code access and calls in the
> > Rust HPET implementation.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> > hw/i386/fw_cfg.c | 4 +---
> > hw/timer/hpet.c | 16 +++++++++-------
> > include/hw/timer/hpet.h | 2 +-
> > 3 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> > index 0e4494627c21..965e6306838a 100644
> > --- a/hw/i386/fw_cfg.c
> > +++ b/hw/i386/fw_cfg.c
> > @@ -26,8 +26,6 @@
> > #include CONFIG_DEVICES
> > #include "target/i386/cpu.h"
> > -struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>
> This breaks if you disable HPET, which is why fw_cfg.c defines it.
Thanks! I did miss this case.
> You can do something like
>
> diff --git a/include/hw/timer/hpet-fw-cfg.h b/include/hw/timer/hpet-fw-cfg.h
> new file mode 100644
> index 00000000000..234a49fc92e
> --- /dev/null
> +++ b/include/hw/timer/hpet-fw-cfg.h
> @@ -0,0 +1,16 @@
> +struct hpet_fw_entry
> +{
> + uint32_t event_timer_block_id;
> + uint64_t address;
> + uint16_t min_tick;
> + uint8_t page_prot;
> +} QEMU_PACKED;
> +
> +struct hpet_fw_config
> +{
> + uint8_t count;
> + struct hpet_fw_entry hpet[8];
> +} QEMU_PACKED;
> +
> +extern struct hpet_fw_config hpet_fw_cfg;
> +
> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> index d17a8d43199..6f7fcbc3c60 100644
> --- a/include/hw/timer/hpet.h
> +++ b/include/hw/timer/hpet.h
> @@ -60,26 +60,12 @@
> #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
> #define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
> -struct hpet_fw_entry
> -{
> - uint32_t event_timer_block_id;
> - uint64_t address;
> - uint16_t min_tick;
> - uint8_t page_prot;
> -} QEMU_PACKED;
> -
> -struct hpet_fw_config
> -{
> - uint8_t count;
> - struct hpet_fw_entry hpet[8];
> -} QEMU_PACKED;
> -
> -extern struct hpet_fw_config hpet_cfg;
> -
> #define TYPE_HPET "hpet"
> #define HPET_INTCAP "hpet-intcap"
> +#include "hw/timer/hpet-fw-cfg.h"
> +
> static inline bool hpet_find(void)
> {
> return object_resolve_path_type("", TYPE_HPET, NULL);
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 285d0eb6ad0..82381e43472 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -62,3 +62,4 @@ typedef enum memory_order {
> #include "qapi/error.h"
> #include "migration/vmstate.h"
> #include "chardev/char-serial.h"
> +#include "hw/timer/hpet-fw-cfg.h"
>
Thank you very much for this example!
> but you will have to use unsafe to access it since it's a "static mut".
I also noticed Philippe's version. And I prefer Phillippe's version,
although his version requires more changes to the rust version, it
provides an opportunity to add more field to the QOM class, so I feel
it's good to check current rust qom class support.
Regrads,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2024-12-05 15:30 ` Paolo Bonzini
2024-12-07 15:28 ` Zhao Liu
@ 2025-01-17 10:31 ` Zhao Liu
2025-01-17 10:15 ` Paolo Bonzini
1 sibling, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2025-01-17 10:31 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
> This breaks if you disable HPET, which is why fw_cfg.c defines it.
>
> You can do something like
>
> diff --git a/include/hw/timer/hpet-fw-cfg.h b/include/hw/timer/hpet-fw-cfg.h
> new file mode 100644
> index 00000000000..234a49fc92e
> --- /dev/null
> +++ b/include/hw/timer/hpet-fw-cfg.h
> @@ -0,0 +1,16 @@
> +struct hpet_fw_entry
> +{
> + uint32_t event_timer_block_id;
> + uint64_t address;
> + uint16_t min_tick;
> + uint8_t page_prot;
> +} QEMU_PACKED;
> +
> +struct hpet_fw_config
> +{
> + uint8_t count;
> + struct hpet_fw_entry hpet[8];
> +} QEMU_PACKED;
> +
> +extern struct hpet_fw_config hpet_fw_cfg;
> +
> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> index d17a8d43199..6f7fcbc3c60 100644
> --- a/include/hw/timer/hpet.h
> +++ b/include/hw/timer/hpet.h
> @@ -60,26 +60,12 @@
> #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
> #define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
> -struct hpet_fw_entry
> -{
> - uint32_t event_timer_block_id;
> - uint64_t address;
> - uint16_t min_tick;
> - uint8_t page_prot;
> -} QEMU_PACKED;
> -
> -struct hpet_fw_config
> -{
> - uint8_t count;
> - struct hpet_fw_entry hpet[8];
> -} QEMU_PACKED;
> -
> -extern struct hpet_fw_config hpet_cfg;
> -
> #define TYPE_HPET "hpet"
> #define HPET_INTCAP "hpet-intcap"
> +#include "hw/timer/hpet-fw-cfg.h"
> +
> static inline bool hpet_find(void)
> {
> return object_resolve_path_type("", TYPE_HPET, NULL);
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 285d0eb6ad0..82381e43472 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -62,3 +62,4 @@ typedef enum memory_order {
> #include "qapi/error.h"
> #include "migration/vmstate.h"
> #include "chardev/char-serial.h"
> +#include "hw/timer/hpet-fw-cfg.h"
>
>
> but you will have to use unsafe to access it since it's a "static mut".
>
Unfortunately, this way doesn't work either, if we disable both
CONFIG_HPET and CONFIG_X_HPET_RUST.
This is because I integrates hpet_fw_cfg into hpet lib which is compiled
under CONFIG_X_HPET_RUST along with other HPET parts.
The place broken is when hpet_fw_cfg is written into machine's fw_cfg (in
hw/i386/fw_cfg.c).
I think we can just wrap such write like:
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 162785019b7a..3635b83620da 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -147,7 +147,14 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
#endif
fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
- fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_fw_cfg, sizeof(hpet_fw_cfg));
+#if defined(CONFIG_HPET) || defined(CONFIG_X_HPET_RUST)
+ PCMachineState *pcms = (PCMachineState *)object_dynamic_cast(OBJECT(ms),
+ TYPE_PC_MACHINE);
+ if (pcms && pcms->hpet_enabled) {
+ fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_fw_cfg, sizeof(hpet_fw_cfg));
+ }
+#endif
+
/* allocate memory for the NUMA channel: one (64bit) word for the number
* of nodes, one word for each VCPU->node and one word for each node to
* hold the amount of memory.
---
The hpet_fw_cfg is written unconditionally since 40ac17cd, because it
concerns ACPI HPET is created unconditionally. But that fact has changed
and ACPI checks HPET device now (hw/i386/acpi-build.c):
static void acpi_get_misc_info(AcpiMiscInfo *info)
{
info->has_hpet = hpet_find();
#ifdef CONFIG_TPM
info->tpm_version = tpm_get_version(tpm_find());
#endif
}
I think this is a thorough enough solution and I can post a separate
patch.
Thanks,
Zhao
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
2025-01-17 10:31 ` Zhao Liu
@ 2025-01-17 10:15 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2025-01-17 10:15 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 11:13 AM Zhao Liu <zhao1.liu@intel.com> wrote:>
> Unfortunately, this way doesn't work either, if we disable both
> CONFIG_HPET and CONFIG_X_HPET_RUST.
>
> This is because I integrate hpet_fw_cfg into hpet lib which is compiled
> under CONFIG_X_HPET_RUST along with other HPET parts.
>
> The place broken is when hpet_fw_cfg is written into machine's fw_cfg (in
> hw/i386/fw_cfg.c).
>
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 162785019b7a..3635b83620da 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -147,7 +147,14 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
> #endif
> fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
>
> - fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_fw_cfg, sizeof(hpet_fw_cfg));
> +#if defined(CONFIG_HPET) || defined(CONFIG_X_HPET_RUST)
> + PCMachineState *pcms = (PCMachineState *)object_dynamic_cast(OBJECT(ms),
> + TYPE_PC_MACHINE);
> + if (pcms && pcms->hpet_enabled) {
> + fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_fw_cfg, sizeof(hpet_fw_cfg));
> + }
> +#endif
> +
> /* allocate memory for the NUMA channel: one (64bit) word for the number
> * of nodes, one word for each VCPU->node and one word for each node to
> * hold the amount of memory.
>
> I think this is a thorough enough solution and I can post a separate
> patch.
Yes, go ahead so that we can evaluate it separately from Rust concerns.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 10/13] rust/timer/hpet: define hpet_fw_cfg
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (8 preceding siblings ...)
2024-12-05 6:07 ` [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 6:07 ` [RFC 11/13] rust/timer/hpet: add basic HPET timer & state Zhao Liu
` (4 subsequent siblings)
14 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
Define HPETFwEntry structure with the same memory layout as
hpet_fw_entry in C.
Further, define the global hpet_fw_cfg variable in Rust which is the
same as the C version. This hpet_fw_cfg variable in Rust will replace
the C version one and allows both Rust code and C code to access it.
The Rust version of hpet_fw_cfg is self-contained, avoiding unsafe
access to C code.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/Cargo.lock | 8 +++
rust/Cargo.toml | 1 +
rust/hw/meson.build | 1 +
rust/hw/timer/hpet/Cargo.toml | 14 +++++
rust/hw/timer/hpet/meson.build | 18 +++++++
rust/hw/timer/hpet/src/fw_cfg.rs | 88 ++++++++++++++++++++++++++++++++
rust/hw/timer/hpet/src/lib.rs | 15 ++++++
rust/hw/timer/meson.build | 1 +
8 files changed, 146 insertions(+)
create mode 100644 rust/hw/timer/hpet/Cargo.toml
create mode 100644 rust/hw/timer/hpet/meson.build
create mode 100644 rust/hw/timer/hpet/src/fw_cfg.rs
create mode 100644 rust/hw/timer/hpet/src/lib.rs
create mode 100644 rust/hw/timer/meson.build
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index 6b19553b6d10..996454af03cf 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -37,6 +37,14 @@ version = "1.12.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b"
+[[package]]
+name = "hpet"
+version = "0.1.0"
+dependencies = [
+ "qemu_api",
+ "qemu_api_macros",
+]
+
[[package]]
name = "itertools"
version = "0.11.0"
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index de0835bf5b5c..fc620bcaac00 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -4,6 +4,7 @@ members = [
"qemu-api-macros",
"qemu-api",
"hw/char/pl011",
+ "hw/timer/hpet",
]
[workspace.lints.rust]
diff --git a/rust/hw/meson.build b/rust/hw/meson.build
index 860196645e71..9749d4adfc96 100644
--- a/rust/hw/meson.build
+++ b/rust/hw/meson.build
@@ -1 +1,2 @@
subdir('char')
+subdir('timer')
diff --git a/rust/hw/timer/hpet/Cargo.toml b/rust/hw/timer/hpet/Cargo.toml
new file mode 100644
index 000000000000..db2ef4642b4f
--- /dev/null
+++ b/rust/hw/timer/hpet/Cargo.toml
@@ -0,0 +1,14 @@
+[package]
+name = "hpet"
+version = "0.1.0"
+edition = "2021"
+authors = ["Zhao Liu <zhao1.liu@intel.com>"]
+license = "GPL-2.0-or-later"
+description = "IA-PC High Precision Event Timer emulation in Rust"
+
+[lib]
+crate-type = ["staticlib"]
+
+[dependencies]
+qemu_api = { path = "../../../qemu-api" }
+qemu_api_macros = { path = "../../../qemu-api-macros" }
diff --git a/rust/hw/timer/hpet/meson.build b/rust/hw/timer/hpet/meson.build
new file mode 100644
index 000000000000..c2d7c0532ca4
--- /dev/null
+++ b/rust/hw/timer/hpet/meson.build
@@ -0,0 +1,18 @@
+_libhpet_rs = static_library(
+ 'hpet',
+ files('src/lib.rs'),
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'rust',
+ dependencies: [
+ qemu_api,
+ qemu_api_macros,
+ ],
+)
+
+rust_devices_ss.add(when: 'CONFIG_X_HPET_RUST', if_true: [declare_dependency(
+ link_whole: [_libhpet_rs],
+ # Putting proc macro crates in `dependencies` is necessary for Meson to find
+ # them when compiling the root per-target static rust lib.
+ dependencies: [qemu_api_macros],
+ variables: {'crate': 'hpet'},
+)])
diff --git a/rust/hw/timer/hpet/src/fw_cfg.rs b/rust/hw/timer/hpet/src/fw_cfg.rs
new file mode 100644
index 000000000000..a057c2778be4
--- /dev/null
+++ b/rust/hw/timer/hpet/src/fw_cfg.rs
@@ -0,0 +1,88 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#![allow(dead_code)]
+
+use qemu_api::{cell::BqlCell, zeroable::Zeroable};
+
+// Each HPETState represents a Event Timer Block. The v1 spec supports
+// up to 8 blocks. QEMU only uses 1 block (in PC machine).
+const HPET_MAX_NUM_EVENT_TIMER_BLOCK: usize = 8;
+
+#[repr(C, packed)]
+#[derive(Copy, Clone, Default)]
+pub struct HPETFwEntry {
+ pub event_timer_block_id: u32,
+ pub address: u64,
+ pub min_tick: u16,
+ pub page_prot: u8,
+}
+
+unsafe impl Zeroable for HPETFwEntry {
+ const ZERO: Self = Self {
+ event_timer_block_id: 0,
+ address: 0,
+ min_tick: 0,
+ page_prot: 0,
+ };
+}
+
+#[repr(C, packed)]
+#[derive(Copy, Clone, Default)]
+pub struct HPETFwConfig {
+ pub count: u8,
+ pub hpet: [HPETFwEntry; HPET_MAX_NUM_EVENT_TIMER_BLOCK],
+}
+
+unsafe impl Zeroable for HPETFwConfig {
+ const ZERO: Self = Self {
+ count: 0,
+ hpet: [Zeroable::ZERO; HPET_MAX_NUM_EVENT_TIMER_BLOCK],
+ };
+}
+
+// Expose to C code to configure firmware.
+// BqlCell<HPETFwConfig> is picked since it has the same memory layout
+// as HPETFwConfig (just like Cell<T>/UnsafeCell<T>/T).
+pub struct HPETFwConfigCell(BqlCell<HPETFwConfig>);
+
+#[allow(non_upper_case_globals)]
+#[no_mangle]
+pub static mut hpet_fw_cfg: HPETFwConfigCell = HPETFwConfigCell(BqlCell::new(HPETFwConfig {
+ count: u8::MAX,
+ ..Zeroable::ZERO
+}));
+
+impl HPETFwConfigCell {
+ pub(crate) fn assign_hpet_id(&mut self) -> usize {
+ if self.0.get().count == u8::MAX {
+ // first instance
+ self.0.get_mut().count = 0;
+ }
+
+ if self.0.get().count == 8 {
+ // TODO: Add error binding: error_setg()
+ panic!("Only 8 instances of HPET is allowed");
+ }
+
+ let id: usize = self.0.get().count.into();
+ self.0.get_mut().count += 1;
+ id
+ }
+
+ pub(crate) fn update_hpet_cfg(
+ &mut self,
+ hpet_id: usize,
+ event_timer_block_id: Option<u32>,
+ address: Option<u64>,
+ ) {
+ if let Some(e) = event_timer_block_id {
+ self.0.get_mut().hpet[hpet_id].event_timer_block_id = e;
+ }
+
+ if let Some(a) = address {
+ self.0.get_mut().hpet[hpet_id].address = a;
+ }
+ }
+}
diff --git a/rust/hw/timer/hpet/src/lib.rs b/rust/hw/timer/hpet/src/lib.rs
new file mode 100644
index 000000000000..04cb41372b94
--- /dev/null
+++ b/rust/hw/timer/hpet/src/lib.rs
@@ -0,0 +1,15 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+//
+// HPET QEMU Device Model
+//
+// This library implements a device model for the IA-PC HPET (High
+// Precision Event Timers) device in QEMU.
+//
+
+#![deny(unsafe_op_in_unsafe_fn)]
+
+extern crate qemu_api;
+
+pub mod fw_cfg;
diff --git a/rust/hw/timer/meson.build b/rust/hw/timer/meson.build
new file mode 100644
index 000000000000..22a84f15536b
--- /dev/null
+++ b/rust/hw/timer/meson.build
@@ -0,0 +1 @@
+subdir('hpet')
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 11/13] rust/timer/hpet: add basic HPET timer & state
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (9 preceding siblings ...)
2024-12-05 6:07 ` [RFC 10/13] rust/timer/hpet: define hpet_fw_cfg Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 20:22 ` Paolo Bonzini
2024-12-05 6:07 ` [RFC 12/13] rust/timer/hpet: add qdev APIs support Zhao Liu
` (3 subsequent siblings)
14 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
Add the HPETTimer and HPETState (HPET timer block), along with their
basic methods and register definitions.
This is in preparation for supporting the QAPI interfaces.
Note, wrap all items in HPETState that may be changed in the callback
called by C code into the BqlCell/BqlRefCell.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/hw/timer/hpet/src/hpet.rs | 638 +++++++++++++++++++++++++++++++++
rust/hw/timer/hpet/src/lib.rs | 1 +
rust/wrapper.h | 1 +
3 files changed, 640 insertions(+)
create mode 100644 rust/hw/timer/hpet/src/hpet.rs
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
new file mode 100644
index 000000000000..9550d8fe438a
--- /dev/null
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -0,0 +1,638 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#![allow(dead_code)]
+
+use core::ptr::{null_mut, NonNull};
+use std::os::raw::c_int;
+
+use qemu_api::{
+ bindings::*,
+ bitops::deposit64,
+ cell::{BqlCell, BqlRefCell},
+ irq::InterruptSource,
+ memattrs::MEMTXATTRS_UNSPECIFIED,
+ timer::{qemu_clock_get_virtual_ns, QEMUTimerImpl},
+};
+
+// Register space for each timer block. (HPET_BASE isn't defined here.)
+const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
+
+pub(crate) const HPET_MIN_TIMERS: usize = 3; // Miniumum recommended hardware implementation.
+const HPET_MAX_TIMERS: usize = 32; // Maximum timers in each timer block.
+
+// Flags that HPETState.flags supports.
+pub(crate) const HPET_FLAG_MSI_SUPPORT_SHIFT: usize = 0;
+
+const HPET_NUM_IRQ_ROUTES: usize = 32;
+const HPET_LEGACY_PIT_INT: u32 = 0; // HPET_LEGACY_RTC_INT isn't defined here.
+const RTC_ISA_IRQ: usize = 8;
+
+const HPET_CLK_PERIOD: u64 = 10; // 10 ns
+const FS_PER_NS: u64 = 1000000; // 1000000 femtoseconds == 1 ns
+
+// General Capabilities and ID Register
+const HPET_CAP_REG: u64 = 0x000;
+// Revision ID (bits 0:7)
+const HPET_CAP_REV_ID_VALUE: u64 = 0x1; // Revision 1 is implemented (refer to v1.0a spec).
+const HPET_CAP_REV_ID_SHIFT: usize = 0;
+// Number of Timers (bits 8:12)
+const HPET_CAP_NUM_TIM_SHIFT: usize = 8;
+// Counter Size (bit 13)
+const HPET_CAP_COUNT_SIZE_CAP_SHIFT: usize = 13;
+// LegacyReplacement Route Capable (bit 15)
+const HPET_CAP_LEG_RT_CAP_SHIFT: usize = 15;
+// Vendor ID (bits 16:31)
+const HPET_CAP_VENDER_ID_VALUE: u64 = 0x8086;
+const HPET_CAP_VENDER_ID_SHIFT: usize = 16;
+// Main Counter Tick Period (bits 32:63)
+const HPET_CAP_CNT_CLK_PERIOD_SHIFT: usize = 32;
+
+// General Configuration Register
+const HPET_CFG_REG: u64 = 0x010;
+// Overall Enable (bit 0)
+const HPET_CFG_ENABLE_SHIFT: usize = 0;
+// LegacyReplacement Route (bit 1)
+const HPET_CFG_LEG_RT_SHIFT: usize = 1;
+// Other bits are reserved.
+const HPET_CFG_WRITE_MASK: u64 = 0x003;
+
+// General Interrupt Status Register
+const HPET_INT_STATUS_REG: u64 = 0x020;
+
+// Main Counter Value Register
+const HPET_COUNTER_REG: u64 = 0x0f0;
+
+// Timer N Configuration and Capability Register (masked by 0x18)
+const HPET_TN_CFG_REG: u64 = 0x000;
+// bit 0, 7, and bits 16:31 are reserved.
+// bit 4, 5, 15, and bits 32:64 are read-only.
+const HPET_TN_CFG_WRITE_MASK: u64 = 0x7f4e;
+// Timer N Interrupt Type (bit 1)
+const HPET_TN_CFG_INT_TYPE_SHIFT: usize = 1;
+// Timer N Interrupt Enable (bit 2)
+const HPET_TN_CFG_INT_ENABLE_SHIFT: usize = 2;
+// Timer N Type (Periodic enabled or not, bit 3)
+const HPET_TN_CFG_PERIODIC_SHIFT: usize = 3;
+// Timer N Periodic Interrupt Capable (support Periodic or not, bit 4)
+const HPET_TN_CFG_PERIODIC_CAP_SHIFT: usize = 4;
+// Timer N Size (timer size is 64-bits or 32 bits, bit 5)
+const HPET_TN_CFG_SIZE_CAP_SHIFT: usize = 5;
+// Timer N Value Set (bit 6)
+const HPET_TN_CFG_SETVAL_SHIFT: usize = 6;
+// Timer N 32-bit Mode (bit 8)
+const HPET_TN_CFG_32BIT_SHIFT: usize = 8;
+// Timer N Interrupt Rout (bits 9:13)
+const HPET_TN_CFG_INT_ROUTE_MASK: u64 = 0x3e00;
+const HPET_TN_CFG_INT_ROUTE_SHIFT: usize = 9;
+// Timer N FSB Interrupt Enable (bit 14)
+const HPET_TN_CFG_FSB_ENABLE_SHIFT: usize = 14;
+// Timer N FSB Interrupt Delivery (bit 15)
+const HPET_TN_CFG_FSB_CAP_SHIFT: usize = 15;
+// Timer N Interrupt Routing Capability (bits 32:63)
+const HPET_TN_CFG_INT_ROUTE_CAP_SHIFT: usize = 32;
+
+// Timer N Comparator Value Register (masked by 0x18)
+const HPET_TN_CMP_REG: u64 = 0x008;
+
+// Timer N FSB Interrupt Route Register (masked by 0x18)
+const HPET_TN_FSB_ROUTE_REG: u64 = 0x010;
+
+fn hpet_next_wrap(cur_tick: u64) -> u64 {
+ (cur_tick | 0xffffffff) + 1
+}
+
+fn hpet_time_after(a: u64, b: u64) -> bool {
+ ((b - a) as i64) < 0
+}
+
+fn ticks_to_ns(value: u64) -> u64 {
+ value * HPET_CLK_PERIOD
+}
+
+fn ns_to_ticks(value: u64) -> u64 {
+ value / HPET_CLK_PERIOD
+}
+
+// Avoid touching the bits that cannot be written.
+fn hpet_fixup_reg(new: u64, old: u64, mask: u64) -> u64 {
+ (new & mask) | (old & !mask)
+}
+
+fn activating_bit(old: u64, new: u64, shift: usize) -> bool {
+ let mask: u64 = 1 << shift;
+ (old & mask == 0) && (new & mask != 0)
+}
+
+fn deactivating_bit(old: u64, new: u64, shift: usize) -> bool {
+ let mask: u64 = 1 << shift;
+ (old & mask != 0) && (new & mask == 0)
+}
+
+/// HPET Timer Abstraction
+#[repr(C)]
+#[derive(Debug, Default, qemu_api_macros::offsets)]
+pub struct HPETTimer {
+ /// timer N index within the timer block (HPETState)
+ index: usize,
+ qemu_timer: Box<QEMUTimer>,
+ /// timer block abstraction containing this timer
+ state: Option<NonNull<HPETState>>,
+
+ /// Memory-mapped, software visible timer registers
+
+ /// Timer N Configuration and Capability Register
+ config: u64,
+ /// Timer N Comparator Value Register
+ cmp: u64,
+ /// Timer N FSB Interrupt Route Register
+ fsb: u64,
+
+ /// Hidden register state
+
+ /// comparator (extended to counter width)
+ cmp64: u64,
+ /// Last value written to comparator
+ period: u64,
+ /// timer pop will indicate wrap for one-shot 32-bit
+ /// mode. Next pop will be actual timer expiration.
+ wrap_flag: u8,
+ /// last value armed, to avoid timer storms
+ last: u64,
+}
+
+impl HPETTimer {
+ fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
+ *self = HPETTimer::default();
+ self.index = index;
+ self.state = NonNull::new(state_ptr);
+ self
+ }
+
+ fn init_timer_with_state(&mut self) {
+ let index = self.index;
+
+ self.qemu_timer = Box::new(HPETState::timer_new_ns(
+ QEMUClockType::QEMU_CLOCK_VIRTUAL,
+ &mut self.get_state_mut().timer[index],
+ ));
+ }
+
+ fn get_state_ref(&self) -> &HPETState {
+ // SAFETY:
+ // the pointer is convertible to a reference
+ unsafe { self.state.unwrap().as_ref() }
+ }
+
+ fn get_state_mut(&mut self) -> &mut HPETState {
+ // SAFETY:
+ // the pointer is convertible to a reference
+ unsafe { self.state.unwrap().as_mut() }
+ }
+
+ fn is_int_active(&self) -> bool {
+ self.get_state_ref().int_status.get() & (1 << self.index) != 0
+ }
+
+ fn is_fsb_route_enabled(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_FSB_ENABLE_SHIFT != 0
+ }
+
+ fn is_periodic(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_PERIODIC_SHIFT != 0
+ }
+
+ fn is_int_enabled(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_INT_ENABLE_SHIFT != 0
+ }
+
+ fn is_32bit_mod(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_32BIT_SHIFT != 0
+ }
+
+ fn is_valset_enabled(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_SETVAL_SHIFT != 0
+ }
+
+ fn clear_valset(&mut self) {
+ self.config &= !(1 << HPET_TN_CFG_SETVAL_SHIFT);
+ }
+
+ /// True if timer interrupt is level triggered; otherwise, edge triggered.
+ fn is_int_level_triggered(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_INT_TYPE_SHIFT != 0
+ }
+
+ /// calculate next value of the general counter that matches the
+ /// target (either entirely, or the low 32-bit only depending on
+ /// the timer mode).
+ fn calculate_cmp64(&self, cur_tick: u64, target: u64) -> u64 {
+ if self.is_32bit_mod() {
+ let mut result: u64 = deposit64(cur_tick, 0, 32, target);
+ if result < cur_tick {
+ result += 0x100000000;
+ }
+ result
+ } else {
+ target
+ }
+ }
+
+ fn update_int_status(&mut self, set: bool) -> &mut Self {
+ let mask: u64 = 1 << self.index;
+
+ if set && self.is_int_level_triggered() {
+ // If Timer N Interrupt Enable bit is 0, "the timer will
+ // still operate and generate appropriate status bits, but
+ // will not cause an interrupt"
+ *self.get_state_mut().int_status.get_mut() |= mask;
+ } else {
+ *self.get_state_mut().int_status.get_mut() &= !mask;
+ }
+ self
+ }
+
+ fn get_individual_route(&self) -> usize {
+ ((self.config & HPET_TN_CFG_INT_ROUTE_MASK) >> HPET_TN_CFG_INT_ROUTE_SHIFT) as usize
+ }
+
+ fn get_int_route(&self) -> usize {
+ if self.index <= 1 && self.get_state_ref().is_legacy_mode() {
+ // If LegacyReplacement Route bit is set, HPET specification requires
+ // timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
+ // timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
+ //
+ // If the LegacyReplacement Route bit is set, the individual routing
+ // bits for timers 0 and 1 (APIC or FSB) will have no impact.
+ //
+ // FIXME: Consider I/O APIC case.
+ if self.index == 0 {
+ 0
+ } else {
+ RTC_ISA_IRQ
+ }
+ } else {
+ // (If the LegacyReplacement Route bit is set) Timer 2-n will be
+ // routed as per the routing in the timer n config registers.
+ // ...
+ // If the LegacyReplacement Route bit is not set, the individual
+ // routing bits for each of the timers are used.
+ self.get_individual_route()
+ }
+ }
+
+ fn set_irq(&mut self, set: bool) {
+ let route = self.get_int_route();
+
+ if set && self.is_int_enabled() && self.get_state_ref().is_hpet_enabled() {
+ if self.is_fsb_route_enabled() {
+ // SAFETY:
+ // the parameters are valid.
+ unsafe {
+ address_space_stl_le(
+ &mut address_space_memory,
+ self.fsb >> 32, // Timer N FSB int addr
+ self.fsb as u32, // Timer N FSB int value, truncate!
+ *MEMTXATTRS_UNSPECIFIED,
+ null_mut(),
+ );
+ }
+ } else if self.is_int_level_triggered() {
+ self.get_state_ref().irqs[route].raise();
+ } else {
+ self.get_state_ref().irqs[route].pulse();
+ }
+ } else if !self.is_fsb_route_enabled() {
+ self.get_state_ref().irqs[route].lower();
+ }
+ }
+
+ fn update_irq(&mut self, set: bool) {
+ self.update_int_status(set).set_irq(set);
+ }
+
+ fn arm_timer(&mut self, tick: u64) {
+ let mut ns = self.get_state_ref().get_ns(tick);
+
+ // Clamp period to reasonable min value (1 us)
+ if self.is_periodic() && ns - self.last < 1000 {
+ ns = self.last + 1000;
+ }
+
+ self.last = ns;
+ self.qemu_timer.as_mut().timer_mod(self.last);
+ }
+
+ fn set_timer(&mut self) {
+ let cur_tick: u64 = self.get_state_ref().get_ticks();
+
+ self.wrap_flag = 0;
+ self.cmp64 = self.calculate_cmp64(cur_tick, self.cmp);
+ if self.is_32bit_mod() {
+ // HPET spec says in one-shot 32-bit mode, generate an interrupt when
+ // counter wraps in addition to an interrupt with comparator match.
+ if !self.is_periodic() && self.cmp64 > hpet_next_wrap(cur_tick) {
+ self.wrap_flag = 1;
+ self.arm_timer(hpet_next_wrap(cur_tick));
+ return;
+ }
+ }
+ self.arm_timer(self.cmp64);
+ }
+
+ fn del_timer(&mut self) {
+ self.qemu_timer.as_mut().timer_del();
+
+ if self.is_int_active() {
+ // For level-triggered interrupt, this leaves interrupt status
+ // register set but lowers irq.
+ self.update_irq(true);
+ }
+ }
+
+ // Configuration and Capability Register
+ fn set_tn_cfg_reg(&mut self, shift: usize, len: usize, val: u64) {
+ // TODO: Add trace point - trace_hpet_ram_write_tn_cfg(addr & 4)
+ let old_val: u64 = self.config;
+ let mut new_val: u64 = deposit64(old_val, shift, len, val);
+ new_val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
+
+ // Switch level-type interrupt to edge-type.
+ if deactivating_bit(old_val, new_val, HPET_TN_CFG_INT_TYPE_SHIFT) {
+ // Do this before changing timer.config; otherwise, if
+ // HPET_TN_FSB is set, update_irq will not lower the qemu_irq.
+ self.update_irq(false);
+ }
+
+ self.config = new_val;
+
+ if activating_bit(old_val, new_val, HPET_TN_CFG_INT_ENABLE_SHIFT) && self.is_int_active() {
+ self.update_irq(true);
+ }
+
+ if self.is_32bit_mod() {
+ self.cmp = self.cmp as u32 as u64; // truncate!
+ self.period = self.period as u32 as u64; // truncate!
+ }
+
+ if self.get_state_ref().is_hpet_enabled() {
+ self.set_timer();
+ }
+ }
+
+ /// Comparator Value Register
+ fn set_tn_cmp_reg(&mut self, shift: usize, len: usize, val: u64) {
+ let mut length = len;
+ let mut value = val;
+
+ // TODO: Add trace point - trace_hpet_ram_write_tn_cmp(addr & 4)
+ if self.is_32bit_mod() {
+ // High 32-bits are zero, leave them untouched.
+ if shift != 0 {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid_tn_cmp()
+ return;
+ }
+ length = 64;
+ value = value as u32 as u64; // truncate!
+ }
+
+ if !self.is_periodic() || self.is_valset_enabled() {
+ self.cmp = deposit64(self.cmp, shift, length, value);
+ }
+
+ if self.is_periodic() {
+ self.period = deposit64(self.period, shift, length, value);
+ }
+
+ self.clear_valset();
+ if self.get_state_ref().is_hpet_enabled() {
+ self.set_timer();
+ }
+ }
+
+ /// FSB Interrupt Route Register
+ fn set_tn_fsb_route_reg(&mut self, shift: usize, len: usize, val: u64) {
+ self.fsb = deposit64(self.fsb, shift, len, val);
+ }
+
+ fn reset(&mut self) {
+ self.del_timer();
+ self.cmp = u64::MAX; // Comparator Match Registers reset to all 1's.
+ self.config = 1 << HPET_TN_CFG_PERIODIC_CAP_SHIFT | 1 << HPET_TN_CFG_SIZE_CAP_SHIFT;
+ if self.get_state_ref().has_msi_flag() {
+ self.config |= 1 << HPET_TN_CFG_FSB_CAP_SHIFT;
+ }
+ // advertise availability of ioapic int
+ self.config |=
+ (self.get_state_ref().int_route_cap as u64) << HPET_TN_CFG_INT_ROUTE_CAP_SHIFT;
+ self.period = 0;
+ self.wrap_flag = 0;
+ }
+
+ /// timer expiration callback
+ fn callback(&mut self) {
+ let period: u64 = self.period;
+ let cur_tick: u64 = self.get_state_ref().get_ticks();
+
+ if self.is_periodic() && period != 0 {
+ while hpet_time_after(cur_tick, self.cmp64) {
+ self.cmp64 += period;
+ }
+ if self.is_32bit_mod() {
+ self.cmp = self.cmp64 as u32 as u64;
+ } else {
+ self.cmp = self.cmp64;
+ }
+ self.arm_timer(self.cmp64);
+ } else if self.wrap_flag != 0 {
+ self.wrap_flag = 0;
+ self.arm_timer(self.cmp64);
+ }
+ self.update_irq(true);
+ }
+}
+
+#[derive(Debug)]
+pub struct HPETTimerInstance(BqlRefCell<HPETTimer>);
+
+impl HPETTimerInstance {
+ fn timer_handler(timer: &mut HPETTimerInstance) {
+ timer.0.borrow_mut().callback()
+ }
+}
+
+/// HPET Event Timer Block Abstraction
+/// Note: Wrap all items that may be changed in the callback called by C
+/// into the BqlCell/BqlRefCell.
+#[repr(C)]
+#[derive(Debug, qemu_api_macros::offsets)]
+pub struct HPETState {
+ parent_obj: SysBusDevice,
+ iomem: MemoryRegion,
+
+ /// Registers: Memory-mapped, software visible registers
+
+ /// General Capabilities and ID Register
+ capability: BqlCell<u64>,
+ /// General Configuration Register
+ config: BqlCell<u64>,
+ /// General Interrupt Status Register
+ int_status: BqlCell<u64>,
+ /// Main Counter Value Register
+ counter: BqlCell<u64>,
+
+ /// Internal state
+
+ /// Capabilities that QEMU HPET supports.
+ /// bit 0: MSI (or FSB) support.
+ pub(crate) flags: BqlCell<u32>,
+
+ /// Offset of main counter relative to qemu clock.
+ hpet_offset: BqlCell<u64>,
+ pub(crate) hpet_offset_saved: bool,
+
+ irqs: [InterruptSource; HPET_NUM_IRQ_ROUTES],
+ rtc_irq_level: BqlCell<u8>,
+ pit_enabled: InterruptSource,
+
+ /// Interrupt Routing Capability.
+ /// This field indicates to which interrupts in the I/O (x) APIC
+ /// the timers' interrupt can be routed, and is encoded in the
+ /// bits 32:64 of timer N's config register:
+ pub(crate) int_route_cap: u32,
+
+ /// HPET timer array managed by this timer block.
+ timer: [HPETTimerInstance; HPET_MAX_TIMERS],
+ pub(crate) num_timers: BqlCell<usize>,
+
+ /// Instance id (HPET timer block ID).
+ hpet_id: BqlCell<usize>,
+}
+
+impl HPETState {
+ fn has_msi_flag(&self) -> bool {
+ self.flags.get() & 1 << HPET_FLAG_MSI_SUPPORT_SHIFT != 0
+ }
+
+ fn is_legacy_mode(&self) -> bool {
+ self.config.get() & 1 << HPET_CFG_LEG_RT_SHIFT != 0
+ }
+
+ fn is_hpet_enabled(&self) -> bool {
+ self.config.get() & 1 << HPET_CFG_ENABLE_SHIFT != 0
+ }
+
+ fn get_ticks(&self) -> u64 {
+ ns_to_ticks(qemu_clock_get_virtual_ns() + self.hpet_offset.get())
+ }
+
+ fn get_ns(&self, tick: u64) -> u64 {
+ ticks_to_ns(tick) - self.hpet_offset.get()
+ }
+
+ fn handle_legacy_irq(&mut self, irq: u32, level: u32) {
+ if irq == HPET_LEGACY_PIT_INT {
+ if !self.is_legacy_mode() {
+ self.irqs[0].set(level != 0);
+ }
+ } else {
+ self.rtc_irq_level.set(level as u8);
+ if !self.is_legacy_mode() {
+ self.irqs[RTC_ISA_IRQ].set(level != 0);
+ }
+ }
+ }
+
+ fn get_timer(&self, timer_id: usize) -> &BqlRefCell<HPETTimer> {
+ &self.timer[timer_id].0
+ }
+
+ fn init_timer(&mut self) {
+ let raw_ptr: *mut HPETState = self;
+
+ for i in 0..HPET_MAX_TIMERS {
+ let mut timer = self.get_timer(i).borrow_mut();
+ timer.init(i, raw_ptr).init_timer_with_state();
+ }
+ }
+
+ /// General Configuration Register
+ fn set_cfg_reg(&mut self, shift: usize, len: usize, val: u64) {
+ let old_val = self.config.get();
+ let mut new_val = deposit64(old_val, shift, len, val);
+
+ new_val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
+ self.config.set(new_val);
+
+ if activating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
+ // Enable main counter and interrupt generation.
+ self.hpet_offset
+ .set(ticks_to_ns(self.counter.get()) - qemu_clock_get_virtual_ns());
+
+ for i in 0..self.num_timers.get() {
+ let mut timer = self.get_timer(i).borrow_mut();
+
+ if timer.is_int_enabled() && timer.is_int_active() {
+ timer.update_irq(true);
+ }
+ timer.set_timer();
+ }
+ } else if deactivating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
+ // Halt main counter and disable interrupt generation.
+ self.counter.set(self.get_ticks());
+
+ for i in 0..self.num_timers.get() {
+ self.get_timer(i).borrow_mut().del_timer();
+ }
+ }
+
+ // i8254 and RTC output pins are disabled when HPET is in legacy mode
+ if activating_bit(old_val, new_val, HPET_CFG_LEG_RT_SHIFT) {
+ self.pit_enabled.set(false);
+ self.irqs[0].lower();
+ self.irqs[RTC_ISA_IRQ].lower();
+ } else if deactivating_bit(old_val, new_val, HPET_CFG_LEG_RT_SHIFT) {
+ // TODO: Add irq binding: qemu_irq_lower(s->irqs[0])
+ self.irqs[0].lower();
+ self.pit_enabled.set(true);
+ self.irqs[RTC_ISA_IRQ].set(self.rtc_irq_level.get() != 0);
+ }
+ }
+
+ /// General Interrupt Status Register: Read/Write Clear
+ fn set_int_status_reg(&mut self, shift: usize, _len: usize, val: u64) {
+ let new_val = val << shift;
+ let cleared = new_val & self.int_status.get();
+
+ for i in 0..self.num_timers.get() {
+ if cleared & (1 << i) != 0 {
+ self.get_timer(i).borrow_mut().update_irq(false);
+ }
+ }
+ }
+
+ /// Main Counter Value Register
+ fn set_counter_reg(&mut self, shift: usize, len: usize, val: u64) {
+ if self.is_hpet_enabled() {
+ // TODO: Add trace point -
+ // trace_hpet_ram_write_counter_write_while_enabled()
+ //
+ // HPET spec says that writes to this register should only be
+ // done while the counter is halted. So this is an undefined
+ // behavior. There's no need to forbid it, but when HPET is
+ // enabled, the changed counter value will not affect the
+ // tick count (i.e., the previously calculated offset will
+ // not be changed as well).
+ }
+ self.counter
+ .set(deposit64(self.counter.get(), shift, len, val));
+ }
+}
+
+impl QEMUTimerImpl for HPETState {
+ type Opaque = HPETTimerInstance;
+
+ const QEMU_TIMER_CB: Option<fn(opaque: &mut HPETTimerInstance)> =
+ Some(HPETTimerInstance::timer_handler);
+}
diff --git a/rust/hw/timer/hpet/src/lib.rs b/rust/hw/timer/hpet/src/lib.rs
index 04cb41372b94..387913bbdfb9 100644
--- a/rust/hw/timer/hpet/src/lib.rs
+++ b/rust/hw/timer/hpet/src/lib.rs
@@ -13,3 +13,4 @@
extern crate qemu_api;
pub mod fw_cfg;
+pub mod hpet;
diff --git a/rust/wrapper.h b/rust/wrapper.h
index 0da42e84933a..f44aba63d2ed 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -64,3 +64,4 @@ typedef enum memory_order {
#include "chardev/char-serial.h"
#include "exec/memattrs.h"
#include "qemu/timer.h"
+#include "exec/address-spaces.h"
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 11/13] rust/timer/hpet: add basic HPET timer & state
2024-12-05 6:07 ` [RFC 11/13] rust/timer/hpet: add basic HPET timer & state Zhao Liu
@ 2024-12-05 20:22 ` Paolo Bonzini
2024-12-05 21:20 ` Paolo Bonzini
2024-12-09 7:26 ` Zhao Liu
0 siblings, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 20:22 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> Add the HPETTimer and HPETState (HPET timer block), along with their
> basic methods and register definitions.
>
> This is in preparation for supporting the QAPI interfaces.
>
> Note, wrap all items in HPETState that may be changed in the callback
> called by C code into the BqlCell/BqlRefCell.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> rust/hw/timer/hpet/src/hpet.rs | 638 +++++++++++++++++++++++++++++++++
> rust/hw/timer/hpet/src/lib.rs | 1 +
> rust/wrapper.h | 1 +
> 3 files changed, 640 insertions(+)
> create mode 100644 rust/hw/timer/hpet/src/hpet.rs
>
> diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
> new file mode 100644
> index 000000000000..9550d8fe438a
> --- /dev/null
> +++ b/rust/hw/timer/hpet/src/hpet.rs
> @@ -0,0 +1,638 @@
> +// Copyright (C) 2024 Intel Corporation.
> +// Author(s): Zhao Liu <zhai1.liu@intel.com>
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#![allow(dead_code)]
> +
> +use core::ptr::{null_mut, NonNull};
> +use std::os::raw::c_int;
> +
> +use qemu_api::{
> + bindings::*,
Let's avoid bindings::*.
> + self.qemu_timer = Box::new(HPETState::timer_new_ns(
Oh! I noticed now that while your API is called timer_new_ns, it is
actually the same as timer_init_full. Let's call it init_full() then.
> + fn get_state_ref(&self) -> &HPETState {
> + // SAFETY:
> + // the pointer is convertible to a reference
> + unsafe { self.state.unwrap().as_ref() }
> + }
> +
> + fn get_state_mut(&mut self) -> &mut HPETState {
> + // SAFETY:
> + // the pointer is convertible to a reference
> + unsafe { self.state.unwrap().as_mut() }
> + }
You should not need get_state_mut(), which also has the advantage of
shortening get_state_ref() to get_state().
> +
> + fn is_int_active(&self) -> bool {
> + self.get_state_ref().int_status.get() & (1 << self.index) != 0
> + }
> +
> + fn is_fsb_route_enabled(&self) -> bool {
> + self.config & 1 << HPET_TN_CFG_FSB_ENABLE_SHIFT != 0
> + }
> +
> + fn is_periodic(&self) -> bool {
> + self.config & 1 << HPET_TN_CFG_PERIODIC_SHIFT != 0
> + }
> +
> + fn is_int_enabled(&self) -> bool {
> + self.config & 1 << HPET_TN_CFG_INT_ENABLE_SHIFT != 0
> + }
> +
> + fn is_32bit_mod(&self) -> bool {
> + self.config & 1 << HPET_TN_CFG_32BIT_SHIFT != 0
> + }
> +
> + fn is_valset_enabled(&self) -> bool {
> + self.config & 1 << HPET_TN_CFG_SETVAL_SHIFT != 0
> + }
> +
> + fn clear_valset(&mut self) {
> + self.config &= !(1 << HPET_TN_CFG_SETVAL_SHIFT);
> + }
> +
> + /// True if timer interrupt is level triggered; otherwise, edge triggered.
> + fn is_int_level_triggered(&self) -> bool {
> + self.config & 1 << HPET_TN_CFG_INT_TYPE_SHIFT != 0
> + }
PL011 is using bilge here. I think it's fair to show the two ways to do
it. If we have devices showing two different things:
- PL011 shows higher-level abstractions for registers
- HPET has a good approach to interior mutability from the beginning
Then it gives a clearer view of the options.
> + fn update_int_status(&mut self, set: bool) -> &mut Self {
> + let mask: u64 = 1 << self.index;
> +
> + if set && self.is_int_level_triggered() {
> + // If Timer N Interrupt Enable bit is 0, "the timer will
> + // still operate and generate appropriate status bits, but
> + // will not cause an interrupt"
> + *self.get_state_mut().int_status.get_mut() |= mask;
> + } else {
> + *self.get_state_mut().int_status.get_mut() &= !mask;
> + }
> + self
> + }
See remarks elsewhere on update_int_status(), and how it uses
get_state_mut() and get_mut().
> + unsafe {
> + address_space_stl_le(
> + &mut address_space_memory,
> + self.fsb >> 32, // Timer N FSB int addr
> + self.fsb as u32, // Timer N FSB int value, truncate!
> + *MEMTXATTRS_UNSPECIFIED,
> + null_mut(),
> + );
> + }
This is the only use of unsafe, whic is not bad at all. Not urgent, but
we should think about the AddressSpace bindings, and whether it makes
sense to use (or steal APIs from) rust-vmm's vm-memory.
> + fn arm_timer(&mut self, tick: u64) {
> + let mut ns = self.get_state_ref().get_ns(tick);
> +
> + // Clamp period to reasonable min value (1 us)
> + if self.is_periodic() && ns - self.last < 1000 {
> + ns = self.last + 1000;
> + }
> +
> + self.last = ns;
> + self.qemu_timer.as_mut().timer_mod(self.last);
> + }
No as_mut(), timer_mod is thread safe. timer_mod() need to take &self.
> + fn del_timer(&mut self) {
> + self.qemu_timer.as_mut().timer_del();
Same as above.
> +#[derive(Debug)]
> +pub struct HPETTimerInstance(BqlRefCell<HPETTimer>);
> +
> +impl HPETTimerInstance {
> + fn timer_handler(timer: &mut HPETTimerInstance) {
> + timer.0.borrow_mut().callback()
> + }
> +}
Also not "&mut" - you don't need it, as "timer.0" is only used to borrow
from the BqlRefCell. Also with a more refined timer abstraction this
doesn't need HPETTimerInstance, it can probably be a global function like
fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
timer_cell.borrow_mut().callback()
}
> + /// General Capabilities and ID Register
> + capability: BqlCell<u64>,
> + /// General Configuration Register
> + config: BqlCell<u64>,
> + /// General Interrupt Status Register
> + int_status: BqlCell<u64>,
> + /// Main Counter Value Register
> + counter: BqlCell<u64>,
> +
> + /// Internal state
> +
> + /// Capabilities that QEMU HPET supports.
> + /// bit 0: MSI (or FSB) support.
> + pub(crate) flags: BqlCell<u32>,
flags doesn't need to be a cell (it's just a property). I'll send a
patch for the C code.
> + /// Offset of main counter relative to qemu clock.
> + hpet_offset: BqlCell<u64>,
> + pub(crate) hpet_offset_saved: bool,
> +
> + irqs: [InterruptSource; HPET_NUM_IRQ_ROUTES],
> + rtc_irq_level: BqlCell<u8>,
> + pit_enabled: InterruptSource,
> +
> + /// Interrupt Routing Capability.
> + /// This field indicates to which interrupts in the I/O (x) APIC
> + /// the timers' interrupt can be routed, and is encoded in the
> + /// bits 32:64 of timer N's config register:
> + pub(crate) int_route_cap: u32,
> +
> + /// HPET timer array managed by this timer block.
> + timer: [HPETTimerInstance; HPET_MAX_TIMERS],
> + pub(crate) num_timers: BqlCell<usize>,
Ah, this needs to be a BqlCell because it can be clamped to
MIN_TIMERS..MAX_TIMERS by realize. Fair enough.
> + /// Instance id (HPET timer block ID).
> + hpet_id: BqlCell<usize>,
> +}
> +
Like flags this does not need to be a cell.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 11/13] rust/timer/hpet: add basic HPET timer & state
2024-12-05 20:22 ` Paolo Bonzini
@ 2024-12-05 21:20 ` Paolo Bonzini
2024-12-09 7:46 ` Zhao Liu
2024-12-09 7:26 ` Zhao Liu
1 sibling, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 21:20 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On Thu, Dec 5, 2024 at 9:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > + /// Instance id (HPET timer block ID).
> > + hpet_id: BqlCell<usize>,
> > +}
> > +
> Like flags this does not need to be a cell.
Well, this *should not* need to be a cell (ideally fw_cfg would call a
C function to generate FW_CFG_HPET) but it can't be done yet, so you
can keep it. flags need not be a BqlCell though.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 11/13] rust/timer/hpet: add basic HPET timer & state
2024-12-05 21:20 ` Paolo Bonzini
@ 2024-12-09 7:46 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-09 7:46 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Thu, Dec 05, 2024 at 10:20:47PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 22:20:47 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 11/13] rust/timer/hpet: add basic HPET timer & state
>
> On Thu, Dec 5, 2024 at 9:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > + /// Instance id (HPET timer block ID).
> > > + hpet_id: BqlCell<usize>,
> > > +}
> > > +
> > Like flags this does not need to be a cell.
>
> Well, this *should not* need to be a cell (ideally fw_cfg would call a
> C function to generate FW_CFG_HPET) but it can't be done yet, so you
> can keep it. flags need not be a BqlCell though.
Thank you! I'll look at how to implement C function's workflow.
At least, I'll leave a TODO to remind me to cleanup this BqlCell.
(Now I did overuse the BqlCell a bit! :-) This is also an opportunity
for me to carefully review the correct usage.)
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 11/13] rust/timer/hpet: add basic HPET timer & state
2024-12-05 20:22 ` Paolo Bonzini
2024-12-05 21:20 ` Paolo Bonzini
@ 2024-12-09 7:26 ` Zhao Liu
1 sibling, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-09 7:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
> > +use qemu_api::{
> > + bindings::*,
>
> Let's avoid bindings::*.
Sure.
> > + self.qemu_timer = Box::new(HPETState::timer_new_ns(
>
> Oh! I noticed now that while your API is called timer_new_ns, it is actually
> the same as timer_init_full. Let's call it init_full() then.
Sure, will use init_full() directly.
> > + fn get_state_ref(&self) -> &HPETState {
> > + // SAFETY:
> > + // the pointer is convertible to a reference
> > + unsafe { self.state.unwrap().as_ref() }
> > + }
> > +
> > + fn get_state_mut(&mut self) -> &mut HPETState {
> > + // SAFETY:
> > + // the pointer is convertible to a reference
> > + unsafe { self.state.unwrap().as_mut() }
> > + }
>
> You should not need get_state_mut(), which also has the advantage of
> shortening get_state_ref() to get_state().
I see now, internal mutability requires using immutable references
as much as possible!
...
> > + /// True if timer interrupt is level triggered; otherwise, edge triggered.
> > + fn is_int_level_triggered(&self) -> bool {
> > + self.config & 1 << HPET_TN_CFG_INT_TYPE_SHIFT != 0
> > + }
>
> PL011 is using bilge here. I think it's fair to show the two ways to do it.
> If we have devices showing two different things:
>
> - PL011 shows higher-level abstractions for registers
>
> - HPET has a good approach to interior mutability from the beginning
>
> Then it gives a clearer view of the options.
Yes, I am used to handling registers in a C-style way, which is more
straightforward and easier for me to use. However, compared to PL011, it
has less abstraction so that it might look a bit messier. The choice of
different options is basically a trade-off between different styles. :-)
> > + fn update_int_status(&mut self, set: bool) -> &mut Self {
> > + let mask: u64 = 1 << self.index;
> > +
> > + if set && self.is_int_level_triggered() {
> > + // If Timer N Interrupt Enable bit is 0, "the timer will
> > + // still operate and generate appropriate status bits, but
> > + // will not cause an interrupt"
> > + *self.get_state_mut().int_status.get_mut() |= mask;
> > + } else {
> > + *self.get_state_mut().int_status.get_mut() &= !mask;
> > + }
> > + self
> > + }
>
> See remarks elsewhere on update_int_status(), and how it uses
> get_state_mut() and get_mut().
Thank you! Will change as your example.
> > + unsafe {
> > + address_space_stl_le(
> > + &mut address_space_memory,
> > + self.fsb >> 32, // Timer N FSB int addr
> > + self.fsb as u32, // Timer N FSB int value, truncate!
> > + *MEMTXATTRS_UNSPECIFIED,
> > + null_mut(),
> > + );
> > + }
>
> This is the only use of unsafe, whic is not bad at all. Not urgent, but we
> should think about the AddressSpace bindings, and whether it makes sense to
> use (or steal APIs from) rust-vmm's vm-memory.
Good idea, I agree that this is a good "starting" point to consider
introducing rust-vmm to QEMU. I'll add it to my TODO list and consider
it. Once I have some ideas, I'll share them.
> > + fn arm_timer(&mut self, tick: u64) {
> > + let mut ns = self.get_state_ref().get_ns(tick);
> > +
> > + // Clamp period to reasonable min value (1 us)
> > + if self.is_periodic() && ns - self.last < 1000 {
> > + ns = self.last + 1000;
> > + }
> > +
> > + self.last = ns;
> > + self.qemu_timer.as_mut().timer_mod(self.last);
> > + }
>
> No as_mut(), timer_mod is thread safe. timer_mod() need to take &self.
Ok, I understand then I need to implement a as_mut_ptr to convert
`&self` to `* mut QEMUTimer`, as you did for SysBusDevice.
> > + fn del_timer(&mut self) {
> > + self.qemu_timer.as_mut().timer_del();
>
> Same as above.
I see, thanks.
> > +#[derive(Debug)]
> > +pub struct HPETTimerInstance(BqlRefCell<HPETTimer>);
> > +
> > +impl HPETTimerInstance {
> > + fn timer_handler(timer: &mut HPETTimerInstance) {
> > + timer.0.borrow_mut().callback()
> > + }
> > +}
>
> Also not "&mut" - you don't need it, as "timer.0" is only used to borrow
> from the BqlRefCell. Also with a more refined timer abstraction this
> doesn't need HPETTimerInstance, it can probably be a global function like
>
> fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
> timer_cell.borrow_mut().callback()
> }
I see, it's clearer!
> > + /// General Capabilities and ID Register
> > + capability: BqlCell<u64>,
> > + /// General Configuration Register
> > + config: BqlCell<u64>,
> > + /// General Interrupt Status Register
> > + int_status: BqlCell<u64>,
> > + /// Main Counter Value Register
> > + counter: BqlCell<u64>,
> > +
> > + /// Internal state
> > +
> > + /// Capabilities that QEMU HPET supports.
> > + /// bit 0: MSI (or FSB) support.
> > + pub(crate) flags: BqlCell<u32>,
>
> flags doesn't need to be a cell (it's just a property). I'll send a patch
> for the C code.
Thank you! I've reviewed that patch.
> > + /// Offset of main counter relative to qemu clock.
> > + hpet_offset: BqlCell<u64>,
> > + pub(crate) hpet_offset_saved: bool,
> > +
> > + irqs: [InterruptSource; HPET_NUM_IRQ_ROUTES],
> > + rtc_irq_level: BqlCell<u8>,
> > + pit_enabled: InterruptSource,
> > +
> > + /// Interrupt Routing Capability.
> > + /// This field indicates to which interrupts in the I/O (x) APIC
> > + /// the timers' interrupt can be routed, and is encoded in the
> > + /// bits 32:64 of timer N's config register:
> > + pub(crate) int_route_cap: u32,
> > +
> > + /// HPET timer array managed by this timer block.
> > + timer: [HPETTimerInstance; HPET_MAX_TIMERS],
> > + pub(crate) num_timers: BqlCell<usize>,
>
> Ah, this needs to be a BqlCell because it can be clamped to
> MIN_TIMERS..MAX_TIMERS by realize. Fair enough.
Yes, it's a corner case.
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 12/13] rust/timer/hpet: add qdev APIs support
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (10 preceding siblings ...)
2024-12-05 6:07 ` [RFC 11/13] rust/timer/hpet: add basic HPET timer & state Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 18:58 ` Paolo Bonzini
2024-12-05 6:07 ` [RFC 13/13] i386: enable rust hpet for pc when rust is enabled Zhao Liu
` (2 subsequent siblings)
14 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
Implement QAPI support for HPET device in qdev.rs.
Additionally, wrap the handling of HPET internal details as traits to be
specifically implemented in hpet.rs.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/hw/timer/hpet/src/fw_cfg.rs | 2 -
rust/hw/timer/hpet/src/hpet.rs | 232 ++++++++++++++++++++++++++++++-
rust/hw/timer/hpet/src/lib.rs | 5 +
rust/hw/timer/hpet/src/qdev.rs | 133 ++++++++++++++++++
4 files changed, 365 insertions(+), 7 deletions(-)
create mode 100644 rust/hw/timer/hpet/src/qdev.rs
diff --git a/rust/hw/timer/hpet/src/fw_cfg.rs b/rust/hw/timer/hpet/src/fw_cfg.rs
index a057c2778be4..6515a428cebb 100644
--- a/rust/hw/timer/hpet/src/fw_cfg.rs
+++ b/rust/hw/timer/hpet/src/fw_cfg.rs
@@ -2,8 +2,6 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-#![allow(dead_code)]
-
use qemu_api::{cell::BqlCell, zeroable::Zeroable};
// Each HPETState represents a Event Timer Block. The v1 spec supports
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 9550d8fe438a..9480633a77dd 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -2,10 +2,8 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-#![allow(dead_code)]
-
-use core::ptr::{null_mut, NonNull};
-use std::os::raw::c_int;
+use core::ptr::{addr_of_mut, null_mut, NonNull};
+use std::os::raw::{c_uint, c_void};
use qemu_api::{
bindings::*,
@@ -13,9 +11,14 @@
cell::{BqlCell, BqlRefCell},
irq::InterruptSource,
memattrs::MEMTXATTRS_UNSPECIFIED,
+ qdev::DeviceGPIOImpl,
+ qom::ObjectType,
timer::{qemu_clock_get_virtual_ns, QEMUTimerImpl},
+ zeroable::Zeroable,
};
+use crate::{fw_cfg::*, qdev::*};
+
// Register space for each timer block. (HPET_BASE isn't defined here.)
const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
@@ -453,6 +456,38 @@ fn callback(&mut self) {
}
}
+impl RamOps for HPETTimer {
+ fn read(&mut self, addr: hwaddr, _size: c_uint) -> u64 {
+ let shift: u64 = (addr & 4) * 8;
+
+ match addr & 0x18 {
+ HPET_TN_CFG_REG => self.config >> shift, // including interrupt capabilities
+ HPET_TN_CMP_REG => self.cmp >> shift, // comparator register
+ HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_read_invalid()
+ // Reserved.
+ 0
+ }
+ }
+ }
+
+ fn write(&mut self, addr: hwaddr, value: u64, size: u64) {
+ let shift = ((addr & 4) * 8) as usize;
+ let len = std::cmp::min(size * 8, 64 - shift as u64) as usize;
+
+ match addr & 0x18 {
+ HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),
+ HPET_TN_CMP_REG => self.set_tn_cmp_reg(shift, len, value),
+ HPET_TN_FSB_ROUTE_REG => self.set_tn_fsb_route_reg(shift, len, value),
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid()
+ // Reserved.
+ }
+ }
+ }
+}
+
#[derive(Debug)]
pub struct HPETTimerInstance(BqlRefCell<HPETTimer>);
@@ -466,7 +501,7 @@ fn timer_handler(timer: &mut HPETTimerInstance) {
/// Note: Wrap all items that may be changed in the callback called by C
/// into the BqlCell/BqlRefCell.
#[repr(C)]
-#[derive(Debug, qemu_api_macros::offsets)]
+#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
pub struct HPETState {
parent_obj: SysBusDevice,
iomem: MemoryRegion,
@@ -636,3 +671,190 @@ impl QEMUTimerImpl for HPETState {
const QEMU_TIMER_CB: Option<fn(opaque: &mut HPETTimerInstance)> =
Some(HPETTimerInstance::timer_handler);
}
+
+impl ObjOps for HPETState {
+ // TODO: Add binding to register idiomatic Rust callback.
+ const HPET_RAM_OPS: MemoryRegionOps = MemoryRegionOps {
+ read: Some(hpet_ram_read),
+ write: Some(hpet_ram_write),
+ read_with_attrs: None,
+ write_with_attrs: None,
+ valid: MemoryRegionOps__bindgen_ty_1 {
+ min_access_size: 4,
+ max_access_size: 8,
+ ..Zeroable::ZERO
+ },
+ impl_: MemoryRegionOps__bindgen_ty_2 {
+ min_access_size: 4,
+ max_access_size: 8,
+ ..Zeroable::ZERO
+ },
+ endianness: device_endian::DEVICE_NATIVE_ENDIAN,
+ };
+
+ unsafe fn init(&mut self) {
+ // SAFETY:
+ // self and self.iomem are guaranteed to be valid at this point since callers
+ // must make sure the `self` reference is valid.
+ unsafe {
+ memory_region_init_io(
+ addr_of_mut!(self.iomem),
+ addr_of_mut!(*self).cast::<Object>(),
+ &Self::HPET_RAM_OPS,
+ addr_of_mut!(*self).cast::<c_void>(),
+ Self::TYPE_NAME.as_ptr(),
+ HPET_REG_SPACE_LEN,
+ );
+ let sbd = addr_of_mut!(*self).cast::<SysBusDevice>();
+ sysbus_init_mmio(sbd, addr_of_mut!(self.iomem));
+ }
+ }
+}
+
+impl DeviceGPIOImpl for HPETState {
+ const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> =
+ Some(HPETState::handle_legacy_irq);
+}
+
+impl QDevOps for HPETState {
+ fn realize(&mut self) {
+ // SAFETY:
+ // caller of C Qdev guarantees that the "self" passed in is a
+ // valid HPETState reference, so it is able to cast as SysBusDevice.
+ let sbd = unsafe { &mut *(addr_of_mut!(*self).cast::<SysBusDevice>()) };
+
+ if self.int_route_cap == 0 {
+ // TODO: Add error binding: warn_report()
+ println!("Hpet's hpet-intcap property not initialized");
+ }
+
+ // SAFETY:
+ // hpet_fw_cfg is contained in a BqlCell and is protected by BQL,
+ // so it's safe to modify it.
+ self.hpet_id.set(unsafe { hpet_fw_cfg.assign_hpet_id() });
+
+ for irq in self.irqs.iter() {
+ sbd.init_irq(irq);
+ }
+
+ if self.num_timers.get() < HPET_MIN_TIMERS {
+ self.num_timers.set(HPET_MIN_TIMERS);
+ } else if self.num_timers.get() > HPET_MAX_TIMERS {
+ self.num_timers.set(HPET_MAX_TIMERS);
+ }
+
+ self.init_timer();
+ // 64-bit General Capabilities and ID Register; LegacyReplacementRoute.
+ self.capability.set(
+ HPET_CAP_REV_ID_VALUE << HPET_CAP_REV_ID_SHIFT |
+ 1 << HPET_CAP_COUNT_SIZE_CAP_SHIFT |
+ 1 << HPET_CAP_LEG_RT_CAP_SHIFT |
+ HPET_CAP_VENDER_ID_VALUE << HPET_CAP_VENDER_ID_SHIFT |
+ ((self.num_timers.get() - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
+ (HPET_CLK_PERIOD * FS_PER_NS) << HPET_CAP_CNT_CLK_PERIOD_SHIFT, // 10 ns
+ );
+
+ self.init_gpio_in(2);
+ self.init_gpio_out(&self.pit_enabled, 1);
+ }
+
+ fn reset(&mut self) {
+ let sbd: *mut SysBusDevice = addr_of_mut!(*self).cast::<SysBusDevice>();
+
+ for i in 0..self.num_timers.get() {
+ self.get_timer(i).borrow_mut().reset();
+ }
+
+ self.pit_enabled.set(true);
+ self.counter.set(0);
+ self.hpet_offset.set(0);
+ self.config.set(0);
+ // SAFETY:
+ // hpet_fw_cfg is contained in a BqlCell and is protected by BQL,
+ // so it's safe to modify it.
+ unsafe {
+ hpet_fw_cfg.update_hpet_cfg(
+ self.hpet_id.get(),
+ Some(self.capability.get() as u32),
+ Some((*sbd).mmio[0].addr),
+ )
+ };
+
+ // to document that the RTC lowers its output on reset as well
+ self.rtc_irq_level.set(0);
+ }
+}
+
+impl RamOps for HPETState {
+ fn read(&mut self, addr: hwaddr, size: c_uint) -> u64 {
+ let shift: u64 = (addr & 4) * 8;
+
+ // address range of all TN regs
+ if (0x100..=0x3ff).contains(&addr) {
+ let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
+
+ // TODO: Add trace point - trace_hpet_ram_read(addr)
+ if timer_id > self.num_timers.get() {
+ // TODO: Add trace point - trace_hpet_timer_id_out_of_range(timer_id)
+ // Reserved.
+ return 0;
+ }
+
+ self.get_timer(timer_id).borrow_mut().read(addr, size)
+ } else {
+ match addr & !4 {
+ HPET_CAP_REG => self.capability.get() >> shift, /* including HPET_PERIOD 0x004 */
+ // (CNT_CLK_PERIOD field)
+ HPET_CFG_REG => self.config.get() >> shift,
+ HPET_COUNTER_REG => {
+ let cur_tick: u64 = if self.is_hpet_enabled() {
+ self.get_ticks()
+ } else {
+ self.counter.get()
+ };
+
+ // TODO: Add trace point - trace_hpet_ram_read_reading_counter(addr & 4,
+ // cur_tick)
+ cur_tick >> shift
+ }
+ HPET_INT_STATUS_REG => self.int_status.get() >> shift,
+ _ => {
+ // TODO: Add trace point- trace_hpet_ram_read_invalid()
+ // Reserved.
+ 0
+ }
+ }
+ }
+ }
+
+ fn write(&mut self, addr: hwaddr, value: u64, size: u64) {
+ let shift = ((addr & 4) * 8) as usize;
+ let len = std::cmp::min(size * 8, 64 - shift as u64) as usize;
+
+ // TODO: Add trace point - trace_hpet_ram_write(addr, value)
+ if (0x100..=0x3ff).contains(&addr) {
+ let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
+
+ // TODO: Add trace point - trace_hpet_ram_write_timer_id(timer_id)
+ if timer_id > self.num_timers.get() {
+ // TODO: Add trace point - trace_hpet_timer_id_out_of_range(timer_id)
+ return;
+ }
+
+ self.get_timer(timer_id)
+ .borrow_mut()
+ .write(addr, value, size)
+ } else {
+ match addr & !0x4 {
+ HPET_CAP_REG => {} // General Capabilities and ID Register: Read Only
+ HPET_CFG_REG => self.set_cfg_reg(shift, len, value),
+ HPET_INT_STATUS_REG => self.set_int_status_reg(shift, len, value),
+ HPET_COUNTER_REG => self.set_counter_reg(shift, len, value),
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid()
+ // Reserved.
+ }
+ }
+ }
+ }
+}
diff --git a/rust/hw/timer/hpet/src/lib.rs b/rust/hw/timer/hpet/src/lib.rs
index 387913bbdfb9..1692dbf19a85 100644
--- a/rust/hw/timer/hpet/src/lib.rs
+++ b/rust/hw/timer/hpet/src/lib.rs
@@ -10,7 +10,12 @@
#![deny(unsafe_op_in_unsafe_fn)]
+use qemu_api::c_str;
+
extern crate qemu_api;
pub mod fw_cfg;
pub mod hpet;
+pub mod qdev;
+
+pub const TYPE_HPET: &::std::ffi::CStr = c_str!("hpet");
diff --git a/rust/hw/timer/hpet/src/qdev.rs b/rust/hw/timer/hpet/src/qdev.rs
new file mode 100644
index 000000000000..6ddfc9422d78
--- /dev/null
+++ b/rust/hw/timer/hpet/src/qdev.rs
@@ -0,0 +1,133 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use core::ptr::NonNull;
+use std::{
+ ffi::CStr,
+ os::raw::{c_uint, c_void},
+};
+
+use qemu_api::{
+ bindings::*,
+ c_str,
+ qdev::DeviceImpl,
+ qom::{ClassInitImpl, ObjectImpl, ObjectType},
+ qom_isa,
+};
+
+use crate::hpet::*;
+
+qom_isa!(HPETState : SysBusDevice, DeviceState, Object);
+
+// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
+#[repr(C)]
+pub struct HPETClass {
+ parent_class: <SysBusDevice as ObjectType>::Class,
+}
+
+unsafe impl ObjectType for HPETState {
+ type Class = HPETClass;
+ const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
+}
+
+impl ClassInitImpl<HPETClass> for HPETState {
+ fn class_init(klass: &mut HPETClass) {
+ <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
+ }
+}
+
+pub(crate) trait ObjOps {
+ const HPET_RAM_OPS: MemoryRegionOps;
+ unsafe fn init(&mut self);
+}
+
+impl ObjectImpl for HPETState {
+ type ParentType = SysBusDevice;
+
+ const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+}
+
+pub(crate) trait RamOps {
+ fn read(&mut self, addr: hwaddr, _size: c_uint) -> u64;
+ fn write(&mut self, addr: hwaddr, value: u64, size: u64);
+}
+
+pub(crate) unsafe extern "C" fn hpet_ram_read(
+ opaque: *mut c_void,
+ addr: hwaddr,
+ size: c_uint,
+) -> u64 {
+ // SAFETY:
+ // the pointer is convertible to a reference
+ let state: &mut HPETState =
+ unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
+
+ state.read(addr, size)
+}
+
+pub(crate) unsafe extern "C" fn hpet_ram_write(
+ opaque: *mut c_void,
+ addr: hwaddr,
+ data: u64,
+ size: c_uint,
+) {
+ // SAFETY:
+ // the pointer is convertible to a reference
+ let state: &mut HPETState =
+ unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
+
+ state.write(addr, data, size as u64);
+}
+
+// TODO: Make these properties user-configurable!
+qemu_api::declare_properties! {
+ HPET_PROPERTIES,
+ qemu_api::define_property!(
+ c_str!("timers"),
+ HPETState,
+ num_timers,
+ unsafe { &qdev_prop_uint8 },
+ u8,
+ default = HPET_MIN_TIMERS
+ ),
+ qemu_api::define_property!(
+ c_str!("msi"),
+ HPETState,
+ flags,
+ unsafe { &qdev_prop_bit },
+ u32,
+ bit = HPET_FLAG_MSI_SUPPORT_SHIFT as u8,
+ default = false,
+ ),
+ qemu_api::define_property!(
+ c_str!("hpet-intcap"),
+ HPETState,
+ int_route_cap,
+ unsafe { &qdev_prop_uint32 },
+ u32,
+ default = 0
+ ),
+ qemu_api::define_property!(
+ c_str!("hpet-offset-saved"),
+ HPETState,
+ hpet_offset_saved,
+ unsafe { &qdev_prop_bool },
+ bool,
+ default = true
+ ),
+}
+
+pub(crate) trait QDevOps {
+ fn realize(&mut self);
+ fn reset(&mut self);
+}
+
+impl DeviceImpl for HPETState {
+ fn properties() -> &'static [Property] {
+ &HPET_PROPERTIES
+ }
+
+ const REALIZE: Option<fn(&mut Self)> = Some(Self::realize);
+ const RESET: Option<fn(&mut Self)> = Some(Self::reset);
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 12/13] rust/timer/hpet: add qdev APIs support
2024-12-05 6:07 ` [RFC 12/13] rust/timer/hpet: add qdev APIs support Zhao Liu
@ 2024-12-05 18:58 ` Paolo Bonzini
2024-12-07 16:05 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 18:58 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> Implement QAPI support for HPET device in qdev.rs.
>
> Additionally, wrap the handling of HPET internal details as traits to be
> specifically implemented in hpet.rs.
Why not just put everything in a simple file to avoid the traits?
> +pub(crate) trait RamOps {
> + fn read(&mut self, addr: hwaddr, _size: c_uint) -> u64;
> + fn write(&mut self, addr: hwaddr, value: u64, size: u64);
> +}
Please try as much as possible to make these "&self", same for the GPIO
handler. That's the point of BqlCell/BqlRefCell. :)
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 12/13] rust/timer/hpet: add qdev APIs support
2024-12-05 18:58 ` Paolo Bonzini
@ 2024-12-07 16:05 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-07 16:05 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Thu, Dec 05, 2024 at 07:58:07PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 19:58:07 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 12/13] rust/timer/hpet: add qdev APIs support
>
> On 12/5/24 07:07, Zhao Liu wrote:
> > Implement QAPI support for HPET device in qdev.rs.
> >
> > Additionally, wrap the handling of HPET internal details as traits to be
> > specifically implemented in hpet.rs.
>
> Why not just put everything in a simple file to avoid the traits?
Ah, I considered that qdev has many unsafe C-style wrappers, to avoid
confusion with HPET state/timer methods... However, looking back now,
qdev and qom are already quite clean. I will merge this into hpet.rs in
the next version.
> > +pub(crate) trait RamOps {
> > + fn read(&mut self, addr: hwaddr, _size: c_uint) -> u64;
> > + fn write(&mut self, addr: hwaddr, value: u64, size: u64);
> > +}
>
> Please try as much as possible to make these "&self", same for the GPIO
> handler. That's the point of BqlCell/BqlRefCell. :)
Sure! Will do as you suggested.
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 13/13] i386: enable rust hpet for pc when rust is enabled
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (11 preceding siblings ...)
2024-12-05 6:07 ` [RFC 12/13] rust/timer/hpet: add qdev APIs support Zhao Liu
@ 2024-12-05 6:07 ` Zhao Liu
2024-12-05 15:20 ` Paolo Bonzini
2024-12-05 6:22 ` [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
2024-12-05 16:28 ` Paolo Bonzini
14 siblings, 1 reply; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:07 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell
Cc: qemu-devel, qemu-rust, Zhao Liu
Add HPET configuration in PC's Kconfig options, and select HPET device
(Rust version) if Rust is supported.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
hw/i386/Kconfig | 2 ++
hw/timer/Kconfig | 1 -
rust/hw/Kconfig | 1 +
rust/hw/timer/Kconfig | 2 ++
4 files changed, 5 insertions(+), 1 deletion(-)
create mode 100644 rust/hw/timer/Kconfig
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 32818480d263..83ab3222c4f0 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -39,6 +39,8 @@ config PC
select PCSPK
select I8257
select MC146818RTC
+ select HPET if !HAVE_RUST
+ select X_HPET_RUST if HAVE_RUST
# For ACPI builder:
select SERIAL_ISA
select ACPI_PCI
diff --git a/hw/timer/Kconfig b/hw/timer/Kconfig
index c96fd5d97ae8..645d7531f40e 100644
--- a/hw/timer/Kconfig
+++ b/hw/timer/Kconfig
@@ -11,7 +11,6 @@ config A9_GTIMER
config HPET
bool
- default y if PC
config I8254
bool
diff --git a/rust/hw/Kconfig b/rust/hw/Kconfig
index 4d934f30afe1..36f92ec02874 100644
--- a/rust/hw/Kconfig
+++ b/rust/hw/Kconfig
@@ -1,2 +1,3 @@
# devices Kconfig
source char/Kconfig
+source timer/Kconfig
diff --git a/rust/hw/timer/Kconfig b/rust/hw/timer/Kconfig
new file mode 100644
index 000000000000..afd980335037
--- /dev/null
+++ b/rust/hw/timer/Kconfig
@@ -0,0 +1,2 @@
+config X_HPET_RUST
+ bool
--
2.34.1
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 13/13] i386: enable rust hpet for pc when rust is enabled
2024-12-05 6:07 ` [RFC 13/13] i386: enable rust hpet for pc when rust is enabled Zhao Liu
@ 2024-12-05 15:20 ` Paolo Bonzini
2024-12-06 9:06 ` Zhao Liu
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 15:20 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> Add HPET configuration in PC's Kconfig options, and select HPET device
> (Rust version) if Rust is supported.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> hw/i386/Kconfig | 2 ++
> hw/timer/Kconfig | 1 -
> rust/hw/Kconfig | 1 +
> rust/hw/timer/Kconfig | 2 ++
> 4 files changed, 5 insertions(+), 1 deletion(-)
> create mode 100644 rust/hw/timer/Kconfig
>
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index 32818480d263..83ab3222c4f0 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -39,6 +39,8 @@ config PC
> select PCSPK
> select I8257
> select MC146818RTC
> + select HPET if !HAVE_RUST
> + select X_HPET_RUST if HAVE_RUST
HPET is optional, so you need to have...
> config HPET
> bool
> - default y if PC
>
+default y if PC && !HAVE_RUST
with a matching "default y if PC && HAVE_RUST" for X_HPET_RUST.
Paolo
> config I8254
> bool
> diff --git a/rust/hw/Kconfig b/rust/hw/Kconfig
> index 4d934f30afe1..36f92ec02874 100644
> --- a/rust/hw/Kconfig
> +++ b/rust/hw/Kconfig
> @@ -1,2 +1,3 @@
> # devices Kconfig
> source char/Kconfig
> +source timer/Kconfig
> diff --git a/rust/hw/timer/Kconfig b/rust/hw/timer/Kconfig
> new file mode 100644
> index 000000000000..afd980335037
> --- /dev/null
> +++ b/rust/hw/timer/Kconfig
> @@ -0,0 +1,2 @@
> +config X_HPET_RUST
> + bool
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 13/13] i386: enable rust hpet for pc when rust is enabled
2024-12-05 15:20 ` Paolo Bonzini
@ 2024-12-06 9:06 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-06 9:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust, Zhao Liu
On Thu, Dec 05, 2024 at 04:20:44PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 16:20:44 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 13/13] i386: enable rust hpet for pc when rust is enabled
>
> On 12/5/24 07:07, Zhao Liu wrote:
> > Add HPET configuration in PC's Kconfig options, and select HPET device
> > (Rust version) if Rust is supported.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> > hw/i386/Kconfig | 2 ++
> > hw/timer/Kconfig | 1 -
> > rust/hw/Kconfig | 1 +
> > rust/hw/timer/Kconfig | 2 ++
> > 4 files changed, 5 insertions(+), 1 deletion(-)
> > create mode 100644 rust/hw/timer/Kconfig
> >
> > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> > index 32818480d263..83ab3222c4f0 100644
> > --- a/hw/i386/Kconfig
> > +++ b/hw/i386/Kconfig
> > @@ -39,6 +39,8 @@ config PC
> > select PCSPK
> > select I8257
> > select MC146818RTC
> > + select HPET if !HAVE_RUST
> > + select X_HPET_RUST if HAVE_RUST
>
> HPET is optional, so you need to have...
Oops, I didn't realize that.
> > config HPET
> > bool
> > - default y if PC
>
> +default y if PC && !HAVE_RUST
>
> with a matching "default y if PC && HAVE_RUST" for X_HPET_RUST.
Thank you!
-Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (12 preceding siblings ...)
2024-12-05 6:07 ` [RFC 13/13] i386: enable rust hpet for pc when rust is enabled Zhao Liu
@ 2024-12-05 6:22 ` Zhao Liu
2024-12-05 16:28 ` Paolo Bonzini
14 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-05 6:22 UTC (permalink / raw)
To: Paolo Bonzini, Michael S . Tsirkin, Manos Pitsidianakis,
Junjie Mao, Alex Bennée, Philippe Mathieu-Daudé,
Peter Maydell, Daniel P. Berrangé, Zhao Liu
Cc: qemu-devel, qemu-rust
> After making empty promises for many months, I have finally written the
> Rust version of HPET :-) I'm also very grateful for the help from Paolo,
> Manos, and Junjie!
>
> Overall, HPET in Rust maintains the same logic as the original C
> version, adhering to the IA-HPET spec v1.0a [1]. While keeping the logic
> unchanged, it attempts to keep up with the current development progress
> of Rust for QEMU, leveraging the latest and ongoing Rust binding updates
> as much as possible, such as BqlCell / BqlRefCell, qom & qdev
> enhancements, irq binding, and more. Additionally, it introduces new
> bindings, including gpio_{in|out}, bitops, memattrs, and timer. Finally,
> based on Paolo's suggestion, the vmstate part is temporarily on hold.
>
> Welcome your comments and feedback!
Based on Paolo's rust-next branch of https://gitlab.com/bonzini/qemu at
the commit 05de50008121 ("rust: qom: move device_id to PL011 class side").
[snip]
> Public and Private in QOM State
> -------------------------------
>
> I recently asked on the mailing list [4] about the reason for using
> "<public>"/"<private>" comments in QOM structures. Peter, Junjie, and
> Balaton provided some explanations and feedback (thank you all).
And thanks Daniel!!
...
> [4]: https://lore.kernel.org/qemu-devel/ZxPZ5oUDRcVroh7o@intel.com/
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust
2024-12-05 6:07 [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
` (13 preceding siblings ...)
2024-12-05 6:22 ` [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust Zhao Liu
@ 2024-12-05 16:28 ` Paolo Bonzini
2024-12-09 7:57 ` Zhao Liu
14 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2024-12-05 16:28 UTC (permalink / raw)
To: Zhao Liu, Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell
Cc: qemu-devel, qemu-rust
On 12/5/24 07:07, Zhao Liu wrote:
> * Proper bindings for MemoryRegionOps, which needs to wrap the ram
> read/write callbacks.
> - I think it shouldn't be complicated because qom/qdev already
> provides good examples.
Sounds good, I have some initial work on PL011.
> * vmstate support.
> - vmstate code for HPET is actually ready, but it will be pending (and
> maybe re-writing) until the vmstate infra gets cleaned up.
Yeah, mostly due to the cells.
> * Error handling.
> - Now HPET just use panic and println to replace error_setg and
> warn_report.
>
> * Trace support.
> - No trace for now.
> Anything that needs to be modified within a callback should be protected
> by BqlCell/BqlRefCell.
>
> Based on this, I am also considering how the opaque parameter in certain
> callbacks should interact with BQL cells. In the timer binding (patch
> 7), I think the opaque parameter accepted by the timer callback should
> be placed in a BQL cell. However, considering generality, I did not make
> further changes and only passed BqlRefCell<HPETTimer> as the opaque
> parameter in the HPET code.
That sounds good.
I'll review your timer bindings and post the infrastructure that I have
for callbacks.
> Furthermore, is it possible in the future to wrap the entire state
> within a BQL cell? This could save the effort of wrapping many state
> members individually when state becomes very huge and complex.
I think it's better if we keep a split between the mutable and immutable
parts. For example properties are immutable.
> QDEV Property
> -------------
>
> To support bit type property, I added another macro variant (in patch 8)
> to allow bitnr parameter. However, I think this lacks scalability.
>
> In qdev-properties.h, it is clear that the PropertyInfo of a property is
> bound to its type. Previously, Junjie and I attempted to do the same in
> Rust by binding PropertyInfo to the type, thereby avoiding the need to
> specify too many parameters in the macro definitions:
>
> https://lore.kernel.org/qemu-devel/20241017143245.1248589-1-zhao1.liu@intel.com/
Oops, sorry. I have applied patch 1, but patch 2 is a bit problematic
because I think it's not const-friendly. Maybe it was applied to older
code?
I haven't looked closely at whether it's possible to use trait { ...
const TYPE: &Property = ... }, maybe only in newer Rust? But as you
point out there is an issue with multiple PropertyInfos that can apply
to the same type, therefore I think the way to go here is to use a
procedural macro and a #[property] attribute. That also makes bit
properties easy:
#[property(bit=0, name="foo")
#[property(bit=1, name="bar")
features: u32,
> MEMTXATTRS_UNSPECIFIED
> ----------------------
>
> MEMTXATTRS_UNSPECIFIED is another global variable. Since it is
> immutable, BQL cell is not needed.
>
> But MemTxAttrs is a structure with bitfields, and the bindings generated
> by bindgen can only be modified through methods. Therefore, it is
> necessary to introduce lazy to initialize MEMTXATTRS_UNSPECIFIED in a
> const context (patch 6).
Ugh, that's a pity. Maybe instead of introducing a new dependency we
can use an always-inlined function?
In an ideal world, bindgen would produce const functions. A hackish but
easy possibility is to modify the generated bindgen code (with sed? :))
to apply const to the bitfield functions.
> Cycle Reference
> ---------------
>
> A common pattern in QEMU devices is that a sub-member of the state
> contains a pointer to the state itself.
>
> For HPETState, it maintains a HPETTimer array, and each HPETTimer also
> has the pointer to parent HPETState. So there's the cycle reference
> issue.
>
> The common way to address this is to use RefCell<Weak<>> [2], but in
> practice, I found it's difficult to be applied in device code. The
> device instance is not created in Rust side, and there's only init()
> method to initialize created device instance. This way, it's hard to be
> compatible with the pattern of creating weak references (at least I
> failed).
>
> Then, I chose NonNull to address this issue, as recommended by the
> author of NonNull and the standard collections [3].
NonNull is okay for now. Later on we will handle it with pinned_init.
However, do not get a &mut HPETState, everything should go through
&HPETState and cells.
> Therefore, I believe that in Rust devices, QOM members should also be
> kept private or at most `pub(crate)`. This is also why I tried to avoid
> using direct `pub` in HPET.
Yes, I agree. I should look at unnecessarily pub things in pl011.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust
2024-12-05 16:28 ` Paolo Bonzini
@ 2024-12-09 7:57 ` Zhao Liu
0 siblings, 0 replies; 72+ messages in thread
From: Zhao Liu @ 2024-12-09 7:57 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S . Tsirkin, Manos Pitsidianakis, Junjie Mao,
Alex Bennée, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel, qemu-rust
On Thu, Dec 05, 2024 at 05:28:45PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 17:28:45 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust
>
> On 12/5/24 07:07, Zhao Liu wrote:
> > * Proper bindings for MemoryRegionOps, which needs to wrap the ram
> > read/write callbacks.
> > - I think it shouldn't be complicated because qom/qdev already
> > provides good examples.
>
> Sounds good, I have some initial work on PL011.
Great! Sorry for the slow response, I just saw this comment. I'm looking
forward to your work on this!
> > Based on this, I am also considering how the opaque parameter in certain
> > callbacks should interact with BQL cells. In the timer binding (patch
> > 7), I think the opaque parameter accepted by the timer callback should
> > be placed in a BQL cell. However, considering generality, I did not make
> > further changes and only passed BqlRefCell<HPETTimer> as the opaque
> > parameter in the HPET code.
>
> That sounds good.
>
> I'll review your timer bindings and post the infrastructure that I have for
> callbacks.
Thank you!
> > Furthermore, is it possible in the future to wrap the entire state
> > within a BQL cell? This could save the effort of wrapping many state
> > members individually when state becomes very huge and complex.
>
> I think it's better if we keep a split between the mutable and immutable
> parts. For example properties are immutable.
I like this idea. Clear splitting helps to standardize the usage of
BqlCell, preventing both underuse and overuse.
> > QDEV Property
> > -------------
> >
> > To support bit type property, I added another macro variant (in patch 8)
> > to allow bitnr parameter. However, I think this lacks scalability.
> >
> > In qdev-properties.h, it is clear that the PropertyInfo of a property is
> > bound to its type. Previously, Junjie and I attempted to do the same in
> > Rust by binding PropertyInfo to the type, thereby avoiding the need to
> > specify too many parameters in the macro definitions:
> >
> > https://lore.kernel.org/qemu-devel/20241017143245.1248589-1-zhao1.liu@intel.com/
>
> Oops, sorry. I have applied patch 1, but patch 2 is a bit problematic
> because I think it's not const-friendly. Maybe it was applied to older
> code?
Yes, it was based on old code base...and it seems outdated.
> I haven't looked closely at whether it's possible to use trait { ... const
> TYPE: &Property = ... }, maybe only in newer Rust? But as you point out
> there is an issue with multiple PropertyInfos that can apply to the same
> type, therefore I think the way to go here is to use a procedural macro and
> a #[property] attribute. That also makes bit properties easy:
>
> #[property(bit=0, name="foo")
> #[property(bit=1, name="bar")
> features: u32,
Thank you, it looks better and overcomes the drawback of the original
proposal.
> > MEMTXATTRS_UNSPECIFIED
> > ----------------------
> >
> > MEMTXATTRS_UNSPECIFIED is another global variable. Since it is
> > immutable, BQL cell is not needed.
> >
> > But MemTxAttrs is a structure with bitfields, and the bindings generated
> > by bindgen can only be modified through methods. Therefore, it is
> > necessary to introduce lazy to initialize MEMTXATTRS_UNSPECIFIED in a
> > const context (patch 6).
>
> Ugh, that's a pity. Maybe instead of introducing a new dependency we can
> use an always-inlined function?
Based your and Richard's suggestion, modifying memattrs avoids
introducing additional library. :-)
> In an ideal world, bindgen would produce const functions. A hackish but
> easy possibility is to modify the generated bindgen code (with sed? :)) to
> apply const to the bitfield functions.
I think it could work, and this time it won't be needed, based on your
newer suggestion. :-)
> > Then, I chose NonNull to address this issue, as recommended by the
> > author of NonNull and the standard collections [3].
>
> NonNull is okay for now. Later on we will handle it with pinned_init.
>
> However, do not get a &mut HPETState, everything should go through
> &HPETState and cells.
Sure. I see.
> > Therefore, I believe that in Rust devices, QOM members should also be
> > kept private or at most `pub(crate)`. This is also why I tried to avoid
> > using direct `pub` in HPET.
>
> Yes, I agree. I should look at unnecessarily pub things in pl011.
Thank you!
Regards,
Zhao
^ permalink raw reply [flat|nested] 72+ messages in thread