Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 43 +++---
rust/hw/char/pl011/src/lib.rs | 1 -
rust/hw/char/pl011/src/memory_ops.rs | 36 -----
rust/qemu-api/meson.build | 1 +
rust/qemu-api/src/lib.rs | 1 +
rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
rust/qemu-api/src/sysbus.rs | 7 +-
rust/qemu-api/src/zeroable.rs | 12 ++
8 files changed, 234 insertions(+), 58 deletions(-)
delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
create mode 100644 rust/qemu-api/src/memory.rs
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 259efacb046..294394c6e82 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -2,7 +2,7 @@
// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
// SPDX-License-Identifier: GPL-2.0-or-later
-use core::ptr::{addr_of_mut, NonNull};
+use core::ptr::{addr_of, addr_of_mut, NonNull};
use std::{
ffi::CStr,
os::raw::{c_int, c_void},
@@ -12,14 +12,14 @@
bindings::{self, *},
c_str, impl_vmstate_forward,
irq::InterruptSource,
+ memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
prelude::*,
- qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
+ qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
};
use crate::{
device_class,
- memory_ops::PL011_OPS,
registers::{self, Interrupt},
RegisterOffset,
};
@@ -490,20 +490,24 @@ impl PL011State {
/// location/instance. All its fields are expected to hold unitialized
/// values with the sole exception of `parent_obj`.
unsafe fn init(&mut self) {
+ static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
+ .read(&PL011State::read)
+ .write(&PL011State::write)
+ .native_endian()
+ .impl_sizes(4, 4)
+ .build();
+
// SAFETY:
//
// self and self.iomem are guaranteed to be valid at this point since callers
// must make sure the `self` reference is valid.
- unsafe {
- memory_region_init_io(
- addr_of_mut!(self.iomem),
- addr_of_mut!(*self).cast::<Object>(),
- &PL011_OPS,
- addr_of_mut!(*self).cast::<c_void>(),
- Self::TYPE_NAME.as_ptr(),
- 0x1000,
- );
- }
+ MemoryRegion::init_io(
+ unsafe { &mut *addr_of_mut!(self.iomem) },
+ addr_of_mut!(*self),
+ &PL011_OPS,
+ "pl011",
+ 0x1000,
+ );
self.regs = Default::default();
@@ -528,8 +532,7 @@ fn post_init(&self) {
}
}
- #[allow(clippy::needless_pass_by_ref_mut)]
- pub fn read(&mut self, offset: hwaddr, _size: u32) -> u64 {
+ pub fn read(&self, offset: hwaddr, _size: u32) -> u64 {
let (update_irq, result) = match RegisterOffset::try_from(offset) {
Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
let device_id = self.get_class().device_id;
@@ -544,16 +547,20 @@ pub fn read(&mut self, offset: hwaddr, _size: u32) -> u64 {
if update_irq {
self.update();
unsafe {
- qemu_chr_fe_accept_input(&mut self.char_backend);
+ qemu_chr_fe_accept_input(addr_of!(self.char_backend) as *mut _);
}
}
result.into()
}
- pub fn write(&mut self, offset: hwaddr, value: u64) {
+ pub fn write(&self, offset: hwaddr, value: u64, _size: u32) {
let mut update_irq = false;
if let Ok(field) = RegisterOffset::try_from(offset) {
- update_irq = self.regs.borrow_mut().write(field, value as u32, &mut self.char_backend);
+ update_irq = self.regs.borrow_mut().write(
+ field,
+ value as u32,
+ addr_of!(self.char_backend) as *mut _,
+ );
} else {
eprintln!("write bad offset {offset} value {value}");
}
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 300c732ae1d..5622e974cbc 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -29,7 +29,6 @@
mod device;
mod device_class;
-mod memory_ops;
pub use device::pl011_create;
diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
deleted file mode 100644
index 95b4df794e4..00000000000
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ /dev/null
@@ -1,36 +0,0 @@
-// Copyright 2024, Linaro Limited
-// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
-// SPDX-License-Identifier: GPL-2.0-or-later
-
-use core::ptr::NonNull;
-use std::os::raw::{c_uint, c_void};
-
-use qemu_api::{bindings::*, zeroable::Zeroable};
-
-use crate::device::PL011State;
-
-pub static PL011_OPS: MemoryRegionOps = MemoryRegionOps {
- read: Some(pl011_read),
- write: Some(pl011_write),
- read_with_attrs: None,
- write_with_attrs: None,
- endianness: device_endian::DEVICE_NATIVE_ENDIAN,
- valid: Zeroable::ZERO,
- impl_: MemoryRegionOps__bindgen_ty_2 {
- min_access_size: 4,
- max_access_size: 4,
- ..Zeroable::ZERO
- },
-};
-
-unsafe extern "C" fn pl011_read(opaque: *mut c_void, addr: hwaddr, size: c_uint) -> u64 {
- assert!(!opaque.is_null());
- let mut state = unsafe { NonNull::new_unchecked(opaque.cast::<PL011State>()) };
- unsafe { state.as_mut() }.read(addr, size)
-}
-
-unsafe extern "C" fn pl011_write(opaque: *mut c_void, addr: hwaddr, data: u64, _size: c_uint) {
- assert!(!opaque.is_null());
- let mut state = unsafe { NonNull::new_unchecked(opaque.cast::<PL011State>()) };
- unsafe { state.as_mut() }.write(addr, data);
-}
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 60944a657de..80eafc7f6bd 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -22,6 +22,7 @@ _qemu_api_rs = static_library(
'src/cell.rs',
'src/c_str.rs',
'src/irq.rs',
+ 'src/memory.rs',
'src/module.rs',
'src/offset_of.rs',
'src/prelude.rs',
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 83c6a987c05..8cc095b13f6 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -18,6 +18,7 @@
pub mod callbacks;
pub mod cell;
pub mod irq;
+pub mod memory;
pub mod module;
pub mod offset_of;
pub mod qdev;
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
new file mode 100644
index 00000000000..963d689c27d
--- /dev/null
+++ b/rust/qemu-api/src/memory.rs
@@ -0,0 +1,191 @@
+// Copyright 2024 Red Hat, Inc.
+// Author(s): Paolo Bonzini <pbonzini@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Bindings for `MemoryRegion` and `MemoryRegionOps`
+
+use std::{
+ ffi::{CStr, CString},
+ marker::{PhantomData, PhantomPinned},
+ os::raw::{c_uint, c_void},
+ ptr::addr_of,
+};
+
+pub use bindings::hwaddr;
+
+use crate::{
+ bindings::{self, device_endian, memory_region_init_io},
+ callbacks::FnCall,
+ prelude::*,
+ zeroable::Zeroable,
+};
+
+pub struct MemoryRegionOps<T>(
+ bindings::MemoryRegionOps,
+ // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
+ // covariance and contravariance; you don't need any of those to understand
+ // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
+ // holds callbacks that take an argument of type &T, except the type is erased
+ // before the callback is stored in the bindings::MemoryRegionOps field.
+ // The argument of PhantomData is a function pointer in order to represent
+ // that relationship; while that will also provide desirable and safe variance
+ // for T, variance is not the point but just a consequence.
+ PhantomData<fn(&T)>,
+);
+
+// SAFETY: When a *const T is passed to the callbacks, the call itself
+// is done in a thread-safe manner. The invocation is okay as long as
+// T itself is `Sync`.
+unsafe impl<T: Sync> Sync for MemoryRegionOps<T> {}
+
+#[derive(Clone)]
+pub struct MemoryRegionOpsBuilder<T>(bindings::MemoryRegionOps, PhantomData<fn(&T)>);
+
+unsafe extern "C" fn memory_region_ops_read_cb<T, F: for<'a> FnCall<(&'a T, hwaddr, u32), u64>>(
+ opaque: *mut c_void,
+ addr: hwaddr,
+ size: c_uint,
+) -> u64 {
+ F::call((unsafe { &*(opaque.cast::<T>()) }, addr, size))
+}
+
+unsafe extern "C" fn memory_region_ops_write_cb<T, F: for<'a> FnCall<(&'a T, hwaddr, u64, u32)>>(
+ opaque: *mut c_void,
+ addr: hwaddr,
+ data: u64,
+ size: c_uint,
+) {
+ F::call((unsafe { &*(opaque.cast::<T>()) }, addr, data, size))
+}
+
+impl<T> MemoryRegionOpsBuilder<T> {
+ #[must_use]
+ pub const fn read<F: for<'a> FnCall<(&'a T, hwaddr, u32), u64>>(mut self, _f: &F) -> Self {
+ self.0.read = Some(memory_region_ops_read_cb::<T, F>);
+ self
+ }
+
+ #[must_use]
+ pub const fn write<F: for<'a> FnCall<(&'a T, hwaddr, u64, u32)>>(mut self, _f: &F) -> Self {
+ self.0.write = Some(memory_region_ops_write_cb::<T, F>);
+ self
+ }
+
+ #[must_use]
+ pub const fn big_endian(mut self) -> Self {
+ self.0.endianness = device_endian::DEVICE_BIG_ENDIAN;
+ self
+ }
+
+ #[must_use]
+ pub const fn little_endian(mut self) -> Self {
+ self.0.endianness = device_endian::DEVICE_LITTLE_ENDIAN;
+ self
+ }
+
+ #[must_use]
+ pub const fn native_endian(mut self) -> Self {
+ self.0.endianness = device_endian::DEVICE_NATIVE_ENDIAN;
+ self
+ }
+
+ #[must_use]
+ pub const fn valid_sizes(mut self, min: u32, max: u32) -> Self {
+ self.0.valid.min_access_size = min;
+ self.0.valid.max_access_size = max;
+ self
+ }
+
+ #[must_use]
+ pub const fn valid_unaligned(mut self) -> Self {
+ self.0.valid.unaligned = true;
+ self
+ }
+
+ #[must_use]
+ pub const fn impl_sizes(mut self, min: u32, max: u32) -> Self {
+ self.0.impl_.min_access_size = min;
+ self.0.impl_.max_access_size = max;
+ self
+ }
+
+ #[must_use]
+ pub const fn impl_unaligned(mut self) -> Self {
+ self.0.impl_.unaligned = true;
+ self
+ }
+
+ #[must_use]
+ pub const fn build(self) -> MemoryRegionOps<T> {
+ MemoryRegionOps::<T>(self.0, PhantomData)
+ }
+
+ #[must_use]
+ pub const fn new() -> Self {
+ Self(bindings::MemoryRegionOps::ZERO, PhantomData)
+ }
+}
+
+impl<T> Default for MemoryRegionOpsBuilder<T> {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+/// 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,
+}
+
+impl MemoryRegion {
+ // inline to ensure that it is not included in tests, which only
+ // link to hwcore and qom. FIXME: inlining is actually the opposite
+ // of what we want, since this is the type-erased version of the
+ // init_io function below. Look into splitting the qemu_api crate.
+ #[inline(always)]
+ unsafe fn do_init_io(
+ slot: *mut bindings::MemoryRegion,
+ owner: *mut Object,
+ ops: &'static bindings::MemoryRegionOps,
+ name: &'static str,
+ size: u64,
+ ) {
+ unsafe {
+ let cstr = CString::new(name).unwrap();
+ memory_region_init_io(
+ slot,
+ owner.cast::<Object>(),
+ ops,
+ owner.cast::<c_void>(),
+ cstr.as_ptr(),
+ size,
+ );
+ }
+ }
+
+ pub fn init_io<T: IsA<Object>>(
+ &mut self,
+ owner: *mut T,
+ ops: &'static MemoryRegionOps<T>,
+ name: &'static str,
+ size: u64,
+ ) {
+ unsafe {
+ Self::do_init_io(&mut self.inner, 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 {
+ type Class = bindings::MemoryRegionClass;
+ const TYPE_NAME: &'static CStr =
+ unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_MEMORY_REGION) };
+}
+qom_isa!(MemoryRegion: Object);
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index e6762b5c145..c27dbf79e43 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -2,7 +2,7 @@
// Author(s): Paolo Bonzini <pbonzini@redhat.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::{ffi::CStr, ptr::addr_of};
+use std::ffi::CStr;
pub use bindings::{SysBusDevice, SysBusDeviceClass};
@@ -10,6 +10,7 @@
bindings,
cell::bql_locked,
irq::InterruptSource,
+ memory::MemoryRegion,
prelude::*,
qdev::{DeviceClass, DeviceState},
qom::ClassInitImpl,
@@ -42,10 +43,10 @@ pub trait SysBusDeviceMethods: ObjectDeref
/// important, since whoever creates the sysbus device will refer to the
/// region with a number that corresponds to the order of calls to
/// `init_mmio`.
- fn init_mmio(&self, iomem: &bindings::MemoryRegion) {
+ fn init_mmio(&self, iomem: &MemoryRegion) {
assert!(bql_locked());
unsafe {
- bindings::sysbus_init_mmio(self.as_mut_ptr(), addr_of!(*iomem) as *mut _);
+ bindings::sysbus_init_mmio(self.as_mut_ptr(), iomem.as_mut_ptr());
}
}
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 57cac96de06..0208381ee38 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -99,6 +99,18 @@ unsafe impl Zeroable for crate::bindings::VMStateDescription {
};
}
+unsafe impl Zeroable for crate::bindings::MemoryRegionOps {
+ const ZERO: Self = Self {
+ read: None,
+ write: None,
+ read_with_attrs: None,
+ write_with_attrs: None,
+ endianness: crate::bindings::device_endian::DEVICE_NATIVE_ENDIAN,
+ valid: Zeroable::ZERO,
+ impl_: Zeroable::ZERO,
+ };
+}
+
unsafe impl Zeroable for crate::bindings::MemoryRegionOps__bindgen_ty_1 {
const ZERO: Self = Self {
min_access_size: 0,
--
2.47.1Hi Paolo,
On 17/1/25 20:40, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 43 +++---
> rust/hw/char/pl011/src/lib.rs | 1 -
> rust/hw/char/pl011/src/memory_ops.rs | 36 -----
> rust/qemu-api/meson.build | 1 +
> rust/qemu-api/src/lib.rs | 1 +
> rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
> rust/qemu-api/src/sysbus.rs | 7 +-
> rust/qemu-api/src/zeroable.rs | 12 ++
> 8 files changed, 234 insertions(+), 58 deletions(-)
> delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
> create mode 100644 rust/qemu-api/src/memory.rs
>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 259efacb046..294394c6e82 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -2,7 +2,7 @@
> // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> // SPDX-License-Identifier: GPL-2.0-or-later
>
> -use core::ptr::{addr_of_mut, NonNull};
> +use core::ptr::{addr_of, addr_of_mut, NonNull};
> use std::{
> ffi::CStr,
> os::raw::{c_int, c_void},
> @@ -12,14 +12,14 @@
> bindings::{self, *},
> c_str, impl_vmstate_forward,
> irq::InterruptSource,
> + memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
> prelude::*,
> - qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
> + qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
> qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
> };
>
> use crate::{
> device_class,
> - memory_ops::PL011_OPS,
> registers::{self, Interrupt},
> RegisterOffset,
> };
> @@ -490,20 +490,24 @@ impl PL011State {
> /// location/instance. All its fields are expected to hold unitialized
> /// values with the sole exception of `parent_obj`.
> unsafe fn init(&mut self) {
> + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> + .read(&PL011State::read)
> + .write(&PL011State::write)
> + .native_endian()
Could we always make .valid_sizes() explicit?
> + .impl_sizes(4, 4)
> + .build();
> +
> // SAFETY:
> //
> // self and self.iomem are guaranteed to be valid at this point since callers
> // must make sure the `self` reference is valid.
> - unsafe {
> - memory_region_init_io(
> - addr_of_mut!(self.iomem),
> - addr_of_mut!(*self).cast::<Object>(),
> - &PL011_OPS,
> - addr_of_mut!(*self).cast::<c_void>(),
> - Self::TYPE_NAME.as_ptr(),
> - 0x1000,
> - );
> - }
> + MemoryRegion::init_io(
> + unsafe { &mut *addr_of_mut!(self.iomem) },
> + addr_of_mut!(*self),
> + &PL011_OPS,
> + "pl011",
> + 0x1000,
> + );
> diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
> index 300c732ae1d..5622e974cbc 100644
> --- a/rust/hw/char/pl011/src/lib.rs
> +++ b/rust/hw/char/pl011/src/lib.rs
> @@ -29,7 +29,6 @@
>
> mod device;
> mod device_class;
> -mod memory_ops;
>
> pub use device::pl011_create;
>
> diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
> deleted file mode 100644
> index 95b4df794e4..00000000000
> --- a/rust/hw/char/pl011/src/memory_ops.rs
> +++ /dev/null
> @@ -1,36 +0,0 @@
> -// Copyright 2024, Linaro Limited
> -// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -
> -use core::ptr::NonNull;
> -use std::os::raw::{c_uint, c_void};
> -
> -use qemu_api::{bindings::*, zeroable::Zeroable};
> -
> -use crate::device::PL011State;
> -
> -pub static PL011_OPS: MemoryRegionOps = MemoryRegionOps {
> - read: Some(pl011_read),
> - write: Some(pl011_write),
> - read_with_attrs: None,
> - write_with_attrs: None,
> - endianness: device_endian::DEVICE_NATIVE_ENDIAN,
> - valid: Zeroable::ZERO,
> - impl_: MemoryRegionOps__bindgen_ty_2 {
> - min_access_size: 4,
> - max_access_size: 4,
> - ..Zeroable::ZERO
> - },
> -};
On Thu, Feb 6, 2025 at 9:40 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Paolo,
>
> On 17/1/25 20:40, Paolo Bonzini wrote:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > rust/hw/char/pl011/src/device.rs | 43 +++---
> > rust/hw/char/pl011/src/lib.rs | 1 -
> > rust/hw/char/pl011/src/memory_ops.rs | 36 -----
> > rust/qemu-api/meson.build | 1 +
> > rust/qemu-api/src/lib.rs | 1 +
> > rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
> > rust/qemu-api/src/sysbus.rs | 7 +-
> > rust/qemu-api/src/zeroable.rs | 12 ++
> > 8 files changed, 234 insertions(+), 58 deletions(-)
> > delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
> > create mode 100644 rust/qemu-api/src/memory.rs
> >
> > diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> > index 259efacb046..294394c6e82 100644
> > --- a/rust/hw/char/pl011/src/device.rs
> > +++ b/rust/hw/char/pl011/src/device.rs
> > @@ -2,7 +2,7 @@
> > // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > // SPDX-License-Identifier: GPL-2.0-or-later
> >
> > -use core::ptr::{addr_of_mut, NonNull};
> > +use core::ptr::{addr_of, addr_of_mut, NonNull};
> > use std::{
> > ffi::CStr,
> > os::raw::{c_int, c_void},
> > @@ -12,14 +12,14 @@
> > bindings::{self, *},
> > c_str, impl_vmstate_forward,
> > irq::InterruptSource,
> > + memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
> > prelude::*,
> > - qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
> > + qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
> > qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
> > };
> >
> > use crate::{
> > device_class,
> > - memory_ops::PL011_OPS,
> > registers::{self, Interrupt},
> > RegisterOffset,
> > };
> > @@ -490,20 +490,24 @@ impl PL011State {
> > /// location/instance. All its fields are expected to hold unitialized
> > /// values with the sole exception of `parent_obj`.
> > unsafe fn init(&mut self) {
> > + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> > + .read(&PL011State::read)
> > + .write(&PL011State::write)
> > + .native_endian()
>
> Could we always make .valid_sizes() explicit?
Yes (for example build() could even fail to compile if you don't have
impl_sizes/valid_sizes set), but why do you want that? I'm not even
sure that all cases of .valid.max_access_size=4 are correct...
Paolo
On 6/2/25 09:46, Paolo Bonzini wrote:
> On Thu, Feb 6, 2025 at 9:40 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Paolo,
>>
>> On 17/1/25 20:40, Paolo Bonzini wrote:
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>> rust/hw/char/pl011/src/device.rs | 43 +++---
>>> rust/hw/char/pl011/src/lib.rs | 1 -
>>> rust/hw/char/pl011/src/memory_ops.rs | 36 -----
>>> rust/qemu-api/meson.build | 1 +
>>> rust/qemu-api/src/lib.rs | 1 +
>>> rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
>>> rust/qemu-api/src/sysbus.rs | 7 +-
>>> rust/qemu-api/src/zeroable.rs | 12 ++
>>> 8 files changed, 234 insertions(+), 58 deletions(-)
>>> delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
>>> create mode 100644 rust/qemu-api/src/memory.rs
>>>
>>> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
>>> index 259efacb046..294394c6e82 100644
>>> --- a/rust/hw/char/pl011/src/device.rs
>>> +++ b/rust/hw/char/pl011/src/device.rs
>>> @@ -2,7 +2,7 @@
>>> // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> // SPDX-License-Identifier: GPL-2.0-or-later
>>>
>>> -use core::ptr::{addr_of_mut, NonNull};
>>> +use core::ptr::{addr_of, addr_of_mut, NonNull};
>>> use std::{
>>> ffi::CStr,
>>> os::raw::{c_int, c_void},
>>> @@ -12,14 +12,14 @@
>>> bindings::{self, *},
>>> c_str, impl_vmstate_forward,
>>> irq::InterruptSource,
>>> + memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
>>> prelude::*,
>>> - qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
>>> + qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
>>> qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
>>> };
>>>
>>> use crate::{
>>> device_class,
>>> - memory_ops::PL011_OPS,
>>> registers::{self, Interrupt},
>>> RegisterOffset,
>>> };
>>> @@ -490,20 +490,24 @@ impl PL011State {
>>> /// location/instance. All its fields are expected to hold unitialized
>>> /// values with the sole exception of `parent_obj`.
>>> unsafe fn init(&mut self) {
>>> + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
>>> + .read(&PL011State::read)
>>> + .write(&PL011State::write)
>>> + .native_endian()
>>
>> Could we always make .valid_sizes() explicit?
>
> Yes (for example build() could even fail to compile if you don't have
> impl_sizes/valid_sizes set), but why do you want that? I'm not even
> sure that all cases of .valid.max_access_size=4 are correct...
Exactly for that :) Not have implicit default values, so correct
values are reviewed when models are added.
On 2/6/25 11:02, Philippe Mathieu-Daudé wrote: >>> Could we always make .valid_sizes() explicit? >> >> Yes (for example build() could even fail to compile if you don't have >> impl_sizes/valid_sizes set), but why do you want that? I'm not even >> sure that all cases of .valid.max_access_size=4 are correct... > > Exactly for that :) Not have implicit default values, so correct > values are reviewed when models are added. But I wouldn't bet that those that we have in C are reviewed and correct... They are incorrect if the hardware accepts 8-byte writes, either discarding the top 4 bytes (then impl must both be 8) or writing to both registers (then impl must be 4). Paolo
On 6/2/25 11:19, Paolo Bonzini wrote: > On 2/6/25 11:02, Philippe Mathieu-Daudé wrote: >>>> Could we always make .valid_sizes() explicit? >>> >>> Yes (for example build() could even fail to compile if you don't have >>> impl_sizes/valid_sizes set), but why do you want that? I'm not even >>> sure that all cases of .valid.max_access_size=4 are correct... >> >> Exactly for that :) Not have implicit default values, so correct >> values are reviewed when models are added. > > But I wouldn't bet that those that we have in C are reviewed and > correct... They are incorrect if the hardware accepts 8-byte writes, > either discarding the top 4 bytes (then impl must both be 8) or writing > to both registers (then impl must be 4). Are you saying in general or for the pl011 model? What I'm asking is to have all rust models explicit the min/max sizes, regardless of whether the C implementations are correct or not. For rust models, we won't rely on default and will have to check the valid range in the specs.
On Fri, Jan 17, 2025 at 08:40:03PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:40:03 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 10/10] rust: bindings for MemoryRegionOps
> X-Mailer: git-send-email 2.47.1
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 43 +++---
> rust/hw/char/pl011/src/lib.rs | 1 -
> rust/hw/char/pl011/src/memory_ops.rs | 36 -----
> rust/qemu-api/meson.build | 1 +
> rust/qemu-api/src/lib.rs | 1 +
> rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
> rust/qemu-api/src/sysbus.rs | 7 +-
> rust/qemu-api/src/zeroable.rs | 12 ++
> 8 files changed, 234 insertions(+), 58 deletions(-)
> delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
> create mode 100644 rust/qemu-api/src/memory.rs
...
> @@ -490,20 +490,24 @@ impl PL011State {
> /// location/instance. All its fields are expected to hold unitialized
> /// values with the sole exception of `parent_obj`.
> unsafe fn init(&mut self) {
> + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> + .read(&PL011State::read)
> + .write(&PL011State::write)
> + .native_endian()
> + .impl_sizes(4, 4)
> + .build();
> +
Nice design. Everything was done smoothly in one go.
> +pub struct MemoryRegionOps<T>(
> + bindings::MemoryRegionOps,
> + // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
> + // covariance and contravariance; you don't need any of those to understand
> + // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
> + // holds callbacks that take an argument of type &T, except the type is erased
> + // before the callback is stored in the bindings::MemoryRegionOps field.
> + // The argument of PhantomData is a function pointer in order to represent
> + // that relationship; while that will also provide desirable and safe variance
> + // for T, variance is not the point but just a consequence.
> + PhantomData<fn(&T)>,
> +);
Wow, it can be wrapped like this!
> +}
> +
> +/// 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,
> +}
> +
> +impl MemoryRegion {
> + // inline to ensure that it is not included in tests, which only
> + // link to hwcore and qom. FIXME: inlining is actually the opposite
> + // of what we want, since this is the type-erased version of the
> + // init_io function below. Look into splitting the qemu_api crate.
Ah, I didn't understand the issue described in this comment. Why would
inlining affect the linking of tests?
> + #[inline(always)]
> + unsafe fn do_init_io(
> + slot: *mut bindings::MemoryRegion,
> + owner: *mut Object,
> + ops: &'static bindings::MemoryRegionOps,
> + name: &'static str,
> + size: u64,
> + ) {
> + unsafe {
> + let cstr = CString::new(name).unwrap();
> + memory_region_init_io(
> + slot,
> + owner.cast::<Object>(),
> + ops,
> + owner.cast::<c_void>(),
> + cstr.as_ptr(),
> + size,
> + );
> + }
> + }
> +
> + pub fn init_io<T: IsA<Object>>(
> + &mut self,
> + owner: *mut T,
> + ops: &'static MemoryRegionOps<T>,
> + name: &'static str,
What about &'static CStr?
Then pl011 could pass `c_str!("pl011")` or `Self::TYPE_NAMSelf::TYPE_NAME`.
Otherwise,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
On Mon, Jan 27, 2025 at 12:53 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > @@ -490,20 +490,24 @@ impl PL011State {
> > /// location/instance. All its fields are expected to hold unitialized
> > /// values with the sole exception of `parent_obj`.
> > unsafe fn init(&mut self) {
> > + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> > + .read(&PL011State::read)
> > + .write(&PL011State::write)
> > + .native_endian()
> > + .impl_sizes(4, 4)
> > + .build();
> > +
>
> Nice design. Everything was done smoothly in one go.
I hope something similar can be done with VMStateDescription too...
> > +pub struct MemoryRegionOps<T>(
> > + bindings::MemoryRegionOps,
> > + // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
> > + // covariance and contravariance; you don't need any of those to understand
> > + // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
> > + // holds callbacks that take an argument of type &T, except the type is erased
> > + // before the callback is stored in the bindings::MemoryRegionOps field.
> > + // The argument of PhantomData is a function pointer in order to represent
> > + // that relationship; while that will also provide desirable and safe variance
> > + // for T, variance is not the point but just a consequence.
> > + PhantomData<fn(&T)>,
> > +);
>
> Wow, it can be wrapped like this!
I like your enthusiasm but I'm not sure what you refer to. ;) Maybe
it's worth documenting this pattern, so please tell me more (after
your holidays).
> > +impl MemoryRegion {
> > + // inline to ensure that it is not included in tests, which only
> > + // link to hwcore and qom. FIXME: inlining is actually the opposite
> > + // of what we want, since this is the type-erased version of the
> > + // init_io function below. Look into splitting the qemu_api crate.
>
> Ah, I didn't understand the issue described in this comment. Why would
> inlining affect the linking of tests?
If you don't inline it, do_init_io will always be linked into the
tests because it is a non-generic function. The tests then fail to
link, because memory_region_init_io is undefined.
This is ugly because do_init_io exists *exactly* to extract the part
that is not generic. (See
https://users.rust-lang.org/t/soft-question-significantly-improve-rust-compile-time-via-minimizing-generics/103632/8
for an example of this; I think there's even a procedural macro crate
that does that for you, but I can't find it right now).
> > + pub fn init_io<T: IsA<Object>>(
> > + &mut self,
> > + owner: *mut T,
> > + ops: &'static MemoryRegionOps<T>,
> > + name: &'static str,
>
> What about &'static CStr?
>
> Then pl011 could pass `c_str!("pl011")` or `Self::TYPE_NAME`.
I think it's better to use a Rust string; there's no reason why the
name of the memory region has to match Self::TYPE_NAME; unlike the
name of the device, the name of the memory region is not visible on
the command line for example.
Thanks,
Paolo
> Otherwise,
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>
>
> > > +pub struct MemoryRegionOps<T>(
> > > + bindings::MemoryRegionOps,
> > > + // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
> > > + // covariance and contravariance; you don't need any of those to understand
> > > + // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
> > > + // holds callbacks that take an argument of type &T, except the type is erased
> > > + // before the callback is stored in the bindings::MemoryRegionOps field.
> > > + // The argument of PhantomData is a function pointer in order to represent
> > > + // that relationship; while that will also provide desirable and safe variance
> > > + // for T, variance is not the point but just a consequence.
> > > + PhantomData<fn(&T)>,
> > > +);
> >
> > Wow, it can be wrapped like this!
>
> I like your enthusiasm but I'm not sure what you refer to. ;) Maybe
> it's worth documenting this pattern, so please tell me more (after
> your holidays).
Throughout the entire holiday, I couldn't think of a better way to
express this. I find it particularly useful when wrapping multiple
callbacks. In the future, I want to explore more use cases where this
pattern can be applied.
> > > +impl MemoryRegion {
> > > + // inline to ensure that it is not included in tests, which only
> > > + // link to hwcore and qom. FIXME: inlining is actually the opposite
> > > + // of what we want, since this is the type-erased version of the
> > > + // init_io function below. Look into splitting the qemu_api crate.
> >
> > Ah, I didn't understand the issue described in this comment. Why would
> > inlining affect the linking of tests?
>
> If you don't inline it, do_init_io will always be linked into the
> tests because it is a non-generic function. The tests then fail to
> link, because memory_region_init_io is undefined.
I find even if I drop the `inline` attribution, the test.rs can still be
compiled (by `make check`), I think it's because test.rs hasn't involved
memory related tests so that do_init_io isn't linked into test.rs.
> This is ugly because do_init_io exists *exactly* to extract the part
> that is not generic. (See
> https://users.rust-lang.org/t/soft-question-significantly-improve-rust-compile-time-via-minimizing-generics/103632/8
> for an example of this; I think there's even a procedural macro crate
> that does that for you, but I can't find it right now).
Thanks! I see. I agree to keep `inline` anyway.
On 2/6/25 10:15, Zhao Liu wrote:
> Throughout the entire holiday, I couldn't think of a better way to
> express this. I find it particularly useful when wrapping multiple
> callbacks. In the future, I want to explore more use cases where this
> pattern can be applied.
Thanks very much. Despite having written most of the bindings code, I
don't want to give the impression that everything that I send to the
mailing list is gold. So it matters to me that you send serious
reviews, and that you can learn something but also suggest
clarifications and improvements.
> I find even if I drop the `inline` attribution, the test.rs can still be
> compiled (by `make check`), I think it's because test.rs hasn't involved
> memory related tests so that do_init_io isn't linked into test.rs.
Hmm, maybe it's only with some linkers. I don't remember.
>> This is ugly because do_init_io exists *exactly* to extract the part
>> that is not generic. (See
>> https://users.rust-lang.org/t/soft-question-significantly-improve-rust-compile-time-via-minimizing-generics/103632/8
>> for an example of this; I think there's even a procedural macro crate
>> that does that for you, but I can't find it right now).
>
> Thanks! I see. I agree to keep `inline` anyway.
In the meanwhile I found it, it's
https://github.com/llogiq/momo/blob/master/wasm/src/lib.rs. QEMU could
add something like that macro to qemu_api_macros, so that
#[qemu_api_macros::upcast]
fn foo<T>(t: &T) where T: IsA<U> {
}
could be converted to
#[inline(always)]
fn foo<T>(&self) where T: IsA<U> {
fn inner_foo(u: &U) {
}
inner_foo(self.upcast())
}
Paolo
© 2016 - 2026 Red Hat, Inc.