| Message ID | 20241209123717.99077-15-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: bundle of prerequisites for HPET implementation | expand |
On Mon, Dec 09, 2024 at 01:37:05PM +0100, Paolo Bonzini wrote: > Date: Mon, 9 Dec 2024 13:37:05 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH 14/26] rust: qom: move bridge for TypeInfo functions out of > pl011 > X-Mailer: git-send-email 2.47.1 > > Allow the ObjectImpl trait to expose Rust functions that avoid raw > pointers (though INSTANCE_INIT for example is still unsafe). > ObjectImpl::TYPE_INFO adds thunks around the functions in > ObjectImpl. > > While at it, document `TypeInfo`. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/hw/char/pl011/src/device.rs | 40 +++++++-------------- > rust/qemu-api/src/definitions.rs | 61 +++++++++++++++++++++++++++++--- > 2 files changed, 69 insertions(+), 32 deletions(-) > > diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs > index 56403c36609..b9f8fb134b5 100644 > --- a/rust/hw/char/pl011/src/device.rs > +++ b/rust/hw/char/pl011/src/device.rs > @@ -110,7 +110,7 @@ impl ObjectImpl for PL011State { > type Class = PL011Class; > const TYPE_NAME: &'static CStr = crate::TYPE_PL011; > const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); > - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_init); > + const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init); No need to keep `unsafe` here? > +/// > +/// - the struct must be `#[repr(C)]` > +/// > +/// - `Class` and `TYPE` must match the data in the `TypeInfo` (this is > +/// automatic if the class is defined via `ObjectImpl`). > +/// > +/// - the first field of the struct must be of the instance struct corresponding > +/// to the superclass declared as `PARENT_TYPE_NAME` > pub trait ObjectImpl: ClassInitImpl + Sized { > + /// The QOM class object corresponding to this struct. Not used yet. > type Class; > + > + /// The name of the type, which can be passed to `object_new()` to > + /// generate an instance of this type. > const TYPE_NAME: &'static CStr; > + > + /// The parent of the type. This should match the first field of > + /// the struct that implements `ObjectImpl`: > const PARENT_TYPE_NAME: Option<&'static CStr>; > + > + /// Whether the object can be instantiated > const ABSTRACT: bool = false; > - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None; > - const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None; > + > + /// Function that is called to initialize an object. The parent class will > + /// have already been initialized so the type is only responsible for > + /// initializing its own members. > + /// > + /// FIXME: The argument is not really a valid reference. `&mut > + /// MaybeUninit<Self>` would be a better description. > + const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = None; And here. > + /// Function that is called to finish initialization of an object, once > + /// `INSTANCE_INIT` functions have been called. > + const INSTANCE_POST_INIT: Option<fn(&mut Self)> = None; > Otherwise, Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> +unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) { > + // SAFETY: obj is an instance of T, since rust_instance_init<T> > + // is called from QOM core as the instance_init function > + // for class T > + unsafe { T::INSTANCE_INIT.unwrap()(&mut *obj.cast::<T>()) } > +} Here's the difference, why doesn't init() narrow the unsafe scope like post_init() does? > +unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut Object) { > + // SAFETY: obj is an instance of T, since rust_instance_post_init<T> > + // is called from QOM core as the instance_post_init function > + // for class T > + // > + // FIXME: it's not really guaranteed that there are no backpointers to > + // obj; it's quite possible that they have been created by instance_init(). > + // The receiver should be &self, not &mut self. > + T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::<T>() }) > +} > +
On 12/10/24 16:50, Zhao Liu wrote: > On Mon, Dec 09, 2024 at 01:37:05PM +0100, Paolo Bonzini wrote: >> Date: Mon, 9 Dec 2024 13:37:05 +0100 >> From: Paolo Bonzini <pbonzini@redhat.com> >> Subject: [PATCH 14/26] rust: qom: move bridge for TypeInfo functions out of >> pl011 >> X-Mailer: git-send-email 2.47.1 >> >> Allow the ObjectImpl trait to expose Rust functions that avoid raw >> pointers (though INSTANCE_INIT for example is still unsafe). >> ObjectImpl::TYPE_INFO adds thunks around the functions in >> ObjectImpl. >> >> While at it, document `TypeInfo`. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> rust/hw/char/pl011/src/device.rs | 40 +++++++-------------- >> rust/qemu-api/src/definitions.rs | 61 +++++++++++++++++++++++++++++--- >> 2 files changed, 69 insertions(+), 32 deletions(-) >> >> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs >> index 56403c36609..b9f8fb134b5 100644 >> --- a/rust/hw/char/pl011/src/device.rs >> +++ b/rust/hw/char/pl011/src/device.rs >> @@ -110,7 +110,7 @@ impl ObjectImpl for PL011State { >> type Class = PL011Class; >> const TYPE_NAME: &'static CStr = crate::TYPE_PL011; >> const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); >> - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_init); >> + const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init); > > No need to keep `unsafe` here? Right now instance_init is called with only the parent initialized, and the remaining memory zeroed; its purpose is to prepare things for instance_post_init which can then be safe (it's also kind of wrong for instance_post_init to receive a &mut Self, because instance_init will create other pointers to the object, for example in a MemoryRegion's "parent" field). The right thing to do would be to have an argument of type &mut MaybeUninit<Self>. Then the caller would do something like let maybe_uninit = obj as *mut MaybeUninit<Self>; unsafe { Self::INSTANCE_INIT(&mut *maybe_uninit); maybe_uninit.assume_init_mut(); } Note however that INSTANCE_INIT would still be unsafe, because its safety promise is that it prepares things for the caller's assume_init_mut(). The way that this will become safe is to use the pinned_init crate from Linux: instance_init returns the initialization as an "impl PinInit<Self>", and then instance_post_init can run with a &self. Until then, however, instance_init has to remain unsafe. Paolo
> > > - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_init); > > > + const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init); > > > > No need to keep `unsafe` here? > > Right now instance_init is called with only the parent initialized, and the > remaining memory zeroed; its purpose is to prepare things for > instance_post_init which can then be safe (it's also kind of wrong for > instance_post_init to receive a &mut Self, because instance_init will create > other pointers to the object, for example in a MemoryRegion's "parent" > field). Thank you for explanation. It makes a lot of sense. > The right thing to do would be to have an argument of type &mut > MaybeUninit<Self>. Then the caller would do something like > > let maybe_uninit = obj as *mut MaybeUninit<Self>; > unsafe { > Self::INSTANCE_INIT(&mut *maybe_uninit); > maybe_uninit.assume_init_mut(); > } > > Note however that INSTANCE_INIT would still be unsafe, because its safety > promise is that it prepares things for the caller's assume_init_mut(). Yes, I feel that this approach more clearly explains the purpose of QOM init. And since we are talking about the safety of INSTANCE_INIT, I think we should add some safety guidelines here, like: * Proper Initialization of pointers and references * Explicit initialization of Non-Zero Fields * In placed memory region is correctly initialized. (Or do you have any additional or clearer guidelines?) This could be the reference when adding SAFETY comment for the device's own `unsafe` init(). :-) And this is also a good explanation to distinguish between initialization in init() and realize(). For example, HPET attempts to initialize the timer array in realize(). > The way that this will become safe is to use the pinned_init crate from > Linux: instance_init returns the initialization as an "impl PinInit<Self>", Then do we need to place OBJECT in some suitable memory location (Box or something) for Rust implementation? > and then instance_post_init can run with a &self. Until then, however, > instance_init has to remain unsafe. Thanks for sharing! It's a very reasonable direction. Regards, Zhao
Il mer 11 dic 2024, 08:41 Zhao Liu <zhao1.liu@intel.com> ha scritto: > And since we are talking about the safety of INSTANCE_INIT, I think we > should add some safety guidelines here, like: > * Proper Initialization of pointers and references > * Explicit initialization of Non-Zero Fields > * In placed memory region is correctly initialized. > Yes that's pretty much it. (Or do you have any additional or clearer guidelines?) > > This could be the reference when adding SAFETY comment for the device's > own `unsafe` init(). :-) > > And this is also a good explanation to distinguish between initialization > in init() and realize(). For example, HPET attempts to initialize the > timer array in realize(). > Generally: - embedded objects will have to be initialized in instance_init unless they are Options - sysbus_init_irq and sysbus_init_mmio can be called in both instance_init and instance_post_init for now, but they will have to be in post_init once the signature of init changes to return impl PinInit - if you don't need properties you can choose between post_init and realize, if you need properties you need to initialize in realize (and then, unlike C, you might need to explicitly allow the pre-realize state; for example using Option<&...> instead of just a reference; or Vec<> instead of an array). > > > The way that this will become safe is to use the pinned_init crate from > > Linux: instance_init returns the initialization as an "impl > PinInit<Self>", > > Then do we need to place OBJECT in some suitable memory location (Box or > something) for Rust implementation? > Allocation is still done by the C code, so I am not sure I understand the question. Rust code allocates QOM objects with object_new() so they are malloc-ed. I discussed it with Manos some time ago and in principle you could use a Box (QOM supports custom freeing functions) but it's a bit complex because the freeing function would have to free the memory without dropping the contents of the Box (the drop is done by QOM via instance_finalize). If you want to allocate the HPETTimers at realize time, I think you can place them in a Vec. I think you won't need NonNull for this, but I am not 100% sure. Alternatively if you want to always prepare all MAX_TIMERS of them and then only use a subset, you can use an array. Either way, probably it makes sense for you to have an "fn timers_iter(&self) -> impl Iter<Item = &BqlRefCell<HPETTimer>>" in HPETState, or something like that. Then you can easily use for loops to walk all timers between 0 and self.num_timers-1. Paolo > and then instance_post_init can run with a &self. Until then, however, > > instance_init has to remain unsafe. > > Thanks for sharing! It's a very reasonable direction. > > Regards, > Zhao > >
> Generally: > > - embedded objects will have to be initialized in instance_init unless they > are Options I see, at least for HPETTimer array, I need to prepare all of them in instance_init()... > - if you don't need properties you can choose between post_init and > realize, if you need properties you need to initialize in realize (and > then, unlike C, you might need to explicitly allow the pre-realize state; > for example using Option<&...> instead of just a reference; or Vec<> > instead of an array). ...in realize(), also need to handle the "timers" property to enable the timers that property requires. > - sysbus_init_irq and sysbus_init_mmio can be called in both instance_init > and instance_post_init for now, but they will have to be in post_init once > the signature of init changes to return impl PinInit make sense, thanks! > > > > > The way that this will become safe is to use the pinned_init crate from > > > Linux: instance_init returns the initialization as an "impl > > PinInit<Self>", > > > > Then do we need to place OBJECT in some suitable memory location (Box or > > something) for Rust implementation? > > > > Allocation is still done by the C code, so I am not sure I understand the > question. Rust code allocates QOM objects with object_new() so they are > malloc-ed. Sorry, I'm not familiar enough with this piece...I have the question because PinInit doc said "you will need a suitable memory location that can hold a T. This can be KBox<T>, Arc<T>, UniqueArc<T> or even the stack (see stack_pin_init!)." I see that the core point is ensuring that structures cannot be moved. Given that object_new() on the C side ensures that the allocated object will not be moved, Rust side does not need to worry about pinning, correct? > I discussed it with Manos some time ago and in principle you > could use a Box (QOM supports custom freeing functions) but it's a bit > complex because the freeing function would have to free the memory without > dropping the contents of the Box (the drop is done by QOM via > instance_finalize). > > If you want to allocate the HPETTimers at realize time, I think you can > place them in a Vec. I think you won't need NonNull for this, but I am not > 100% sure. Alternatively if you want to always prepare all MAX_TIMERS of > them and then only use a subset, you can use an array. Vec seems to lack proper vmstate support. I understand that we need to modify VMSTATE_STRUCT_VARRAY_POINTER_* to introduce a variant for Vec. Since the array support is already available, I chose to use an array instead (although vmstate is disabled for now). > Either way, probably it makes sense for you to have an "fn > timers_iter(&self) -> impl Iter<Item = &BqlRefCell<HPETTimer>>" in > HPETState, or something like that. Then you can easily use for loops to > walk all timers between 0 and self.num_timers-1. Good idea, yes, will do. Thanks, Zhao
On Wed, Dec 11, 2024 at 5:38 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > > Generally: > > > > - embedded objects will have to be initialized in instance_init unless they > > are Options > > I see, at least for HPETTimer array, I need to prepare all of them in > instance_init()... > > ...in realize(), also need to handle the "timers" property to enable > the timers that property requires. I see -- though, thinking more about it, since you have fn init_timer(&mut self) { let raw_ptr: *mut HPETState = self; for i in 0..HPET_MAX_TIMERS { let mut timer = self.get_timer(i).borrow_mut(); timer.init(i, raw_ptr).init_timer_with_state(); } } It seems to me that you can do everything in instance_init. Later on a function like the above will become something like impl HPETTimer { fn init_timer(hpet: NonNull<HPETState>, n: usize) -> impl PinInit<Self> { pin_init!(&this in HPETTimer { index: n, qemu_timer <- Timer::init_ns(...), state: hpet, config: 0, cmp: 0, fsb: 0, cmp64: 0, period: 0, wrap_flag: false, last: 0, } } } But even now you can write something that takes a &mut self as the first argument. It's undefined behavior but it's okay as long as we have a path forward. > > > > The way that this will become safe is to use the pinned_init crate from > > > > Linux: instance_init returns the initialization as an "impl > > > PinInit<Self>", > > > > > > Then do we need to place OBJECT in some suitable memory location (Box or > > > something) for Rust implementation? > > > > > > > Allocation is still done by the C code, so I am not sure I understand the > > question. Rust code allocates QOM objects with object_new() so they are > > malloc-ed. > > Sorry, I'm not familiar enough with this piece...I have the question > because PinInit doc said "you will need a suitable memory location that > can hold a T. This can be KBox<T>, Arc<T>, UniqueArc<T> or even the > stack (see stack_pin_init!)." Ah, I see. You can use __pinned_init directly on the memory that is passed to rust_instance_init. See for example the implementation of InPlaceWrite for Box (https://docs.rs/pinned-init/latest/src/pinned_init/lib.rs.html#1307). > I see that the core point is ensuring that structures cannot be moved. > Given that object_new() on the C side ensures that the allocated object > will not be moved, Rust side does not need to worry about pinning, correct? Sort of... You still need to worry about it for two reasons: 1) if you have &mut Self you can move values out of the object using e.g. mem::replace or mem::swap. Those would move the value in memory and cause trouble (think of moving a QEMUTimer while it is pointed to by the QEMUTimerList). This is solved by 1) using &Self all the time + interior mutability 2) using pinned_init's "PinnedDrop" functionality, because &Self can be used in QEMU-specific APIs but (obviously) not in the built-in Drop trait. 2) right now marking something as pinned is an indirect way to tell the compiler and miri that there are external references to it. For a longer discussion you can read https://crates.io/crates/pinned-aliasable or https://gist.github.com/Darksonn/1567538f56af1a8038ecc3c664a42462. Linux does this with a wrapper type similar to the one in pinned-aliasable: /// Stores an opaque value. /// /// This is meant to be used with FFI objects that are never interpreted by Rust code. #[repr(transparent)] pub struct Opaque<T> { value: UnsafeCell<MaybeUninit<T>>, _pin: PhantomPinned, } It's on my todo list to introduce it in qemu_api::cell and (for example) change qom::Object from pub use bindings::Object to pub type Object = Opaque<bindings::Object>; Or something like that. > Vec seems to lack proper vmstate support. I understand that we need to > modify VMSTATE_STRUCT_VARRAY_POINTER_* to introduce a variant for Vec. > > Since the array support is already available, I chose to use an array > instead (although vmstate is disabled for now). Yes, you're right. Palo
> I see -- though, thinking more about it, since you have > > fn init_timer(&mut self) { > let raw_ptr: *mut HPETState = self; > > for i in 0..HPET_MAX_TIMERS { > let mut timer = self.get_timer(i).borrow_mut(); > timer.init(i, raw_ptr).init_timer_with_state(); > } > } > > It seems to me that you can do everything in instance_init. Later on a > function like the above > will become something like > > impl HPETTimer { > fn init_timer(hpet: NonNull<HPETState>, n: usize) -> impl PinInit<Self> { Thank you! I should pass NonNull type other than `*mut HPETState` for now :-) > pin_init!(&this in HPETTimer { > index: n, > qemu_timer <- Timer::init_ns(...), > state: hpet, > config: 0, > cmp: 0, > fsb: 0, > cmp64: 0, > period: 0, > wrap_flag: false, > last: 0, > } > } > } That's the right and ideal way, and I like it. > But even now you can write something that takes a &mut self as the > first argument. It's undefined behavior but it's okay as long as we > have a path forward. Yes, I agree. In the next version, I can follow your suggestion and put these embedded items into instance_init(), to be better prepared for the next step. > > > > > The way that this will become safe is to use the pinned_init crate from > > > > > Linux: instance_init returns the initialization as an "impl > > > > PinInit<Self>", > > > > > > > > Then do we need to place OBJECT in some suitable memory location (Box or > > > > something) for Rust implementation? > > > > > > > > > > Allocation is still done by the C code, so I am not sure I understand the > > > question. Rust code allocates QOM objects with object_new() so they are > > > malloc-ed. > > > > Sorry, I'm not familiar enough with this piece...I have the question > > because PinInit doc said "you will need a suitable memory location that > > can hold a T. This can be KBox<T>, Arc<T>, UniqueArc<T> or even the > > stack (see stack_pin_init!)." > > Ah, I see. You can use __pinned_init directly on the memory that is > passed to rust_instance_init. See for example the implementation of > InPlaceWrite for Box > (https://docs.rs/pinned-init/latest/src/pinned_init/lib.rs.html#1307). Thank you! I understand your intention. A similar implementation would also be quite natural in rust_instance_init. > > I see that the core point is ensuring that structures cannot be moved. > > Given that object_new() on the C side ensures that the allocated object > > will not be moved, Rust side does not need to worry about pinning, correct? > > Sort of... You still need to worry about it for two reasons: > > 1) if you have &mut Self you can move values out of the object using > e.g. mem::replace or mem::swap. Those would move the value in memory > and cause trouble (think of moving a QEMUTimer while it is pointed to > by the QEMUTimerList). This is solved by 1) using &Self all the time + > interior mutability With your help and through our discussions, I have gained a clearer understanding of this intention. > 2) using pinned_init's "PinnedDrop" functionality, > because &Self can be used in QEMU-specific APIs but (obviously) not in > the built-in Drop trait. > > 2) right now marking something as pinned is an indirect way to tell > the compiler and miri that there are external references to it. For a > longer discussion you can read > https://crates.io/crates/pinned-aliasable or > https://gist.github.com/Darksonn/1567538f56af1a8038ecc3c664a42462. Thanks for sharing! > Linux does this with a wrapper type similar to the one in pinned-aliasable: > > /// Stores an opaque value. > /// > /// This is meant to be used with FFI objects that are never > interpreted by Rust code. > #[repr(transparent)] > pub struct Opaque<T> { > value: UnsafeCell<MaybeUninit<T>>, > _pin: PhantomPinned, > } > > It's on my todo list to introduce it in qemu_api::cell and (for > example) change qom::Object from > > pub use bindings::Object > > to > > pub type Object = Opaque<bindings::Object>; > > Or something like that. Yes, I agree with this idea. It's what we need. Regards, Zhao
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 56403c36609..b9f8fb134b5 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -110,7 +110,7 @@ impl ObjectImpl for PL011State { type Class = PL011Class; const TYPE_NAME: &'static CStr = crate::TYPE_PL011; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_init); + const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init); } #[repr(C)] @@ -615,19 +615,6 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> { } } -/// # 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_init(obj: *mut Object) { - unsafe { - debug_assert!(!obj.is_null()); - let mut state = NonNull::new_unchecked(obj.cast::<PL011State>()); - state.as_mut().init(); - } -} - #[repr(C)] #[derive(Debug, qemu_api_macros::Object)] /// PL011 Luminary device model. @@ -640,19 +627,16 @@ pub struct PL011LuminaryClass { _inner: [u8; 0], } -/// Initializes a pre-allocated, unitialized instance of `PL011Luminary`. -/// -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011Luminary`]. We also expect the device is -/// readable/writeable from one thread at any time. -pub unsafe extern "C" fn pl011_luminary_init(obj: *mut Object) { - unsafe { - debug_assert!(!obj.is_null()); - let mut state = NonNull::new_unchecked(obj.cast::<PL011Luminary>()); - let state = state.as_mut(); - state.parent_obj.device_id = DeviceId::Luminary; +impl PL011Luminary { + /// Initializes a pre-allocated, unitialized instance of `PL011Luminary`. + /// + /// # Safety + /// + /// We expect the FFI user of this function to pass a valid pointer, that + /// has the same size as [`PL011Luminary`]. We also expect the device is + /// readable/writeable from one thread at any time. + unsafe fn init(&mut self) { + self.parent_obj.device_id = DeviceId::Luminary; } } @@ -660,7 +644,7 @@ impl ObjectImpl for PL011Luminary { type Class = PL011LuminaryClass; const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011); - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_luminary_init); + const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init); } impl DeviceImpl for PL011Luminary {} diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs index d64a581a5cc..078ba30d61a 100644 --- a/rust/qemu-api/src/definitions.rs +++ b/rust/qemu-api/src/definitions.rs @@ -8,6 +8,24 @@ use crate::bindings::{Object, ObjectClass, TypeInfo}; +unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) { + // SAFETY: obj is an instance of T, since rust_instance_init<T> + // is called from QOM core as the instance_init function + // for class T + unsafe { T::INSTANCE_INIT.unwrap()(&mut *obj.cast::<T>()) } +} + +unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut Object) { + // SAFETY: obj is an instance of T, since rust_instance_post_init<T> + // is called from QOM core as the instance_post_init function + // for class T + // + // FIXME: it's not really guaranteed that there are no backpointers to + // obj; it's quite possible that they have been created by instance_init(). + // The receiver should be &self, not &mut self. + T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::<T>() }) +} + unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) { // SAFETY: obj is an instance of T, since drop_object<T> // is called from QOM core as the instance_finalize function @@ -16,13 +34,42 @@ } /// Trait a type must implement to be registered with QEMU. +/// +/// # Safety +/// +/// - the struct must be `#[repr(C)]` +/// +/// - `Class` and `TYPE` must match the data in the `TypeInfo` (this is +/// automatic if the class is defined via `ObjectImpl`). +/// +/// - the first field of the struct must be of the instance struct corresponding +/// to the superclass declared as `PARENT_TYPE_NAME` pub trait ObjectImpl: ClassInitImpl + Sized { + /// The QOM class object corresponding to this struct. Not used yet. type Class; + + /// The name of the type, which can be passed to `object_new()` to + /// generate an instance of this type. const TYPE_NAME: &'static CStr; + + /// The parent of the type. This should match the first field of + /// the struct that implements `ObjectImpl`: const PARENT_TYPE_NAME: Option<&'static CStr>; + + /// Whether the object can be instantiated const ABSTRACT: bool = false; - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None; - const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None; + + /// Function that is called to initialize an object. The parent class will + /// have already been initialized so the type is only responsible for + /// initializing its own members. + /// + /// FIXME: The argument is not really a valid reference. `&mut + /// MaybeUninit<Self>` would be a better description. + const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = None; + + /// Function that is called to finish initialization of an object, once + /// `INSTANCE_INIT` functions have been called. + const INSTANCE_POST_INIT: Option<fn(&mut Self)> = None; const TYPE_INFO: TypeInfo = TypeInfo { name: Self::TYPE_NAME.as_ptr(), @@ -33,8 +80,14 @@ pub trait ObjectImpl: ClassInitImpl + Sized { }, instance_size: core::mem::size_of::<Self>(), instance_align: core::mem::align_of::<Self>(), - instance_init: Self::INSTANCE_INIT, - instance_post_init: Self::INSTANCE_POST_INIT, + instance_init: match Self::INSTANCE_INIT { + None => None, + Some(_) => Some(rust_instance_init::<Self>), + }, + instance_post_init: match Self::INSTANCE_POST_INIT { + None => None, + Some(_) => Some(rust_instance_post_init::<Self>), + }, instance_finalize: Some(drop_object::<Self>), abstract_: Self::ABSTRACT, class_size: core::mem::size_of::<Self::Class>(),
Allow the ObjectImpl trait to expose Rust functions that avoid raw pointers (though INSTANCE_INIT for example is still unsafe). ObjectImpl::TYPE_INFO adds thunks around the functions in ObjectImpl. While at it, document `TypeInfo`. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- rust/hw/char/pl011/src/device.rs | 40 +++++++-------------- rust/qemu-api/src/definitions.rs | 61 +++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 32 deletions(-)