There is no need to monkeypatch DeviceId::Luminary into the already-initialized
PL011State.  Instead, now that we can define a class hierarchy, we can define
PL011Class and make device_id a field in there.

There is also no need anymore to have "Arm" as zero, so change DeviceId into a
wrapper for the array; all it does is provide an Index<hwaddr> implementation
because arrays can only be indexed by usize.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 59 +++++++++++++++-----------------
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index e85d13c5a2b..41220c99a83 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -5,7 +5,7 @@
 use core::ptr::{addr_of_mut, NonNull};
 use std::{
     ffi::CStr,
-    os::raw::{c_int, c_uchar, c_uint, c_void},
+    os::raw::{c_int, c_uint, c_void},
 };
 
 use qemu_api::{
@@ -14,7 +14,7 @@
     irq::InterruptSource,
     prelude::*,
     qdev::DeviceImpl,
-    qom::ObjectImpl,
+    qom::{ClassInitImpl, ObjectImpl},
 };
 
 use crate::{
@@ -35,27 +35,20 @@
 /// QEMU sourced constant.
 pub const PL011_FIFO_DEPTH: usize = 16_usize;
 
-#[derive(Clone, Copy, Debug)]
-enum DeviceId {
-    #[allow(dead_code)]
-    Arm = 0,
-    Luminary,
-}
+#[derive(Clone, Copy)]
+struct DeviceId(&'static [u8; 8]);
 
 impl std::ops::Index<hwaddr> for DeviceId {
-    type Output = c_uchar;
+    type Output = u8;
 
     fn index(&self, idx: hwaddr) -> &Self::Output {
-        match self {
-            Self::Arm => &Self::PL011_ID_ARM[idx as usize],
-            Self::Luminary => &Self::PL011_ID_LUMINARY[idx as usize],
-        }
+        &self.0[idx as usize]
     }
 }
 
 impl DeviceId {
-    const PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
-    const PL011_ID_LUMINARY: [c_uchar; 8] = [0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1];
+    const ARM: Self = Self(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
+    const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
 }
 
 #[repr(C)]
@@ -102,17 +95,28 @@ pub struct PL011State {
     pub clock: NonNull<Clock>,
     #[doc(alias = "migrate_clk")]
     pub migrate_clock: bool,
-    /// The byte string that identifies the device.
-    device_id: DeviceId,
 }
 
 qom_isa!(PL011State : SysBusDevice, DeviceState, Object);
 
+pub struct PL011Class {
+    parent_class: <SysBusDevice as ObjectType>::Class,
+    /// The byte string that identifies the device.
+    device_id: DeviceId,
+}
+
 unsafe impl ObjectType for PL011State {
-    type Class = <SysBusDevice as ObjectType>::Class;
+    type Class = PL011Class;
     const TYPE_NAME: &'static CStr = crate::TYPE_PL011;
 }
 
+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);
+    }
+}
+
 impl ObjectImpl for PL011State {
     type ParentType = SysBusDevice;
 
@@ -190,7 +194,8 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
 
         std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
             Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
-                u64::from(self.device_id[(offset - 0xfe0) >> 2])
+                let device_id = self.get_class().device_id;
+                u64::from(device_id[(offset - 0xfe0) >> 2])
             }
             Err(_) => {
                 // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
@@ -621,16 +626,10 @@ pub struct PL011Luminary {
     parent_obj: PL011State,
 }
 
-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;
+impl ClassInitImpl<PL011Class> for PL011Luminary {
+    fn class_init(klass: &mut PL011Class) {
+        klass.device_id = DeviceId::LUMINARY;
+        <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
     }
 }
 
@@ -643,8 +642,6 @@ unsafe impl ObjectType for PL011Luminary {
 
 impl ObjectImpl for PL011Luminary {
     type ParentType = PL011State;
-
-    const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
 }
 
 impl DeviceImpl for PL011Luminary {}
-- 
2.47.1

On Mon, Dec 09, 2024 at 01:37:15PM +0100, Paolo Bonzini wrote:
> Date: Mon,  9 Dec 2024 13:37:15 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 24/26] rust: qom: move device_id to PL011 class side
> X-Mailer: git-send-email 2.47.1
> 
> There is no need to monkeypatch DeviceId::Luminary into the already-initialized
> PL011State.  Instead, now that we can define a class hierarchy, we can define
> PL011Class and make device_id a field in there.
> 
> There is also no need anymore to have "Arm" as zero, so change DeviceId into a
> wrapper for the array; all it does is provide an Index<hwaddr> implementation
> because arrays can only be indexed by usize.
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 59 +++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 31 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

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

However, SysBusDevice (and Device) is a bit different. Its TypeInfo is
registered on the C side, and the class_init method on the Rust side is not
actually a real QOM class_init (because it is not registered on the Rust
side).

Therefore, the call here seems valid from the code logic's perspective.
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.

So, for devices like SysBusDevice that are registered on the C side,
should we not implement class_init and also not call it explicitly?

Or should we distinguish between two different usages of class_init? One
is registered in TypeInfo (only as a callback in rust_class_init) - perhaps
rename it as qom_class_init, and the other is used as a helper for Rust-side
calls (similar to the recursive usage here) - maybe rename it as
class_inter_init.

> +    }
> +}

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

Maybe the confusion is because I implemented class_init twice instead of
using a separate trait "PL011Impl"?

Paolo

So, for devices like SysBusDevice that are registered on the C side,
> should we not implement class_init and also not call it explicitly?
>
> Or should we distinguish between two different usages of class_init? One
> is registered in TypeInfo (only as a callback in rust_class_init) - perhaps
> rename it as qom_class_init, and the other is used as a helper for
> Rust-side
> calls (similar to the recursive usage here) - maybe rename it as
> class_inter_init.
>
> > +    }
> > +}
>
>

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

Il mer 18 dic 2024, 07:39 Zhao Liu <zhao1.liu@intel.com> ha scritto:

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

No, I don't think so. You have the same thing already with
PL011State/PL011Luminary.

There, you have

* object_class_init
* device_class_init
* sysbus_device_class_init
* <PL011State as ClassInitImpl<PL011Class>>::class_init
  * <PL011State as ClassInitImpl<SysBusDeviceClass>>::class_init
    * <PL011State as ClassInitImpl<DeviceClass>>::class_init
      * <PL011State as ClassInitImpl<ObjectClass>>::class_init
* <PL011Luminary as ClassInitImpl<PL011Class>>::class_init
  * <PL011Luminary as ClassInitImpl<SysBusDeviceClass>>::class_init
    * <PL011Luminary as ClassInitImpl<DeviceClass>>::class_init
      * <PL011Luminary as ClassInitImpl<ObjectClass>>::class_init

But note that these calls are all different and indeed the last three are
empty (all vtable entries are None). This is like a C class_init
implementation that does not set any of sdc, dc or oc.

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

Ugh, you're right. Maybe ClassInitImpl should just be merged into
ObjectImpl etc. as a default method implementation. I will check.

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

No, Self is not PL011Class. Self is PL011State (or PL011Luminary/ and it
always remains the same. What changes is *what part* of the class is
overwritten, but the order of calls from qom/object.c follows the same
logic in both C and Rust.

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

Do you still think that this is the case? I will look into how to avoid the
problem with the orphan rule, but otherwise I think things are fine.

Paolo

>

> No, Self is not PL011Class. Self is PL011State (or PL011Luminary/ and it
> always remains the same. What changes is *what part* of the class is
> overwritten, but the order of calls from qom/object.c follows the same
> logic in both C and Rust.

Thanks! Now I feel I see!

For C side, type_initialize() will allocate a new class instance by
`ti->class = g_malloc0(ti->class_size)`, then actually C side's parent
class_init will initialize that new class instance.

For Rust side, the initialization call chain will initialize Self's
embedded parent class, one by one.

So that's fine!

> > 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.
> >
> 
> Do you still think that this is the case?

No, now this patch is fine for me!

Thanks,
Zhao

On 12/18/24 08:14, Paolo Bonzini wrote:
>     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.
> 
> Ugh, you're right. Maybe ClassInitImpl should just be merged into 
> ObjectImpl etc. as a default method implementation. I will check.

Ok, I think we can make it mostly a documentation issue:

diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 2b472915707..ee95eda0447 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -451,13 +451,14 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
  /// Each struct will implement this trait with `T` equal to each
  /// superclass.  For example, a device should implement at least
  /// `ClassInitImpl<`[`DeviceClass`](crate::qdev::DeviceClass)`>` and
-/// `ClassInitImpl<`[`ObjectClass`]`>`.
+/// `ClassInitImpl<`[`ObjectClass`]`>`.  Such implementations are
+/// made in one of two ways.
  ///
-/// Fortunately, this is almost never necessary.  Instead, the Rust
-/// implementation of methods will usually come from a trait like
-/// [`ObjectImpl`] or [`DeviceImpl`](crate::qdev::DeviceImpl).
-/// `ClassInitImpl` then can be provided by blanket implementations
-/// that operate on all implementors of the `*Impl`* trait.  For example:
+/// For most superclasses, `ClassInitImpl` is provided by the `qemu-api`
+/// crate itself.  The Rust implementation of methods will come from a
+/// trait like [`ObjectImpl`] or [`DeviceImpl`](crate::qdev::DeviceImpl),
+/// and `ClassInitImpl` is provided by blanket implementations that
+/// operate on all implementors of the `*Impl`* trait.  For example:
  ///
  /// ```ignore
  /// impl<T> ClassInitImpl<DeviceClass> for T
@@ -469,11 +470,37 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
  /// after initializing the `DeviceClass` part of the class struct,
  /// the parent [`ObjectClass`] is initialized as well.
  ///
-/// The only case in which a manual implementation of the trait is needed
-/// is for interfaces (note that there is no Rust example yet for using
-/// interfaces).  In this case, unlike the C case, the Rust class _has_
-/// to define its own class struct `FooClass` to go together with
-/// `ClassInitImpl<FooClass>`.
+/// In some other cases, manual implementation of the trait is needed.
+/// These are the following:
+///
+/// * for classes that implement interfaces, the Rust code _has_
+///   to define its own class struct `FooClass` and implement
+///   `ClassInitImpl<FooClass>`.  `ClassInitImpl<FooClass>`'s
+///   `class_init` method will then forward to multiple other
+///   `class_init`s, for the interfaces as well as the superclass.
+///   (Note that there is no Rust example yet for using
+///   interfaces).
+///
+/// * for classes implemented outside the ``qemu-api`` crate, it's
+///   not possible to add blanket implementations like the above one,
+///   due to orphan rules.  In that case, the easiest solution is to
+///   implement `ClassInitImpl<YourSuperclass>` for each subclass,
+///   and not have a `YourSuperclassImpl` trait at all:
+///
+///   ```ignore
+///   impl ClassInitImpl<YourSuperclass> for YourSubclass {
+///       fn class_init(klass: &mut YourSuperclass) {
+///           klass.some_method = Some(Self::some_method);
+///           <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
+///       }
+///   }
+///   ```
+///
+///   While this method incurs a small amount of code duplication,
+///   it is generally limited to the recursive call on the last line.
+///   This is because classes defined in Rust do not need the same
+///   glue code that is needed when the classes are defined in C code.
+///   You may consider using a macro if you have many subclasses.
  pub trait ClassInitImpl<T> {
      /// Initialize `klass` to point to the virtual method implementations
      /// for `Self`.  On entry, the virtual method pointers are set to


Optionally, something like this can be squashed in this patch, but I
do not think it's worth the savings of... 3 lines of code:

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 41220c99a83..cbd3abb96ec 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -46,11 +46,6 @@ fn index(&self, idx: hwaddr) -> &Self::Output {
      }
  }
  
-impl DeviceId {
-    const ARM: Self = Self(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
-    const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
-}
-
  #[repr(C)]
  #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
  /// PL011 Device Model in QEMU
@@ -112,7 +107,8 @@ unsafe impl ObjectType for PL011State {
  
  impl ClassInitImpl<PL011Class> for PL011State {
      fn class_init(klass: &mut PL011Class) {
-        klass.device_id = DeviceId::ARM;
+        klass.device_id = DeviceId(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
+
          <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
      }
  }
@@ -628,7 +624,8 @@ pub struct PL011Luminary {
  
  impl ClassInitImpl<PL011Class> for PL011Luminary {
      fn class_init(klass: &mut PL011Class) {
-        klass.device_id = DeviceId::LUMINARY;
+        klass.device_id = DeviceId(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
+
          <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
      }
  }

On Wed, Dec 18, 2024 at 11:26:35AM +0100, Paolo Bonzini wrote:
> Date: Wed, 18 Dec 2024 11:26:35 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 24/26] rust: qom: move device_id to PL011 class side
> 
> On 12/18/24 08:14, Paolo Bonzini wrote:
> >     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.
> > 
> > Ugh, you're right. Maybe ClassInitImpl should just be merged into
> > ObjectImpl etc. as a default method implementation. I will check.
> 
> Ok, I think we can make it mostly a documentation issue:
> 
> diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
> index 2b472915707..ee95eda0447 100644
> --- a/rust/qemu-api/src/qom.rs
> +++ b/rust/qemu-api/src/qom.rs
> @@ -451,13 +451,14 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
>  /// Each struct will implement this trait with `T` equal to each
>  /// superclass.  For example, a device should implement at least
>  /// `ClassInitImpl<`[`DeviceClass`](crate::qdev::DeviceClass)`>` and
> -/// `ClassInitImpl<`[`ObjectClass`]`>`.
> +/// `ClassInitImpl<`[`ObjectClass`]`>`.  Such implementations are
> +/// made in one of two ways.
>  ///
> -/// Fortunately, this is almost never necessary.  Instead, the Rust
> -/// implementation of methods will usually come from a trait like
> -/// [`ObjectImpl`] or [`DeviceImpl`](crate::qdev::DeviceImpl).
> -/// `ClassInitImpl` then can be provided by blanket implementations
> -/// that operate on all implementors of the `*Impl`* trait.  For example:
> +/// For most superclasses, `ClassInitImpl` is provided by the `qemu-api`
> +/// crate itself.  The Rust implementation of methods will come from a
> +/// trait like [`ObjectImpl`] or [`DeviceImpl`](crate::qdev::DeviceImpl),
> +/// and `ClassInitImpl` is provided by blanket implementations that
> +/// operate on all implementors of the `*Impl`* trait.  For example:
>  ///
>  /// ```ignore
>  /// impl<T> ClassInitImpl<DeviceClass> for T
> @@ -469,11 +470,37 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
>  /// after initializing the `DeviceClass` part of the class struct,
>  /// the parent [`ObjectClass`] is initialized as well.
>  ///
> -/// The only case in which a manual implementation of the trait is needed
> -/// is for interfaces (note that there is no Rust example yet for using
> -/// interfaces).  In this case, unlike the C case, the Rust class _has_
> -/// to define its own class struct `FooClass` to go together with
> -/// `ClassInitImpl<FooClass>`.
> +/// In some other cases, manual implementation of the trait is needed.
> +/// These are the following:
> +///
> +/// * for classes that implement interfaces, the Rust code _has_
> +///   to define its own class struct `FooClass` and implement
> +///   `ClassInitImpl<FooClass>`.  `ClassInitImpl<FooClass>`'s
> +///   `class_init` method will then forward to multiple other
> +///   `class_init`s, for the interfaces as well as the superclass.
> +///   (Note that there is no Rust example yet for using
> +///   interfaces).
> +///
> +/// * for classes implemented outside the ``qemu-api`` crate, it's
> +///   not possible to add blanket implementations like the above one,
> +///   due to orphan rules.  In that case, the easiest solution is to
> +///   implement `ClassInitImpl<YourSuperclass>` for each subclass,
> +///   and not have a `YourSuperclassImpl` trait at all:
> +///
> +///   ```ignore
> +///   impl ClassInitImpl<YourSuperclass> for YourSubclass {
> +///       fn class_init(klass: &mut YourSuperclass) {
> +///           klass.some_method = Some(Self::some_method);
> +///           <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
> +///       }
> +///   }
> +///   ```

BTW, maybe we could also squash my previous example in test? :-)

> +///
> +///   While this method incurs a small amount of code duplication,
> +///   it is generally limited to the recursive call on the last line.
> +///   This is because classes defined in Rust do not need the same
> +///   glue code that is needed when the classes are defined in C code.

Now I understand this advantage.

> +///   You may consider using a macro if you have many subclasses.

Yes, a custom macro is enough!

>  pub trait ClassInitImpl<T> {
>      /// Initialize `klass` to point to the virtual method implementations
>      /// for `Self`.  On entry, the virtual method pointers are set to

All the above changes look good to me!

> Optionally, something like this can be squashed in this patch, but I
> do not think it's worth the savings of... 3 lines of code:
> 
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 41220c99a83..cbd3abb96ec 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -46,11 +46,6 @@ fn index(&self, idx: hwaddr) -> &Self::Output {
>      }
>  }
> -impl DeviceId {
> -    const ARM: Self = Self(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
> -    const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
> -}
> -
>  #[repr(C)]
>  #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
>  /// PL011 Device Model in QEMU
> @@ -112,7 +107,8 @@ unsafe impl ObjectType for PL011State {
>  impl ClassInitImpl<PL011Class> for PL011State {
>      fn class_init(klass: &mut PL011Class) {
> -        klass.device_id = DeviceId::ARM;
> +        klass.device_id = DeviceId(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
> +
>          <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
>      }
>  }
> @@ -628,7 +624,8 @@ pub struct PL011Luminary {
>  impl ClassInitImpl<PL011Class> for PL011Luminary {
>      fn class_init(klass: &mut PL011Class) {
> -        klass.device_id = DeviceId::LUMINARY;
> +        klass.device_id = DeviceId(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
> +
>          <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
>      }
>  }

Still fine for me. :-)

Regards,
Zhao

Il mer 18 dic 2024, 15:32 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> On Wed, Dec 18, 2024 at 11:26:35AM +0100, Paolo Bonzini wrote:
> > Date: Wed, 18 Dec 2024 11:26:35 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: Re: [PATCH 24/26] rust: qom: move device_id to PL011 class side
> >
> > On 12/18/24 08:14, Paolo Bonzini wrote:
> > >     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.
>
> BTW, maybe we could also squash my previous example in test? :-)
>

Sure.

Paolo