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