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 <pbonz...@redhat.com>
> Subject: [PATCH 10/10] rust: bindings for MemoryRegionOps
> X-Mailer: git-send-email 2.47.1
>
> Signed-off-by: Paolo Bonzini <pbonz...@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....@intel.com>
