This removes undefined behavior associated to writing to uninitialized
fields, and makes it possible to remove "unsafe" from the instance_init
implementation.
However, the init function itself is still unsafe, because it must promise
(as a sort as MaybeUninit::assume_init) that all fields have been
initialized.
Reviewed-by: Zhao Liu <zhao1....@intel.com>
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 34 ++++++++++------------
rust/hw/timer/hpet/src/device.rs | 16 ++++-------
rust/qemu-api/src/memory.rs | 12 ++++----
rust/qemu-api/src/qdev.rs | 49 +++++++++++++++++++-------------
rust/qemu-api/src/qom.rs | 9 ++++--
5 files changed, 63 insertions(+), 57 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index be8387f6f2d..2d416cd9a3c 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.pitsidiana...@linaro.org>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::{ffi::CStr, mem::size_of, ptr::addr_of_mut};
+use std::{ffi::CStr, mem::size_of};
use qemu_api::{
chardev::{CharBackend, Chardev, Event},
@@ -11,9 +11,10 @@
memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
prelude::*,
qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType,
ResettablePhasesImpl},
- qom::{ObjectImpl, Owned, ParentField},
+ qom::{ObjectImpl, Owned, ParentField, ParentInit},
static_assert,
sysbus::{SysBusDevice, SysBusDeviceImpl},
+ uninit_field_mut,
vmstate::VMStateDescription,
};
@@ -163,7 +164,7 @@ impl PL011Impl for PL011State {
impl ObjectImpl for PL011State {
type ParentType = SysBusDevice;
- const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+ const INSTANCE_INIT: Option<unsafe fn(ParentInit<Self>)> =
Some(Self::init);
const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
}
@@ -488,7 +489,7 @@ impl PL011State {
/// `PL011State` type. It must not be called more than once on the same
/// location/instance. All its fields are expected to hold uninitialized
/// values with the sole exception of `parent_obj`.
- unsafe fn init(&mut self) {
+ unsafe fn init(mut this: ParentInit<Self>) {
static PL011_OPS: MemoryRegionOps<PL011State> =
MemoryRegionOpsBuilder::<PL011State>::new()
.read(&PL011State::read)
.write(&PL011State::write)
@@ -496,28 +497,23 @@ unsafe fn init(&mut self) {
.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.
+ // SAFETY: this and this.iomem are guaranteed to be valid at this point
MemoryRegion::init_io(
- unsafe { &mut *addr_of_mut!(self.iomem) },
- addr_of_mut!(*self),
+ &mut uninit_field_mut!(*this, iomem),
&PL011_OPS,
"pl011",
0x1000,
);
- self.regs = Default::default();
+ uninit_field_mut!(*this, regs).write(Default::default());
- // SAFETY:
- //
- // self.clock is not initialized at this point; but since `Owned<_>` is
- // not Drop, we can overwrite the undefined value without side effects;
- // it's not sound but, because for all PL011State instances are created
- // by QOM code which calls this function to initialize the fields, at
- // leastno code is able to access an invalid self.clock value.
- self.clock = self.init_clock_in("clk", &Self::clock_update,
ClockEvent::ClockUpdate);
+ let clock = DeviceState::init_clock_in(
+ &mut this,
+ "clk",
+ &Self::clock_update,
+ ClockEvent::ClockUpdate,
+ );
+ uninit_field_mut!(*this, clock).write(clock);
}
const fn clock_update(&self, _event: ClockEvent) {
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 340ca1d355d..a281927781e 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -21,8 +21,8 @@
hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder,
MEMTXATTRS_UNSPECIFIED,
},
prelude::*,
- qdev::{DeviceImpl, DeviceMethods, DeviceState, Property, ResetType,
ResettablePhasesImpl},
- qom::{ObjectImpl, ObjectType, ParentField},
+ qdev::{DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
+ qom::{ObjectImpl, ObjectType, ParentField, ParentInit},
qom_isa,
sysbus::{SysBusDevice, SysBusDeviceImpl},
timer::{Timer, CLOCK_VIRTUAL, NANOSECONDS_PER_SECOND},
@@ -697,7 +697,7 @@ fn set_counter_reg(&self, shift: u32, len: u32, val: u64) {
.set(self.counter.get().deposit(shift, len, val));
}
- unsafe fn init(&mut self) {
+ unsafe fn init(mut this: ParentInit<Self>) {
static HPET_RAM_OPS: MemoryRegionOps<HPETState> =
MemoryRegionOpsBuilder::<HPETState>::new()
.read(&HPETState::read)
@@ -707,18 +707,14 @@ unsafe fn init(&mut self) {
.impl_sizes(4, 8)
.build();
- // SAFETY:
- // self and self.iomem are guaranteed to be valid at this point since
callers
- // must make sure the `self` reference is valid.
MemoryRegion::init_io(
- unsafe { &mut *addr_of_mut!(self.iomem) },
- addr_of_mut!(*self),
+ &mut uninit_field_mut!(*this, iomem),
&HPET_RAM_OPS,
"hpet",
HPET_REG_SPACE_LEN,
);
- Self::init_timers(unsafe { &mut *((self as *mut
Self).cast::<MaybeUninit<Self>>()) });
+ Self::init_timers(&mut this);
}
fn post_init(&self) {
@@ -900,7 +896,7 @@ unsafe impl ObjectType for HPETState {
impl ObjectImpl for HPETState {
type ParentType = SysBusDevice;
- const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+ const INSTANCE_INIT: Option<unsafe fn(ParentInit<Self>)> =
Some(Self::init);
const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
}
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index 9ef2694bd62..e40fad6cf19 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -16,6 +16,7 @@
callbacks::FnCall,
cell::Opaque,
prelude::*,
+ uninit::MaybeUninitField,
zeroable::Zeroable,
};
@@ -147,7 +148,7 @@ impl MemoryRegion {
#[inline(always)]
unsafe fn do_init_io(
slot: *mut bindings::MemoryRegion,
- owner: *mut Object,
+ owner: *mut bindings::Object,
ops: &'static bindings::MemoryRegionOps,
name: &'static str,
size: u64,
@@ -156,7 +157,7 @@ unsafe fn do_init_io(
let cstr = CString::new(name).unwrap();
memory_region_init_io(
slot,
- owner.cast::<bindings::Object>(),
+ owner,
ops,
owner.cast::<c_void>(),
cstr.as_ptr(),
@@ -166,16 +167,15 @@ unsafe fn do_init_io(
}
pub fn init_io<T: IsA<Object>>(
- &mut self,
- owner: *mut T,
+ this: &mut MaybeUninitField<'_, T, Self>,
ops: &'static MemoryRegionOps<T>,
name: &'static str,
size: u64,
) {
unsafe {
Self::do_init_io(
- self.0.as_mut_ptr(),
- owner.cast::<Object>(),
+ this.as_mut_ptr().cast(),
+ MaybeUninitField::parent_mut(this).cast(),
&ops.0,
name,
size,
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 0610959f467..36f02fb57db 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -19,7 +19,7 @@
error::{Error, Result},
irq::InterruptSource,
prelude::*,
- qom::{ObjectClass, ObjectImpl, Owned},
+ qom::{ObjectClass, ObjectImpl, Owned, ParentInit},
vmstate::VMStateDescription,
};
@@ -247,15 +247,9 @@ unsafe impl ObjectType for DeviceState {
}
qom_isa!(DeviceState: Object);
-/// Trait for methods exposed by the [`DeviceState`] class. The methods can be
-/// called on all objects that have the trait `IsA<DeviceState>`.
-///
-/// The trait should only be used through the blanket implementation,
-/// which guarantees safety via `IsA`.
-pub trait DeviceMethods: ObjectDeref
-where
- Self::Target: IsA<DeviceState>,
-{
+/// Initialization methods take a [`ParentInit`] and can be called as
+/// associated functions.
+impl DeviceState {
/// Add an input clock named `name`. Invoke the callback with
/// `self` as the first parameter for the events that are requested.
///
@@ -266,12 +260,15 @@ pub trait DeviceMethods: ObjectDeref
/// which Rust code has a reference to a child object) it would be
/// possible for this function to return a `&Clock` too.
#[inline]
- fn init_clock_in<F: for<'a> FnCall<(&'a Self::Target, ClockEvent)>>(
- &self,
+ pub fn init_clock_in<T: DeviceImpl, F: for<'a> FnCall<(&'a T,
ClockEvent)>>(
+ this: &mut ParentInit<T>,
name: &str,
_cb: &F,
events: ClockEvent,
- ) -> Owned<Clock> {
+ ) -> Owned<Clock>
+ where
+ T::ParentType: IsA<DeviceState>,
+ {
fn do_init_clock_in(
dev: &DeviceState,
name: &str,
@@ -287,10 +284,10 @@ fn do_init_clock_in(
unsafe {
let cstr = CString::new(name).unwrap();
let clk = bindings::qdev_init_clock_in(
- dev.as_mut_ptr(),
+ dev.0.as_mut_ptr(),
cstr.as_ptr(),
cb,
- dev.as_void_ptr(),
+ dev.0.as_void_ptr(),
events.0,
);
@@ -307,12 +304,12 @@ fn do_init_clock_in(
// SAFETY: the opaque is "this", which is indeed a pointer to T
F::call((unsafe { &*(opaque.cast::<T>()) }, event))
}
- Some(rust_clock_cb::<Self::Target, F>)
+ Some(rust_clock_cb::<T, F>)
} else {
None
};
- do_init_clock_in(self.upcast(), name, cb, events)
+ do_init_clock_in(unsafe { this.upcast_mut() }, name, cb, events)
}
/// Add an output clock named `name`.
@@ -324,16 +321,30 @@ fn do_init_clock_in(
/// which Rust code has a reference to a child object) it would be
/// possible for this function to return a `&Clock` too.
#[inline]
- fn init_clock_out(&self, name: &str) -> Owned<Clock> {
+ pub fn init_clock_out<T: DeviceImpl>(this: &mut ParentInit<T>, name: &str)
-> Owned<Clock>
+ where
+ T::ParentType: IsA<DeviceState>,
+ {
unsafe {
let cstr = CString::new(name).unwrap();
- let clk =
bindings::qdev_init_clock_out(self.upcast().as_mut_ptr(), cstr.as_ptr());
+ let dev: &mut DeviceState = this.upcast_mut();
+ let clk = bindings::qdev_init_clock_out(dev.0.as_mut_ptr(),
cstr.as_ptr());
let clk: &Clock = Clock::from_raw(clk);
Owned::from(clk)
}
}
+}
+/// Trait for methods exposed by the [`DeviceState`] class. The methods can be
+/// called on all objects that have the trait `IsA<DeviceState>`.
+///
+/// The trait should only be used through the blanket implementation,
+/// which guarantees safety via `IsA`.
+pub trait DeviceMethods: ObjectDeref
+where
+ Self::Target: IsA<DeviceState>,
+{
fn prop_set_chr(&self, propname: &str, chr: &Owned<Chardev>) {
assert!(bql_locked());
let c_propname = CString::new(propname).unwrap();
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 04d102591dc..e20ee014cb1 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -382,12 +382,15 @@ fn deref_mut(&mut self) -> &mut Self::Target {
}
unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut
bindings::Object) {
- let mut state = NonNull::new(obj).unwrap().cast::<T>();
+ let mut state = NonNull::new(obj).unwrap().cast::<MaybeUninit<T>>();
+
// 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()(state.as_mut());
+ ParentInit::with(state.as_mut(), |parent_init| {
+ T::INSTANCE_INIT.unwrap()(parent_init);
+ });
}
}
@@ -654,7 +657,7 @@ pub trait ObjectImpl: ObjectType + IsA<Object> {
///
/// 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;
+ const INSTANCE_INIT: Option<unsafe fn(ParentInit<Self>)> = None;
/// Function that is called to finish initialization of an object, once
/// `INSTANCE_INIT` functions have been called.
--
2.49.0
