Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/bindings.rs |  3 ---
 rust/qemu-api/src/memory.rs   | 30 ++++++++++++++++--------------
 2 files changed, 16 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..fdb1ea11fcf 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,15 @@ 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(),
+                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

>  impl MemoryRegion {
>      // inline to ensure that it is not included in tests, which only
> @@ -174,13 +174,15 @@ 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(),

I'm not sure why the wrapper doesn't work here.

If I change this to `self.as_mut_ptr()`, then compiler tries to apply
`ObjectDeref::as_mut_ptr` and complains:

the trait `qom::ObjectType` is not implemented for `bindings::MemoryRegion`


But when I modify the function signature to &self like:

diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index fdb1ea11fcf9..a82348c4a564 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -167,7 +167,7 @@ unsafe fn do_init_io(
     }

     pub fn init_io<T: IsA<Object>>(
-        &mut self,
+        &self,
         owner: *mut T,
         ops: &'static MemoryRegionOps<T>,
         name: &'static str,

Then the Wrapper's as_mut_ptr() can work.

> +                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
> 
>

On 2/25/25 10:14, Zhao Liu wrote:
>>   impl MemoryRegion {
>>       // inline to ensure that it is not included in tests, which only
>> @@ -174,13 +174,15 @@ 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(),
> 
> I'm not sure why the wrapper doesn't work here.
> 
> If I change this to `self.as_mut_ptr()`, then compiler tries to apply
> `ObjectDeref::as_mut_ptr` and complains:
> 
> the trait `qom::ObjectType` is not implemented for `bindings::MemoryRegion`

It's because the implementation of ObjectDeref for "&mut MemoryRegion" 
wins over coercing "&mut MemoryRegion" to "&MemoryRegion" and then 
calling MemoryRegion::as_mut_ptr().

I added a comment

         // 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.

Paolo

> 
> But when I modify the function signature to &self like:
> 
> diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
> index fdb1ea11fcf9..a82348c4a564 100644
> --- a/rust/qemu-api/src/memory.rs
> +++ b/rust/qemu-api/src/memory.rs
> @@ -167,7 +167,7 @@ unsafe fn do_init_io(
>       }
> 
>       pub fn init_io<T: IsA<Object>>(
> -        &mut self,
> +        &self,
>           owner: *mut T,
>           ops: &'static MemoryRegionOps<T>,
>           name: &'static str,
> 
> Then the Wrapper's as_mut_ptr() can work.
> 
>> +                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
>>
>>
> 
>