Re: [PATCH 24/26] rust: qom: move device_id to PL011 class side

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

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 <pbonzini@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.liu@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