diff mbox series

[PULL,25/41] rust: qom: put class_init together from multiple ClassInitImpl<>

Message ID 20241219083228.363430-26-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/41] migration: Constify migration_properties | expand

Commit Message

Paolo Bonzini Dec. 19, 2024, 8:32 a.m. UTC
Parameterize the implementation of ClassInitImpl so that it is
possible to call up the chain of implementations, one superclass at
a time starting at ClassInitImpl<Self::Class>.

In order to avoid having to implement (for example)
ClassInitImpl<PL011Class>, also remove the dummy PL011Class and
PL011LuminaryClass structs and specify the same ObjectType::Class as
the superclass.  In the future this default behavior can be handled by
a procedural macro, by looking at the first field in the struct.

Note that the new trait is safe: the calls are started by
rust_class_init<>(), which is not public and can convert the class
pointer to a Rust reference.

Since CLASS_BASE_INIT applies to the type that is being defined,
and only to it, move it to ObjectImpl.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs  |  19 +----
 rust/qemu-api/src/definitions.rs  | 111 ++++++++++++++++++++++++------
 rust/qemu-api/src/device_class.rs |  50 +++++---------
 rust/qemu-api/src/sysbus.rs       |  18 ++++-
 rust/qemu-api/tests/tests.rs      |   9 +--
 5 files changed, 127 insertions(+), 80 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 2, 2025, 5:04 p.m. UTC | #1
Hi,

On 19/12/24 09:32, Paolo Bonzini wrote:
> Parameterize the implementation of ClassInitImpl so that it is
> possible to call up the chain of implementations, one superclass at
> a time starting at ClassInitImpl<Self::Class>.
> 
> In order to avoid having to implement (for example)
> ClassInitImpl<PL011Class>, also remove the dummy PL011Class and
> PL011LuminaryClass structs and specify the same ObjectType::Class as
> the superclass.  In the future this default behavior can be handled by
> a procedural macro, by looking at the first field in the struct.
> 
> Note that the new trait is safe: the calls are started by
> rust_class_init<>(), which is not public and can convert the class
> pointer to a Rust reference.
> 
> Since CLASS_BASE_INIT applies to the type that is being defined,
> and only to it, move it to ObjectImpl.
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs  |  19 +----
>   rust/qemu-api/src/definitions.rs  | 111 ++++++++++++++++++++++++------
>   rust/qemu-api/src/device_class.rs |  50 +++++---------
>   rust/qemu-api/src/sysbus.rs       |  18 ++++-
>   rust/qemu-api/tests/tests.rs      |   9 +--
>   5 files changed, 127 insertions(+), 80 deletions(-)


> diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
> index c98f0b2c7da..dcec5488291 100644
> --- a/rust/qemu-api/src/device_class.rs
> +++ b/rust/qemu-api/src/device_class.rs


> -/// # Safety
> -///
> -/// We expect the FFI user of this function to pass a valid pointer that
> -/// can be downcasted to type `DeviceClass`, because `T` implements
> -/// `DeviceImpl`.
> -pub unsafe extern "C" fn rust_device_class_init<T: DeviceImpl>(
> -    klass: *mut ObjectClass,
> -    _: *mut c_void,
> -) {
> -    let mut dc = ::core::ptr::NonNull::new(klass.cast::<DeviceClass>()).unwrap();
> -    unsafe {
> -        let dc = dc.as_mut();
> +impl<T> ClassInitImpl<DeviceClass> for T
> +where
> +    T: DeviceImpl,
> +{
> +    fn class_init(dc: &mut DeviceClass) {
>           if <T as DeviceImpl>::REALIZE.is_some() {
>               dc.realize = Some(rust_realize_fn::<T>);
>           }
>           if <T as DeviceImpl>::RESET.is_some() {
> -            bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
> +            unsafe {
> +                bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));

Pre-existing, but since it appears on this patch, Rust device models
should not implement this legacy interface. If a non-Rust parent
implements it, I think we should convert the non-Rust parent before
adding a Rust child. No clue how to check a parent don't implement
this interface in Rust.

Generally, we shouldn't access legacy API from Rust IMHO.

> +            }
>           }
>           if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
>               dc.vmsd = vmsd;
>           }
>           let prop = <T as DeviceImpl>::properties();
>           if !prop.is_empty() {
> -            bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
> +            unsafe {
> +                bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
> +            }
>           }
>       }
>   }
Philippe Mathieu-Daudé Jan. 3, 2025, 8:48 a.m. UTC | #2
+Markus, Daniel & Peter

On 3/1/25 10:02, Zhao Liu wrote:
>>> -/// # Safety
>>> -///
>>> -/// We expect the FFI user of this function to pass a valid pointer that
>>> -/// can be downcasted to type `DeviceClass`, because `T` implements
>>> -/// `DeviceImpl`.
>>> -pub unsafe extern "C" fn rust_device_class_init<T: DeviceImpl>(
>>> -    klass: *mut ObjectClass,
>>> -    _: *mut c_void,
>>> -) {
>>> -    let mut dc = ::core::ptr::NonNull::new(klass.cast::<DeviceClass>()).unwrap();
>>> -    unsafe {
>>> -        let dc = dc.as_mut();
>>> +impl<T> ClassInitImpl<DeviceClass> for T
>>> +where
>>> +    T: DeviceImpl,
>>> +{
>>> +    fn class_init(dc: &mut DeviceClass) {
>>>            if <T as DeviceImpl>::REALIZE.is_some() {
>>>                dc.realize = Some(rust_realize_fn::<T>);
>>>            }
>>>            if <T as DeviceImpl>::RESET.is_some() {
>>> -            bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
>>> +            unsafe {
>>> +                bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
>>
>> Pre-existing, but since it appears on this patch, Rust device models
>> should not implement this legacy interface.
> 
> Rust pl011 is just faithful to the C functionality, and it's really time
> to make the rust version drop the previous technical debt too!
> 
> But I think this patch is OK for now since using legacy interface is the
> most simplified approach, and next step we can consider how to convert
> it to the modern interface with proper rust binding (my initial thought
> tells me that there is at least some effort needed here to explore
> whether a simple resettable trait would be enough...).
> 
>> If a non-Rust parent
>> implements it, I think we should convert the non-Rust parent before
>> adding a Rust child. No clue how to check a parent don't implement
>> this interface in Rust.
> 
> For C side, I also didn't find some case to check parent's
> legacy_reset before device_class_set_legacy_reset().
> 
> Maybe we should add `assert(!dc->legacy_reset)` in
> device_class_set_legacy_reset()?
> 
> Similarly, device_class_set_parent_[realize|unrealize] are worth
> adding assert().
> 
> If you agree, I'd be happy to add assert() to these three functions as
> well. :-)
> 
>> Generally, we shouldn't access legacy API from Rust IMHO.
>>
>>> +            }
>>>            }
>>>            if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
>>>                dc.vmsd = vmsd;
>>>            }
>>>            let prop = <T as DeviceImpl>::properties();
>>>            if !prop.is_empty() {
>>> -            bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
>>> +            unsafe {
>>> +                bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
>>> +            }
>>>            }
>>>        }
>>>    }
>>
Zhao Liu Jan. 3, 2025, 9:02 a.m. UTC | #3
> > -/// # Safety
> > -///
> > -/// We expect the FFI user of this function to pass a valid pointer that
> > -/// can be downcasted to type `DeviceClass`, because `T` implements
> > -/// `DeviceImpl`.
> > -pub unsafe extern "C" fn rust_device_class_init<T: DeviceImpl>(
> > -    klass: *mut ObjectClass,
> > -    _: *mut c_void,
> > -) {
> > -    let mut dc = ::core::ptr::NonNull::new(klass.cast::<DeviceClass>()).unwrap();
> > -    unsafe {
> > -        let dc = dc.as_mut();
> > +impl<T> ClassInitImpl<DeviceClass> for T
> > +where
> > +    T: DeviceImpl,
> > +{
> > +    fn class_init(dc: &mut DeviceClass) {
> >           if <T as DeviceImpl>::REALIZE.is_some() {
> >               dc.realize = Some(rust_realize_fn::<T>);
> >           }
> >           if <T as DeviceImpl>::RESET.is_some() {
> > -            bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
> > +            unsafe {
> > +                bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
> 
> Pre-existing, but since it appears on this patch, Rust device models
> should not implement this legacy interface. 

Rust pl011 is just faithful to the C functionality, and it's really time
to make the rust version drop the previous technical debt too! 

But I think this patch is OK for now since using legacy interface is the
most simplified approach, and next step we can consider how to convert
it to the modern interface with proper rust binding (my initial thought
tells me that there is at least some effort needed here to explore
whether a simple resettable trait would be enough...).

> If a non-Rust parent
> implements it, I think we should convert the non-Rust parent before
> adding a Rust child. No clue how to check a parent don't implement
> this interface in Rust.

For C side, I also didn't find some case to check parent's
legacy_reset before device_class_set_legacy_reset().

Maybe we should add `assert(!dc->legacy_reset)` in 
device_class_set_legacy_reset()?

Similarly, device_class_set_parent_[realize|unrealize] are worth
adding assert().

If you agree, I'd be happy to add assert() to these three functions as
well. :-)

> Generally, we shouldn't access legacy API from Rust IMHO.
> 
> > +            }
> >           }
> >           if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
> >               dc.vmsd = vmsd;
> >           }
> >           let prop = <T as DeviceImpl>::properties();
> >           if !prop.is_empty() {
> > -            bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
> > +            unsafe {
> > +                bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
> > +            }
> >           }
> >       }
> >   }
>
Paolo Bonzini Jan. 6, 2025, 11:53 a.m. UTC | #4
Il gio 2 gen 2025, 18:04 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:

> > +            unsafe {
> > +                bindings::device_class_set_legacy_reset(dc,
> Some(rust_reset_fn::<T>));
>
> Pre-existing, but since it appears on this patch, Rust device models
> should not implement this legacy interface. If a non-Rust parent
> implements it, I think we should convert the non-Rust parent before
> adding a Rust child. No clue how to check a parent don't implement
> this interface in Rust.
>
> Generally, we shouldn't access legacy API from Rust IMHO.
>

I disagree that device_class_set_legacy_reset() should not be used.
Three-phase reset is only needed for buses, and requires more code in order
to implement the Resettable interface. Devices gain nothing compared to
using device_class_set_legacy_reset().

In fact, perhaps it should have been named something like
device_class_set_simple_reset()...

Paolo
Peter Maydell Jan. 6, 2025, 1:32 p.m. UTC | #5
On Mon, 6 Jan 2025 at 11:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il gio 2 gen 2025, 18:04 Philippe Mathieu-Daudé <philmd@linaro.org> ha scritto:
>> Pre-existing, but since it appears on this patch, Rust device models
>> should not implement this legacy interface. If a non-Rust parent
>> implements it, I think we should convert the non-Rust parent before
>> adding a Rust child. No clue how to check a parent don't implement
>> this interface in Rust.
>>
>> Generally, we shouldn't access legacy API from Rust IMHO.
>
>
> I disagree that device_class_set_legacy_reset() should not be used. Three-phase reset is only needed for buses, and requires more code in order to implement the Resettable interface. Devices gain nothing compared to using device_class_set_legacy_reset().

Devices using device_class_set_legacy_reset() *are* using
three-phase reset, just with a different function signature:
-- all that device_class_set_legacy_reset() does is register the
provided reset function as the 'hold' phase reset via a
trivial passthrough function that adjusts for the function
signature being slightly different. (This is different from
the situation prior to commit 5fdb6cd27211eff, where we still
had more of the "legacy reset" machinery around with various
transitional handling to make it interwork with three-phase.)

I think here I agree with Philippe that we might as well
provide only the new API to Rust devices.

-- PMM
Paolo Bonzini Jan. 6, 2025, 3:21 p.m. UTC | #6
Il lun 6 gen 2025, 14:32 Peter Maydell <peter.maydell@linaro.org> ha
scritto:

> I think here I agree with Philippe that we might as well
> provide only the new API to Rust devices.
>

Ok, I wasn't thinking of doing that because there isn't right now an easy
way to add interfaces to Rust-defined classes. However, all devices are
Resettable and Device isn't defined in Rust, so it's not hard to add a
ResettableImpl trait in rust/qemu-api/srv/qdev.rs, and initialize it for
all devices.

If anybody wants to do it as an exercise, I am happy to help, otherwise I
can prepare a patch too.

Paolo


> -- PMM
>
>
Philippe Mathieu-Daudé Jan. 7, 2025, 3:50 p.m. UTC | #7
On 7/1/25 16:55, Zhao Liu wrote:
> On Mon, Jan 06, 2025 at 04:21:16PM +0100, Paolo Bonzini wrote:
>> Date: Mon, 6 Jan 2025 16:21:16 +0100
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> Subject: Re: [PULL 25/41] rust: qom: put class_init together from multiple
>>   ClassInitImpl<>
>>
>> Il lun 6 gen 2025, 14:32 Peter Maydell <peter.maydell@linaro.org> ha
>> scritto:
>>
>>> I think here I agree with Philippe that we might as well
>>> provide only the new API to Rust devices.
>>>
>>
>> Ok, I wasn't thinking of doing that because there isn't right now an easy
>> way to add interfaces to Rust-defined classes. However, all devices are
>> Resettable and Device isn't defined in Rust, so it's not hard to add a
>> ResettableImpl trait in rust/qemu-api/srv/qdev.rs, and initialize it for
>> all devices.
>>
>> If anybody wants to do it as an exercise, I am happy to help, otherwise I
>> can prepare a patch too.
>>
> 
> If possible and if no one else wants to practice, I would also like to
> give it a try (I'll add it to my list).

Go ahead! (I'm very busy right now)

Thanks :)
Zhao Liu Jan. 7, 2025, 3:55 p.m. UTC | #8
On Mon, Jan 06, 2025 at 04:21:16PM +0100, Paolo Bonzini wrote:
> Date: Mon, 6 Jan 2025 16:21:16 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PULL 25/41] rust: qom: put class_init together from multiple
>  ClassInitImpl<>
> 
> Il lun 6 gen 2025, 14:32 Peter Maydell <peter.maydell@linaro.org> ha
> scritto:
> 
> > I think here I agree with Philippe that we might as well
> > provide only the new API to Rust devices.
> >
> 
> Ok, I wasn't thinking of doing that because there isn't right now an easy
> way to add interfaces to Rust-defined classes. However, all devices are
> Resettable and Device isn't defined in Rust, so it's not hard to add a
> ResettableImpl trait in rust/qemu-api/srv/qdev.rs, and initialize it for
> all devices.
> 
> If anybody wants to do it as an exercise, I am happy to help, otherwise I
> can prepare a patch too.
>

If possible and if no one else wants to practice, I would also like to
give it a try (I'll add it to my list).

Zhao
Paolo Bonzini Jan. 7, 2025, 4:03 p.m. UTC | #9
On Tue, Jan 7, 2025 at 4:37 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > If anybody wants to do it as an exercise, I am happy to help, otherwise I
> > can prepare a patch too.
>
> If possible and if no one else wants to practice, I would also like to
> give it a try (I'll add it to my list).

Hmm, thinking more about it, it does require a bit that is slightly
more complicated than I anticipated, which is a binding to
object_class_dynamic_cast(). I placed something completely untested
(except it compiles...) in rust-next. It uses a new trait
InterfaceType that can be used as

    ResettableClass::interface_init::<T, DeviceState>(dc);

but I don't really like the two parameters and also I don't like that
class_init is called differently for classes and interfaces. It's
probably possible to design a better API, but I didn't spend too much
time on it because it may require rethinking how classes are declared
(not a huge deal, as we have only four of them).

Feel free to give it a try, but it seems like a relatively low
priority item compared to upstreaming timers (i.e. HPET) and
MemoryRegionOps; or figuring out logging.

Paolo
Philippe Mathieu-Daudé Jan. 7, 2025, 4:24 p.m. UTC | #10
On 7/1/25 17:03, Paolo Bonzini wrote:
> On Tue, Jan 7, 2025 at 4:37 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>>> If anybody wants to do it as an exercise, I am happy to help, otherwise I
>>> can prepare a patch too.
>>
>> If possible and if no one else wants to practice, I would also like to
>> give it a try (I'll add it to my list).
> 
> Hmm, thinking more about it, it does require a bit that is slightly
> more complicated than I anticipated, which is a binding to
> object_class_dynamic_cast(). I placed something completely untested
> (except it compiles...) in rust-next. It uses a new trait
> InterfaceType that can be used as
> 
>      ResettableClass::interface_init::<T, DeviceState>(dc);
> 
> but I don't really like the two parameters and also I don't like that
> class_init is called differently for classes and interfaces. It's
> probably possible to design a better API, but I didn't spend too much
> time on it because it may require rethinking how classes are declared
> (not a huge deal, as we have only four of them).

Are you saying this is not a problem related to QDev Reset, but
a limitation with any QOM interface, and we can not instantiate
any type implementing TYPE_INTERFACE? As in:

   .interfaces = (InterfaceInfo[]) {
     ...
   },

> Feel free to give it a try, but it seems like a relatively low
> priority item compared to upstreaming timers (i.e. HPET) and
> MemoryRegionOps; or figuring out logging.
> 
> Paolo
>
Paolo Bonzini Jan. 7, 2025, 4:29 p.m. UTC | #11
On Tue, Jan 7, 2025 at 5:24 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > but I don't really like the two parameters and also I don't like that
> > class_init is called differently for classes and interfaces. It's
> > probably possible to design a better API, but I didn't spend too much
> > time on it because it may require rethinking how classes are declared
> > (not a huge deal, as we have only four of them).
>
> Are you saying this is not a problem related to QDev Reset, but
> a limitation with any QOM interface, and we can not instantiate
> any type implementing TYPE_INTERFACE? As in:
>
>    .interfaces = (InterfaceInfo[]) {
>      ...
>    },

So far there was no need for that, so it is not implemented. There are
three parts:

1) adding interfaces to the TypeInfo

2) filling in vtables for the interfaces

3) filling in the ResettableClass vtable based on a trait

None of these is supported by Rust code right now, but only (2) and
(3) are needed for qdev reset. That's because the Resettable interface
is declared in DeviceState rather than in the individual devices.

(2) boils down to wrapping object_class_dynamic_cast into a
nice-enough API. The commit that I added to rust-next covers that and
it should work, but the API is a bit unorthogonal.

Paolo
Philippe Mathieu-Daudé Jan. 7, 2025, 4:43 p.m. UTC | #12
On 7/1/25 17:29, Paolo Bonzini wrote:
> On Tue, Jan 7, 2025 at 5:24 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>> but I don't really like the two parameters and also I don't like that
>>> class_init is called differently for classes and interfaces. It's
>>> probably possible to design a better API, but I didn't spend too much
>>> time on it because it may require rethinking how classes are declared
>>> (not a huge deal, as we have only four of them).
>>
>> Are you saying this is not a problem related to QDev Reset, but
>> a limitation with any QOM interface, and we can not instantiate
>> any type implementing TYPE_INTERFACE? As in:
>>
>>     .interfaces = (InterfaceInfo[]) {
>>       ...
>>     },
> 
> So far there was no need for that, so it is not implemented. There are
> three parts:
> 
> 1) adding interfaces to the TypeInfo
> 
> 2) filling in vtables for the interfaces
> 
> 3) filling in the ResettableClass vtable based on a trait
> 
> None of these is supported by Rust code right now, but only (2) and
> (3) are needed for qdev reset. That's because the Resettable interface
> is declared in DeviceState rather than in the individual devices.
> 
> (2) boils down to wrapping object_class_dynamic_cast into a
> nice-enough API. The commit that I added to rust-next covers that and
> it should work, but the API is a bit unorthogonal.

OK, thanks for explaining.

So currently we can not implement any device requiring one of
these interfaces:

$ git grep -hwA1 INTERFACE_CHECK
31:    INTERFACE_CHECK(void, obj, TYPE_RESETTABLE_INTERFACE);
--
12:     INTERFACE_CHECK(AcpiDevAmlIf, (obj), TYPE_ACPI_DEV_AML_IF)
--
24:     INTERFACE_CHECK(AcpiDeviceIf, (obj), \
25-                     TYPE_ACPI_DEVICE_IF)
--
16:    INTERFACE_CHECK(ARMLinuxBootIf, (obj), TYPE_ARM_LINUX_BOOT_IF)
--
29:     INTERFACE_CHECK(FWPathProvider, (obj), TYPE_FW_PATH_PROVIDER)
--
23:     INTERFACE_CHECK(HotplugHandler, (obj), TYPE_HOTPLUG_HANDLER)
--
12:    INTERFACE_CHECK(InterruptStatsProvider, (obj), \
13-                    TYPE_INTERRUPT_STATS_PROVIDER)
--
113:     INTERFACE_CHECK(IPMIInterface, (obj), TYPE_IPMI_INTERFACE)
--
25:    INTERFACE_CHECK(IsaDma, (obj), TYPE_ISADMA)
--
26:     INTERFACE_CHECK(MemoryDeviceState, (obj), TYPE_MEMORY_DEVICE)
--
21:     INTERFACE_CHECK(XlnxCfiIf, (obj), TYPE_XLNX_CFI_IF)
--
33:     INTERFACE_CHECK(NMIState, (obj), TYPE_NMI)
--
30:    INTERFACE_CHECK(PnvXScomInterface, (obj), TYPE_PNV_XSCOM_INTERFACE)
--
50:    INTERFACE_CHECK(SpaprInterruptController, (obj), TYPE_SPAPR_INTC)
--
141:    INTERFACE_CHECK(XICSFabric, (obj), TYPE_XICS_FABRIC)
--
156:    INTERFACE_CHECK(XiveNotifier, (obj), TYPE_XIVE_NOTIFIER)
--
431:    INTERFACE_CHECK(XivePresenter, (obj), TYPE_XIVE_PRESENTER)
--
463:    INTERFACE_CHECK(XiveFabric, (obj), TYPE_XIVE_FABRIC)
--
38:    INTERFACE_CHECK(Nvram, (obj), TYPE_NVRAM)
--
12:     INTERFACE_CHECK(StreamSink, (obj), TYPE_STREAM_SINK)
--
20:    INTERFACE_CHECK(VMStateIf, (obj), TYPE_VMSTATE_IF)
--
14:     INTERFACE_CHECK(UserCreatable, (obj), \
15-                     TYPE_USER_CREATABLE)
--
35:    INTERFACE_CHECK(TPMIf, (obj), TYPE_TPM_IF)
--
35:    INTERFACE_CHECK(IDAUInterface, (obj), TYPE_IDAU_INTERFACE)
--
23:     INTERFACE_CHECK(TestIf, (obj), TYPE_TEST_IF)

Not a big deal at this point, but just to keep it in mind.

Regards,

Phil.
diff mbox series

Patch

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 3e29442a625..d9e9f35f456 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -13,7 +13,6 @@ 
     c_str,
     definitions::ObjectImpl,
     device_class::DeviceImpl,
-    impl_device_class,
     irq::InterruptSource,
     prelude::*,
 };
@@ -108,7 +107,7 @@  pub struct PL011State {
 }
 
 unsafe impl ObjectType for PL011State {
-    type Class = PL011Class;
+    type Class = <SysBusDevice as ObjectType>::Class;
     const TYPE_NAME: &'static CStr = crate::TYPE_PL011;
 }
 
@@ -118,11 +117,6 @@  impl ObjectImpl for PL011State {
     const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
 }
 
-#[repr(C)]
-pub struct PL011Class {
-    _inner: [u8; 0],
-}
-
 impl DeviceImpl for PL011State {
     fn properties() -> &'static [Property] {
         &device_class::PL011_PROPERTIES
@@ -134,8 +128,6 @@  fn vmsd() -> Option<&'static VMStateDescription> {
     const RESET: Option<fn(&mut Self)> = Some(Self::reset);
 }
 
-impl_device_class!(PL011State);
-
 impl PL011State {
     /// Initializes a pre-allocated, unitialized instance of `PL011State`.
     ///
@@ -627,11 +619,6 @@  pub struct PL011Luminary {
     parent_obj: PL011State,
 }
 
-#[repr(C)]
-pub struct PL011LuminaryClass {
-    _inner: [u8; 0],
-}
-
 impl PL011Luminary {
     /// Initializes a pre-allocated, unitialized instance of `PL011Luminary`.
     ///
@@ -646,7 +633,7 @@  unsafe fn init(&mut self) {
 }
 
 unsafe impl ObjectType for PL011Luminary {
-    type Class = PL011LuminaryClass;
+    type Class = <PL011State as ObjectType>::Class;
     const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY;
 }
 
@@ -657,5 +644,3 @@  impl ObjectImpl for PL011Luminary {
 }
 
 impl DeviceImpl for PL011Luminary {}
-
-impl_device_class!(PL011Luminary);
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
index df91a2e31a9..13f8f6fd2a9 100644
--- a/rust/qemu-api/src/definitions.rs
+++ b/rust/qemu-api/src/definitions.rs
@@ -26,6 +26,16 @@ 
     T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::<T>() })
 }
 
+unsafe extern "C" fn rust_class_init<T: ObjectType + ClassInitImpl<T::Class>>(
+    klass: *mut ObjectClass,
+    _data: *mut c_void,
+) {
+    // SAFETY: klass is a T::Class, since rust_class_init<T>
+    // is called from QOM core as the class_init function
+    // for class T
+    T::class_init(unsafe { &mut *klass.cast::<T::Class>() })
+}
+
 /// Trait exposed by all structs corresponding to QOM objects.
 ///
 /// # Safety
@@ -50,7 +60,8 @@ 
 /// - likewise, the first field of the `Class` must be of the class struct
 ///   corresponding to the superclass, which is `ObjectImpl::ParentType::Class`.
 pub unsafe trait ObjectType: Sized {
-    /// The QOM class object corresponding to this struct.  Not used yet.
+    /// The QOM class object corresponding to this struct.  This is used
+    /// to automatically generate a `class_init` method.
     type Class;
 
     /// The name of the type, which can be passed to `object_new()` to
@@ -59,7 +70,7 @@  pub unsafe trait ObjectType: Sized {
 }
 
 /// Trait a type must implement to be registered with QEMU.
-pub trait ObjectImpl: ObjectType + ClassInitImpl {
+pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
     /// The parent of the type.  This should match the first field of
     /// the struct that implements `ObjectImpl`:
     type ParentType: ObjectType;
@@ -80,6 +91,15 @@  pub trait ObjectImpl: ObjectType + ClassInitImpl {
     /// `INSTANCE_INIT` functions have been called.
     const INSTANCE_POST_INIT: Option<fn(&mut Self)> = None;
 
+    /// Called on descendent classes after all parent class initialization
+    /// has occurred, but before the class itself is initialized.  This
+    /// is only useful if a class is not a leaf, and can be used to undo
+    /// the effects of copying the contents of the parent's class struct
+    /// to the descendants.
+    const CLASS_BASE_INIT: Option<
+        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void),
+    > = None;
+
     const TYPE_INFO: TypeInfo = TypeInfo {
         name: Self::TYPE_NAME.as_ptr(),
         parent: Self::ParentType::TYPE_NAME.as_ptr(),
@@ -96,37 +116,86 @@  pub trait ObjectImpl: ObjectType + ClassInitImpl {
         instance_finalize: Self::INSTANCE_FINALIZE,
         abstract_: Self::ABSTRACT,
         class_size: core::mem::size_of::<Self::Class>(),
-        class_init: <Self as ClassInitImpl>::CLASS_INIT,
-        class_base_init: <Self as ClassInitImpl>::CLASS_BASE_INIT,
+        class_init: Some(rust_class_init::<Self>),
+        class_base_init: Self::CLASS_BASE_INIT,
         class_data: core::ptr::null_mut(),
         interfaces: core::ptr::null_mut(),
     };
 }
 
-/// Trait used to fill in a class struct.
+/// Internal trait used to automatically fill in a class struct.
 ///
 /// Each QOM class that has virtual methods describes them in a
 /// _class struct_.  Class structs include a parent field corresponding
 /// to the vtable of the parent class, all the way up to [`ObjectClass`].
-/// Each QOM type has one such class struct.
+/// Each QOM type has one such class struct; this trait takes care of
+/// initializing the `T` part of the class struct, for the type that
+/// implements the trait.
 ///
-/// The Rust implementation of methods will usually come from a trait
-/// like [`ObjectImpl`] or [`DeviceImpl`](crate::device_class::DeviceImpl).
-pub trait ClassInitImpl {
-    /// Function that is called after all parent class initialization
-    /// has occurred.  On entry, the virtual method pointers are set to
+/// Each struct will implement this trait with `T` equal to each
+/// superclass.  For example, a device should implement at least
+/// `ClassInitImpl<`[`DeviceClass`](crate::bindings::DeviceClass)`>`.
+/// Such implementations are made in one of two ways.
+///
+/// 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::device_class::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
+/// where
+///     T: DeviceImpl,
+/// ```
+///
+/// The other case is when manual implementation of the trait is needed.
+/// This covers the following cases:
+///
+/// * if a class implements a QOM interface, 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
     /// the default values coming from the parent classes; the function
     /// can change them to override virtual methods of a parent class.
-    const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)>;
-
-    /// Called on descendent classes after all parent class initialization
-    /// has occurred, but before the class itself is initialized.  This
-    /// is only useful if a class is not a leaf, and can be used to undo
-    /// the effects of copying the contents of the parent's class struct
-    /// to the descendants.
-    const CLASS_BASE_INIT: Option<
-        unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void),
-    >;
+    ///
+    /// The virtual method implementations usually come from another
+    /// trait, for example [`DeviceImpl`](crate::device_class::DeviceImpl)
+    /// when `T` is [`DeviceClass`](crate::bindings::DeviceClass).
+    ///
+    /// On entry, `klass`'s parent class is initialized, while the other fields
+    /// are all zero; it is therefore assumed that all fields in `T` can be
+    /// zeroed, otherwise it would not be possible to provide the class as a
+    /// `&mut T`.  TODO: add a bound of [`Zeroable`](crate::zeroable::Zeroable)
+    /// to T; this is more easily done once Zeroable does not require a manual
+    /// implementation (Rust 1.75.0).
+    fn class_init(klass: &mut T);
 }
 
 #[macro_export]
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
index c98f0b2c7da..dcec5488291 100644
--- a/rust/qemu-api/src/device_class.rs
+++ b/rust/qemu-api/src/device_class.rs
@@ -2,10 +2,11 @@ 
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use std::{ffi::CStr, os::raw::c_void};
+use std::ffi::CStr;
 
 use crate::{
-    bindings::{self, DeviceClass, DeviceState, Error, ObjectClass, Property, VMStateDescription},
+    bindings::{self, DeviceClass, DeviceState, Error, Property, VMStateDescription},
+    definitions::ClassInitImpl,
     prelude::*,
 };
 
@@ -44,7 +45,7 @@  fn vmsd() -> Option<&'static VMStateDescription> {
 /// # Safety
 ///
 /// This function is only called through the QOM machinery and
-/// the `impl_device_class!` macro.
+/// used by the `ClassInitImpl<DeviceClass>` trait.
 /// We expect the FFI user of this function to pass a valid pointer that
 /// can be downcasted to type `T`. We also expect the device is
 /// readable/writeable from one thread at any time.
@@ -65,48 +66,31 @@  fn vmsd() -> Option<&'static VMStateDescription> {
     T::RESET.unwrap()(unsafe { &mut *state });
 }
 
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer that
-/// can be downcasted to type `DeviceClass`, because `T` implements
-/// `DeviceImpl`.
-pub unsafe extern "C" fn rust_device_class_init<T: DeviceImpl>(
-    klass: *mut ObjectClass,
-    _: *mut c_void,
-) {
-    let mut dc = ::core::ptr::NonNull::new(klass.cast::<DeviceClass>()).unwrap();
-    unsafe {
-        let dc = dc.as_mut();
+impl<T> ClassInitImpl<DeviceClass> for T
+where
+    T: DeviceImpl,
+{
+    fn class_init(dc: &mut DeviceClass) {
         if <T as DeviceImpl>::REALIZE.is_some() {
             dc.realize = Some(rust_realize_fn::<T>);
         }
         if <T as DeviceImpl>::RESET.is_some() {
-            bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
+            unsafe {
+                bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
+            }
         }
         if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
             dc.vmsd = vmsd;
         }
         let prop = <T as DeviceImpl>::properties();
         if !prop.is_empty() {
-            bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
+            unsafe {
+                bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
+            }
         }
     }
 }
 
-#[macro_export]
-macro_rules! impl_device_class {
-    ($type:ty) => {
-        impl $crate::definitions::ClassInitImpl for $type {
-            const CLASS_INIT: Option<
-                unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut ::std::os::raw::c_void),
-            > = Some($crate::device_class::rust_device_class_init::<$type>);
-            const CLASS_BASE_INIT: Option<
-                unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut ::std::os::raw::c_void),
-            > = None;
-        }
-    };
-}
-
 #[macro_export]
 macro_rules! define_property {
     ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty, default = $defval:expr$(,)*) => {
@@ -148,8 +132,8 @@  macro_rules! declare_properties {
     };
 }
 
-unsafe impl ObjectType for bindings::DeviceState {
-    type Class = bindings::DeviceClass;
+unsafe impl ObjectType for DeviceState {
+    type Class = DeviceClass;
     const TYPE_NAME: &'static CStr =
         unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) };
 }
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 5ee068541cf..5d15b317405 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -6,7 +6,13 @@ 
 
 pub use bindings::{SysBusDevice, SysBusDeviceClass};
 
-use crate::{bindings, cell::bql_locked, irq::InterruptSource, prelude::*};
+use crate::{
+    bindings::{self, DeviceClass},
+    cell::bql_locked,
+    definitions::ClassInitImpl,
+    irq::InterruptSource,
+    prelude::*,
+};
 
 unsafe impl ObjectType for SysBusDevice {
     type Class = SysBusDeviceClass;
@@ -14,6 +20,16 @@  unsafe impl ObjectType for SysBusDevice {
         unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_SYS_BUS_DEVICE) };
 }
 
+// TODO: add SysBusDeviceImpl
+impl<T> ClassInitImpl<SysBusDeviceClass> for T
+where
+    T: ClassInitImpl<DeviceClass>,
+{
+    fn class_init(sdc: &mut SysBusDeviceClass) {
+        <T as ClassInitImpl<DeviceClass>>::class_init(&mut sdc.parent_class);
+    }
+}
+
 impl SysBusDevice {
     /// Return `self` cast to a mutable pointer, for use in calls to C code.
     const fn as_mut_ptr(&self) -> *mut SysBusDevice {
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 278efe967fe..ed3a555e76d 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -26,11 +26,6 @@  pub struct DummyState {
         pub migrate_clock: bool,
     }
 
-    #[repr(C)]
-    pub struct DummyClass {
-        pub _parent: DeviceClass,
-    }
-
     declare_properties! {
         DUMMY_PROPERTIES,
             define_property!(
@@ -43,7 +38,7 @@  pub struct DummyClass {
     }
 
     unsafe impl ObjectType for DummyState {
-        type Class = DummyClass;
+        type Class = <DeviceState as ObjectType>::Class;
         const TYPE_NAME: &'static CStr = c_str!("dummy");
     }
 
@@ -61,8 +56,6 @@  fn vmsd() -> Option<&'static VMStateDescription> {
         }
     }
 
-    impl_device_class!(DummyState);
-
     unsafe {
         module_call_init(module_init_type::MODULE_INIT_QOM);
         object_unref(object_new(DummyState::TYPE_NAME.as_ptr()).cast());