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

[Zhao Liu]

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