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

[Paolo Bonzini]

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);
     }
 }