Re: [PATCH 10/10] rust: bindings for MemoryRegionOps

[Zhao Liu <zhao1.liu@intel.com>]

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>