On Tue, Dec 17, 2024 at 05:50:09PM +0100, Paolo Bonzini wrote:
> Date: Tue, 17 Dec 2024 17:50:09 +0100
> From: Paolo Bonzini <pbonz...@redhat.com>
> Subject: Re: [PATCH 24/26] rust: qom: move device_id to PL011 class side
>
> Il mar 17 dic 2024, 04:39 Zhao Liu <zhao1....@intel.com> ha scritto:
>
> > > +impl ClassInitImpl<PL011Class> for PL011State {
> > > + fn class_init(klass: &mut PL011Class) {
> > > + klass.device_id = DeviceId::ARM;
> > > + <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut
> > klass.parent_class);
> >
> > This seems a bit of a conflict with the C version of QOM semantics. In C,
> > class_init is registered in TypeInfo, and then the QOM code will
> > automatically call the parent's class_init without needing to explicitly
> > call the parent's in the child's class_init.
> >
>
> This is the same in Rust.
>
> The difference is that in C you have a single class_init function that sets
> all members of ObjectClass, DeviceClass, etc. In Rust each class has one
> trait and there is a chain of ClassInitImpl implementations—one filling in
> "oc" from ObjectImpl, one filling in "dc" from DeviceImpl and so on.
>
> But in both cases you get a chain of calls from qom/object.c.
>
> Therefore, the call here seems valid from the code logic's perspective.
I supposed a case, where there is such a QOM (QEMU Object Model)
structure relationship:
* DummyState / DummyClass: defined in Rust side, and registered the
TypeInfo by `Object` macro.
- So its class_init will be called by C QOM code.
* DummyChildState / DummyChildClass: defined in Rust side as the
child-object of DummyState, and registered the TypeInfo by `Object`
macro. And suppose it can inherit the trait of DummyClass -
ClassInitImpl<DummyClass> (but I found a gap here, as detailed later;
I expect it should be able to inherit normally).
- So its class_init will be called by C QOM code. In C code call chain,
its parent's class_init should be called by C before its own
class_init.
- However, note that according to the Rust class initialization call
chain, it should also call the parent's class_init within its own
class_init.
- :( the parent's class_init gets called twice.
If you agree this case indeed exists, then I think we should distinguish
between different class_init methods for the Rust and C call chains.
Moving on to another topic, about the gap (or question :-)) where a
child class inherits the ClassInitImpl trait from the parent, please see
my test case example below: Doing something similar to SysBusDevice and
DeviceState using a generic T outside of the QOM library would violate
the orphan rule.
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 7edadf911cca..8cae222a37be 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -13,8 +13,8 @@
use qemu_api::{
bindings::*,
c_str, declare_properties, define_property,
- qdev::{DeviceImpl, DeviceState, Property},
- qom::{ObjectCast, ObjectCastMut, ObjectImpl, ObjectMethods, ObjectType},
+ qdev::{DeviceClass, DeviceImpl, DeviceState, Property},
+ qom::{ClassInitImpl, ObjectCast, ObjectCastMut, ObjectImpl, ObjectMethods,
ObjectType},
qom_isa,
vmstate::VMStateDescription,
zeroable::Zeroable,
@@ -37,6 +37,10 @@ pub struct DummyState {
qom_isa!(DummyState: Object, DeviceState);
+pub struct DummyClass {
+ parent_class: <DeviceState as ObjectType>::Class,
+}
+
declare_properties! {
DUMMY_PROPERTIES,
define_property!(
@@ -49,7 +53,7 @@ pub struct DummyState {
}
unsafe impl ObjectType for DummyState {
- type Class = <DeviceState as ObjectType>::Class;
+ type Class = DummyClass;
const TYPE_NAME: &'static CStr = c_str!("dummy");
}
@@ -67,6 +71,51 @@ fn vmsd() -> Option<&'static VMStateDescription> {
}
}
+// `impl<T> ClassInitImpl<DummyClass> for T` doesn't work since it violates
orphan rule.
+impl ClassInitImpl<DummyClass> for DummyState {
+ fn class_init(klass: &mut DummyClass) {
+ <Self as ClassInitImpl<DeviceClass>>::class_init(&mut
klass.parent_class);
+ }
+}
+
+#[derive(qemu_api_macros::offsets)]
+#[repr(C)]
+#[derive(qemu_api_macros::Object)]
+pub struct DummyChildState {
+ parent: DummyState,
+ migrate_clock: bool,
+}
+
+qom_isa!(DummyChildState: Object, DeviceState, DummyState);
+
+pub struct DummyChildClass {
+ parent_class: <DummyState as ObjectType>::Class,
+}
+
+unsafe impl ObjectType for DummyChildState {
+ type Class = DummyChildClass;
+ const TYPE_NAME: &'static CStr = c_str!("dummy_child");
+}
+
+impl ObjectImpl for DummyChildState {
+ type ParentType = DummyState;
+ const ABSTRACT: bool = false;
+}
+
+impl DeviceImpl for DummyChildState {}
+
+impl ClassInitImpl<DummyClass> for DummyChildState {
+ fn class_init(klass: &mut DummyClass) {
+ <Self as ClassInitImpl<DeviceClass>>::class_init(&mut
klass.parent_class);
+ }
+}
+
+impl ClassInitImpl<DummyChildClass> for DummyChildState {
+ fn class_init(klass: &mut DummyChildClass) {
+ <Self as ClassInitImpl<DummyClass>>::class_init(&mut
klass.parent_class);
+ }
+}
+
fn init_qom() {
static ONCE: Mutex<Cell<bool>> = Mutex::new(Cell::new(false));
@@ -85,6 +134,7 @@ fn test_object_new() {
init_qom();
unsafe {
object_unref(object_new(DummyState::TYPE_NAME.as_ptr()).cast());
+ object_unref(object_new(DummyChildState::TYPE_NAME.as_ptr()).cast());
}
}
> > But, when there is deeper class inheritance, it seems impossible to
> > prevent class_init from being called both by the C side's QOM code and by
> > this kind of recursive case on the Rust side.
> >
>
> Note that here you have two parameters: what class is being filled (the
> argument C of ClassInitImpl<C>) *and* what type is being initialized
> (that's Self).
>
> The "recursion" is only on the argument C, and matches the way C code
> implements class_init.
For Rust side, PL011Class' class_init calls SysBusDeviceClass' class_init,
and SysBusDeviceClass will also call DeviceClass' class_init. So this is
also recursion, right?
> Maybe the confusion is because I implemented class_init twice instead of
> using a separate trait "PL011Impl"?
Ah, yes! But I think the Rust call chain should not use class_init anymore
but should use a different method. This way, the original class_init would
only serve the C QOM. A separate trait might break the inheritance
relationship similar to ClassInitImpl.
Regards,
Zhao
