* [PULL 01/25] chardev: express dependency on io/
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
@ 2025-03-09 10:30 ` Paolo Bonzini
2025-03-09 10:30 ` [PULL 02/25] scripts: dump stdin on meson-buildoptions error Paolo Bonzini
` (23 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé
chardev is using qio functions, so express that in the Meson internal
dependency. (I found this when adding character devices bindings for
Rust; they initially needed the io dependency added by hand).
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 8b9fda4d95e..67ec2b78319 100644
--- a/meson.build
+++ b/meson.build
@@ -4015,7 +4015,7 @@ libchardev = static_library('chardev', chardev_ss.sources() + genh,
build_by_default: false)
chardev = declare_dependency(objects: libchardev.extract_all_objects(recursive: false),
- dependencies: chardev_ss.dependencies())
+ dependencies: [chardev_ss.dependencies(), io])
hwcore_ss = hwcore_ss.apply({})
libhwcore = static_library('hwcore', sources: hwcore_ss.sources() + genh,
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 02/25] scripts: dump stdin on meson-buildoptions error
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
2025-03-09 10:30 ` [PULL 01/25] chardev: express dependency on io/ Paolo Bonzini
@ 2025-03-09 10:30 ` Paolo Bonzini
2025-03-09 10:30 ` [PULL 03/25] rust: cell: add wrapper for FFI types Paolo Bonzini
` (22 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Nabih Estefan, Patrick Venture
From: Nabih Estefan <nabihestefan@google.com>
Dump sys.stdin when it errors on meson-buildoptions.py, letting us debug
the build errors instead of just saying "Couldn't parse"
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
Signed-off-by: Patrick Venture <venture@google.com>
Link: https://lore.kernel.org/r/20250227180454.2006757-1-venture@google.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/meson-buildoptions.py | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/scripts/meson-buildoptions.py b/scripts/meson-buildoptions.py
index 4814a8ff61f..a3e22471b2f 100644
--- a/scripts/meson-buildoptions.py
+++ b/scripts/meson-buildoptions.py
@@ -241,8 +241,14 @@ def print_parse(options):
print(" esac")
print("}")
-
-options = load_options(json.load(sys.stdin))
+json_data = sys.stdin.read()
+try:
+ options = load_options(json.loads(json_data))
+except:
+ print("Failure in scripts/meson-buildoptions.py parsing stdin as json",
+ file=sys.stderr)
+ print(json_data, file=sys.stderr)
+ sys.exit(1)
print("# This file is generated by meson-buildoptions.py, do not edit!")
print_help(options)
print_parse(options)
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 03/25] rust: cell: add wrapper for FFI types
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
2025-03-09 10:30 ` [PULL 01/25] chardev: express dependency on io/ Paolo Bonzini
2025-03-09 10:30 ` [PULL 02/25] scripts: dump stdin on meson-buildoptions error Paolo Bonzini
@ 2025-03-09 10:30 ` Paolo Bonzini
2025-03-09 10:30 ` [PULL 04/25] rust: qemu_api_macros: add Wrapper derive macro Paolo Bonzini
` (21 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Inspired by the same-named type in Linux. This type provides the compiler
with a correct view of what goes on with FFI types. In addition, it
separates the glue code from the bindgen-generated code, allowing
traits such as Send, Sync or Zeroable to be specified independently
for C and Rust structs.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/devel/rust.rst | 34 +++++--
rust/qemu-api/src/cell.rs | 204 ++++++++++++++++++++++++++++++++++++--
2 files changed, 223 insertions(+), 15 deletions(-)
diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 5d8aa3a45bc..784c3e40bdc 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -296,15 +296,33 @@ of ``&mut self``; access to internal fields must use *interior mutability*
to go from a shared reference to a ``&mut``.
Whenever C code provides you with an opaque ``void *``, avoid converting it
-to a Rust mutable reference, and use a shared reference instead. Rust code
-will then have to use QEMU's ``BqlRefCell`` and ``BqlCell`` type, which
-enforce that locking rules for the "Big QEMU Lock" are respected. These cell
-types are also known to the ``vmstate`` crate, which is able to "look inside"
-them when building an in-memory representation of a ``struct``'s layout.
-Note that the same is not true of a ``RefCell`` or ``Mutex``.
+to a Rust mutable reference, and use a shared reference instead. The
+``qemu_api::cell`` module provides wrappers that can be used to tell the
+Rust compiler about interior mutability, and optionally to enforce locking
+rules for the "Big QEMU Lock". In the future, similar cell types might
+also be provided for ``AioContext``-based locking as well.
-In the future, similar cell types might also be provided for ``AioContext``-based
-locking as well.
+In particular, device code will usually rely on the ``BqlRefCell`` and
+``BqlCell`` type to ensure that data is accessed correctly under the
+"Big QEMU Lock". These cell types are also known to the ``vmstate``
+crate, which is able to "look inside" them when building an in-memory
+representation of a ``struct``'s layout. Note that the same is not true
+of a ``RefCell`` or ``Mutex``.
+
+Bindings code instead will usually use the ``Opaque`` type, which hides
+the contents of the underlying struct and can be easily converted to
+a raw pointer, for use in calls to C functions. It can be used for
+example as follows::
+
+ #[repr(transparent)]
+ #[derive(Debug)]
+ pub struct Object(Opaque<bindings::Object>);
+
+The bindings will then manually check for the big QEMU lock with
+assertions, which allows the wrapper to be declared thread-safe::
+
+ unsafe impl Send for Object {}
+ unsafe impl Sync for Object {}
Writing bindings to C code
''''''''''''''''''''''''''
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index eae4e2ce786..2889abb868e 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -27,7 +27,7 @@
// IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
-//! BQL-protected mutable containers.
+//! QEMU-specific mutable containers
//!
//! Rust memory safety is based on this rule: Given an object `T`, it is only
//! possible to have one of the following:
@@ -43,8 +43,10 @@
//! usually have their pointer shared with the "outside world very early in
//! their lifetime", for example when they create their
//! [`MemoryRegion`s](crate::bindings::MemoryRegion). Therefore, individual
-//! parts of a device must be made mutable in a controlled manner through the
-//! use of cell types.
+//! parts of a device must be made mutable in a controlled manner; this module
+//! provides the tools to do so.
+//!
+//! ## Cell types
//!
//! [`BqlCell<T>`] and [`BqlRefCell<T>`] allow doing this via the Big QEMU Lock.
//! While they are essentially the same single-threaded primitives that are
@@ -71,7 +73,7 @@
//! QEMU device implementations is usually incorrect and can lead to
//! thread-safety issues.
//!
-//! ## `BqlCell<T>`
+//! ### `BqlCell<T>`
//!
//! [`BqlCell<T>`] implements interior mutability by moving values in and out of
//! the cell. That is, an `&mut T` to the inner value can never be obtained as
@@ -91,7 +93,7 @@
//! - [`set`](BqlCell::set): this method replaces the interior value,
//! dropping the replaced value.
//!
-//! ## `BqlRefCell<T>`
+//! ### `BqlRefCell<T>`
//!
//! [`BqlRefCell<T>`] uses Rust's lifetimes to implement "dynamic borrowing", a
//! process whereby one can claim temporary, exclusive, mutable access to the
@@ -111,13 +113,82 @@
//! 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.
+//!
+//! ## Opaque wrappers
+//!
+//! The cell types from the previous section are useful at the boundaries
+//! of code that requires interior mutability. When writing glue code that
+//! interacts directly with C structs, however, it is useful to operate
+//! at a lower level.
+//!
+//! C functions often violate Rust's fundamental assumptions about memory
+//! safety by modifying memory even if it is shared. Furthermore, C structs
+//! often start their life uninitialized and may be populated lazily.
+//!
+//! For this reason, this module provides the [`Opaque<T>`] type to opt out
+//! of Rust's usual guarantees about the wrapped type. Access to the wrapped
+//! value is always through raw pointers, obtained via methods like
+//! [`as_mut_ptr()`](Opaque::as_mut_ptr) and [`as_ptr()`](Opaque::as_ptr). These
+//! pointers can then be passed to C functions or dereferenced; both actions
+//! require `unsafe` blocks, making it clear where safety guarantees must be
+//! manually verified. For example
+//!
+//! ```ignore
+//! unsafe {
+//! let state = Opaque::<MyStruct>::uninit();
+//! qemu_struct_init(state.as_mut_ptr());
+//! }
+//! ```
+//!
+//! [`Opaque<T>`] will usually be wrapped one level further, so that
+//! bridge methods can be added to the wrapper:
+//!
+//! ```ignore
+//! pub struct MyStruct(Opaque<bindings::MyStruct>);
+//!
+//! impl MyStruct {
+//! fn new() -> Pin<Box<MyStruct>> {
+//! let result = Box::pin(unsafe { Opaque::uninit() });
+//! unsafe { qemu_struct_init(result.as_mut_ptr()) };
+//! result
+//! }
+//! }
+//! ```
+//!
+//! This pattern of wrapping bindgen-generated types in [`Opaque<T>`] provides
+//! several advantages:
+//!
+//! * The choice of traits to be implemented is not limited by the
+//! bindgen-generated code. For example, [`Drop`] can be added without
+//! disabling [`Copy`] on the underlying bindgen type
+//!
+//! * [`Send`] and [`Sync`] implementations can be controlled by the wrapper
+//! type rather than being automatically derived from the C struct's layout
+//!
+//! * Methods can be implemented in a separate crate from the bindgen-generated
+//! bindings
+//!
+//! * [`Debug`](std::fmt::Debug) and [`Display`](std::fmt::Display)
+//! implementations can be customized to be more readable than the raw C
+//! struct representation
+//!
+//! The [`Opaque<T>`] type does not include BQL validation; it is possible to
+//! assert in the code that the right lock is taken, to use it together
+//! with a custom lock guard type, or to let C code take the lock, as
+//! appropriate. It is also possible to use it with non-thread-safe
+//! types, since by default (unlike [`BqlCell`] and [`BqlRefCell`]
+//! it is neither `Sync` nor `Send`.
+//!
+//! While [`Opaque<T>`] is necessary for C interop, it should be used sparingly
+//! and only at FFI boundaries. For QEMU-specific types that need interior
+//! mutability, prefer [`BqlCell`] or [`BqlRefCell`].
use std::{
cell::{Cell, UnsafeCell},
cmp::Ordering,
fmt,
- marker::PhantomData,
- mem,
+ marker::{PhantomData, PhantomPinned},
+ mem::{self, MaybeUninit},
ops::{Deref, DerefMut},
ptr::NonNull,
};
@@ -840,3 +911,122 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
(**self).fmt(f)
}
}
+
+/// Stores an opaque value that is shared with C code.
+///
+/// Often, C structs can changed when calling a C function even if they are
+/// behind a shared Rust reference, or they can be initialized lazily and have
+/// invalid bit patterns (e.g. `3` for a [`bool`]). This goes against Rust's
+/// strict aliasing rules, which normally prevent mutation through shared
+/// references.
+///
+/// Wrapping the struct with `Opaque<T>` ensures that the Rust compiler does not
+/// assume the usual constraints that Rust structs require, and allows using
+/// shared references on the Rust side.
+///
+/// `Opaque<T>` is `#[repr(transparent)]`, so that it matches the memory layout
+/// of `T`.
+#[repr(transparent)]
+pub struct Opaque<T> {
+ value: UnsafeCell<MaybeUninit<T>>,
+ // PhantomPinned also allows multiple references to the `Opaque<T>`, i.e.
+ // one `&mut Opaque<T>` can coexist with a `&mut T` or any number of `&T`;
+ // see https://docs.rs/pinned-aliasable/latest/pinned_aliasable/.
+ _pin: PhantomPinned,
+}
+
+impl<T> Opaque<T> {
+ /// Creates a new shared reference from a C pointer
+ ///
+ /// # Safety
+ ///
+ /// The pointer must be valid, though it need not point to a valid value.
+ pub unsafe fn from_raw<'a>(ptr: *mut T) -> &'a Self {
+ let ptr = NonNull::new(ptr).unwrap().cast::<Self>();
+ // SAFETY: Self is a transparent wrapper over T
+ unsafe { ptr.as_ref() }
+ }
+
+ /// Creates a new opaque object with uninitialized contents.
+ ///
+ /// # Safety
+ ///
+ /// Ultimately the pointer to the returned value will be dereferenced
+ /// in another `unsafe` block, for example when passing it to a C function,
+ /// but the functions containing the dereference are usually safe. The
+ /// value returned from `uninit()` must be initialized and pinned before
+ /// calling them.
+ #[allow(clippy::missing_const_for_fn)]
+ pub unsafe fn uninit() -> Self {
+ Self {
+ value: UnsafeCell::new(MaybeUninit::uninit()),
+ _pin: PhantomPinned,
+ }
+ }
+
+ /// Creates a new opaque object with zeroed contents.
+ ///
+ /// # Safety
+ ///
+ /// Ultimately the pointer to the returned value will be dereferenced
+ /// in another `unsafe` block, for example when passing it to a C function,
+ /// but the functions containing the dereference are usually safe. The
+ /// value returned from `uninit()` must be pinned (and possibly initialized)
+ /// before calling them.
+ #[allow(clippy::missing_const_for_fn)]
+ pub unsafe fn zeroed() -> Self {
+ Self {
+ value: UnsafeCell::new(MaybeUninit::zeroed()),
+ _pin: PhantomPinned,
+ }
+ }
+
+ /// Returns a raw mutable pointer to the opaque data.
+ pub const fn as_mut_ptr(&self) -> *mut T {
+ UnsafeCell::get(&self.value).cast()
+ }
+
+ /// Returns a raw pointer to the opaque data.
+ pub const fn as_ptr(&self) -> *const T {
+ self.as_mut_ptr() as *const _
+ }
+
+ /// Returns a raw pointer to the opaque data that can be passed to a
+ /// C function as `void *`.
+ pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
+ UnsafeCell::get(&self.value).cast()
+ }
+
+ /// Converts a raw pointer to the wrapped type.
+ pub const fn raw_get(slot: *mut Self) -> *mut T {
+ // Compare with Linux's raw_get method, which goes through an UnsafeCell
+ // because it takes a *const Self instead.
+ slot.cast()
+ }
+}
+
+impl<T> fmt::Debug for Opaque<T> {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ let mut name: String = "Opaque<".to_string();
+ name += std::any::type_name::<T>();
+ name += ">";
+ f.debug_tuple(&name).field(&self.as_ptr()).finish()
+ }
+}
+
+impl<T: Default> Opaque<T> {
+ /// Creates a new opaque object with default contents.
+ ///
+ /// # Safety
+ ///
+ /// Ultimately the pointer to the returned value will be dereferenced
+ /// in another `unsafe` block, for example when passing it to a C function,
+ /// but the functions containing the dereference are usually safe. The
+ /// value returned from `uninit()` must be pinned before calling them.
+ pub unsafe fn new() -> Self {
+ Self {
+ value: UnsafeCell::new(MaybeUninit::new(T::default())),
+ _pin: PhantomPinned,
+ }
+ }
+}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 04/25] rust: qemu_api_macros: add Wrapper derive macro
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (2 preceding siblings ...)
2025-03-09 10:30 ` [PULL 03/25] rust: cell: add wrapper for FFI types Paolo Bonzini
@ 2025-03-09 10:30 ` Paolo Bonzini
2025-03-09 10:30 ` [PULL 05/25] rust: vmstate: add std::pin::Pin as transparent wrapper Paolo Bonzini
` (20 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Add a derive macro that makes it easy to peel off all the layers of
specialness (UnsafeCell, MaybeUninit, etc.) and just get a pointer
to the wrapped type; and likewise add them back starting from a
*mut.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/devel/rust.rst | 8 +--
rust/qemu-api-macros/src/lib.rs | 90 ++++++++++++++++++++++++++++++++-
rust/qemu-api/meson.build | 7 +--
rust/qemu-api/src/cell.rs | 45 +++++++++++++++++
4 files changed, 141 insertions(+), 9 deletions(-)
diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 784c3e40bdc..88bdec1eb28 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -315,11 +315,13 @@ a raw pointer, for use in calls to C functions. It can be used for
example as follows::
#[repr(transparent)]
- #[derive(Debug)]
+ #[derive(Debug, qemu_api_macros::Wrapper)]
pub struct Object(Opaque<bindings::Object>);
-The bindings will then manually check for the big QEMU lock with
-assertions, which allows the wrapper to be declared thread-safe::
+where the special ``derive`` macro provides useful methods such as
+``from_raw``, ``as_ptr`, ``as_mut_ptr`` and ``raw_get``. The bindings will
+then manually check for the big QEMU lock with assertions, which allows
+the wrapper to be declared thread-safe::
unsafe impl Send for Object {}
unsafe impl Sync for Object {}
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 7ec218202f4..eda0d46d122 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -6,7 +6,7 @@
use quote::quote;
use syn::{
parse_macro_input, parse_quote, punctuated::Punctuated, spanned::Spanned, token::Comma, Data,
- DeriveInput, Field, Fields, Ident, Meta, Path, Token, Type, Variant, Visibility,
+ DeriveInput, Field, Fields, FieldsUnnamed, Ident, Meta, Path, Token, Type, Variant, Visibility,
};
mod utils;
@@ -33,6 +33,35 @@ fn get_fields<'a>(
}
}
+fn get_unnamed_field<'a>(input: &'a DeriveInput, msg: &str) -> Result<&'a Field, MacroError> {
+ if let Data::Struct(s) = &input.data {
+ let unnamed = match &s.fields {
+ Fields::Unnamed(FieldsUnnamed {
+ unnamed: ref fields,
+ ..
+ }) => fields,
+ _ => {
+ return Err(MacroError::Message(
+ format!("Tuple struct required for {}", msg),
+ s.fields.span(),
+ ))
+ }
+ };
+ if unnamed.len() != 1 {
+ return Err(MacroError::Message(
+ format!("A single field is required for {}", msg),
+ s.fields.span(),
+ ));
+ }
+ Ok(&unnamed[0])
+ } else {
+ Err(MacroError::Message(
+ format!("Struct required for {}", msg),
+ input.ident.span(),
+ ))
+ }
+}
+
fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), MacroError> {
let expected = parse_quote! { #[repr(C)] };
@@ -46,6 +75,19 @@ fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), MacroError> {
}
}
+fn is_transparent_repr(input: &DeriveInput, msg: &str) -> Result<(), MacroError> {
+ let expected = parse_quote! { #[repr(transparent)] };
+
+ if input.attrs.iter().any(|attr| attr == &expected) {
+ Ok(())
+ } else {
+ Err(MacroError::Message(
+ format!("#[repr(transparent)] required for {}", msg),
+ input.ident.span(),
+ ))
+ }
+}
+
fn derive_object_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
is_c_repr(&input, "#[derive(Object)]")?;
@@ -72,6 +114,52 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
TokenStream::from(expanded)
}
+fn derive_opaque_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
+ is_transparent_repr(&input, "#[derive(Wrapper)]")?;
+
+ let name = &input.ident;
+ let field = &get_unnamed_field(&input, "#[derive(Wrapper)]")?;
+ let typ = &field.ty;
+
+ // TODO: how to add "::qemu_api"? For now, this is only used in the
+ // qemu_api crate so it's not a problem.
+ Ok(quote! {
+ unsafe impl crate::cell::Wrapper for #name {
+ type Wrapped = <#typ as crate::cell::Wrapper>::Wrapped;
+ }
+ impl #name {
+ pub unsafe fn from_raw<'a>(ptr: *mut <Self as crate::cell::Wrapper>::Wrapped) -> &'a Self {
+ let ptr = ::std::ptr::NonNull::new(ptr).unwrap().cast::<Self>();
+ unsafe { ptr.as_ref() }
+ }
+
+ pub const fn as_mut_ptr(&self) -> *mut <Self as crate::cell::Wrapper>::Wrapped {
+ self.0.as_mut_ptr()
+ }
+
+ pub const fn as_ptr(&self) -> *const <Self as crate::cell::Wrapper>::Wrapped {
+ self.0.as_ptr()
+ }
+
+ pub const fn as_void_ptr(&self) -> *mut ::core::ffi::c_void {
+ self.0.as_void_ptr()
+ }
+
+ pub const fn raw_get(slot: *mut Self) -> *mut <Self as crate::cell::Wrapper>::Wrapped {
+ slot.cast()
+ }
+ }
+ })
+}
+
+#[proc_macro_derive(Wrapper)]
+pub fn derive_opaque(input: TokenStream) -> TokenStream {
+ let input = parse_macro_input!(input as DeriveInput);
+ let expanded = derive_opaque_or_error(input).unwrap_or_else(Into::into);
+
+ TokenStream::from(expanded)
+}
+
#[rustfmt::skip::macros(quote)]
fn derive_offsets_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
is_c_repr(&input, "#[derive(offsets)]")?;
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index bcf1cf780f3..6e52c4bad74 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -42,16 +42,13 @@ _qemu_api_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: _qemu_api_cfg,
- dependencies: libc_dep,
+ dependencies: [libc_dep, qemu_api_macros],
)
rust.test('rust-qemu-api-tests', _qemu_api_rs,
suite: ['unit', 'rust'])
-qemu_api = declare_dependency(
- link_with: _qemu_api_rs,
- dependencies: qemu_api_macros,
-)
+qemu_api = declare_dependency(link_with: _qemu_api_rs)
# Rust executables do not support objects, so add an intermediate step.
rust_qemu_api_objs = static_library(
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index 2889abb868e..448638e8967 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -1030,3 +1030,48 @@ pub unsafe fn new() -> Self {
}
}
}
+
+/// Annotates [`Self`] as a transparent wrapper for another type.
+///
+/// Usually defined via the [`qemu_api_macros::Wrapper`] derive macro.
+///
+/// # Examples
+///
+/// ```
+/// # use std::mem::ManuallyDrop;
+/// # use qemu_api::cell::Wrapper;
+/// #[repr(transparent)]
+/// pub struct Example {
+/// inner: ManuallyDrop<String>,
+/// }
+///
+/// unsafe impl Wrapper for Example {
+/// type Wrapped = String;
+/// }
+/// ```
+///
+/// # Safety
+///
+/// `Self` must be a `#[repr(transparent)]` wrapper for the `Wrapped` type,
+/// whether directly or indirectly.
+///
+/// # Methods
+///
+/// By convention, types that implement Wrapper also implement the following
+/// methods:
+///
+/// ```ignore
+/// pub const unsafe fn from_raw<'a>(value: *mut Self::Wrapped) -> &'a Self;
+/// pub const unsafe fn as_mut_ptr(&self) -> *mut Self::Wrapped;
+/// pub const unsafe fn as_ptr(&self) -> *const Self::Wrapped;
+/// pub const unsafe fn raw_get(slot: *mut Self) -> *const Self::Wrapped;
+/// ```
+///
+/// They are not defined here to allow them to be `const`.
+pub unsafe trait Wrapper {
+ type Wrapped;
+}
+
+unsafe impl<T> Wrapper for Opaque<T> {
+ type Wrapped = T;
+}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 05/25] rust: vmstate: add std::pin::Pin as transparent wrapper
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (3 preceding siblings ...)
2025-03-09 10:30 ` [PULL 04/25] rust: qemu_api_macros: add Wrapper derive macro Paolo Bonzini
@ 2025-03-09 10:30 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 06/25] rust: hpet: embed Timer without the Option and Box indirection Paolo Bonzini
` (19 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:30 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/vmstate.rs | 1 +
1 file changed, 1 insertion(+)
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 24a4dc81e7f..1e7ba531e2a 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -330,6 +330,7 @@ unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
impl_vmstate_transparent!(std::cell::Cell<T> where T: VMState);
impl_vmstate_transparent!(std::cell::UnsafeCell<T> where T: VMState);
+impl_vmstate_transparent!(std::pin::Pin<T> where T: VMState);
impl_vmstate_transparent!(crate::cell::BqlCell<T> where T: VMState);
impl_vmstate_transparent!(crate::cell::BqlRefCell<T> where T: VMState);
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 06/25] rust: hpet: embed Timer without the Option and Box indirection
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (4 preceding siblings ...)
2025-03-09 10:30 ` [PULL 05/25] rust: vmstate: add std::pin::Pin as transparent wrapper Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 07/25] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements Paolo Bonzini
` (18 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
This simplifies things for migration, since Option<Box<QEMUTimer>> does not
implement VMState.
This also shows a soundness issue because Timer::new() will leave a NULL
timer list pointer, which can then be dereferenced by Timer::modify(). It
will be fixed shortly.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/hpet.rs | 59 ++++++++++++++++------------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index be27eb0eff4..02c81ae048f 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -151,14 +151,14 @@ fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
/// HPET Timer Abstraction
#[repr(C)]
-#[derive(Debug, Default, qemu_api_macros::offsets)]
+#[derive(Debug, qemu_api_macros::offsets)]
pub struct HPETTimer {
/// timer N index within the timer block (`HPETState`)
#[doc(alias = "tn")]
index: usize,
- qemu_timer: Option<Box<Timer>>,
+ qemu_timer: Timer,
/// timer block abstraction containing this timer
- state: Option<NonNull<HPETState>>,
+ state: NonNull<HPETState>,
// Memory-mapped, software visible timer registers
/// Timer N Configuration and Capability Register
@@ -181,32 +181,34 @@ pub struct HPETTimer {
}
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(&mut self, index: usize, state: &HPETState) {
+ *self = HPETTimer {
+ index,
+ qemu_timer: Timer::new(),
+ state: NonNull::new(state as *const _ as *mut _).unwrap(),
+ config: 0,
+ cmp: 0,
+ fsb: 0,
+ cmp64: 0,
+ period: 0,
+ wrap_flag: 0,
+ last: 0,
+ };
- fn init_timer_with_state(&mut self) {
- self.qemu_timer = Some(Box::new({
- let mut t = Timer::new();
- t.init_full(
- None,
- CLOCK_VIRTUAL,
- Timer::NS,
- 0,
- timer_handler,
- &self.get_state().timers[self.index],
- );
- t
- }));
+ self.qemu_timer.init_full(
+ None,
+ CLOCK_VIRTUAL,
+ Timer::NS,
+ 0,
+ timer_handler,
+ &state.timers[self.index],
+ )
}
fn get_state(&self) -> &HPETState {
// SAFETY:
// the pointer is convertible to a reference
- unsafe { self.state.unwrap().as_ref() }
+ unsafe { self.state.as_ref() }
}
fn is_int_active(&self) -> bool {
@@ -330,7 +332,7 @@ fn arm_timer(&mut self, tick: u64) {
}
self.last = ns;
- self.qemu_timer.as_ref().unwrap().modify(self.last);
+ self.qemu_timer.modify(self.last);
}
fn set_timer(&mut self) {
@@ -353,7 +355,7 @@ fn set_timer(&mut self) {
fn del_timer(&mut self) {
// Just remove the timer from the timer_list without destroying
// this timer instance.
- self.qemu_timer.as_ref().unwrap().delete();
+ self.qemu_timer.delete();
if self.is_int_active() {
// For level-triggered interrupt, this leaves interrupt status
@@ -581,13 +583,8 @@ fn handle_legacy_irq(&self, irq: u32, level: u32) {
}
fn init_timer(&self) {
- let raw_ptr: *mut HPETState = self as *const HPETState as *mut HPETState;
-
for (index, timer) in self.timers.iter().enumerate() {
- timer
- .borrow_mut()
- .init(index, raw_ptr)
- .init_timer_with_state();
+ timer.borrow_mut().init(index, self);
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 07/25] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (5 preceding siblings ...)
2025-03-09 10:31 ` [PULL 06/25] rust: hpet: embed Timer without the Option and Box indirection Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 08/25] rust: irq: wrap IRQState with Opaque<> Paolo Bonzini
` (17 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Timers must be pinned in memory, because modify() stores a pointer to them
in the TimerList. To express this requirement, change init_full() to take
a pinned reference. Because the only way to obtain a Timer is through
Timer::new(), which is unsafe, modify() can assume that the timer it got
was later initialized; and because the initialization takes a Pin<&mut
Timer> modify() can assume that the timer is pinned. In the future the
pinning requirement will be expressed through the pin_init crate instead.
Note that Timer is a bit different from other users of Opaque, in that
it is created in Rust code rather than C code. This is why it has to
use the unsafe constructors provided by Opaque; and in fact Timer::new()
is also unsafe, because it leaves it to the caller to invoke init_full()
before modify(). Without a call to init_full(), modify() will cause a
NULL pointer dereference.
An alternative could be to combine new() + init_full() by returning a
pinned box; however, using a reference makes it easier to express
the requirement that the opaque outlives the timer.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
meson.build | 7 -----
rust/hw/timer/hpet/src/hpet.rs | 10 ++++++--
rust/qemu-api/src/timer.rs | 47 ++++++++++++++++++++++++++--------
3 files changed, 44 insertions(+), 20 deletions(-)
diff --git a/meson.build b/meson.build
index 67ec2b78319..6da4eb317c2 100644
--- a/meson.build
+++ b/meson.build
@@ -4100,13 +4100,6 @@ if have_rust
foreach enum : c_bitfields
bindgen_args += ['--bitfield-enum', enum]
endforeach
- c_nocopy = [
- 'QEMUTimer',
- ]
- # Used to customize Drop trait
- foreach struct : c_nocopy
- bindgen_args += ['--no-copy', struct]
- endforeach
# TODO: Remove this comment when the clang/libclang mismatch issue is solved.
#
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 02c81ae048f..3d3d6ef8eec 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -4,6 +4,7 @@
use std::{
ffi::CStr,
+ pin::Pin,
ptr::{addr_of_mut, null_mut, NonNull},
slice::from_ref,
};
@@ -184,7 +185,9 @@ impl HPETTimer {
fn init(&mut self, index: usize, state: &HPETState) {
*self = HPETTimer {
index,
- qemu_timer: Timer::new(),
+ // SAFETY: the HPETTimer will only be used after the timer
+ // is initialized below.
+ qemu_timer: unsafe { Timer::new() },
state: NonNull::new(state as *const _ as *mut _).unwrap(),
config: 0,
cmp: 0,
@@ -195,7 +198,10 @@ fn init(&mut self, index: usize, state: &HPETState) {
last: 0,
};
- self.qemu_timer.init_full(
+ // SAFETY: HPETTimer is only used as part of HPETState, which is
+ // always pinned.
+ let qemu_timer = unsafe { Pin::new_unchecked(&mut self.qemu_timer) };
+ qemu_timer.init_full(
None,
CLOCK_VIRTUAL,
Timer::NS,
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
index a593538917a..f0b04ef95d7 100644
--- a/rust/qemu-api/src/timer.rs
+++ b/rust/qemu-api/src/timer.rs
@@ -2,31 +2,51 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::os::raw::{c_int, c_void};
+use std::{
+ os::raw::{c_int, c_void},
+ pin::Pin,
+};
use crate::{
bindings::{self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType},
callbacks::FnCall,
+ cell::Opaque,
};
-pub type Timer = bindings::QEMUTimer;
-pub type TimerListGroup = bindings::QEMUTimerListGroup;
+/// A safe wrapper around [`bindings::QEMUTimer`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Timer(Opaque<bindings::QEMUTimer>);
+
+unsafe impl Send for Timer {}
+unsafe impl Sync for Timer {}
+
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct TimerListGroup(Opaque<bindings::QEMUTimerListGroup>);
+
+unsafe impl Send for TimerListGroup {}
+unsafe impl Sync for TimerListGroup {}
impl Timer {
pub const MS: u32 = bindings::SCALE_MS;
pub const US: u32 = bindings::SCALE_US;
pub const NS: u32 = bindings::SCALE_NS;
- pub fn new() -> Self {
- Default::default()
- }
-
- const fn as_mut_ptr(&self) -> *mut Self {
- self as *const Timer as *mut _
+ /// Create a `Timer` struct without initializing it.
+ ///
+ /// # Safety
+ ///
+ /// The timer must be initialized before it is armed with
+ /// [`modify`](Self::modify).
+ pub unsafe fn new() -> Self {
+ // SAFETY: requirements relayed to callers of Timer::new
+ Self(unsafe { Opaque::zeroed() })
}
+ /// Create a new timer with the given attributes.
pub fn init_full<'timer, 'opaque: 'timer, T, F>(
- &'timer mut self,
+ self: Pin<&'timer mut Self>,
timer_list_group: Option<&TimerListGroup>,
clk_type: ClockType,
scale: u32,
@@ -51,7 +71,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(
- self,
+ self.as_mut_ptr(),
if let Some(g) = timer_list_group {
g as *const TimerListGroup as *mut _
} else {
@@ -67,14 +87,19 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
}
pub fn modify(&self, expire_time: u64) {
+ // SAFETY: the only way to obtain a Timer safely is via methods that
+ // take a Pin<&mut Self>, therefore the timer is pinned
unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
}
pub fn delete(&self) {
+ // SAFETY: the only way to obtain a Timer safely is via methods that
+ // take a Pin<&mut Self>, therefore the timer is pinned
unsafe { timer_del(self.as_mut_ptr()) }
}
}
+// FIXME: use something like PinnedDrop from the pinned_init crate
impl Drop for Timer {
fn drop(&mut self) {
self.delete()
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 08/25] rust: irq: wrap IRQState with Opaque<>
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (6 preceding siblings ...)
2025-03-09 10:31 ` [PULL 07/25] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 09/25] rust: qom: wrap Object " Paolo Bonzini
` (16 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/irq.rs | 15 ++++++++++-----
rust/qemu-api/src/sysbus.rs | 1 +
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs
index 34c19263c23..1222d4fde30 100644
--- a/rust/qemu-api/src/irq.rs
+++ b/rust/qemu-api/src/irq.rs
@@ -8,10 +8,16 @@
use crate::{
bindings::{self, qemu_set_irq},
+ cell::Opaque,
prelude::*,
qom::ObjectClass,
};
+/// An opaque wrapper around [`bindings::IRQState`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct IRQState(Opaque<bindings::IRQState>);
+
/// Interrupt sources are used by devices to pass changes to a value (typically
/// a boolean). The interrupt sink is usually an interrupt controller or
/// GPIO controller.
@@ -21,8 +27,7 @@
/// method sends a `true` value to the sink. If the guest has to see a
/// different polarity, that change is performed by the board between the
/// device and the interrupt controller.
-pub type IRQState = bindings::IRQState;
-
+///
/// Interrupts are implemented as a pointer to the interrupt "sink", which has
/// type [`IRQState`]. A device exposes its source as a QOM link property using
/// a function such as [`SysBusDeviceMethods::init_irq`], and
@@ -40,7 +45,7 @@ pub struct InterruptSource<T = bool>
where
c_int: From<T>,
{
- cell: BqlCell<*mut IRQState>,
+ cell: BqlCell<*mut bindings::IRQState>,
_marker: PhantomData<T>,
}
@@ -79,11 +84,11 @@ pub fn set(&self, level: T) {
}
}
- pub(crate) const fn as_ptr(&self) -> *mut *mut IRQState {
+ pub(crate) const fn as_ptr(&self) -> *mut *mut bindings::IRQState {
self.cell.as_ptr()
}
- pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut IRQState {
+ pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut bindings::IRQState {
assert!(!slice.is_empty());
slice[0].as_ptr()
}
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 04821a2b9b3..48803a655f9 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -79,6 +79,7 @@ fn mmio_map(&self, id: u32, addr: u64) {
fn connect_irq(&self, id: u32, irq: &Owned<IRQState>) {
assert!(bql_locked());
let id: i32 = id.try_into().unwrap();
+ let irq: &IRQState = irq;
unsafe {
bindings::sysbus_connect_irq(self.as_mut_ptr(), id, irq.as_mut_ptr());
}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 09/25] rust: qom: wrap Object with Opaque<>
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (7 preceding siblings ...)
2025-03-09 10:31 ` [PULL 08/25] rust: irq: wrap IRQState with Opaque<> Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 10/25] rust: qdev: wrap Clock and DeviceState " Paolo Bonzini
` (15 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/memory.rs | 2 +-
rust/qemu-api/src/qdev.rs | 6 +++---
rust/qemu-api/src/qom.rs | 35 ++++++++++++++++++++++-------------
4 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index d2868639ff6..be6dd68c09c 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -46,9 +46,6 @@ unsafe impl Sync for MemoryRegion {}
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
-unsafe impl Send for Object {}
-unsafe impl Sync for Object {}
-
unsafe impl Send for SysBusDevice {}
unsafe impl Sync for SysBusDevice {}
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index 682951ab44e..713c494ca2e 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -157,7 +157,7 @@ unsafe fn do_init_io(
let cstr = CString::new(name).unwrap();
memory_region_init_io(
slot,
- owner.cast::<Object>(),
+ owner.cast::<bindings::Object>(),
ops,
owner.cast::<c_void>(),
cstr.as_ptr(),
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index c136457090c..1a4d1f38762 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -52,7 +52,7 @@ pub trait ResettablePhasesImpl {
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
unsafe extern "C" fn rust_resettable_enter_fn<T: ResettablePhasesImpl>(
- obj: *mut Object,
+ obj: *mut bindings::Object,
typ: ResetType,
) {
let state = NonNull::new(obj).unwrap().cast::<T>();
@@ -65,7 +65,7 @@ pub trait ResettablePhasesImpl {
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
unsafe extern "C" fn rust_resettable_hold_fn<T: ResettablePhasesImpl>(
- obj: *mut Object,
+ obj: *mut bindings::Object,
typ: ResetType,
) {
let state = NonNull::new(obj).unwrap().cast::<T>();
@@ -78,7 +78,7 @@ pub trait ResettablePhasesImpl {
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
unsafe extern "C" fn rust_resettable_exit_fn<T: ResettablePhasesImpl>(
- obj: *mut Object,
+ obj: *mut bindings::Object,
typ: ResetType,
) {
let state = NonNull::new(obj).unwrap().cast::<T>();
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 5488643a2fd..2defbd23516 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -101,16 +101,24 @@
ptr::NonNull,
};
-pub use bindings::{Object, ObjectClass};
+pub use bindings::ObjectClass;
use crate::{
bindings::{
self, object_class_dynamic_cast, object_dynamic_cast, object_get_class,
object_get_typename, object_new, object_ref, object_unref, TypeInfo,
},
- cell::bql_locked,
+ cell::{bql_locked, Opaque},
};
+/// A safe wrapper around [`bindings::Object`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Object(Opaque<bindings::Object>);
+
+unsafe impl Send for Object {}
+unsafe impl Sync for Object {}
+
/// Marker trait: `Self` can be statically upcasted to `P` (i.e. `P` is a direct
/// or indirect parent of `Self`).
///
@@ -199,7 +207,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
}
}
-unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) {
+unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut bindings::Object) {
let mut state = NonNull::new(obj).unwrap().cast::<T>();
// SAFETY: obj is an instance of T, since rust_instance_init<T>
// is called from QOM core as the instance_init function
@@ -209,7 +217,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
}
}
-unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut Object) {
+unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut bindings::Object) {
let state = NonNull::new(obj).unwrap().cast::<T>();
// SAFETY: obj is an instance of T, since rust_instance_post_init<T>
// is called from QOM core as the instance_post_init function
@@ -230,7 +238,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
<T as ObjectImpl>::CLASS_INIT(unsafe { klass.as_mut() })
}
-unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) {
+unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut bindings::Object) {
// SAFETY: obj is an instance of T, since drop_object<T> is called
// from the QOM core function object_deinit() as the instance_finalize
// function for class T. Note that while object_deinit() will drop the
@@ -280,14 +288,14 @@ pub unsafe trait ObjectType: Sized {
/// Return the receiver as an Object. This is always safe, even
/// if this type represents an interface.
fn as_object(&self) -> &Object {
- unsafe { &*self.as_object_ptr() }
+ unsafe { &*self.as_ptr().cast() }
}
/// Return the receiver as a const raw pointer to Object.
/// This is preferrable to `as_object_mut_ptr()` if a C
/// function only needs a `const Object *`.
- fn as_object_ptr(&self) -> *const Object {
- self.as_ptr().cast()
+ fn as_object_ptr(&self) -> *const bindings::Object {
+ self.as_object().as_ptr()
}
/// Return the receiver as a mutable raw pointer to Object.
@@ -297,8 +305,8 @@ fn as_object_ptr(&self) -> *const Object {
/// This cast is always safe, but because the result is mutable
/// and the incoming reference is not, this should only be used
/// for calls to C functions, and only if needed.
- unsafe fn as_object_mut_ptr(&self) -> *mut Object {
- self.as_object_ptr() as *mut _
+ unsafe fn as_object_mut_ptr(&self) -> *mut bindings::Object {
+ self.as_object().as_mut_ptr()
}
}
@@ -621,7 +629,7 @@ pub trait ObjectImpl: ObjectType + IsA<Object> {
/// 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.
-unsafe extern "C" fn rust_unparent_fn<T: ObjectImpl>(dev: *mut Object) {
+unsafe extern "C" fn rust_unparent_fn<T: ObjectImpl>(dev: *mut bindings::Object) {
let state = NonNull::new(dev).unwrap().cast::<T>();
T::UNPARENT.unwrap()(unsafe { state.as_ref() });
}
@@ -796,8 +804,9 @@ fn new() -> Owned<Self> {
// SAFETY: the object created by object_new is allocated on
// the heap and has a reference count of 1
unsafe {
- let obj = &*object_new(Self::TYPE_NAME.as_ptr());
- Owned::from_raw(obj.unsafe_cast::<Self>())
+ let raw_obj = object_new(Self::TYPE_NAME.as_ptr());
+ let obj = Object::from_raw(raw_obj).unsafe_cast::<Self>();
+ Owned::from_raw(obj)
}
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 10/25] rust: qdev: wrap Clock and DeviceState with Opaque<>
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (8 preceding siblings ...)
2025-03-09 10:31 ` [PULL 09/25] rust: qom: wrap Object " Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 11/25] rust: hpet: do not access fields of SysBusDevice Paolo Bonzini
` (14 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 6 ----
rust/qemu-api/src/qdev.rs | 68 ++++++++++++++++++++++++-----------
rust/qemu-api/src/vmstate.rs | 2 +-
3 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index be6dd68c09c..6e70a75a0e6 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -34,12 +34,6 @@ unsafe impl Sync for CharBackend {}
unsafe impl Send for Chardev {}
unsafe impl Sync for Chardev {}
-unsafe impl Send for Clock {}
-unsafe impl Sync for Clock {}
-
-unsafe impl Send for DeviceState {}
-unsafe impl Sync for DeviceState {}
-
unsafe impl Send for MemoryRegion {}
unsafe impl Sync for MemoryRegion {}
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 1a4d1f38762..1c4a67b5728 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -10,12 +10,12 @@
ptr::NonNull,
};
-pub use bindings::{Clock, ClockEvent, DeviceClass, DeviceState, Property, ResetType};
+pub use bindings::{ClockEvent, DeviceClass, Property, ResetType};
use crate::{
bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error, ResettableClass},
callbacks::FnCall,
- cell::bql_locked,
+ cell::{bql_locked, Opaque},
chardev::Chardev,
irq::InterruptSource,
prelude::*,
@@ -23,6 +23,22 @@
vmstate::VMStateDescription,
};
+/// A safe wrapper around [`bindings::Clock`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Clock(Opaque<bindings::Clock>);
+
+unsafe impl Send for Clock {}
+unsafe impl Sync for Clock {}
+
+/// A safe wrapper around [`bindings::DeviceState`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct DeviceState(Opaque<bindings::DeviceState>);
+
+unsafe impl Send for DeviceState {}
+unsafe impl Sync for DeviceState {}
+
/// Trait providing the contents of the `ResettablePhases` struct,
/// which is part of the QOM `Resettable` interface.
pub trait ResettablePhasesImpl {
@@ -117,7 +133,10 @@ fn vmsd() -> Option<&'static VMStateDescription> {
/// 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.
-unsafe extern "C" fn rust_realize_fn<T: DeviceImpl>(dev: *mut DeviceState, _errp: *mut *mut Error) {
+unsafe extern "C" fn rust_realize_fn<T: DeviceImpl>(
+ dev: *mut bindings::DeviceState,
+ _errp: *mut *mut Error,
+) {
let state = NonNull::new(dev).unwrap().cast::<T>();
T::REALIZE.unwrap()(unsafe { state.as_ref() });
}
@@ -251,7 +270,7 @@ fn init_clock_in<F: for<'a> FnCall<(&'a Self::Target, ClockEvent)>>(
events: ClockEvent,
) -> Owned<Clock> {
fn do_init_clock_in(
- dev: *mut DeviceState,
+ dev: &DeviceState,
name: &str,
cb: Option<unsafe extern "C" fn(*mut c_void, ClockEvent)>,
events: ClockEvent,
@@ -265,14 +284,15 @@ fn do_init_clock_in(
unsafe {
let cstr = CString::new(name).unwrap();
let clk = bindings::qdev_init_clock_in(
- dev,
+ dev.as_mut_ptr(),
cstr.as_ptr(),
cb,
- dev.cast::<c_void>(),
+ dev.as_void_ptr(),
events.0,
);
- Owned::from(&*clk)
+ let clk: &Clock = Clock::from_raw(clk);
+ Owned::from(clk)
}
}
@@ -289,7 +309,7 @@ fn do_init_clock_in(
None
};
- do_init_clock_in(self.as_mut_ptr(), name, cb, events)
+ do_init_clock_in(self.upcast(), name, cb, events)
}
/// Add an output clock named `name`.
@@ -304,9 +324,10 @@ fn do_init_clock_in(
fn init_clock_out(&self, name: &str) -> Owned<Clock> {
unsafe {
let cstr = CString::new(name).unwrap();
- let clk = bindings::qdev_init_clock_out(self.as_mut_ptr(), cstr.as_ptr());
+ let clk = bindings::qdev_init_clock_out(self.upcast().as_mut_ptr(), cstr.as_ptr());
- Owned::from(&*clk)
+ let clk: &Clock = Clock::from_raw(clk);
+ Owned::from(clk)
}
}
@@ -314,7 +335,11 @@ fn prop_set_chr(&self, propname: &str, chr: &Owned<Chardev>) {
assert!(bql_locked());
let c_propname = CString::new(propname).unwrap();
unsafe {
- bindings::qdev_prop_set_chr(self.as_mut_ptr(), c_propname.as_ptr(), chr.as_mut_ptr());
+ bindings::qdev_prop_set_chr(
+ self.upcast().as_mut_ptr(),
+ c_propname.as_ptr(),
+ chr.as_mut_ptr(),
+ );
}
}
@@ -323,8 +348,17 @@ fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(
num_lines: u32,
_cb: F,
) {
- let _: () = F::ASSERT_IS_SOME;
+ fn do_init_gpio_in(
+ dev: &DeviceState,
+ num_lines: u32,
+ gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int),
+ ) {
+ unsafe {
+ qdev_init_gpio_in(dev.as_mut_ptr(), Some(gpio_in_cb), num_lines as c_int);
+ }
+ }
+ let _: () = F::ASSERT_IS_SOME;
unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
opaque: *mut c_void,
line: c_int,
@@ -337,19 +371,13 @@ fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, 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.as_mut_ptr::<DeviceState>(),
- Some(gpio_in_cb),
- num_lines as c_int,
- );
- }
+ do_init_gpio_in(self.upcast(), num_lines, gpio_in_cb);
}
fn init_gpio_out(&self, pins: &[InterruptSource]) {
unsafe {
qdev_init_gpio_out(
- self.as_mut_ptr::<DeviceState>(),
+ self.upcast().as_mut_ptr(),
InterruptSource::slice_as_ptr(pins),
pins.len() as c_int,
);
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 1e7ba531e2a..f0510ae769d 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -470,7 +470,7 @@ macro_rules! vmstate_clock {
$crate::assert_field_type!(
$struct_name,
$field_name,
- $crate::qom::Owned<$crate::bindings::Clock>
+ $crate::qom::Owned<$crate::qdev::Clock>
);
$crate::offset_of!($struct_name, $field_name)
},
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 11/25] rust: hpet: do not access fields of SysBusDevice
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (9 preceding siblings ...)
2025-03-09 10:31 ` [PULL 10/25] rust: qdev: wrap Clock and DeviceState " Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 12/25] rust: sysbus: wrap SysBusDevice with Opaque<> Paolo Bonzini
` (13 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Fields of SysBusDevice must only be accessed with the BQL taken. Add
a wrapper that verifies that.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/hpet.rs | 4 +---
rust/qemu-api/src/sysbus.rs | 12 ++++++++++++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 3d3d6ef8eec..d989360ede8 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -730,8 +730,6 @@ fn realize(&self) {
}
fn reset_hold(&self, _type: ResetType) {
- let sbd = self.upcast::<SysBusDevice>();
-
for timer in self.timers.iter().take(self.num_timers.get()) {
timer.borrow_mut().reset();
}
@@ -744,7 +742,7 @@ fn reset_hold(&self, _type: ResetType) {
HPETFwConfig::update_hpet_cfg(
self.hpet_id.get(),
self.capability.get() as u32,
- sbd.mmio[0].addr,
+ self.mmio_addr(0).unwrap(),
);
// to document that the RTC lowers its output on reset as well
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 48803a655f9..0790576d446 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -64,6 +64,18 @@ fn init_irq(&self, irq: &InterruptSource) {
}
}
+ // TODO: do we want a type like GuestAddress here?
+ fn mmio_addr(&self, id: u32) -> Option<u64> {
+ assert!(bql_locked());
+ let sbd = self.upcast();
+ let id: usize = id.try_into().unwrap();
+ if sbd.mmio[id].memory.is_null() {
+ None
+ } else {
+ Some(sbd.mmio[id].addr)
+ }
+ }
+
// TODO: do we want a type like GuestAddress here?
fn mmio_map(&self, id: u32, addr: u64) {
assert!(bql_locked());
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 12/25] rust: sysbus: wrap SysBusDevice with Opaque<>
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (10 preceding siblings ...)
2025-03-09 10:31 ` [PULL 11/25] rust: hpet: do not access fields of SysBusDevice Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 13/25] rust: memory: wrap MemoryRegion " Paolo Bonzini
` (12 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/sysbus.rs | 29 +++++++++++++++++++++--------
2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 6e70a75a0e6..b791ca6d87f 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -40,9 +40,6 @@ unsafe impl Sync for MemoryRegion {}
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
-unsafe impl Send for SysBusDevice {}
-unsafe impl Sync for SysBusDevice {}
-
// SAFETY: this is a pure data struct
unsafe impl Send for CoalescedMemoryRange {}
unsafe impl Sync for CoalescedMemoryRange {}
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 0790576d446..e92502a8fe6 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -6,11 +6,11 @@
use std::{ffi::CStr, ptr::addr_of_mut};
-pub use bindings::{SysBusDevice, SysBusDeviceClass};
+pub use bindings::SysBusDeviceClass;
use crate::{
bindings,
- cell::bql_locked,
+ cell::{bql_locked, Opaque},
irq::{IRQState, InterruptSource},
memory::MemoryRegion,
prelude::*,
@@ -18,6 +18,14 @@
qom::Owned,
};
+/// A safe wrapper around [`bindings::SysBusDevice`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct SysBusDevice(Opaque<bindings::SysBusDevice>);
+
+unsafe impl Send for SysBusDevice {}
+unsafe impl Sync for SysBusDevice {}
+
unsafe impl ObjectType for SysBusDevice {
type Class = SysBusDeviceClass;
const TYPE_NAME: &'static CStr =
@@ -49,7 +57,7 @@ pub trait SysBusDeviceMethods: ObjectDeref
fn init_mmio(&self, iomem: &MemoryRegion) {
assert!(bql_locked());
unsafe {
- bindings::sysbus_init_mmio(self.as_mut_ptr(), iomem.as_mut_ptr());
+ bindings::sysbus_init_mmio(self.upcast().as_mut_ptr(), iomem.as_mut_ptr());
}
}
@@ -60,14 +68,16 @@ fn init_mmio(&self, iomem: &MemoryRegion) {
fn init_irq(&self, irq: &InterruptSource) {
assert!(bql_locked());
unsafe {
- bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr());
+ bindings::sysbus_init_irq(self.upcast().as_mut_ptr(), irq.as_ptr());
}
}
// TODO: do we want a type like GuestAddress here?
fn mmio_addr(&self, id: u32) -> Option<u64> {
assert!(bql_locked());
- let sbd = self.upcast();
+ // SAFETY: the BQL ensures that no one else writes to sbd.mmio[], and
+ // the SysBusDevice must be initialized to get an IsA<SysBusDevice>.
+ let sbd = unsafe { *self.upcast().as_ptr() };
let id: usize = id.try_into().unwrap();
if sbd.mmio[id].memory.is_null() {
None
@@ -81,7 +91,7 @@ fn mmio_map(&self, id: u32, addr: u64) {
assert!(bql_locked());
let id: i32 = id.try_into().unwrap();
unsafe {
- bindings::sysbus_mmio_map(self.as_mut_ptr(), id, addr);
+ bindings::sysbus_mmio_map(self.upcast().as_mut_ptr(), id, addr);
}
}
@@ -93,7 +103,7 @@ fn connect_irq(&self, id: u32, irq: &Owned<IRQState>) {
let id: i32 = id.try_into().unwrap();
let irq: &IRQState = irq;
unsafe {
- bindings::sysbus_connect_irq(self.as_mut_ptr(), id, irq.as_mut_ptr());
+ bindings::sysbus_connect_irq(self.upcast().as_mut_ptr(), id, irq.as_mut_ptr());
}
}
@@ -101,7 +111,10 @@ fn sysbus_realize(&self) {
// TODO: return an Error
assert!(bql_locked());
unsafe {
- bindings::sysbus_realize(self.as_mut_ptr(), addr_of_mut!(bindings::error_fatal));
+ bindings::sysbus_realize(
+ self.upcast().as_mut_ptr(),
+ addr_of_mut!(bindings::error_fatal),
+ );
}
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 13/25] rust: memory: wrap MemoryRegion with Opaque<>
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (11 preceding siblings ...)
2025-03-09 10:31 ` [PULL 12/25] rust: sysbus: wrap SysBusDevice with Opaque<> Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 14/25] rust: chardev: wrap Chardev " Paolo Bonzini
` (11 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/memory.rs | 35 +++++++++++++++++++++--------------
2 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index b791ca6d87f..26cc8de0cf2 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -34,9 +34,6 @@ unsafe impl Sync for CharBackend {}
unsafe impl Send for Chardev {}
unsafe impl Sync for Chardev {}
-unsafe impl Send for MemoryRegion {}
-unsafe impl Sync for MemoryRegion {}
-
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index 713c494ca2e..eff9f09fd7f 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -6,9 +6,8 @@
use std::{
ffi::{CStr, CString},
- marker::{PhantomData, PhantomPinned},
+ marker::PhantomData,
os::raw::{c_uint, c_void},
- ptr::addr_of,
};
pub use bindings::{hwaddr, MemTxAttrs};
@@ -16,6 +15,7 @@
use crate::{
bindings::{self, device_endian, memory_region_init_io},
callbacks::FnCall,
+ cell::Opaque,
prelude::*,
zeroable::Zeroable,
};
@@ -132,13 +132,13 @@ fn default() -> Self {
}
}
-/// A safe wrapper around [`bindings::MemoryRegion`]. Compared to the
-/// underlying C struct it is marked as pinned because the QOM tree
-/// contains a pointer to it.
-pub struct MemoryRegion {
- inner: bindings::MemoryRegion,
- _pin: PhantomPinned,
-}
+/// A safe wrapper around [`bindings::MemoryRegion`].
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct MemoryRegion(Opaque<bindings::MemoryRegion>);
+
+unsafe impl Send for MemoryRegion {}
+unsafe impl Sync for MemoryRegion {}
impl MemoryRegion {
// inline to ensure that it is not included in tests, which only
@@ -174,13 +174,20 @@ pub fn init_io<T: IsA<Object>>(
size: u64,
) {
unsafe {
- Self::do_init_io(&mut self.inner, owner.cast::<Object>(), &ops.0, name, size);
+ Self::do_init_io(
+ // self.0.as_mut_ptr() needed because Rust tries to call
+ // ObjectDeref::as_mut_ptr() on "&mut Self", instead of coercing
+ // to "&Self" and then calling MemoryRegion::as_mut_ptr().
+ // Revisit if/when ObjectCastMut is not needed anymore; it is
+ // only used in a couple places for initialization.
+ self.0.as_mut_ptr(),
+ owner.cast::<Object>(),
+ &ops.0,
+ name,
+ size,
+ );
}
}
-
- pub(crate) const fn as_mut_ptr(&self) -> *mut bindings::MemoryRegion {
- addr_of!(self.inner) as *mut _
- }
}
unsafe impl ObjectType for MemoryRegion {
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 14/25] rust: chardev: wrap Chardev with Opaque<>
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (12 preceding siblings ...)
2025-03-09 10:31 ` [PULL 13/25] rust: memory: wrap MemoryRegion " Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 15/25] rust: bindings: remove more unnecessary Send/Sync impls Paolo Bonzini
` (10 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/chardev.rs | 8 ++++++--
rust/qemu-api/src/qdev.rs | 1 +
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 26cc8de0cf2..c3f36108bd5 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -31,9 +31,6 @@ unsafe impl Sync for BusState {}
unsafe impl Send for CharBackend {}
unsafe impl Sync for CharBackend {}
-unsafe impl Send for Chardev {}
-unsafe impl Sync for Chardev {}
-
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
diff --git a/rust/qemu-api/src/chardev.rs b/rust/qemu-api/src/chardev.rs
index 74cfb634e5f..a35b9217e90 100644
--- a/rust/qemu-api/src/chardev.rs
+++ b/rust/qemu-api/src/chardev.rs
@@ -6,9 +6,13 @@
use std::ffi::CStr;
-use crate::{bindings, prelude::*};
+use crate::{bindings, cell::Opaque, prelude::*};
+
+/// A safe wrapper around [`bindings::Chardev`].
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct Chardev(Opaque<bindings::Chardev>);
-pub type Chardev = bindings::Chardev;
pub type ChardevClass = bindings::ChardevClass;
unsafe impl ObjectType for Chardev {
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 1c4a67b5728..18b4a9ba687 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -334,6 +334,7 @@ fn init_clock_out(&self, name: &str) -> Owned<Clock> {
fn prop_set_chr(&self, propname: &str, chr: &Owned<Chardev>) {
assert!(bql_locked());
let c_propname = CString::new(propname).unwrap();
+ let chr: &Chardev = chr;
unsafe {
bindings::qdev_prop_set_chr(
self.upcast().as_mut_ptr(),
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 15/25] rust: bindings: remove more unnecessary Send/Sync impls
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (13 preceding siblings ...)
2025-03-09 10:31 ` [PULL 14/25] rust: chardev: wrap Chardev " Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 16/25] rust: chardev: provide basic bindings to character devices Paolo Bonzini
` (9 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Send and Sync are now implemented on the opaque wrappers. Remove them
from the bindings module, unless the structs are pure data containers
and/or have no C functions defined on them.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index c3f36108bd5..3c1d297581e 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -25,15 +25,11 @@
// SAFETY: these are implemented in C; the bindings need to assert that the
// BQL is taken, either directly or via `BqlCell` and `BqlRefCell`.
-unsafe impl Send for BusState {}
-unsafe impl Sync for BusState {}
-
+// When bindings for character devices are introduced, this can be
+// moved to the Opaque<> wrapper in src/chardev.rs.
unsafe impl Send for CharBackend {}
unsafe impl Sync for CharBackend {}
-unsafe impl Send for ObjectClass {}
-unsafe impl Sync for ObjectClass {}
-
// SAFETY: this is a pure data struct
unsafe impl Send for CoalescedMemoryRange {}
unsafe impl Sync for CoalescedMemoryRange {}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 16/25] rust: chardev: provide basic bindings to character devices
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (14 preceding siblings ...)
2025-03-09 10:31 ` [PULL 15/25] rust: bindings: remove more unnecessary Send/Sync impls Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 17/25] rust: pl011: move register definitions out of lib.rs Paolo Bonzini
` (8 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Most of the character device API is pretty simple, with "0 or -errno"
or "number of bytes or -errno" as the convention for return codes.
Add safe wrappers for the API to the CharBackend bindgen-generated
struct.
The API is not complete, but it covers the parts that are used
by the PL011 device, plus qemu_chr_fe_write which is needed to
implement the standard library Write trait.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/meson.build | 17 ++-
rust/qemu-api/src/chardev.rs | 242 +++++++++++++++++++++++++++++++++-
rust/qemu-api/src/zeroable.rs | 1 +
3 files changed, 255 insertions(+), 5 deletions(-)
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 6e52c4bad74..a3f226ccc2a 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -54,7 +54,19 @@ qemu_api = declare_dependency(link_with: _qemu_api_rs)
rust_qemu_api_objs = static_library(
'rust_qemu_api_objs',
objects: [libqom.extract_all_objects(recursive: false),
- libhwcore.extract_all_objects(recursive: false)])
+ libhwcore.extract_all_objects(recursive: false),
+ libchardev.extract_all_objects(recursive: false),
+ libcrypto.extract_all_objects(recursive: false),
+ libauthz.extract_all_objects(recursive: false),
+ libio.extract_all_objects(recursive: false)])
+rust_qemu_api_deps = declare_dependency(
+ dependencies: [
+ qom_ss.dependencies(),
+ chardev_ss.dependencies(),
+ crypto_ss.dependencies(),
+ authz_ss.dependencies(),
+ io_ss.dependencies()],
+ link_whole: [rust_qemu_api_objs, libqemuutil])
test('rust-qemu-api-integration',
executable(
@@ -63,8 +75,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],
- link_whole: [rust_qemu_api_objs, libqemuutil]),
+ dependencies: [qemu_api, qemu_api_macros, rust_qemu_api_deps]),
args: [
'--test', '--test-threads', '1',
'--format', 'pretty',
diff --git a/rust/qemu-api/src/chardev.rs b/rust/qemu-api/src/chardev.rs
index a35b9217e90..11e6c45afaf 100644
--- a/rust/qemu-api/src/chardev.rs
+++ b/rust/qemu-api/src/chardev.rs
@@ -3,10 +3,28 @@
// SPDX-License-Identifier: GPL-2.0-or-later
//! Bindings for character devices
+//!
+//! Character devices in QEMU can run under the big QEMU lock or in a separate
+//! `GMainContext`. Here we only support the former, because the bindings
+//! enforce that the BQL is taken whenever the functions in [`CharBackend`] are
+//! called.
-use std::ffi::CStr;
+use std::{
+ ffi::CStr,
+ fmt::{self, Debug},
+ io::{self, ErrorKind, Write},
+ marker::PhantomPinned,
+ os::raw::{c_int, c_void},
+ ptr::addr_of_mut,
+ slice,
+};
-use crate::{bindings, cell::Opaque, prelude::*};
+use crate::{
+ bindings,
+ callbacks::FnCall,
+ cell::{BqlRefMut, Opaque},
+ prelude::*,
+};
/// A safe wrapper around [`bindings::Chardev`].
#[repr(transparent)]
@@ -14,6 +32,226 @@
pub struct Chardev(Opaque<bindings::Chardev>);
pub type ChardevClass = bindings::ChardevClass;
+pub type Event = bindings::QEMUChrEvent;
+
+/// A safe wrapper around [`bindings::CharBackend`], denoting the character
+/// back-end that is used for example by a device. Compared to the
+/// underlying C struct it adds BQL protection, and is marked as pinned
+/// because the QOM object ([`bindings::Chardev`]) contains a pointer to
+/// the `CharBackend`.
+pub struct CharBackend {
+ inner: BqlRefCell<bindings::CharBackend>,
+ _pin: PhantomPinned,
+}
+
+impl Write for BqlRefMut<'_, bindings::CharBackend> {
+ fn flush(&mut self) -> io::Result<()> {
+ Ok(())
+ }
+
+ fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+ let chr: &mut bindings::CharBackend = self;
+
+ let len = buf.len().try_into().unwrap();
+ let r = unsafe { bindings::qemu_chr_fe_write(addr_of_mut!(*chr), buf.as_ptr(), len) };
+ errno::into_io_result(r).map(|cnt| cnt as usize)
+ }
+
+ fn write_all(&mut self, buf: &[u8]) -> io::Result<()> {
+ let chr: &mut bindings::CharBackend = self;
+
+ let len = buf.len().try_into().unwrap();
+ let r = unsafe { bindings::qemu_chr_fe_write_all(addr_of_mut!(*chr), buf.as_ptr(), len) };
+ errno::into_io_result(r).and_then(|cnt| {
+ if cnt as usize == buf.len() {
+ Ok(())
+ } else {
+ Err(ErrorKind::WriteZero.into())
+ }
+ })
+ }
+}
+
+impl Debug for CharBackend {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ // SAFETY: accessed just to print the values
+ let chr = self.inner.as_ptr();
+ Debug::fmt(unsafe { &*chr }, f)
+ }
+}
+
+// FIXME: use something like PinnedDrop from the pinned_init crate
+impl Drop for CharBackend {
+ fn drop(&mut self) {
+ self.disable_handlers();
+ }
+}
+
+impl CharBackend {
+ /// Enable the front-end's character device handlers, if there is an
+ /// associated `Chardev`.
+ pub fn enable_handlers<
+ 'chardev,
+ 'owner: 'chardev,
+ T,
+ CanReceiveFn: for<'a> FnCall<(&'a T,), u32>,
+ ReceiveFn: for<'a, 'b> FnCall<(&'a T, &'b [u8])>,
+ EventFn: for<'a> FnCall<(&'a T, Event)>,
+ >(
+ // When "self" is dropped, the handlers are automatically disabled.
+ // However, this is not necessarily true if the owner is dropped.
+ // So require the owner to outlive the character device.
+ &'chardev self,
+ owner: &'owner T,
+ _can_receive: CanReceiveFn,
+ _receive: ReceiveFn,
+ _event: EventFn,
+ ) {
+ unsafe extern "C" fn rust_can_receive_cb<T, F: for<'a> FnCall<(&'a T,), u32>>(
+ opaque: *mut c_void,
+ ) -> c_int {
+ // SAFETY: the values are safe according to the contract of
+ // enable_handlers() and qemu_chr_fe_set_handlers()
+ let owner: &T = unsafe { &*(opaque.cast::<T>()) };
+ let r = F::call((owner,));
+ r.try_into().unwrap()
+ }
+
+ unsafe extern "C" fn rust_receive_cb<T, F: for<'a, 'b> FnCall<(&'a T, &'b [u8])>>(
+ opaque: *mut c_void,
+ buf: *const u8,
+ size: c_int,
+ ) {
+ // SAFETY: the values are safe according to the contract of
+ // enable_handlers() and qemu_chr_fe_set_handlers()
+ let owner: &T = unsafe { &*(opaque.cast::<T>()) };
+ let buf = unsafe { slice::from_raw_parts(buf, size.try_into().unwrap()) };
+ F::call((owner, buf))
+ }
+
+ unsafe extern "C" fn rust_event_cb<T, F: for<'a> FnCall<(&'a T, Event)>>(
+ opaque: *mut c_void,
+ event: Event,
+ ) {
+ // SAFETY: the values are safe according to the contract of
+ // enable_handlers() and qemu_chr_fe_set_handlers()
+ let owner: &T = unsafe { &*(opaque.cast::<T>()) };
+ F::call((owner, event))
+ }
+
+ let _: () = CanReceiveFn::ASSERT_IS_SOME;
+ let receive_cb: Option<unsafe extern "C" fn(*mut c_void, *const u8, c_int)> =
+ if ReceiveFn::is_some() {
+ Some(rust_receive_cb::<T, ReceiveFn>)
+ } else {
+ None
+ };
+ let event_cb: Option<unsafe extern "C" fn(*mut c_void, Event)> = if EventFn::is_some() {
+ Some(rust_event_cb::<T, EventFn>)
+ } else {
+ None
+ };
+
+ let mut chr = self.inner.borrow_mut();
+ // SAFETY: the borrow promises that the BQL is taken
+ unsafe {
+ bindings::qemu_chr_fe_set_handlers(
+ addr_of_mut!(*chr),
+ Some(rust_can_receive_cb::<T, CanReceiveFn>),
+ receive_cb,
+ event_cb,
+ None,
+ (owner as *const T as *mut T).cast::<c_void>(),
+ core::ptr::null_mut(),
+ true,
+ );
+ }
+ }
+
+ /// Disable the front-end's character device handlers.
+ pub fn disable_handlers(&self) {
+ let mut chr = self.inner.borrow_mut();
+ // SAFETY: the borrow promises that the BQL is taken
+ unsafe {
+ bindings::qemu_chr_fe_set_handlers(
+ addr_of_mut!(*chr),
+ None,
+ None,
+ None,
+ None,
+ core::ptr::null_mut(),
+ core::ptr::null_mut(),
+ true,
+ );
+ }
+ }
+
+ /// Notify that the frontend is ready to receive data.
+ pub fn accept_input(&self) {
+ let mut chr = self.inner.borrow_mut();
+ // SAFETY: the borrow promises that the BQL is taken
+ unsafe { bindings::qemu_chr_fe_accept_input(addr_of_mut!(*chr)) }
+ }
+
+ /// Temporarily borrow the character device, allowing it to be used
+ /// as an implementor of `Write`. Note that it is not valid to drop
+ /// the big QEMU lock while the character device is borrowed, as
+ /// that might cause C code to write to the character device.
+ pub fn borrow_mut(&self) -> impl Write + '_ {
+ self.inner.borrow_mut()
+ }
+
+ /// Send a continuous stream of zero bits on the line if `enabled` is
+ /// true, or a short stream if `enabled` is false.
+ pub fn send_break(&self, long: bool) -> io::Result<()> {
+ let mut chr = self.inner.borrow_mut();
+ let mut duration: c_int = long.into();
+ // SAFETY: the borrow promises that the BQL is taken
+ let r = unsafe {
+ bindings::qemu_chr_fe_ioctl(
+ addr_of_mut!(*chr),
+ bindings::CHR_IOCTL_SERIAL_SET_BREAK as i32,
+ addr_of_mut!(duration).cast::<c_void>(),
+ )
+ };
+
+ errno::into_io_result(r).map(|_| ())
+ }
+
+ /// Write data to a character backend from the front end. This function
+ /// will send data from the front end to the back end. Unlike
+ /// `write`, this function will block if the back end cannot
+ /// consume all of the data attempted to be written.
+ ///
+ /// Returns the number of bytes consumed (0 if no associated Chardev) or an
+ /// error.
+ pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
+ let len = buf.len().try_into().unwrap();
+ // SAFETY: qemu_chr_fe_write is thread-safe
+ let r = unsafe { bindings::qemu_chr_fe_write(self.inner.as_ptr(), buf.as_ptr(), len) };
+ errno::into_io_result(r).map(|cnt| cnt as usize)
+ }
+
+ /// Write data to a character backend from the front end. This function
+ /// will send data from the front end to the back end. Unlike
+ /// `write`, this function will block if the back end cannot
+ /// consume all of the data attempted to be written.
+ ///
+ /// Returns the number of bytes consumed (0 if no associated Chardev) or an
+ /// error.
+ pub fn write_all(&self, buf: &[u8]) -> io::Result<()> {
+ let len = buf.len().try_into().unwrap();
+ // SAFETY: qemu_chr_fe_write_all is thread-safe
+ let r = unsafe { bindings::qemu_chr_fe_write_all(self.inner.as_ptr(), buf.as_ptr(), len) };
+ errno::into_io_result(r).and_then(|cnt| {
+ if cnt as usize == buf.len() {
+ Ok(())
+ } else {
+ Err(ErrorKind::WriteZero.into())
+ }
+ })
+ }
+}
unsafe impl ObjectType for Chardev {
type Class = ChardevClass;
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 47b6977828d..a3415a2ebcc 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -106,3 +106,4 @@ fn default() -> Self {
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
impl_zeroable!(crate::bindings::MemoryRegionOps);
impl_zeroable!(crate::bindings::MemTxAttrs);
+impl_zeroable!(crate::bindings::CharBackend);
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 17/25] rust: pl011: move register definitions out of lib.rs
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (15 preceding siblings ...)
2025-03-09 10:31 ` [PULL 16/25] rust: chardev: provide basic bindings to character devices Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 18/25] rust: pl011: clean up visibilities of callbacks Paolo Bonzini
` (7 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 7 +-
rust/hw/char/pl011/src/lib.rs | 509 +---------------------------
rust/hw/char/pl011/src/registers.rs | 506 +++++++++++++++++++++++++++
3 files changed, 512 insertions(+), 510 deletions(-)
create mode 100644 rust/hw/char/pl011/src/registers.rs
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index d0857b470c9..01540654cc9 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -26,10 +26,13 @@
use crate::{
device_class,
- registers::{self, Interrupt},
- RegisterOffset,
+ registers::{self, Interrupt, RegisterOffset},
};
+// TODO: You must disable the UART before any of the control registers are
+// reprogrammed. When the UART is disabled in the middle of transmission or
+// reception, it completes the current character before stopping
+
/// Integer Baud Rate Divider, `UARTIBRD`
const IBRD_MASK: u32 = 0xffff;
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 1bf46c65af2..45c13ba899e 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -18,516 +18,9 @@
mod device;
mod device_class;
+mod registers;
pub use device::pl011_create;
pub const TYPE_PL011: &::std::ffi::CStr = c_str!("pl011");
pub const TYPE_PL011_LUMINARY: &::std::ffi::CStr = c_str!("pl011_luminary");
-
-/// Offset of each register from the base memory address of the device.
-///
-/// # Source
-/// ARM DDI 0183G, Table 3-1 p.3-3
-#[doc(alias = "offset")]
-#[allow(non_camel_case_types)]
-#[repr(u64)]
-#[derive(Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
-enum RegisterOffset {
- /// Data Register
- ///
- /// A write to this register initiates the actual data transmission
- #[doc(alias = "UARTDR")]
- DR = 0x000,
- /// Receive Status Register or Error Clear Register
- #[doc(alias = "UARTRSR")]
- #[doc(alias = "UARTECR")]
- RSR = 0x004,
- /// Flag Register
- ///
- /// A read of this register shows if transmission is complete
- #[doc(alias = "UARTFR")]
- FR = 0x018,
- /// Fractional Baud Rate Register
- ///
- /// responsible for baud rate speed
- #[doc(alias = "UARTFBRD")]
- FBRD = 0x028,
- /// `IrDA` Low-Power Counter Register
- #[doc(alias = "UARTILPR")]
- ILPR = 0x020,
- /// Integer Baud Rate Register
- ///
- /// Responsible for baud rate speed
- #[doc(alias = "UARTIBRD")]
- IBRD = 0x024,
- /// line control register (data frame format)
- #[doc(alias = "UARTLCR_H")]
- LCR_H = 0x02C,
- /// Toggle UART, transmission or reception
- #[doc(alias = "UARTCR")]
- CR = 0x030,
- /// Interrupt FIFO Level Select Register
- #[doc(alias = "UARTIFLS")]
- FLS = 0x034,
- /// Interrupt Mask Set/Clear Register
- #[doc(alias = "UARTIMSC")]
- IMSC = 0x038,
- /// Raw Interrupt Status Register
- #[doc(alias = "UARTRIS")]
- RIS = 0x03C,
- /// Masked Interrupt Status Register
- #[doc(alias = "UARTMIS")]
- MIS = 0x040,
- /// Interrupt Clear Register
- #[doc(alias = "UARTICR")]
- ICR = 0x044,
- /// DMA control Register
- #[doc(alias = "UARTDMACR")]
- DMACR = 0x048,
- ///// Reserved, offsets `0x04C` to `0x07C`.
- //Reserved = 0x04C,
-}
-
-mod registers {
- //! Device registers exposed as typed structs which are backed by arbitrary
- //! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
- use bilge::prelude::*;
- use qemu_api::impl_vmstate_bitsized;
-
- /// Receive Status Register / Data Register common error bits
- ///
- /// The `UARTRSR` register is updated only when a read occurs
- /// from the `UARTDR` register with the same status information
- /// that can also be obtained by reading the `UARTDR` register
- #[bitsize(8)]
- #[derive(Clone, Copy, Default, DebugBits, FromBits)]
- pub struct Errors {
- pub framing_error: bool,
- pub parity_error: bool,
- pub break_error: bool,
- pub overrun_error: bool,
- _reserved_unpredictable: u4,
- }
-
- // TODO: FIFO Mode has different semantics
- /// Data Register, `UARTDR`
- ///
- /// The `UARTDR` register is the data register.
- ///
- /// For words to be transmitted:
- ///
- /// - if the FIFOs are enabled, data written to this location is pushed onto
- /// the transmit
- /// FIFO
- /// - if the FIFOs are not enabled, data is stored in the transmitter
- /// holding register (the
- /// bottom word of the transmit FIFO).
- ///
- /// The write operation initiates transmission from the UART. The data is
- /// prefixed with a start bit, appended with the appropriate parity bit
- /// (if parity is enabled), and a stop bit. The resultant word is then
- /// transmitted.
- ///
- /// For received words:
- ///
- /// - if the FIFOs are enabled, the data byte and the 4-bit status (break,
- /// frame, parity,
- /// and overrun) is pushed onto the 12-bit wide receive FIFO
- /// - if the FIFOs are not enabled, the data byte and status are stored in
- /// the receiving
- /// holding register (the bottom word of the receive FIFO).
- ///
- /// The received data byte is read by performing reads from the `UARTDR`
- /// register along with the corresponding status information. The status
- /// information can also be read by a read of the `UARTRSR/UARTECR`
- /// register.
- ///
- /// # Note
- ///
- /// You must disable the UART before any of the control registers are
- /// reprogrammed. When the UART is disabled in the middle of
- /// transmission or reception, it completes the current character before
- /// stopping.
- ///
- /// # Source
- /// ARM DDI 0183G 3.3.1 Data Register, UARTDR
- #[bitsize(32)]
- #[derive(Clone, Copy, Default, DebugBits, FromBits)]
- #[doc(alias = "UARTDR")]
- pub struct Data {
- pub data: u8,
- pub errors: Errors,
- _reserved: u16,
- }
- impl_vmstate_bitsized!(Data);
-
- impl Data {
- // bilge is not very const-friendly, unfortunately
- pub const BREAK: Self = Self { value: 1 << 10 };
- }
-
- // TODO: FIFO Mode has different semantics
- /// Receive Status Register / Error Clear Register, `UARTRSR/UARTECR`
- ///
- /// The UARTRSR/UARTECR register is the receive status register/error clear
- /// register. Receive status can also be read from the `UARTRSR`
- /// register. If the status is read from this register, then the status
- /// information for break, framing and parity corresponds to the
- /// data character read from the [Data register](Data), `UARTDR` prior to
- /// reading the UARTRSR register. The status information for overrun is
- /// set immediately when an overrun condition occurs.
- ///
- ///
- /// # Note
- /// The received data character must be read first from the [Data
- /// Register](Data), `UARTDR` before reading the error status associated
- /// with that data character from the `UARTRSR` register. This read
- /// sequence cannot be reversed, because the `UARTRSR` register is
- /// updated only when a read occurs from the `UARTDR` register. However,
- /// the status information can also be obtained by reading the `UARTDR`
- /// register
- ///
- /// # Source
- /// ARM DDI 0183G 3.3.2 Receive Status Register/Error Clear Register,
- /// UARTRSR/UARTECR
- #[bitsize(32)]
- #[derive(Clone, Copy, DebugBits, FromBits)]
- pub struct ReceiveStatusErrorClear {
- pub errors: Errors,
- _reserved_unpredictable: u24,
- }
- impl_vmstate_bitsized!(ReceiveStatusErrorClear);
-
- impl ReceiveStatusErrorClear {
- pub fn set_from_data(&mut self, data: Data) {
- self.set_errors(data.errors());
- }
-
- pub fn reset(&mut self) {
- // All the bits are cleared to 0 on reset.
- *self = Self::default();
- }
- }
-
- impl Default for ReceiveStatusErrorClear {
- fn default() -> Self {
- 0.into()
- }
- }
-
- #[bitsize(32)]
- #[derive(Clone, Copy, DebugBits, FromBits)]
- /// Flag Register, `UARTFR`
- #[doc(alias = "UARTFR")]
- pub struct Flags {
- /// CTS Clear to send. This bit is the complement of the UART clear to
- /// send, `nUARTCTS`, modem status input. That is, the bit is 1
- /// when `nUARTCTS` is LOW.
- pub clear_to_send: bool,
- /// DSR Data set ready. This bit is the complement of the UART data set
- /// ready, `nUARTDSR`, modem status input. That is, the bit is 1 when
- /// `nUARTDSR` is LOW.
- pub data_set_ready: bool,
- /// DCD Data carrier detect. This bit is the complement of the UART data
- /// carrier detect, `nUARTDCD`, modem status input. That is, the bit is
- /// 1 when `nUARTDCD` is LOW.
- pub data_carrier_detect: bool,
- /// BUSY UART busy. If this bit is set to 1, the UART is busy
- /// transmitting data. This bit remains set until the complete
- /// byte, including all the stop bits, has been sent from the
- /// shift register. This bit is set as soon as the transmit FIFO
- /// becomes non-empty, regardless of whether the UART is enabled
- /// or not.
- pub busy: bool,
- /// RXFE Receive FIFO empty. The meaning of this bit depends on the
- /// state of the FEN bit in the UARTLCR_H register. If the FIFO
- /// is disabled, this bit is set when the receive holding
- /// register is empty. If the FIFO is enabled, the RXFE bit is
- /// set when the receive FIFO is empty.
- pub receive_fifo_empty: bool,
- /// TXFF Transmit FIFO full. The meaning of this bit depends on the
- /// state of the FEN bit in the UARTLCR_H register. If the FIFO
- /// is disabled, this bit is set when the transmit holding
- /// register is full. If the FIFO is enabled, the TXFF bit is
- /// set when the transmit FIFO is full.
- pub transmit_fifo_full: bool,
- /// RXFF Receive FIFO full. The meaning of this bit depends on the state
- /// of the FEN bit in the UARTLCR_H register. If the FIFO is
- /// disabled, this bit is set when the receive holding register
- /// is full. If the FIFO is enabled, the RXFF bit is set when
- /// the receive FIFO is full.
- pub receive_fifo_full: bool,
- /// Transmit FIFO empty. The meaning of this bit depends on the state of
- /// the FEN bit in the [Line Control register](LineControl),
- /// `UARTLCR_H`. If the FIFO is disabled, this bit is set when the
- /// transmit holding register is empty. If the FIFO is enabled,
- /// the TXFE bit is set when the transmit FIFO is empty. This
- /// bit does not indicate if there is data in the transmit shift
- /// register.
- pub transmit_fifo_empty: bool,
- /// `RI`, is `true` when `nUARTRI` is `LOW`.
- pub ring_indicator: bool,
- _reserved_zero_no_modify: u23,
- }
- impl_vmstate_bitsized!(Flags);
-
- impl Flags {
- pub fn reset(&mut self) {
- *self = Self::default();
- }
- }
-
- impl Default for Flags {
- fn default() -> Self {
- let mut ret: Self = 0.into();
- // After reset TXFF, RXFF, and BUSY are 0, and TXFE and RXFE are 1
- ret.set_receive_fifo_empty(true);
- ret.set_transmit_fifo_empty(true);
- ret
- }
- }
-
- #[bitsize(32)]
- #[derive(Clone, Copy, DebugBits, FromBits)]
- /// Line Control Register, `UARTLCR_H`
- #[doc(alias = "UARTLCR_H")]
- pub struct LineControl {
- /// BRK Send break.
- ///
- /// If this bit is set to `1`, a low-level is continually output on the
- /// `UARTTXD` output, after completing transmission of the
- /// current character. For the proper execution of the break command,
- /// the software must set this bit for at least two complete
- /// frames. For normal use, this bit must be cleared to `0`.
- pub send_break: bool,
- /// 1 PEN Parity enable:
- ///
- /// - 0 = parity is disabled and no parity bit added to the data frame
- /// - 1 = parity checking and generation is enabled.
- ///
- /// See Table 3-11 on page 3-14 for the parity truth table.
- pub parity_enabled: bool,
- /// EPS Even parity select. Controls the type of parity the UART uses
- /// during transmission and reception:
- /// - 0 = odd parity. The UART generates or checks for an odd number of
- /// 1s in the data and parity bits.
- /// - 1 = even parity. The UART generates or checks for an even number
- /// of 1s in the data and parity bits.
- /// This bit has no effect when the `PEN` bit disables parity checking
- /// and generation. See Table 3-11 on page 3-14 for the parity
- /// truth table.
- pub parity: Parity,
- /// 3 STP2 Two stop bits select. If this bit is set to 1, two stop bits
- /// are transmitted at the end of the frame. The receive
- /// logic does not check for two stop bits being received.
- pub two_stops_bits: bool,
- /// FEN Enable FIFOs:
- /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
- /// 1-byte-deep holding registers 1 = transmit and receive FIFO
- /// buffers are enabled (FIFO mode).
- pub fifos_enabled: Mode,
- /// WLEN Word length. These bits indicate the number of data bits
- /// transmitted or received in a frame as follows: b11 = 8 bits
- /// b10 = 7 bits
- /// b01 = 6 bits
- /// b00 = 5 bits.
- pub word_length: WordLength,
- /// 7 SPS Stick parity select.
- /// 0 = stick parity is disabled
- /// 1 = either:
- /// • if the EPS bit is 0 then the parity bit is transmitted and checked
- /// as a 1 • if the EPS bit is 1 then the parity bit is
- /// transmitted and checked as a 0. This bit has no effect when
- /// the PEN bit disables parity checking and generation. See Table 3-11
- /// on page 3-14 for the parity truth table.
- pub sticky_parity: bool,
- /// 31:8 - Reserved, do not modify, read as zero.
- _reserved_zero_no_modify: u24,
- }
- impl_vmstate_bitsized!(LineControl);
-
- impl LineControl {
- pub fn reset(&mut self) {
- // All the bits are cleared to 0 when reset.
- *self = 0.into();
- }
- }
-
- impl Default for LineControl {
- fn default() -> Self {
- 0.into()
- }
- }
-
- #[bitsize(1)]
- #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
- /// `EPS` "Even parity select", field of [Line Control
- /// register](LineControl).
- pub enum Parity {
- /// - 0 = odd parity. The UART generates or checks for an odd number of
- /// 1s in the data and parity bits.
- Odd = 0,
- /// - 1 = even parity. The UART generates or checks for an even number
- /// of 1s in the data and parity bits.
- Even = 1,
- }
-
- #[bitsize(1)]
- #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
- /// `FEN` "Enable FIFOs" or Device mode, field of [Line Control
- /// register](LineControl).
- pub enum Mode {
- /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
- /// 1-byte-deep holding registers
- Character = 0,
- /// 1 = transmit and receive FIFO buffers are enabled (FIFO mode).
- FIFO = 1,
- }
-
- #[bitsize(2)]
- #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
- /// `WLEN` Word length, field of [Line Control register](LineControl).
- ///
- /// These bits indicate the number of data bits transmitted or received in a
- /// frame as follows:
- pub enum WordLength {
- /// b11 = 8 bits
- _8Bits = 0b11,
- /// b10 = 7 bits
- _7Bits = 0b10,
- /// b01 = 6 bits
- _6Bits = 0b01,
- /// b00 = 5 bits.
- _5Bits = 0b00,
- }
-
- /// Control Register, `UARTCR`
- ///
- /// The `UARTCR` register is the control register. All the bits are cleared
- /// to `0` on reset except for bits `9` and `8` that are set to `1`.
- ///
- /// # Source
- /// ARM DDI 0183G, 3.3.8 Control Register, `UARTCR`, Table 3-12
- #[bitsize(32)]
- #[doc(alias = "UARTCR")]
- #[derive(Clone, Copy, DebugBits, FromBits)]
- pub struct Control {
- /// `UARTEN` UART enable: 0 = UART is disabled. If the UART is disabled
- /// in the middle of transmission or reception, it completes the current
- /// character before stopping. 1 = the UART is enabled. Data
- /// transmission and reception occurs for either UART signals or SIR
- /// signals depending on the setting of the SIREN bit.
- pub enable_uart: bool,
- /// `SIREN` `SIR` enable: 0 = IrDA SIR ENDEC is disabled. `nSIROUT`
- /// remains LOW (no light pulse generated), and signal transitions on
- /// SIRIN have no effect. 1 = IrDA SIR ENDEC is enabled. Data is
- /// transmitted and received on nSIROUT and SIRIN. UARTTXD remains HIGH,
- /// in the marking state. Signal transitions on UARTRXD or modem status
- /// inputs have no effect. This bit has no effect if the UARTEN bit
- /// disables the UART.
- pub enable_sir: bool,
- /// `SIRLP` SIR low-power IrDA mode. This bit selects the IrDA encoding
- /// mode. If this bit is cleared to 0, low-level bits are transmitted as
- /// an active high pulse with a width of 3/ 16th of the bit period. If
- /// this bit is set to 1, low-level bits are transmitted with a pulse
- /// width that is 3 times the period of the IrLPBaud16 input signal,
- /// regardless of the selected bit rate. Setting this bit uses less
- /// power, but might reduce transmission distances.
- pub sir_lowpower_irda_mode: u1,
- /// Reserved, do not modify, read as zero.
- _reserved_zero_no_modify: u4,
- /// `LBE` Loopback enable. If this bit is set to 1 and the SIREN bit is
- /// set to 1 and the SIRTEST bit in the Test Control register, UARTTCR
- /// on page 4-5 is set to 1, then the nSIROUT path is inverted, and fed
- /// through to the SIRIN path. The SIRTEST bit in the test register must
- /// be set to 1 to override the normal half-duplex SIR operation. This
- /// must be the requirement for accessing the test registers during
- /// normal operation, and SIRTEST must be cleared to 0 when loopback
- /// testing is finished. This feature reduces the amount of external
- /// coupling required during system test. If this bit is set to 1, and
- /// the SIRTEST bit is set to 0, the UARTTXD path is fed through to the
- /// UARTRXD path. In either SIR mode or UART mode, when this bit is set,
- /// the modem outputs are also fed through to the modem inputs. This bit
- /// is cleared to 0 on reset, to disable loopback.
- pub enable_loopback: bool,
- /// `TXE` Transmit enable. If this bit is set to 1, the transmit section
- /// of the UART is enabled. Data transmission occurs for either UART
- /// signals, or SIR signals depending on the setting of the SIREN bit.
- /// When the UART is disabled in the middle of transmission, it
- /// completes the current character before stopping.
- pub enable_transmit: bool,
- /// `RXE` Receive enable. If this bit is set to 1, the receive section
- /// of the UART is enabled. Data reception occurs for either UART
- /// signals or SIR signals depending on the setting of the SIREN bit.
- /// When the UART is disabled in the middle of reception, it completes
- /// the current character before stopping.
- pub enable_receive: bool,
- /// `DTR` Data transmit ready. This bit is the complement of the UART
- /// data transmit ready, `nUARTDTR`, modem status output. That is, when
- /// the bit is programmed to a 1 then `nUARTDTR` is LOW.
- pub data_transmit_ready: bool,
- /// `RTS` Request to send. This bit is the complement of the UART
- /// request to send, `nUARTRTS`, modem status output. That is, when the
- /// bit is programmed to a 1 then `nUARTRTS` is LOW.
- pub request_to_send: bool,
- /// `Out1` This bit is the complement of the UART Out1 (`nUARTOut1`)
- /// modem status output. That is, when the bit is programmed to a 1 the
- /// output is 0. For DTE this can be used as Data Carrier Detect (DCD).
- pub out_1: bool,
- /// `Out2` This bit is the complement of the UART Out2 (`nUARTOut2`)
- /// modem status output. That is, when the bit is programmed to a 1, the
- /// output is 0. For DTE this can be used as Ring Indicator (RI).
- pub out_2: bool,
- /// `RTSEn` RTS hardware flow control enable. If this bit is set to 1,
- /// RTS hardware flow control is enabled. Data is only requested when
- /// there is space in the receive FIFO for it to be received.
- pub rts_hardware_flow_control_enable: bool,
- /// `CTSEn` CTS hardware flow control enable. If this bit is set to 1,
- /// CTS hardware flow control is enabled. Data is only transmitted when
- /// the `nUARTCTS` signal is asserted.
- pub cts_hardware_flow_control_enable: bool,
- /// 31:16 - Reserved, do not modify, read as zero.
- _reserved_zero_no_modify2: u16,
- }
- impl_vmstate_bitsized!(Control);
-
- impl Control {
- pub fn reset(&mut self) {
- *self = 0.into();
- self.set_enable_receive(true);
- self.set_enable_transmit(true);
- }
- }
-
- impl Default for Control {
- fn default() -> Self {
- let mut ret: Self = 0.into();
- ret.reset();
- ret
- }
- }
-
- /// Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC
- pub struct Interrupt(pub u32);
-
- impl Interrupt {
- pub const OE: Self = Self(1 << 10);
- pub const BE: Self = Self(1 << 9);
- pub const PE: Self = Self(1 << 8);
- pub const FE: Self = Self(1 << 7);
- pub const RT: Self = Self(1 << 6);
- pub const TX: Self = Self(1 << 5);
- pub const RX: Self = Self(1 << 4);
- pub const DSR: Self = Self(1 << 3);
- pub const DCD: Self = Self(1 << 2);
- pub const CTS: Self = Self(1 << 1);
- pub const RI: Self = Self(1 << 0);
-
- pub const E: Self = Self(Self::OE.0 | Self::BE.0 | Self::PE.0 | Self::FE.0);
- pub const MS: Self = Self(Self::RI.0 | Self::DSR.0 | Self::DCD.0 | Self::CTS.0);
- }
-}
-
-// TODO: You must disable the UART before any of the control registers are
-// reprogrammed. When the UART is disabled in the middle of transmission or
-// reception, it completes the current character before stopping
diff --git a/rust/hw/char/pl011/src/registers.rs b/rust/hw/char/pl011/src/registers.rs
new file mode 100644
index 00000000000..cd92fa2c300
--- /dev/null
+++ b/rust/hw/char/pl011/src/registers.rs
@@ -0,0 +1,506 @@
+// Copyright 2024, Linaro Limited
+// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Device registers exposed as typed structs which are backed by arbitrary
+//! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
+
+use bilge::prelude::*;
+use qemu_api::impl_vmstate_bitsized;
+
+/// Offset of each register from the base memory address of the device.
+///
+/// # Source
+/// ARM DDI 0183G, Table 3-1 p.3-3
+#[doc(alias = "offset")]
+#[allow(non_camel_case_types)]
+#[repr(u64)]
+#[derive(Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
+pub enum RegisterOffset {
+ /// Data Register
+ ///
+ /// A write to this register initiates the actual data transmission
+ #[doc(alias = "UARTDR")]
+ DR = 0x000,
+ /// Receive Status Register or Error Clear Register
+ #[doc(alias = "UARTRSR")]
+ #[doc(alias = "UARTECR")]
+ RSR = 0x004,
+ /// Flag Register
+ ///
+ /// A read of this register shows if transmission is complete
+ #[doc(alias = "UARTFR")]
+ FR = 0x018,
+ /// Fractional Baud Rate Register
+ ///
+ /// responsible for baud rate speed
+ #[doc(alias = "UARTFBRD")]
+ FBRD = 0x028,
+ /// `IrDA` Low-Power Counter Register
+ #[doc(alias = "UARTILPR")]
+ ILPR = 0x020,
+ /// Integer Baud Rate Register
+ ///
+ /// Responsible for baud rate speed
+ #[doc(alias = "UARTIBRD")]
+ IBRD = 0x024,
+ /// line control register (data frame format)
+ #[doc(alias = "UARTLCR_H")]
+ LCR_H = 0x02C,
+ /// Toggle UART, transmission or reception
+ #[doc(alias = "UARTCR")]
+ CR = 0x030,
+ /// Interrupt FIFO Level Select Register
+ #[doc(alias = "UARTIFLS")]
+ FLS = 0x034,
+ /// Interrupt Mask Set/Clear Register
+ #[doc(alias = "UARTIMSC")]
+ IMSC = 0x038,
+ /// Raw Interrupt Status Register
+ #[doc(alias = "UARTRIS")]
+ RIS = 0x03C,
+ /// Masked Interrupt Status Register
+ #[doc(alias = "UARTMIS")]
+ MIS = 0x040,
+ /// Interrupt Clear Register
+ #[doc(alias = "UARTICR")]
+ ICR = 0x044,
+ /// DMA control Register
+ #[doc(alias = "UARTDMACR")]
+ DMACR = 0x048,
+ ///// Reserved, offsets `0x04C` to `0x07C`.
+ //Reserved = 0x04C,
+}
+
+/// Receive Status Register / Data Register common error bits
+///
+/// The `UARTRSR` register is updated only when a read occurs
+/// from the `UARTDR` register with the same status information
+/// that can also be obtained by reading the `UARTDR` register
+#[bitsize(8)]
+#[derive(Clone, Copy, Default, DebugBits, FromBits)]
+pub struct Errors {
+ pub framing_error: bool,
+ pub parity_error: bool,
+ pub break_error: bool,
+ pub overrun_error: bool,
+ _reserved_unpredictable: u4,
+}
+
+// TODO: FIFO Mode has different semantics
+/// Data Register, `UARTDR`
+///
+/// The `UARTDR` register is the data register.
+///
+/// For words to be transmitted:
+///
+/// - if the FIFOs are enabled, data written to this location is pushed onto the
+/// transmit
+/// FIFO
+/// - if the FIFOs are not enabled, data is stored in the transmitter holding
+/// register (the
+/// bottom word of the transmit FIFO).
+///
+/// The write operation initiates transmission from the UART. The data is
+/// prefixed with a start bit, appended with the appropriate parity bit
+/// (if parity is enabled), and a stop bit. The resultant word is then
+/// transmitted.
+///
+/// For received words:
+///
+/// - if the FIFOs are enabled, the data byte and the 4-bit status (break,
+/// frame, parity,
+/// and overrun) is pushed onto the 12-bit wide receive FIFO
+/// - if the FIFOs are not enabled, the data byte and status are stored in the
+/// receiving
+/// holding register (the bottom word of the receive FIFO).
+///
+/// The received data byte is read by performing reads from the `UARTDR`
+/// register along with the corresponding status information. The status
+/// information can also be read by a read of the `UARTRSR/UARTECR`
+/// register.
+///
+/// # Note
+///
+/// You must disable the UART before any of the control registers are
+/// reprogrammed. When the UART is disabled in the middle of
+/// transmission or reception, it completes the current character before
+/// stopping.
+///
+/// # Source
+/// ARM DDI 0183G 3.3.1 Data Register, UARTDR
+#[bitsize(32)]
+#[derive(Clone, Copy, Default, DebugBits, FromBits)]
+#[doc(alias = "UARTDR")]
+pub struct Data {
+ pub data: u8,
+ pub errors: Errors,
+ _reserved: u16,
+}
+impl_vmstate_bitsized!(Data);
+
+impl Data {
+ // bilge is not very const-friendly, unfortunately
+ pub const BREAK: Self = Self { value: 1 << 10 };
+}
+
+// TODO: FIFO Mode has different semantics
+/// Receive Status Register / Error Clear Register, `UARTRSR/UARTECR`
+///
+/// The UARTRSR/UARTECR register is the receive status register/error clear
+/// register. Receive status can also be read from the `UARTRSR`
+/// register. If the status is read from this register, then the status
+/// information for break, framing and parity corresponds to the
+/// data character read from the [Data register](Data), `UARTDR` prior to
+/// reading the UARTRSR register. The status information for overrun is
+/// set immediately when an overrun condition occurs.
+///
+///
+/// # Note
+/// The received data character must be read first from the [Data
+/// Register](Data), `UARTDR` before reading the error status associated
+/// with that data character from the `UARTRSR` register. This read
+/// sequence cannot be reversed, because the `UARTRSR` register is
+/// updated only when a read occurs from the `UARTDR` register. However,
+/// the status information can also be obtained by reading the `UARTDR`
+/// register
+///
+/// # Source
+/// ARM DDI 0183G 3.3.2 Receive Status Register/Error Clear Register,
+/// UARTRSR/UARTECR
+#[bitsize(32)]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+pub struct ReceiveStatusErrorClear {
+ pub errors: Errors,
+ _reserved_unpredictable: u24,
+}
+impl_vmstate_bitsized!(ReceiveStatusErrorClear);
+
+impl ReceiveStatusErrorClear {
+ pub fn set_from_data(&mut self, data: Data) {
+ self.set_errors(data.errors());
+ }
+
+ pub fn reset(&mut self) {
+ // All the bits are cleared to 0 on reset.
+ *self = Self::default();
+ }
+}
+
+impl Default for ReceiveStatusErrorClear {
+ fn default() -> Self {
+ 0.into()
+ }
+}
+
+#[bitsize(32)]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+/// Flag Register, `UARTFR`
+#[doc(alias = "UARTFR")]
+pub struct Flags {
+ /// CTS Clear to send. This bit is the complement of the UART clear to
+ /// send, `nUARTCTS`, modem status input. That is, the bit is 1
+ /// when `nUARTCTS` is LOW.
+ pub clear_to_send: bool,
+ /// DSR Data set ready. This bit is the complement of the UART data set
+ /// ready, `nUARTDSR`, modem status input. That is, the bit is 1 when
+ /// `nUARTDSR` is LOW.
+ pub data_set_ready: bool,
+ /// DCD Data carrier detect. This bit is the complement of the UART data
+ /// carrier detect, `nUARTDCD`, modem status input. That is, the bit is
+ /// 1 when `nUARTDCD` is LOW.
+ pub data_carrier_detect: bool,
+ /// BUSY UART busy. If this bit is set to 1, the UART is busy
+ /// transmitting data. This bit remains set until the complete
+ /// byte, including all the stop bits, has been sent from the
+ /// shift register. This bit is set as soon as the transmit FIFO
+ /// becomes non-empty, regardless of whether the UART is enabled
+ /// or not.
+ pub busy: bool,
+ /// RXFE Receive FIFO empty. The meaning of this bit depends on the
+ /// state of the FEN bit in the UARTLCR_H register. If the FIFO
+ /// is disabled, this bit is set when the receive holding
+ /// register is empty. If the FIFO is enabled, the RXFE bit is
+ /// set when the receive FIFO is empty.
+ pub receive_fifo_empty: bool,
+ /// TXFF Transmit FIFO full. The meaning of this bit depends on the
+ /// state of the FEN bit in the UARTLCR_H register. If the FIFO
+ /// is disabled, this bit is set when the transmit holding
+ /// register is full. If the FIFO is enabled, the TXFF bit is
+ /// set when the transmit FIFO is full.
+ pub transmit_fifo_full: bool,
+ /// RXFF Receive FIFO full. The meaning of this bit depends on the state
+ /// of the FEN bit in the UARTLCR_H register. If the FIFO is
+ /// disabled, this bit is set when the receive holding register
+ /// is full. If the FIFO is enabled, the RXFF bit is set when
+ /// the receive FIFO is full.
+ pub receive_fifo_full: bool,
+ /// Transmit FIFO empty. The meaning of this bit depends on the state of
+ /// the FEN bit in the [Line Control register](LineControl),
+ /// `UARTLCR_H`. If the FIFO is disabled, this bit is set when the
+ /// transmit holding register is empty. If the FIFO is enabled,
+ /// the TXFE bit is set when the transmit FIFO is empty. This
+ /// bit does not indicate if there is data in the transmit shift
+ /// register.
+ pub transmit_fifo_empty: bool,
+ /// `RI`, is `true` when `nUARTRI` is `LOW`.
+ pub ring_indicator: bool,
+ _reserved_zero_no_modify: u23,
+}
+impl_vmstate_bitsized!(Flags);
+
+impl Flags {
+ pub fn reset(&mut self) {
+ *self = Self::default();
+ }
+}
+
+impl Default for Flags {
+ fn default() -> Self {
+ let mut ret: Self = 0.into();
+ // After reset TXFF, RXFF, and BUSY are 0, and TXFE and RXFE are 1
+ ret.set_receive_fifo_empty(true);
+ ret.set_transmit_fifo_empty(true);
+ ret
+ }
+}
+
+#[bitsize(32)]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+/// Line Control Register, `UARTLCR_H`
+#[doc(alias = "UARTLCR_H")]
+pub struct LineControl {
+ /// BRK Send break.
+ ///
+ /// If this bit is set to `1`, a low-level is continually output on the
+ /// `UARTTXD` output, after completing transmission of the
+ /// current character. For the proper execution of the break command,
+ /// the software must set this bit for at least two complete
+ /// frames. For normal use, this bit must be cleared to `0`.
+ pub send_break: bool,
+ /// 1 PEN Parity enable:
+ ///
+ /// - 0 = parity is disabled and no parity bit added to the data frame
+ /// - 1 = parity checking and generation is enabled.
+ ///
+ /// See Table 3-11 on page 3-14 for the parity truth table.
+ pub parity_enabled: bool,
+ /// EPS Even parity select. Controls the type of parity the UART uses
+ /// during transmission and reception:
+ /// - 0 = odd parity. The UART generates or checks for an odd number of 1s
+ /// in the data and parity bits.
+ /// - 1 = even parity. The UART generates or checks for an even number of 1s
+ /// in the data and parity bits.
+ /// This bit has no effect when the `PEN` bit disables parity checking
+ /// and generation. See Table 3-11 on page 3-14 for the parity
+ /// truth table.
+ pub parity: Parity,
+ /// 3 STP2 Two stop bits select. If this bit is set to 1, two stop bits
+ /// are transmitted at the end of the frame. The receive
+ /// logic does not check for two stop bits being received.
+ pub two_stops_bits: bool,
+ /// FEN Enable FIFOs:
+ /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
+ /// 1-byte-deep holding registers 1 = transmit and receive FIFO
+ /// buffers are enabled (FIFO mode).
+ pub fifos_enabled: Mode,
+ /// WLEN Word length. These bits indicate the number of data bits
+ /// transmitted or received in a frame as follows: b11 = 8 bits
+ /// b10 = 7 bits
+ /// b01 = 6 bits
+ /// b00 = 5 bits.
+ pub word_length: WordLength,
+ /// 7 SPS Stick parity select.
+ /// 0 = stick parity is disabled
+ /// 1 = either:
+ /// • if the EPS bit is 0 then the parity bit is transmitted and checked
+ /// as a 1 • if the EPS bit is 1 then the parity bit is
+ /// transmitted and checked as a 0. This bit has no effect when
+ /// the PEN bit disables parity checking and generation. See Table 3-11
+ /// on page 3-14 for the parity truth table.
+ pub sticky_parity: bool,
+ /// 31:8 - Reserved, do not modify, read as zero.
+ _reserved_zero_no_modify: u24,
+}
+impl_vmstate_bitsized!(LineControl);
+
+impl LineControl {
+ pub fn reset(&mut self) {
+ // All the bits are cleared to 0 when reset.
+ *self = 0.into();
+ }
+}
+
+impl Default for LineControl {
+ fn default() -> Self {
+ 0.into()
+ }
+}
+
+#[bitsize(1)]
+#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+/// `EPS` "Even parity select", field of [Line Control
+/// register](LineControl).
+pub enum Parity {
+ /// - 0 = odd parity. The UART generates or checks for an odd number of 1s
+ /// in the data and parity bits.
+ Odd = 0,
+ /// - 1 = even parity. The UART generates or checks for an even number of 1s
+ /// in the data and parity bits.
+ Even = 1,
+}
+
+#[bitsize(1)]
+#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+/// `FEN` "Enable FIFOs" or Device mode, field of [Line Control
+/// register](LineControl).
+pub enum Mode {
+ /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
+ /// 1-byte-deep holding registers
+ Character = 0,
+ /// 1 = transmit and receive FIFO buffers are enabled (FIFO mode).
+ FIFO = 1,
+}
+
+#[bitsize(2)]
+#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+/// `WLEN` Word length, field of [Line Control register](LineControl).
+///
+/// These bits indicate the number of data bits transmitted or received in a
+/// frame as follows:
+pub enum WordLength {
+ /// b11 = 8 bits
+ _8Bits = 0b11,
+ /// b10 = 7 bits
+ _7Bits = 0b10,
+ /// b01 = 6 bits
+ _6Bits = 0b01,
+ /// b00 = 5 bits.
+ _5Bits = 0b00,
+}
+
+/// Control Register, `UARTCR`
+///
+/// The `UARTCR` register is the control register. All the bits are cleared
+/// to `0` on reset except for bits `9` and `8` that are set to `1`.
+///
+/// # Source
+/// ARM DDI 0183G, 3.3.8 Control Register, `UARTCR`, Table 3-12
+#[bitsize(32)]
+#[doc(alias = "UARTCR")]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+pub struct Control {
+ /// `UARTEN` UART enable: 0 = UART is disabled. If the UART is disabled
+ /// in the middle of transmission or reception, it completes the current
+ /// character before stopping. 1 = the UART is enabled. Data
+ /// transmission and reception occurs for either UART signals or SIR
+ /// signals depending on the setting of the SIREN bit.
+ pub enable_uart: bool,
+ /// `SIREN` `SIR` enable: 0 = IrDA SIR ENDEC is disabled. `nSIROUT`
+ /// remains LOW (no light pulse generated), and signal transitions on
+ /// SIRIN have no effect. 1 = IrDA SIR ENDEC is enabled. Data is
+ /// transmitted and received on nSIROUT and SIRIN. UARTTXD remains HIGH,
+ /// in the marking state. Signal transitions on UARTRXD or modem status
+ /// inputs have no effect. This bit has no effect if the UARTEN bit
+ /// disables the UART.
+ pub enable_sir: bool,
+ /// `SIRLP` SIR low-power IrDA mode. This bit selects the IrDA encoding
+ /// mode. If this bit is cleared to 0, low-level bits are transmitted as
+ /// an active high pulse with a width of 3/ 16th of the bit period. If
+ /// this bit is set to 1, low-level bits are transmitted with a pulse
+ /// width that is 3 times the period of the IrLPBaud16 input signal,
+ /// regardless of the selected bit rate. Setting this bit uses less
+ /// power, but might reduce transmission distances.
+ pub sir_lowpower_irda_mode: u1,
+ /// Reserved, do not modify, read as zero.
+ _reserved_zero_no_modify: u4,
+ /// `LBE` Loopback enable. If this bit is set to 1 and the SIREN bit is
+ /// set to 1 and the SIRTEST bit in the Test Control register, UARTTCR
+ /// on page 4-5 is set to 1, then the nSIROUT path is inverted, and fed
+ /// through to the SIRIN path. The SIRTEST bit in the test register must
+ /// be set to 1 to override the normal half-duplex SIR operation. This
+ /// must be the requirement for accessing the test registers during
+ /// normal operation, and SIRTEST must be cleared to 0 when loopback
+ /// testing is finished. This feature reduces the amount of external
+ /// coupling required during system test. If this bit is set to 1, and
+ /// the SIRTEST bit is set to 0, the UARTTXD path is fed through to the
+ /// UARTRXD path. In either SIR mode or UART mode, when this bit is set,
+ /// the modem outputs are also fed through to the modem inputs. This bit
+ /// is cleared to 0 on reset, to disable loopback.
+ pub enable_loopback: bool,
+ /// `TXE` Transmit enable. If this bit is set to 1, the transmit section
+ /// of the UART is enabled. Data transmission occurs for either UART
+ /// signals, or SIR signals depending on the setting of the SIREN bit.
+ /// When the UART is disabled in the middle of transmission, it
+ /// completes the current character before stopping.
+ pub enable_transmit: bool,
+ /// `RXE` Receive enable. If this bit is set to 1, the receive section
+ /// of the UART is enabled. Data reception occurs for either UART
+ /// signals or SIR signals depending on the setting of the SIREN bit.
+ /// When the UART is disabled in the middle of reception, it completes
+ /// the current character before stopping.
+ pub enable_receive: bool,
+ /// `DTR` Data transmit ready. This bit is the complement of the UART
+ /// data transmit ready, `nUARTDTR`, modem status output. That is, when
+ /// the bit is programmed to a 1 then `nUARTDTR` is LOW.
+ pub data_transmit_ready: bool,
+ /// `RTS` Request to send. This bit is the complement of the UART
+ /// request to send, `nUARTRTS`, modem status output. That is, when the
+ /// bit is programmed to a 1 then `nUARTRTS` is LOW.
+ pub request_to_send: bool,
+ /// `Out1` This bit is the complement of the UART Out1 (`nUARTOut1`)
+ /// modem status output. That is, when the bit is programmed to a 1 the
+ /// output is 0. For DTE this can be used as Data Carrier Detect (DCD).
+ pub out_1: bool,
+ /// `Out2` This bit is the complement of the UART Out2 (`nUARTOut2`)
+ /// modem status output. That is, when the bit is programmed to a 1, the
+ /// output is 0. For DTE this can be used as Ring Indicator (RI).
+ pub out_2: bool,
+ /// `RTSEn` RTS hardware flow control enable. If this bit is set to 1,
+ /// RTS hardware flow control is enabled. Data is only requested when
+ /// there is space in the receive FIFO for it to be received.
+ pub rts_hardware_flow_control_enable: bool,
+ /// `CTSEn` CTS hardware flow control enable. If this bit is set to 1,
+ /// CTS hardware flow control is enabled. Data is only transmitted when
+ /// the `nUARTCTS` signal is asserted.
+ pub cts_hardware_flow_control_enable: bool,
+ /// 31:16 - Reserved, do not modify, read as zero.
+ _reserved_zero_no_modify2: u16,
+}
+impl_vmstate_bitsized!(Control);
+
+impl Control {
+ pub fn reset(&mut self) {
+ *self = 0.into();
+ self.set_enable_receive(true);
+ self.set_enable_transmit(true);
+ }
+}
+
+impl Default for Control {
+ fn default() -> Self {
+ let mut ret: Self = 0.into();
+ ret.reset();
+ ret
+ }
+}
+
+/// Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC
+pub struct Interrupt(pub u32);
+
+impl Interrupt {
+ pub const OE: Self = Self(1 << 10);
+ pub const BE: Self = Self(1 << 9);
+ pub const PE: Self = Self(1 << 8);
+ pub const FE: Self = Self(1 << 7);
+ pub const RT: Self = Self(1 << 6);
+ pub const TX: Self = Self(1 << 5);
+ pub const RX: Self = Self(1 << 4);
+ pub const DSR: Self = Self(1 << 3);
+ pub const DCD: Self = Self(1 << 2);
+ pub const CTS: Self = Self(1 << 1);
+ pub const RI: Self = Self(1 << 0);
+
+ pub const E: Self = Self(Self::OE.0 | Self::BE.0 | Self::PE.0 | Self::FE.0);
+ pub const MS: Self = Self(Self::RI.0 | Self::DSR.0 | Self::DCD.0 | Self::CTS.0);
+}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 18/25] rust: pl011: clean up visibilities of callbacks
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (16 preceding siblings ...)
2025-03-09 10:31 ` [PULL 17/25] rust: pl011: move register definitions out of lib.rs Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 19/25] rust: pl011: switch to safe chardev operation Paolo Bonzini
` (6 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Do not make callbacks unnecessarily "pub", they are only used
through function pointers.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 01540654cc9..4cdbbf4b73d 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -537,7 +537,7 @@ fn post_init(&self) {
}
}
- pub fn read(&self, offset: hwaddr, _size: u32) -> u64 {
+ fn read(&self, offset: hwaddr, _size: u32) -> u64 {
match RegisterOffset::try_from(offset) {
Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
let device_id = self.get_class().device_id;
@@ -560,7 +560,7 @@ pub fn read(&self, offset: hwaddr, _size: u32) -> u64 {
}
}
- pub fn write(&self, offset: hwaddr, value: u64, _size: u32) {
+ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
let mut update_irq = false;
if let Ok(field) = RegisterOffset::try_from(offset) {
// qemu_chr_fe_write_all() calls into the can_receive
@@ -621,7 +621,7 @@ pub fn event(&self, event: QEMUChrEvent) {
}
}
- pub fn realize(&self) {
+ 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 {
@@ -638,11 +638,11 @@ pub fn realize(&self) {
}
}
- pub fn reset_hold(&self, _type: ResetType) {
+ fn reset_hold(&self, _type: ResetType) {
self.regs.borrow_mut().reset();
}
- pub fn update(&self) {
+ fn update(&self) {
let regs = self.regs.borrow();
let flags = regs.int_level & regs.int_enabled;
for (irq, i) in self.interrupts.iter().zip(IRQMASK) {
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 19/25] rust: pl011: switch to safe chardev operation
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (17 preceding siblings ...)
2025-03-09 10:31 ` [PULL 18/25] rust: pl011: clean up visibilities of callbacks Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-19 19:25 ` Peter Maydell
2025-03-09 10:31 ` [PULL 20/25] rust: pl011: pass around registers::Data Paolo Bonzini
` (5 subsequent siblings)
24 siblings, 1 reply; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Switch bindings::CharBackend with chardev::CharBackend. This removes
occurrences of "unsafe" due to FFI and switches the wrappers for receive,
can_receive and event callbacks to the common ones implemented by
chardev::CharBackend.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 119 +++++++------------------------
1 file changed, 25 insertions(+), 94 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 4cdbbf4b73d..4e282bc9e9d 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -2,18 +2,10 @@
// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::{
- ffi::CStr,
- os::raw::{c_int, c_void},
- ptr::{addr_of, addr_of_mut, NonNull},
-};
+use std::{ffi::CStr, ptr::addr_of_mut};
use qemu_api::{
- bindings::{
- qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers,
- qemu_chr_fe_write_all, CharBackend, QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK,
- },
- chardev::Chardev,
+ chardev::{CharBackend, Chardev, Event},
impl_vmstate_forward,
irq::{IRQState, InterruptSource},
memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
@@ -235,7 +227,7 @@ pub(self) fn write(
&mut self,
offset: RegisterOffset,
value: u32,
- char_backend: *mut CharBackend,
+ char_backend: &CharBackend,
) -> bool {
// eprintln!("write offset {offset} value {value}");
use RegisterOffset::*;
@@ -269,17 +261,9 @@ pub(self) fn write(
self.reset_tx_fifo();
}
let update = (self.line_control.send_break() != new_val.send_break()) && {
- let mut break_enable: c_int = new_val.send_break().into();
- // SAFETY: self.char_backend is a valid CharBackend instance after it's been
- // initialized in realize().
- unsafe {
- qemu_chr_fe_ioctl(
- char_backend,
- CHR_IOCTL_SERIAL_SET_BREAK as i32,
- addr_of_mut!(break_enable).cast::<c_void>(),
- );
- }
- self.loopback_break(break_enable > 0)
+ let break_enable = new_val.send_break();
+ let _ = char_backend.send_break(break_enable);
+ self.loopback_break(break_enable)
};
self.line_control = new_val;
self.set_read_trigger();
@@ -551,9 +535,7 @@ fn read(&self, offset: hwaddr, _size: u32) -> u64 {
let (update_irq, result) = self.regs.borrow_mut().read(field);
if update_irq {
self.update();
- unsafe {
- qemu_chr_fe_accept_input(addr_of!(self.char_backend) as *mut _);
- }
+ self.char_backend.accept_input();
}
result.into()
}
@@ -567,21 +549,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
// callback, so handle writes before entering PL011Registers.
if field == RegisterOffset::DR {
// ??? Check if transmitter is enabled.
- let ch: u8 = value as u8;
- // SAFETY: char_backend is a valid CharBackend instance after it's been
- // initialized in realize().
+ let ch: [u8; 1] = [value as u8];
// XXX this blocks entire thread. Rewrite to use
// qemu_chr_fe_write and background I/O callbacks
- unsafe {
- qemu_chr_fe_write_all(addr_of!(self.char_backend) as *mut _, &ch, 1);
- }
+ let _ = self.char_backend.write_all(&ch);
}
- update_irq = self.regs.borrow_mut().write(
- field,
- value as u32,
- addr_of!(self.char_backend) as *mut _,
- );
+ update_irq = self
+ .regs
+ .borrow_mut()
+ .write(field, value as u32, &self.char_backend);
} else {
eprintln!("write bad offset {offset} value {value}");
}
@@ -590,15 +567,18 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
}
}
- pub fn can_receive(&self) -> bool {
- // trace_pl011_can_receive(s->lcr, s->read_count, r);
+ fn can_receive(&self) -> u32 {
let regs = self.regs.borrow();
- regs.read_count < regs.fifo_depth()
+ // trace_pl011_can_receive(s->lcr, s->read_count, r);
+ u32::from(regs.read_count < regs.fifo_depth())
}
- pub fn receive(&self, ch: u32) {
+ fn receive(&self, buf: &[u8]) {
+ if buf.is_empty() {
+ return;
+ }
let mut regs = self.regs.borrow_mut();
- let update_irq = !regs.loopback_enabled() && regs.put_fifo(ch);
+ let update_irq = !regs.loopback_enabled() && regs.put_fifo(buf[0].into());
// Release the BqlRefCell before calling self.update()
drop(regs);
@@ -607,10 +587,10 @@ pub fn receive(&self, ch: u32) {
}
}
- pub fn event(&self, event: QEMUChrEvent) {
+ fn event(&self, event: Event) {
let mut update_irq = false;
let mut regs = self.regs.borrow_mut();
- if event == QEMUChrEvent::CHR_EVENT_BREAK && !regs.loopback_enabled() {
+ if event == Event::CHR_EVENT_BREAK && !regs.loopback_enabled() {
update_irq = regs.put_fifo(registers::Data::BREAK.into());
}
// Release the BqlRefCell before calling self.update()
@@ -622,20 +602,8 @@ pub fn event(&self, event: QEMUChrEvent) {
}
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!(self.char_backend) as *mut CharBackend,
- Some(pl011_can_receive),
- Some(pl011_receive),
- Some(pl011_event),
- None,
- addr_of!(*self).cast::<c_void>() as *mut c_void,
- core::ptr::null_mut(),
- true,
- );
- }
+ self.char_backend
+ .enable_handlers(self, Self::can_receive, Self::receive, Self::event);
}
fn reset_hold(&self, _type: ResetType) {
@@ -666,43 +634,6 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
Interrupt::E.0,
];
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011State`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-pub unsafe extern "C" fn pl011_can_receive(opaque: *mut c_void) -> c_int {
- let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
- unsafe { state.as_ref().can_receive().into() }
-}
-
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011State`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-///
-/// The buffer and size arguments must also be valid.
-pub unsafe extern "C" fn pl011_receive(opaque: *mut c_void, buf: *const u8, size: c_int) {
- let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
- unsafe {
- if size > 0 {
- debug_assert!(!buf.is_null());
- state.as_ref().receive(u32::from(buf.read_volatile()));
- }
- }
-}
-
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011State`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-pub unsafe extern "C" fn pl011_event(opaque: *mut c_void, event: QEMUChrEvent) {
- let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
- unsafe { state.as_ref().event(event) }
-}
-
/// # Safety
///
/// We expect the FFI user of this function to pass a valid pointer for `chr`
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PULL 19/25] rust: pl011: switch to safe chardev operation
2025-03-09 10:31 ` [PULL 19/25] rust: pl011: switch to safe chardev operation Paolo Bonzini
@ 2025-03-19 19:25 ` Peter Maydell
2025-03-19 20:51 ` Paolo Bonzini
0 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2025-03-19 19:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Sun, 9 Mar 2025 at 10:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Switch bindings::CharBackend with chardev::CharBackend. This removes
> occurrences of "unsafe" due to FFI and switches the wrappers for receive,
> can_receive and event callbacks to the common ones implemented by
> chardev::CharBackend.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Hi -- this commit seems to have broken use of the PL011 in
boards/SoCs that directly embed it in their state structs, so
"qemu-system-arm -M raspi2b -display none" now asserts on startup.
The Rust PL011's state struct size is now larger than the
C state struct size, so it trips the assert in the QOM code
that we didn't try to initialize a type into less memory than
it needs. (I don't understand *why* the type size has changed,
because this commit doesn't at first glance seem to be adding
anything to the state struct...but it definitely goes up from
0x540 to 0x550.)
(It would be good if we had a compile time check that the state
struct sizes matched between Rust and C, rather than having it
only be caught in runtime asserts. This does cause failures in
check-functional, at least, so it's not completely untested.)
Here's the repro and gdb backtrace:
$ gdb --args ./build/rust/qemu-system-arm -M raspi2b -display none
[...]
**
ERROR:../../qom/object.c:562:object_initialize_with_type: assertion
failed: (size >= type->instance_size)
Bail out! ERROR:../../qom/object.c:562:object_initialize_with_type:
assertion failed: (size >= type->instance_size)
Thread 1 "qemu-system-arm" received signal SIGABRT, Aborted.
Download failed: Invalid argument. Continuing without source file
./nptl/./nptl/pthread_kill.c.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimised
out>) at ./nptl/pthread_kill.c:44
warning: 44 ./nptl/pthread_kill.c: No such file or directory
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=<optimised out>) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=<optimised out>) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=<optimised out>,
signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3 0x00007ffff4a4527e in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x00007ffff4a288ff in __GI_abort () at ./stdlib/abort.c:79
#5 0x00007ffff6e58f5b in g_assertion_message
(domain=domain@entry=0x0, file=file@entry=0x55555678fdeb
"../../qom/object.c", line=line@entry=562,
func=func@entry=0x5555567906d0 <__func__.33>
"object_initialize_with_type", message=message@entry=0x555557f7f400
"assertion failed: (size >= type->instance_size)") at
../../../glib/gtestutils.c:3331
#6 0x00007ffff6ec1a97 in g_assertion_message_expr
(domain=0x0, file=0x55555678fdeb "../../qom/object.c", line=562,
func=0x5555567906d0 <__func__.33> "object_initialize_with_type",
expr=<optimised out>) at ../../../glib/gtestutils.c:3357
#7 0x0000555556188bc6 in object_initialize_with_type
(obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at
../../qom/object.c:562
#8 0x0000555556188cb5 in object_initialize (data=0x555557d4e190,
size=1344, typename=0x5555566d9142 "pl011")
at ../../qom/object.c:578
#9 0x0000555556188e3d in object_initialize_child_with_propsv
(parentobj=0x555557d45710, propname=0x5555566d9148 "uart0",
childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011",
errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd360) at
../../qom/object.c:608
#10 0x0000555556188db6 in object_initialize_child_with_props
(parentobj=0x555557d45710, propname=0x5555566d9148 "uart0",
childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011",
errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591
#11 0x0000555556188f3b in object_initialize_child_internal
(parent=0x555557d45710, propname=0x5555566d9148 "uart0",
child=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011")
at ../../qom/object.c:645
#12 0x0000555555d446ea in raspi_peripherals_base_init
(obj=0x555557d45710) at ../../hw/arm/bcm2835_peripherals.c:100
#13 0x0000555556188639 in object_init_with_type (obj=0x555557d45710,
ti=0x5555579d4af0) at ../../qom/object.c:428
#14 0x000055555618861b in object_init_with_type (obj=0x555557d45710,
ti=0x5555579d4950) at ../../qom/object.c:424
#15 0x0000555556188c49 in object_initialize_with_type
(obj=0x555557d45710, size=597040, type=0x5555579d4950)
at ../../qom/object.c:570
#16 0x0000555556188cb5 in object_initialize (data=0x555557d45710,
size=597040, typename=0x555556738ca5 "bcm2835-peripherals")
at ../../qom/object.c:578
#17 0x0000555556188e3d in object_initialize_child_with_propsv
(parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals",
childobj=0x555557d45710, size=597040, type=0x555556738ca5
"bcm2835-peripherals", errp=0x5555578636f8 <error_abort>,
vargs=0x7fffffffd630) at ../../qom/object.c:608
#18 0x0000555556188db6 in object_initialize_child_with_props
(parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals",
childobj=0x555557d45710, size=597040, type=0x555556738ca5
"bcm2835-peripherals", errp=0x5555578636f8 <error_abort>) at
../../qom/object.c:591
#19 0x0000555556188f3b in object_initialize_child_internal
(parent=0x555557d34760, propname=0x555556738cb9 "peripherals",
child=0x555557d45710, size=597040, type=0x555556738ca5
"bcm2835-peripherals") at ../../qom/object.c:645
#20 0x0000555555f07080 in bcm283x_init (obj=0x555557d34760) at
../../hw/arm/bcm2836.c:49
#21 0x0000555556188639 in object_init_with_type (obj=0x555557d34760,
ti=0x5555579af8a0) at ../../qom/object.c:428
#22 0x000055555618861b in object_init_with_type (obj=0x555557d34760,
ti=0x5555579af6e0) at ../../qom/object.c:424
#23 0x0000555556188c49 in object_initialize_with_type
(obj=0x555557d34760, size=666592, type=0x5555579af6e0)
at ../../qom/object.c:570
#24 0x0000555556188cb5 in object_initialize (data=0x555557d34760,
size=666592, typename=0x555556739030 "bcm2836")
at ../../qom/object.c:578
#25 0x0000555556188e3d in object_initialize_child_with_propsv
(parentobj=0x555557d34500, propname=0x55555673917b "soc",
childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836",
errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd8f0) at
../../qom/object.c:608
#26 0x0000555556188db6 in object_initialize_child_with_props
(parentobj=0x555557d34500, propname=0x55555673917b "soc",
childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836",
errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591
#27 0x0000555556188f3b in object_initialize_child_internal
(parent=0x555557d34500, propname=0x55555673917b "soc",
child=0x555557d34760, size=666592, type=0x555556739030 "bcm2836")
at ../../qom/object.c:645
#28 0x0000555555f0859b in raspi_machine_init (machine=0x555557d34500)
at ../../hw/arm/raspi.c:313
#29 0x00005555559d4674 in machine_run_board_init
(machine=0x555557d34500, mem_path=0x0, errp=0x7fffffffda90)
at ../../hw/core/machine.c:1680
#30 0x0000555555d8615b in qemu_init_board () at ../../system/vl.c:2709
#31 0x0000555555d8650c in qmp_x_exit_preconfig (errp=0x555557863700
<error_fatal>) at ../../system/vl.c:2805
#32 0x0000555555d891bf in qemu_init (argc=5, argv=0x7fffffffde48) at
../../system/vl.c:3838
#33 0x000055555634c832 in main (argc=5, argv=0x7fffffffde48) at
../../system/main.c:68
(gdb) frame 7
#7 0x0000555556188bc6 in object_initialize_with_type
(obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at
../../qom/object.c:562
562 g_assert(size >= type->instance_size);
(gdb) print *type
$2 = {name = 0x555557a0bec0 "pl011", class_size = 208, instance_size =
1360, instance_align = 16,
class_init = 0x55555634ede0
<qemu_api::qom::rust_class_init<pl011::device::PL011State>>,
class_base_init = 0x0, class_data = 0x0,
instance_init = 0x55555634f0f0
<qemu_api::qom::rust_instance_init<pl011::device::PL011State>>,
instance_post_init = 0x55555634f1e0
<qemu_api::qom::rust_instance_post_init<pl011::device::PL011State>>,
instance_finalize = 0x55555634eb40
<qemu_api::qom::drop_object<pl011::device::PL011State>>, abstract =
false,
parent = 0x555557a0bee0 "sys-bus-device", parent_type =
0x55555798c650, class = 0x555557a72370, num_interfaces = 0, interfaces
= {
{typename = 0x0} <repeats 32 times>}}
(gdb) print /x type->instance_size
$3 = 0x550
(gdb) print /x size
$4 = 0x540
thanks
-- PMM
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 19/25] rust: pl011: switch to safe chardev operation
2025-03-19 19:25 ` Peter Maydell
@ 2025-03-19 20:51 ` Paolo Bonzini
2025-03-20 10:39 ` Peter Maydell
0 siblings, 1 reply; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-19 20:51 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel
On 3/19/25 20:25, Peter Maydell wrote:
> On Sun, 9 Mar 2025 at 10:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Switch bindings::CharBackend with chardev::CharBackend. This removes
>> occurrences of "unsafe" due to FFI and switches the wrappers for receive,
>> can_receive and event callbacks to the common ones implemented by
>> chardev::CharBackend.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Hi -- this commit seems to have broken use of the PL011 in
> boards/SoCs that directly embed it in their state structs, so
> "qemu-system-arm -M raspi2b -display none" now asserts on startup.
>
> The Rust PL011's state struct size is now larger than the
> C state struct size, so it trips the assert in the QOM code
> that we didn't try to initialize a type into less memory than
> it needs. (I don't understand *why* the type size has changed,
> because this commit doesn't at first glance seem to be adding
> anything to the state struct...but it definitely goes up from
> 0x540 to 0x550.)
Thanks very much for reporting this.
The reason why it changes is that it switches the imported symbol from
bindings::CharBackend (the C struct) to chardev::CharBackend which has
two extra values in it (a count and some debugging info to provide
better backtraces on error). It is guaranteed to _start_ with a
bindings::CharBackend, which is helpful for the qdev property, but it's
bigger.
I don't think there's a good fix other than not using an embedded PL011,
since you probably would not have that option available when using a
device without a Rust equivalent.
Paolo
> (It would be good if we had a compile time check that the state
> struct sizes matched between Rust and C, rather than having it
> only be caught in runtime asserts. This does cause failures in
> check-functional, at least, so it's not completely untested.)
>
> Here's the repro and gdb backtrace:
>
> $ gdb --args ./build/rust/qemu-system-arm -M raspi2b -display none
> [...]
> **
> ERROR:../../qom/object.c:562:object_initialize_with_type: assertion
> failed: (size >= type->instance_size)
> Bail out! ERROR:../../qom/object.c:562:object_initialize_with_type:
> assertion failed: (size >= type->instance_size)
>
> Thread 1 "qemu-system-arm" received signal SIGABRT, Aborted.
> Download failed: Invalid argument. Continuing without source file
> ./nptl/./nptl/pthread_kill.c.
> __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimised
> out>) at ./nptl/pthread_kill.c:44
> warning: 44 ./nptl/pthread_kill.c: No such file or directory
> (gdb) bt
> #0 __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=<optimised out>) at ./nptl/pthread_kill.c:44
> #1 __pthread_kill_internal (signo=6, threadid=<optimised out>) at
> ./nptl/pthread_kill.c:78
> #2 __GI___pthread_kill (threadid=<optimised out>,
> signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> #3 0x00007ffff4a4527e in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4 0x00007ffff4a288ff in __GI_abort () at ./stdlib/abort.c:79
> #5 0x00007ffff6e58f5b in g_assertion_message
> (domain=domain@entry=0x0, file=file@entry=0x55555678fdeb
> "../../qom/object.c", line=line@entry=562,
> func=func@entry=0x5555567906d0 <__func__.33>
> "object_initialize_with_type", message=message@entry=0x555557f7f400
> "assertion failed: (size >= type->instance_size)") at
> ../../../glib/gtestutils.c:3331
> #6 0x00007ffff6ec1a97 in g_assertion_message_expr
> (domain=0x0, file=0x55555678fdeb "../../qom/object.c", line=562,
> func=0x5555567906d0 <__func__.33> "object_initialize_with_type",
> expr=<optimised out>) at ../../../glib/gtestutils.c:3357
> #7 0x0000555556188bc6 in object_initialize_with_type
> (obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at
> ../../qom/object.c:562
> #8 0x0000555556188cb5 in object_initialize (data=0x555557d4e190,
> size=1344, typename=0x5555566d9142 "pl011")
> at ../../qom/object.c:578
> #9 0x0000555556188e3d in object_initialize_child_with_propsv
> (parentobj=0x555557d45710, propname=0x5555566d9148 "uart0",
> childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011",
> errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd360) at
> ../../qom/object.c:608
> #10 0x0000555556188db6 in object_initialize_child_with_props
> (parentobj=0x555557d45710, propname=0x5555566d9148 "uart0",
> childobj=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011",
> errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591
> #11 0x0000555556188f3b in object_initialize_child_internal
> (parent=0x555557d45710, propname=0x5555566d9148 "uart0",
> child=0x555557d4e190, size=1344, type=0x5555566d9142 "pl011")
> at ../../qom/object.c:645
> #12 0x0000555555d446ea in raspi_peripherals_base_init
> (obj=0x555557d45710) at ../../hw/arm/bcm2835_peripherals.c:100
> #13 0x0000555556188639 in object_init_with_type (obj=0x555557d45710,
> ti=0x5555579d4af0) at ../../qom/object.c:428
> #14 0x000055555618861b in object_init_with_type (obj=0x555557d45710,
> ti=0x5555579d4950) at ../../qom/object.c:424
> #15 0x0000555556188c49 in object_initialize_with_type
> (obj=0x555557d45710, size=597040, type=0x5555579d4950)
> at ../../qom/object.c:570
> #16 0x0000555556188cb5 in object_initialize (data=0x555557d45710,
> size=597040, typename=0x555556738ca5 "bcm2835-peripherals")
> at ../../qom/object.c:578
> #17 0x0000555556188e3d in object_initialize_child_with_propsv
> (parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals",
> childobj=0x555557d45710, size=597040, type=0x555556738ca5
> "bcm2835-peripherals", errp=0x5555578636f8 <error_abort>,
> vargs=0x7fffffffd630) at ../../qom/object.c:608
> #18 0x0000555556188db6 in object_initialize_child_with_props
> (parentobj=0x555557d34760, propname=0x555556738cb9 "peripherals",
> childobj=0x555557d45710, size=597040, type=0x555556738ca5
> "bcm2835-peripherals", errp=0x5555578636f8 <error_abort>) at
> ../../qom/object.c:591
> #19 0x0000555556188f3b in object_initialize_child_internal
> (parent=0x555557d34760, propname=0x555556738cb9 "peripherals",
> child=0x555557d45710, size=597040, type=0x555556738ca5
> "bcm2835-peripherals") at ../../qom/object.c:645
> #20 0x0000555555f07080 in bcm283x_init (obj=0x555557d34760) at
> ../../hw/arm/bcm2836.c:49
> #21 0x0000555556188639 in object_init_with_type (obj=0x555557d34760,
> ti=0x5555579af8a0) at ../../qom/object.c:428
> #22 0x000055555618861b in object_init_with_type (obj=0x555557d34760,
> ti=0x5555579af6e0) at ../../qom/object.c:424
> #23 0x0000555556188c49 in object_initialize_with_type
> (obj=0x555557d34760, size=666592, type=0x5555579af6e0)
> at ../../qom/object.c:570
> #24 0x0000555556188cb5 in object_initialize (data=0x555557d34760,
> size=666592, typename=0x555556739030 "bcm2836")
> at ../../qom/object.c:578
> #25 0x0000555556188e3d in object_initialize_child_with_propsv
> (parentobj=0x555557d34500, propname=0x55555673917b "soc",
> childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836",
> errp=0x5555578636f8 <error_abort>, vargs=0x7fffffffd8f0) at
> ../../qom/object.c:608
> #26 0x0000555556188db6 in object_initialize_child_with_props
> (parentobj=0x555557d34500, propname=0x55555673917b "soc",
> childobj=0x555557d34760, size=666592, type=0x555556739030 "bcm2836",
> errp=0x5555578636f8 <error_abort>) at ../../qom/object.c:591
> #27 0x0000555556188f3b in object_initialize_child_internal
> (parent=0x555557d34500, propname=0x55555673917b "soc",
> child=0x555557d34760, size=666592, type=0x555556739030 "bcm2836")
> at ../../qom/object.c:645
> #28 0x0000555555f0859b in raspi_machine_init (machine=0x555557d34500)
> at ../../hw/arm/raspi.c:313
> #29 0x00005555559d4674 in machine_run_board_init
> (machine=0x555557d34500, mem_path=0x0, errp=0x7fffffffda90)
> at ../../hw/core/machine.c:1680
> #30 0x0000555555d8615b in qemu_init_board () at ../../system/vl.c:2709
> #31 0x0000555555d8650c in qmp_x_exit_preconfig (errp=0x555557863700
> <error_fatal>) at ../../system/vl.c:2805
> #32 0x0000555555d891bf in qemu_init (argc=5, argv=0x7fffffffde48) at
> ../../system/vl.c:3838
> #33 0x000055555634c832 in main (argc=5, argv=0x7fffffffde48) at
> ../../system/main.c:68
> (gdb) frame 7
> #7 0x0000555556188bc6 in object_initialize_with_type
> (obj=0x555557d4e190, size=1344, type=0x555557a0bd40) at
> ../../qom/object.c:562
> 562 g_assert(size >= type->instance_size);
> (gdb) print *type
> $2 = {name = 0x555557a0bec0 "pl011", class_size = 208, instance_size =
> 1360, instance_align = 16,
> class_init = 0x55555634ede0
> <qemu_api::qom::rust_class_init<pl011::device::PL011State>>,
> class_base_init = 0x0, class_data = 0x0,
> instance_init = 0x55555634f0f0
> <qemu_api::qom::rust_instance_init<pl011::device::PL011State>>,
> instance_post_init = 0x55555634f1e0
> <qemu_api::qom::rust_instance_post_init<pl011::device::PL011State>>,
> instance_finalize = 0x55555634eb40
> <qemu_api::qom::drop_object<pl011::device::PL011State>>, abstract =
> false,
> parent = 0x555557a0bee0 "sys-bus-device", parent_type =
> 0x55555798c650, class = 0x555557a72370, num_interfaces = 0, interfaces
> = {
> {typename = 0x0} <repeats 32 times>}}
> (gdb) print /x type->instance_size
> $3 = 0x550
> (gdb) print /x size
> $4 = 0x540
>
> thanks
> -- PMM
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 19/25] rust: pl011: switch to safe chardev operation
2025-03-19 20:51 ` Paolo Bonzini
@ 2025-03-20 10:39 ` Peter Maydell
0 siblings, 0 replies; 30+ messages in thread
From: Peter Maydell @ 2025-03-20 10:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Wed, 19 Mar 2025 at 20:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/19/25 20:25, Peter Maydell wrote:
> > Hi -- this commit seems to have broken use of the PL011 in
> > boards/SoCs that directly embed it in their state structs, so
> > "qemu-system-arm -M raspi2b -display none" now asserts on startup.
> >
> > The Rust PL011's state struct size is now larger than the
> > C state struct size, so it trips the assert in the QOM code
> > that we didn't try to initialize a type into less memory than
> > it needs.
> The reason why it changes is that it switches the imported symbol from
> bindings::CharBackend (the C struct) to chardev::CharBackend which has
> two extra values in it (a count and some debugging info to provide
> better backtraces on error). It is guaranteed to _start_ with a
> bindings::CharBackend, which is helpful for the qdev property, but it's
> bigger.
>
> I don't think there's a good fix other than not using an embedded PL011,
> since you probably would not have that option available when using a
> device without a Rust equivalent.
(do you mean "without a C equivalent" there?)
Hmm. The embedded-struct approach to devices is extremely common,
especially in more recent QEMU C code. Generally those users of
an embedded struct don't actually poke around inside the struct,
so we don't need to have the Rust code match the C layout, but
we do at least need the size to be no bigger than the C version.
(There are some exceptions for some devices, not including the PL011,
where people do poke around inside the struct of a device they've
created...)
There has been discussion that we ought to move (back!) to a
"users of the device just get a pointer to it and the struct
is opaque to them" design pattern -- command line generation
and wiring up of machines would also make that a more natural
approach -- but that's a long term process we'd need to plan
and go through.
I had some ideas a long time ago for making "code outside the
C device implementation touched a field in the device struct"
be a compile error using attribute("deprecated") hidden via macros,
so we could resurrect that as a way to confirm that nobody is
trying to do dubious things before we do a C-to-rust conversion
of a device model.
For the moment, I guess the expedient thing to do would be to add
an extra 16 bytes of padding to the C PL011State struct ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PULL 20/25] rust: pl011: pass around registers::Data
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (18 preceding siblings ...)
2025-03-09 10:31 ` [PULL 19/25] rust: pl011: switch to safe chardev operation Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 21/25] rust: hpet: decode HPET registers into enums Paolo Bonzini
` (4 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
The values stored in the Fifo are instances of the bitfield-struct
registers::Data. Convert as soon as possible the value written
into DR, and always refer to the bitfield struct; it's generally
cleaner other than PL011State::receive having to do a double
conversion u8=>u32=>registers::Data.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 4e282bc9e9d..af93ae8bebe 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -234,7 +234,7 @@ pub(self) fn write(
match offset {
DR => {
// interrupts always checked
- let _ = self.loopback_tx(value);
+ let _ = self.loopback_tx(value.into());
self.int_level |= Interrupt::TX.0;
return true;
}
@@ -301,7 +301,7 @@ pub(self) fn write(
#[inline]
#[must_use]
- fn loopback_tx(&mut self, value: u32) -> bool {
+ fn loopback_tx(&mut self, value: registers::Data) -> bool {
// Caveat:
//
// In real hardware, TX loopback happens at the serial-bit level
@@ -370,7 +370,7 @@ fn loopback_mdmctrl(&mut self) -> bool {
}
fn loopback_break(&mut self, enable: bool) -> bool {
- enable && self.loopback_tx(registers::Data::BREAK.into())
+ enable && self.loopback_tx(registers::Data::BREAK)
}
fn set_read_trigger(&mut self) {
@@ -429,11 +429,11 @@ pub fn fifo_depth(&self) -> u32 {
}
#[must_use]
- pub fn put_fifo(&mut self, value: u32) -> bool {
+ pub fn put_fifo(&mut self, value: registers::Data) -> bool {
let depth = self.fifo_depth();
assert!(depth > 0);
let slot = (self.read_pos + self.read_count) & (depth - 1);
- self.read_fifo[slot] = registers::Data::from(value);
+ self.read_fifo[slot] = value;
self.read_count += 1;
self.flags.set_receive_fifo_empty(false);
if self.read_count == depth {
@@ -578,7 +578,8 @@ fn receive(&self, buf: &[u8]) {
return;
}
let mut regs = self.regs.borrow_mut();
- let update_irq = !regs.loopback_enabled() && regs.put_fifo(buf[0].into());
+ let c: u32 = buf[0].into();
+ let update_irq = !regs.loopback_enabled() && regs.put_fifo(c.into());
// Release the BqlRefCell before calling self.update()
drop(regs);
@@ -591,7 +592,7 @@ fn event(&self, event: Event) {
let mut update_irq = false;
let mut regs = self.regs.borrow_mut();
if event == Event::CHR_EVENT_BREAK && !regs.loopback_enabled() {
- update_irq = regs.put_fifo(registers::Data::BREAK.into());
+ update_irq = regs.put_fifo(registers::Data::BREAK);
}
// Release the BqlRefCell before calling self.update()
drop(regs);
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 21/25] rust: hpet: decode HPET registers into enums
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (19 preceding siblings ...)
2025-03-09 10:31 ` [PULL 20/25] rust: pl011: pass around registers::Data Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-21 12:03 ` Peter Maydell
2025-03-09 10:31 ` [PULL 22/25] rust: cell: add full example of declaring a SysBusDevice Paolo Bonzini
` (3 subsequent siblings)
24 siblings, 1 reply; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
Generalize timer_and_addr() to decode all registers into a single enum
HPETRegister, and use the TryInto derive to separate valid and
invalid values.
The main advantage lies in checking that all registers are enumerated
in the "match" statements.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/Cargo.toml | 2 +
rust/hw/char/pl011/src/lib.rs | 2 -
rust/hw/timer/hpet/src/hpet.rs | 222 +++++++++++++++++----------------
3 files changed, 119 insertions(+), 107 deletions(-)
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 5041d6291fd..ab1185a8143 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -37,6 +37,8 @@ result_unit_err = "allow"
should_implement_trait = "deny"
# can be for a reason, e.g. in callbacks
unused_self = "allow"
+# common in device crates
+upper_case_acronyms = "allow"
# default-allow lints
as_ptr_cast_mut = "deny"
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 45c13ba899e..dbae76991c9 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -12,8 +12,6 @@
//! See [`PL011State`](crate::device::PL011State) for the device model type and
//! the [`registers`] module for register types.
-#![allow(clippy::upper_case_acronyms)]
-
use qemu_api::c_str;
mod device;
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index d989360ede8..20e0afdfca8 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -48,8 +48,6 @@
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). Revision 1 is implemented (refer to v1.0a spec).
const HPET_CAP_REV_ID_VALUE: u64 = 0x1;
const HPET_CAP_REV_ID_SHIFT: usize = 0;
@@ -65,8 +63,6 @@
/// 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;
/// Legacy Replacement Route (bit 1)
@@ -74,14 +70,6 @@
/// 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;
@@ -109,11 +97,51 @@
/// 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;
+#[derive(qemu_api_macros::TryInto)]
+#[repr(u64)]
+#[allow(non_camel_case_types)]
+/// Timer registers, masked by 0x18
+enum TimerRegister {
+ /// Timer N Configuration and Capability Register
+ CFG = 0,
+ /// Timer N Comparator Value Register
+ CMP = 8,
+ /// Timer N FSB Interrupt Route Register
+ ROUTE = 16,
+}
-/// Timer N FSB Interrupt Route Register (masked by 0x18)
-const HPET_TN_FSB_ROUTE_REG: u64 = 0x010;
+#[derive(qemu_api_macros::TryInto)]
+#[repr(u64)]
+#[allow(non_camel_case_types)]
+/// Global registers
+enum GlobalRegister {
+ /// General Capabilities and ID Register
+ CAP = 0,
+ /// General Configuration Register
+ CFG = 0x10,
+ /// General Interrupt Status Register
+ INT_STATUS = 0x20,
+ /// Main Counter Value Register
+ COUNTER = 0xF0,
+}
+
+enum HPETRegister<'a> {
+ /// Global register in the range from `0` to `0xff`
+ Global(GlobalRegister),
+
+ /// Register in the timer block `0x100`...`0x3ff`
+ Timer(&'a BqlRefCell<HPETTimer>, TimerRegister),
+
+ /// Invalid address
+ #[allow(dead_code)]
+ Unknown(hwaddr),
+}
+
+struct HPETAddrDecode<'a> {
+ shift: u32,
+ len: u32,
+ reg: HPETRegister<'a>,
+}
const fn hpet_next_wrap(cur_tick: u64) -> u64 {
(cur_tick | 0xffffffff) + 1
@@ -471,33 +499,21 @@ fn callback(&mut self) {
self.update_irq(true);
}
- const fn read(&self, addr: hwaddr, _size: u32) -> u64 {
- let shift: u64 = (addr & 4) * 8;
-
- match addr & !4 {
- 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
- }
+ const fn read(&self, reg: TimerRegister) -> u64 {
+ use TimerRegister::*;
+ match reg {
+ CFG => self.config, // including interrupt capabilities
+ CMP => self.cmp, // comparator register
+ ROUTE => self.fsb,
}
}
- fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
- let shift = ((addr & 4) * 8) as u32;
- let len = std::cmp::min(size * 8, 64 - shift);
-
- match addr & !4 {
- 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.
- }
+ fn write(&mut self, reg: TimerRegister, value: u64, shift: u32, len: u32) {
+ use TimerRegister::*;
+ match reg {
+ CFG => self.set_tn_cfg_reg(shift, len, value),
+ CMP => self.set_tn_cmp_reg(shift, len, value),
+ ROUTE => self.set_tn_fsb_route_reg(shift, len, value),
}
}
}
@@ -749,77 +765,73 @@ fn reset_hold(&self, _type: ResetType) {
self.rtc_irq_level.set(0);
}
- fn timer_and_addr(&self, addr: hwaddr) -> Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
- let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
-
- // TODO: Add trace point - trace_hpet_ram_[read|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)
- None
- } else {
- // Keep the complete address so that HPETTimer's read and write could
- // detect the invalid access.
- Some((&self.timers[timer_id], addr & 0x1F))
- }
- }
-
- fn read(&self, addr: hwaddr, size: u32) -> u64 {
- let shift: u64 = (addr & 4) * 8;
-
- // address range of all TN regs
- // TODO: Add trace point - trace_hpet_ram_read(addr)
- if (0x100..=0x3ff).contains(&addr) {
- match self.timer_and_addr(addr) {
- None => 0, // Reserved,
- Some((timer, tn_addr)) => timer.borrow_mut().read(tn_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(&self, addr: hwaddr, value: u64, size: u32) {
+ fn decode(&self, mut addr: hwaddr, size: u32) -> HPETAddrDecode {
let shift = ((addr & 4) * 8) as u32;
let len = std::cmp::min(size * 8, 64 - shift);
- // TODO: Add trace point - trace_hpet_ram_write(addr, value)
- if (0x100..=0x3ff).contains(&addr) {
- match self.timer_and_addr(addr) {
- None => (), // Reserved.
- Some((timer, tn_addr)) => timer.borrow_mut().write(tn_addr, value, size),
- }
+ addr &= !4;
+ let reg = if (0..=0xff).contains(&addr) {
+ GlobalRegister::try_from(addr).map(HPETRegister::Global)
} 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.
+ let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
+ if timer_id <= self.num_timers.get() {
+ // TODO: Add trace point - trace_hpet_ram_[read|write]_timer_id(timer_id)
+ TimerRegister::try_from(addr)
+ .map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg))
+ } else {
+ // TODO: Add trace point - trace_hpet_timer_id_out_of_range(timer_id)
+ Err(addr)
+ }
+ };
+
+ // reg is now a Result<HPETRegister, hwaddr>
+ // convert the Err case into HPETRegister as well
+ let reg = reg.unwrap_or_else(HPETRegister::Unknown);
+ HPETAddrDecode { shift, len, reg }
+ }
+
+ fn read(&self, addr: hwaddr, size: u32) -> u64 {
+ // TODO: Add trace point - trace_hpet_ram_read(addr)
+ let HPETAddrDecode { shift, reg, .. } = self.decode(addr, size);
+
+ use GlobalRegister::*;
+ use HPETRegister::*;
+ (match reg {
+ Timer(timer, tn_reg) => timer.borrow_mut().read(tn_reg),
+ Global(CAP) => self.capability.get(), /* including HPET_PERIOD 0x004 */
+ Global(CFG) => self.config.get(),
+ Global(INT_STATUS) => self.int_status.get(),
+ Global(COUNTER) => {
+ // TODO: Add trace point
+ // trace_hpet_ram_read_reading_counter(addr & 4, cur_tick)
+ if self.is_hpet_enabled() {
+ self.get_ticks()
+ } else {
+ self.counter.get()
}
}
+ Unknown(_) => {
+ // TODO: Add trace point- trace_hpet_ram_read_invalid()
+ 0
+ }
+ }) >> shift
+ }
+
+ fn write(&self, addr: hwaddr, value: u64, size: u32) {
+ let HPETAddrDecode { shift, len, reg } = self.decode(addr, size);
+
+ // TODO: Add trace point - trace_hpet_ram_write(addr, value)
+ use GlobalRegister::*;
+ use HPETRegister::*;
+ match reg {
+ Timer(timer, tn_reg) => timer.borrow_mut().write(tn_reg, value, shift, len),
+ Global(CAP) => {} // General Capabilities and ID Register: Read Only
+ Global(CFG) => self.set_cfg_reg(shift, len, value),
+ Global(INT_STATUS) => self.set_int_status_reg(shift, len, value),
+ Global(COUNTER) => self.set_counter_reg(shift, len, value),
+ Unknown(_) => {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid()
+ }
}
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PULL 21/25] rust: hpet: decode HPET registers into enums
2025-03-09 10:31 ` [PULL 21/25] rust: hpet: decode HPET registers into enums Paolo Bonzini
@ 2025-03-21 12:03 ` Peter Maydell
0 siblings, 0 replies; 30+ messages in thread
From: Peter Maydell @ 2025-03-21 12:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Sun, 9 Mar 2025 at 10:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Generalize timer_and_addr() to decode all registers into a single enum
> HPETRegister, and use the TryInto derive to separate valid and
> invalid values.
>
> The main advantage lies in checking that all registers are enumerated
> in the "match" statements.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Hi; this commit seems to break 'make check-functional'. Several
x86 tests fail; the one I looked at was x86_64_tuxrun:
$ (cd build/rust; PYTHONPATH=../../python:../../tests/functional
QEMU_TEST_QEMU_BINARY=./qemu-system-x86_64 ./pyvenv/bin/python3
../../tests/functional/test_x86_64_tuxrun.py; )
TAP version 13
/home/petmay01/.cache/qemu/download/4b8b2a99117519c5290e1202cb36eb6c7aaba92b357b5160f5970cf5fb78a751:
1073741824 bytes
Traceback (most recent call last):
File "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/tests/functional/test_x86_64_tuxrun.py",
line 31, in test_x86_64
self.common_tuxrun(kernel_asset=self.ASSET_X86_64_KERNEL,
File "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/tests/functional/qemu_test/tuxruntest.py",
line 147, in common_tuxrun
self.run_tuxtest_tests(haltmsg)
File "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/tests/functional/qemu_test/tuxruntest.py",
line 117, in run_tuxtest_tests
self.wait_for_console_pattern('tuxtest login:')
File "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/tests/functional/qemu_test/tuxruntest.py",
line 67, in wait_for_console_pattern
wait_for_console_pattern(self, success_message,
File "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/tests/functional/qemu_test/cmd.py",
line 160, in wait_for_console_pattern
_console_interaction(test, success_message, failure_message, None, vm=vm)
File "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/tests/functional/qemu_test/cmd.py",
line 116, in _console_interaction
if _console_read_line_until_match(test, vm,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/tests/functional/qemu_test/cmd.py",
line 67, in _console_read_line_until_match
test.fail(
AssertionError: 'b'Kernel panic - not syncing'' found in console,
expected 'b'tuxtest login:''
not ok 1 test_x86_64_tuxrun.TuxRunX86Test.test_x86_64
1..1
The console output from the guest shows that it fails trying to set
up the timer:
2025-03-21 12:01:10,676: printk: console [ttyS0] enabled
2025-03-21 12:01:10,678: ACPI: Core revision 20230331
2025-03-21 12:01:10,695: clocksource: hpet: mask: 0xffffffff
max_cycles: 0xffffffff, max_idle_ns: 19112604467 ns
2025-03-21 12:01:10,714: APIC: Switch to symmetric I/O mode setup
2025-03-21 12:01:10,728: ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
2025-03-21 12:01:10,774: ..MP-BIOS bug: 8254 timer not connected to IO-APIC
2025-03-21 12:01:10,774: ...trying to set up timer (IRQ0) through the 8259A ...
2025-03-21 12:01:10,775: ..... (found apic 0 pin 2) ...
2025-03-21 12:01:10,818: ....... failed.
2025-03-21 12:01:10,818: ...trying to set up timer as Virtual Wire IRQ...
2025-03-21 12:01:10,862: ..... failed.
2025-03-21 12:01:10,862: ...trying to set up timer as ExtINT IRQ...
2025-03-21 12:01:10,910: ..... failed :(.
Incidentally, this is the second case of a 'make check-functional'
failure I've found in this pullreq, which suggests we should improve
our CI coverage so that it's doing a check-functional test on
a config with Rust enabled for at least the arm and x86 targets.
thanks
-- PMM
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PULL 22/25] rust: cell: add full example of declaring a SysBusDevice
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (20 preceding siblings ...)
2025-03-09 10:31 ` [PULL 21/25] rust: hpet: decode HPET registers into enums Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 23/25] rust: qom: remove operations on &mut Paolo Bonzini
` (2 subsequent siblings)
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/cell.rs | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index 448638e8967..ab0785a2692 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -73,6 +73,34 @@
//! QEMU device implementations is usually incorrect and can lead to
//! thread-safety issues.
//!
+//! ### Example
+//!
+//! ```
+//! # use qemu_api::prelude::*;
+//! # use qemu_api::{c_str, cell::BqlRefCell, irq::InterruptSource, irq::IRQState};
+//! # use qemu_api::{sysbus::SysBusDevice, qom::Owned, qom::ParentField};
+//! # const N_GPIOS: usize = 8;
+//! # struct PL061Registers { /* ... */ }
+//! # unsafe impl ObjectType for PL061State {
+//! # type Class = <SysBusDevice as ObjectType>::Class;
+//! # const TYPE_NAME: &'static std::ffi::CStr = c_str!("pl061");
+//! # }
+//! struct PL061State {
+//! parent_obj: ParentField<SysBusDevice>,
+//!
+//! // Configuration is read-only after initialization
+//! pullups: u32,
+//! pulldowns: u32,
+//!
+//! // Single values shared with C code use BqlCell, in this case via InterruptSource
+//! out: [InterruptSource; N_GPIOS],
+//! interrupt: InterruptSource,
+//!
+//! // Larger state accessed by device methods uses BqlRefCell or Mutex
+//! registers: BqlRefCell<PL061Registers>,
+//! }
+//! ```
+//!
//! ### `BqlCell<T>`
//!
//! [`BqlCell<T>`] implements interior mutability by moving values in and out of
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 23/25] rust: qom: remove operations on &mut
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (21 preceding siblings ...)
2025-03-09 10:31 ` [PULL 22/25] rust: cell: add full example of declaring a SysBusDevice Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 24/25] meson.build: default to -gsplit-dwarf for debug info Paolo Bonzini
2025-03-09 10:31 ` [PULL 25/25] rust: pl011: Allow NULL chardev argument to pl011_create() Paolo Bonzini
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel
The dubious casts of mutable references to objects are not used
anymore: the wrappers for qdev_init_clock_in and for IRQ and MMIO
initialization can be called directly on the subclasses, without
casts, plus they take a shared reference so they can just use
"upcast()" instead of "upcast_mut()". Remove them.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/memory.rs | 5 ---
rust/qemu-api/src/prelude.rs | 1 -
rust/qemu-api/src/qom.rs | 83 ------------------------------------
rust/qemu-api/tests/tests.rs | 34 +--------------
4 files changed, 2 insertions(+), 121 deletions(-)
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index eff9f09fd7f..fdb1ea11fcf 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -175,11 +175,6 @@ pub fn init_io<T: IsA<Object>>(
) {
unsafe {
Self::do_init_io(
- // self.0.as_mut_ptr() needed because Rust tries to call
- // ObjectDeref::as_mut_ptr() on "&mut Self", instead of coercing
- // to "&Self" and then calling MemoryRegion::as_mut_ptr().
- // Revisit if/when ObjectCastMut is not needed anymore; it is
- // only used in a couple places for initialization.
self.0.as_mut_ptr(),
owner.cast::<Object>(),
&ops.0,
diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
index 634acf37a85..43bfcd5fcab 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -17,7 +17,6 @@
pub use crate::qom::IsA;
pub use crate::qom::Object;
pub use crate::qom::ObjectCast;
-pub use crate::qom::ObjectCastMut;
pub use crate::qom::ObjectDeref;
pub use crate::qom::ObjectClassMethods;
pub use crate::qom::ObjectMethods;
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 2defbd23516..34d7bc0ef96 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -463,90 +463,7 @@ unsafe fn unsafe_cast<'a, U: ObjectType>(self) -> &'a U
impl<T: ObjectType> ObjectDeref for &T {}
impl<T: ObjectType> ObjectCast for &T {}
-/// Trait for mutable type casting operations in the QOM hierarchy.
-///
-/// This trait provides the mutable counterparts to [`ObjectCast`]'s conversion
-/// functions. Unlike `ObjectCast`, this trait returns `Result` for fallible
-/// conversions to preserve the original smart pointer if the cast fails. This
-/// is necessary because mutable references cannot be copied, so a failed cast
-/// must return ownership of the original reference. For example:
-///
-/// ```ignore
-/// let mut dev = get_device();
-/// // If this fails, we need the original `dev` back to try something else
-/// match dev.dynamic_cast_mut::<FooDevice>() {
-/// Ok(foodev) => /* use foodev */,
-/// Err(dev) => /* still have ownership of dev */
-/// }
-/// ```
-pub trait ObjectCastMut: Sized + ObjectDeref + DerefMut
-where
- Self::Target: ObjectType,
-{
- /// Safely convert from a derived type to one of its parent types.
- ///
- /// This is always safe; the [`IsA`] trait provides static verification
- /// that `Self` dereferences to `U` or a child of `U`.
- fn upcast_mut<'a, U: ObjectType>(self) -> &'a mut U
- where
- Self::Target: IsA<U>,
- Self: 'a,
- {
- // SAFETY: soundness is declared via IsA<U>, which is an unsafe trait
- unsafe { self.unsafe_cast_mut::<U>() }
- }
-
- /// Attempt to convert to a derived type.
- ///
- /// Returns `Ok(..)` if the object is of type `U`, or `Err(self)` if the
- /// object if the conversion failed. This is verified at runtime by
- /// checking the object's type information.
- fn downcast_mut<'a, U: IsA<Self::Target>>(self) -> Result<&'a mut U, Self>
- where
- Self: 'a,
- {
- self.dynamic_cast_mut::<U>()
- }
-
- /// Attempt to convert between any two types in the QOM hierarchy.
- ///
- /// Returns `Ok(..)` if the object is of type `U`, or `Err(self)` if the
- /// object if the conversion failed. This is verified at runtime by
- /// checking the object's type information.
- fn dynamic_cast_mut<'a, U: ObjectType>(self) -> Result<&'a mut U, Self>
- where
- Self: 'a,
- {
- unsafe {
- // SAFETY: upcasting to Object is always valid, and the
- // return type is either NULL or the argument itself
- let result: *mut U =
- object_dynamic_cast(self.as_object_mut_ptr(), U::TYPE_NAME.as_ptr()).cast();
-
- result.as_mut().ok_or(self)
- }
- }
-
- /// Convert to any QOM type without verification.
- ///
- /// # Safety
- ///
- /// What safety? You need to know yourself that the cast is correct; only
- /// use when performance is paramount. It is still better than a raw
- /// pointer `cast()`, which does not even check that you remain in the
- /// realm of QOM `ObjectType`s.
- ///
- /// `unsafe_cast::<Object>()` is always safe.
- unsafe fn unsafe_cast_mut<'a, U: ObjectType>(self) -> &'a mut U
- where
- Self: 'a,
- {
- unsafe { &mut *self.as_mut_ptr::<Self::Target>().cast::<U>() }
- }
-}
-
impl<T: ObjectType> ObjectDeref for &mut T {}
-impl<T: ObjectType> ObjectCastMut for &mut T {}
/// Trait a type must implement to be registered with QEMU.
pub trait ObjectImpl: ObjectType + IsA<Object> {
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index e3985782a38..269122e7ec1 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -2,13 +2,10 @@
// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::{
- ffi::{c_void, CStr},
- ptr::{addr_of, addr_of_mut},
-};
+use std::{ffi::CStr, ptr::addr_of};
use qemu_api::{
- bindings::{module_call_init, module_init_type, object_new, object_unref, qdev_prop_bool},
+ bindings::{module_call_init, module_init_type, qdev_prop_bool},
c_str,
cell::{self, BqlCell},
declare_properties, define_property,
@@ -182,30 +179,3 @@ fn test_cast() {
assert_eq!(addr_of!(*sbd_ref), p_ptr.cast());
}
}
-
-#[test]
-#[allow(clippy::shadow_unrelated)]
-/// Test casts on mutable references.
-fn test_cast_mut() {
- init_qom();
- let p: *mut DummyState = unsafe { object_new(DummyState::TYPE_NAME.as_ptr()).cast() };
-
- let p_ref: &mut DummyState = unsafe { &mut *p };
- let obj_ref: &mut Object = p_ref.upcast_mut();
- assert_eq!(addr_of_mut!(*obj_ref), p.cast());
-
- let sbd_ref: Result<&mut SysBusDevice, &mut Object> = obj_ref.dynamic_cast_mut();
- let obj_ref = sbd_ref.unwrap_err();
-
- let dev_ref: Result<&mut DeviceState, &mut Object> = obj_ref.downcast_mut();
- let dev_ref = dev_ref.unwrap();
- assert_eq!(addr_of_mut!(*dev_ref), p.cast());
-
- // SAFETY: the cast is wrong, but the value is only used for comparison
- unsafe {
- let sbd_ref: &mut SysBusDevice = obj_ref.unsafe_cast_mut();
- assert_eq!(addr_of_mut!(*sbd_ref), p.cast());
-
- object_unref(p_ref.as_object_mut_ptr().cast::<c_void>());
- }
-}
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 24/25] meson.build: default to -gsplit-dwarf for debug info
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (22 preceding siblings ...)
2025-03-09 10:31 ` [PULL 23/25] rust: qom: remove operations on &mut Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
2025-03-09 10:31 ` [PULL 25/25] rust: pl011: Allow NULL chardev argument to pl011_create() Paolo Bonzini
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Bennée, Daniel P. Berrangé
From: Alex Bennée <alex.bennee@linaro.org>
This option is supported by both gcc (since 4.7) and clang (since
7.0). Not only does this make the linkers job easier by reducing the
amount of ELF it needs to parse it also reduces the total build size
quite considerably. In my case a default build went from 5.8G to
3.9G (vs 1.9G for --disable-debug-info).
The --disable-split-debug option allows distros to keep all the info
together for ease of packaging.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Link: https://lore.kernel.org/r/20250306161631.2477685-1-alex.bennee@linaro.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
meson.build | 6 ++++++
meson_options.txt | 2 ++
scripts/meson-buildoptions.sh | 2 ++
3 files changed, 10 insertions(+)
diff --git a/meson.build b/meson.build
index 6da4eb317c2..4899d896de0 100644
--- a/meson.build
+++ b/meson.build
@@ -601,6 +601,10 @@ if get_option('tsan')
qemu_ldflags = ['-fsanitize=thread'] + qemu_ldflags
endif
+if get_option('debug') and get_option('split_debug')
+ qemu_cflags += '-gsplit-dwarf'
+endif
+
# Detect support for PT_GNU_RELRO + DT_BIND_NOW.
# The combination is known as "full relro", because .got.plt is read-only too.
qemu_ldflags += cc.get_supported_link_arguments('-Wl,-z,relro', '-Wl,-z,now')
@@ -4583,6 +4587,8 @@ if have_rust
summary_info += {'bindgen': bindgen.full_path()}
summary_info += {'bindgen version': bindgen.version()}
endif
+# option_cflags is purely for the summary display, meson will pass
+# -g/-O options directly
option_cflags = (get_option('debug') ? ['-g'] : [])
if get_option('optimization') != 'plain'
option_cflags += ['-O' + get_option('optimization')]
diff --git a/meson_options.txt b/meson_options.txt
index 59d973bca00..3432123fee2 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -362,6 +362,8 @@ option('debug_mutex', type: 'boolean', value: false,
description: 'mutex debugging support')
option('debug_stack_usage', type: 'boolean', value: false,
description: 'measure coroutine stack usage')
+option('split_debug', type: 'boolean', value: true,
+ description: 'split debug info from object files')
option('qom_cast_debug', type: 'boolean', value: true,
description: 'cast debugging support')
option('slirp_smbd', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 3e8e00852b2..aca6e688302 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -504,6 +504,8 @@ _meson_option_parse() {
--disable-strict-rust-lints) printf "%s" -Dstrict_rust_lints=false ;;
--enable-strip) printf "%s" -Dstrip=true ;;
--disable-strip) printf "%s" -Dstrip=false ;;
+ --enable-split-debug) printf "%s" -Dsplit_debug=true ;;
+ --disable-split-debug) printf "%s" -Dsplit_debug=false ;;
--sysconfdir=*) quote_sh "-Dsysconfdir=$2" ;;
--enable-tcg) printf "%s" -Dtcg=enabled ;;
--disable-tcg) printf "%s" -Dtcg=disabled ;;
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 25/25] rust: pl011: Allow NULL chardev argument to pl011_create()
2025-03-09 10:30 [PULL 00/25] (Mostly) Rust patches for QEMU 10.0 soft freeze Paolo Bonzini
` (23 preceding siblings ...)
2025-03-09 10:31 ` [PULL 24/25] meson.build: default to -gsplit-dwarf for debug info Paolo Bonzini
@ 2025-03-09 10:31 ` Paolo Bonzini
24 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2025-03-09 10:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Philippe Mathieu-Daudé
From: Peter Maydell <peter.maydell@linaro.org>
It's valid for the caller to pass a NULL chardev to pl011_create();
this means "don't set the chardev property on the device", which
in turn means "act like there's no chardev". All the chardev
frontend APIs (in C, at least) accept a NULL pointer to mean
"do nothing".
This fixes some failures in 'make check-functional' when Rust support
is enabled.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Link: https://lore.kernel.org/r/20250307190051.3274226-1-peter.maydell@linaro.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index af93ae8bebe..f137b49feaf 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -648,10 +648,12 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
// SAFETY: The callers promise that they have owned references.
// They do not gift them to pl011_create, so use `Owned::from`.
let irq = unsafe { Owned::<IRQState>::from(&*irq) };
- let chr = unsafe { Owned::<Chardev>::from(&*chr) };
let dev = PL011State::new();
- dev.prop_set_chr("chardev", &chr);
+ if !chr.is_null() {
+ let chr = unsafe { Owned::<Chardev>::from(&*chr) };
+ dev.prop_set_chr("chardev", &chr);
+ }
dev.sysbus_realize();
dev.mmio_map(0, addr);
dev.connect_irq(0, &irq);
--
2.48.1
^ permalink raw reply related [flat|nested] 30+ messages in thread