Re: [PATCH 03/11] rust/qemu-api-macros: introduce Device proc macro

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/24/24 16:03, Manos Pitsidianakis wrote:
> Add a new derive procedural macro to declare device models. Add
> corresponding DeviceImpl trait after already existing ObjectImpl trait.
> At the same time, add instance_init, instance_post_init,
> instance_finalize methods to the ObjectImpl trait and call them from the
> ObjectImplUnsafe trait, which is generated by the procedural macro.
> 
> This allows all the boilerplate device model registration to be handled
> by macros, and all pertinent details to be declared through proc macro
> attributes or trait associated constants and methods.
> The device class can now be generated automatically and the name can be
> optionally overridden:
> 
>    ------------------------ >8 ------------------------
>   #[repr(C)]
>   #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)]
>   #[device(class_name_override = PL011Class)]
>   /// PL011 Device Model in QEMU
>   pub struct PL011State {
>    ------------------------ >8 ------------------------
> 
> Properties are now marked as field attributes:
> 
>    ------------------------ >8 ------------------------
>   #[property(name = c"chardev", qdev_prop = qdev_prop_chr)]
>   pub char_backend: CharBackend,
>    ------------------------ >8 ------------------------

Let's look at this from the point of view of separating the code into 
multiple patches.

This part is probably the easiest to extract.  We can start the device 
impl trait with just the properties:

pub trait DeviceImpl {
     const PROPERTIES: &[Property] = &[];
}

and then the device_class_init! macro can use <$type as 
DeviceImpl>::PROPERTIES instead of $props.

  qemu_api::device_class_init! {
-    pl011_class_init,
-    props => PL011_PROPERTIES,
+    PL011State, pl011_class_init,
      realize_fn => Some(pl011_realize),
      legacy_reset_fn => Some(pl011_reset),
      vmsd => VMSTATE_PL011,
  }

> Object methods (instance_init, etc) methods are now trait methods:
> 
>    ------------------------ >8 ------------------------
>   /// Trait a type must implement to be registered with QEMU.
>   pub trait ObjectImpl {
>       type Class: ClassImpl;
>       const TYPE_NAME: &'static CStr;
>       const PARENT_TYPE_NAME: Option<&'static CStr>;
>       const ABSTRACT: bool;
> 
>       unsafe fn instance_init(&mut self) {}
>       fn instance_post_init(&mut self) {}
>       fn instance_finalize(&mut self) {}
>   }
>    ------------------------ >8 ------------------------

Some comments here:

- instance_init should take either "instance: *mut self" or even better 
"instance: &mut MaybeUninit<self>".

-  for all Rust objects the implementation of instance_finalize() should 
just be ptr::drop_in_place(obj), so this need not be included in the trait.

TYPE_NAME/PARENT_TYPE_NAME/ABSTRACT could be passed to the 
#[derive(Object)] macro, for example:

#[derive(Object)]
// abstract can be optional
#[object(type_name="pl011", parent_type=DeviceState, abstract=false,
          class_type=PL011Class)]

while the "fn instance_init()" remains as a trait implementation in 
pl011/src/device.rs.

Because a trait can be implemented only once, this suggests separating 
the trait(s) in two: one for the constants that can be generated from 
the macro, one for the functions that form the vtable.  This is true for 
both objects and devices:

pub trait ObjectInfo {
     type Class: ClassImpl;
     const TYPE_NAME: &'static CStr;
     const PARENT_TYPE_NAME: Option<&'static CStr>;
     const ABSTRACT: bool;
}

pub trait ObjectImpl: ObjectInfo {
     unsafe fn instance_init(instance: &mut MaybeUninit<self>) {}
}


and likewise:

> Device methods (realize/reset etc) are now safe idiomatic trait methods:
> 
>    ------------------------ >8 ------------------------
>   /// Implementation methods for device types.
>   pub trait DeviceImpl: ObjectImpl {
>       fn realize(&mut self) {}
>       fn reset(&mut self) {}
>   }
>    ------------------------ >8 ------------------------

pub trait DeviceInfo {
     const PROPERTIES: &[Property] = &[];
}

pub trait DeviceImpl: ObjectImpl + DeviceInfo {
     fn realize(&mut self) {}
     fn reset(&mut self) {}
     const VMSTATE: Option<VMStateDescription> = None;
}

(Generally, don't read too much in the code - the syntax might have 
issues but you get the idea).


Anyhow, going forward to the property attribute:

> +    #[property(name = c"migrate-clk", qdev_prop = qdev_prop_bool)]

There are two issues here:

First, the default is true, so 1) this has to be fixed in QEMU (will 
do), 2) it is important to support it in #[property()].

Second, this provides a false sense of safety, because I could specify 
qdev_prop_chr here.  Instead, the qdev_prop type should be derived by 
the field type.

Third, since we are at it there's no need to use c"" in the attribute. 
The c_str!() macro that I am adding for backwards compatibility to old 
versions of Rust might actually come in handy here.


The part where I have most comments, and some ideas of how to make your 
work a little more maintainable, is the implementation of class_init 
(and all that depends on it).

Let's start with these generated functions:

> +        pub unsafe extern "C" fn #realize_fn(
> +            dev: *mut ::qemu_api::bindings::DeviceState,
> +            errp: *mut *mut ::qemu_api::bindings::Error,
> +        ) {
> +            let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name)));
> +            unsafe {
> +                ::qemu_api::objects::DeviceImpl::realize(instance.as_mut());
> +            }
> +        }
> +
> +        #[no_mangle]
> +        pub unsafe extern "C" fn #reset_fn(
> +            dev: *mut ::qemu_api::bindings::DeviceState,
> +        ) {
> +            let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name)));
> +            unsafe {
> +                ::qemu_api::objects::DeviceImpl::reset(instance.as_mut());
> +            }
> +        }

This can be handled entirely in Rust code, outside the macro.  If you add:

unsafe extern "C" fn realize_fn_unsafe<T: DeviceImpl>(
     dev: *mut DeviceState,
     errp: *mut *mut Error,
) {
     let mut instance = NonNull::new(dev.cast::<T>()).
         expect("Expected dev to be a non-null pointer");
     unsafe {
         ::qemu_api::objects::DeviceImpl::realize(instance.as_mut());
     }
}

unsafe extern "C" fn reset_fn_unsafe<T: DeviceImpl>(
     dev: *mut ::qemu_api::bindings::DeviceState,
) {
     let mut instance = NonNull::new(dev.cast::<T>()).
         expect("Expected dev to be a non-null pointer");
     unsafe {
         ::qemu_api::objects::DeviceImpl::reset(instance.as_mut());
     }
}

then the functions can be used directly instead of #realize_fn and 
#reset_fn with a blanket implementation of DeviceImplUnsafe:


unsafe impl DeviceImplUnsafe for T: DeviceImpl {
     const REALIZE: ... = Some(realize_fn_unsafe::<T>);
     const RESET: ... = Some(realize_fn_unsafe::<T>);
}

Going on to the implementation of the safe functions:

> +impl DeviceImpl for PL011State {
> +    fn realize(&mut self) {

These are not quite traits.  First, you can implement only some of the 
functions.  Second, if you don't implement them they are not overwritten 
by the class_init method.

So this points to a different implementation as an attribute macro, 
which is able to rewrite everything in the body of the impl.  For example:

#[qom_class_init]
impl DeviceImpl for PL011State {
     fn realize(&mut self) { ... }
     fn reset(&mut self) { ... }

     const VMSTATE: ... = {}
     const CATEGORY: ... = {}
}

can be transformed into:

#[qom_class_init]
impl DeviceImpl for PL011State {
     // fns are wrapped and transformed into consts
     const REALIZE: Option<fn(&mut self)> = {
         fn realize(&mut self) { ... }
         Some(realize)
     };
     const RESET: Option<fn(&mut self)> = {
         fn reset(&mut self) { ... }
         Some(reset)
     };

     // while associated consts (and perhaps types?) remain as is
     const VMSTATE: ... = {}
     const CATEGORY: ... = {}
}

The above blanket implementation of DeviceImplUnsafe is easily adjusted 
to support non-overridden methods:

unsafe impl DeviceImplUnsafe for T: DeviceImpl {
     const REALIZE: ... = <T as DeviceImpl>::REALIZE::map(
         || realize_fn_unsafe::<T>);
     const RESET: ... = <T as DeviceImpl>::RESET::map(
         || realize_fn_unsafe::<T>);
}


You can also keep out of the macro the class_init method itself.  Here 
I'm adding it to DeviceImplUnsafe:

pub trait DeviceImplUnsafe {
     unsafe fn class_init(klass: *mut ObjectClass, data: *mut c_void) {
         let mut dc = NonNull::new(klass.cast::<DeviceClass>()).unwrap();
         unsafe {
             dc.as_mut().realize = Self::REALIZE;
             bindings::device_class_set_legacy_reset(
                                  dc.as_mut(), Self::RESET);
             device_class_set_props(dc.as_mut(),
                                    <Self as DeviceInfo>::PROPS);
             if let Some(vmsd) = <Self as DeviceInfo>::VMSTATE {
                 dc.as_mut().vmsd = vmsd;
             }
             if let Some(cat) = <Self as DeviceInfo>::CATEGORY {
                 dc.as_mut().category = cat;
             }

             // maybe in the future for unparent
             // <Self as ObjectImplUnsafe>::class_init(klass, data)
         }
     }
}

And with all this machinery in place the device_class_init! macro 
becomes simply

     device_class_init!(PL011State);

(because the arguments have moved to DeviceInfo or DeviceImpl).


Why is this important? Because the only code transformation is the 
generation of properties and vtables, and the bindings can be developed 
almost entirely in qemu_api instead of qemu_api_macros.  This has 
several advantages:

1) it's much easier: simpler error messages, no macro indirection, no 
super long global crate paths

2) it allows simplifying the pl011 code piecewise, even before 
introducing procedural macro code

3) it's easier to add comments


It also becomes much easier to separate the work in separate patches, or 
even separate series.  Replacing the arguments to device_class_init!() 
with DeviceImpl + DeviceImplUnsafe can be introduced first: posted, 
reviewed, merged.  All the remaining tasks are pretty much independent:

1) splitting out ObjectInfo and introducing #[object] to automate it 
(i.e. extending derive(Object))

2) splitting out DeviceInfo and introducing #[property] to automate it 
(i.e. derive(Device))

3) the #[qom_class_init] macro


A final word: I absolutely don't want you to think that your work is of 
no value.  It's totally okay to throw away the first version of 
something.  You showed that it _is_ possible to have idiomatic code with 
the help of procedural macros.  Even if the implementation can be 
improved, the _idea_ remains yours.

Paolo