Re: [RFC PATCH v2 3/5] rust: add PL011 device model

[Manos Pitsidianakis]

On Wed, 12 Jun 2024 15:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
> I think this is extremely useful to show where we could go in the task
> of creating more idiomatic bindings.
>
> On Tue, Jun 11, 2024 at 12:34 PM Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> > +fn main() {
> > +    println!("cargo::rerun-if-env-changed=MESON_BUILD_DIR");
> > +    println!("cargo::rerun-if-env-changed=MESON_BUILD_ROOT");
> > +    println!("cargo::rerun-if-changed=src/generated.rs.inc");
>
> Why do you need this rerun-if-changed?

To show an error from build.rs in case the file is deleted and the build 
is not via meson

> > +pub const PL011_ARM_INFO: TypeInfo = TypeInfo {
> > +    name: TYPE_PL011.as_ptr(),
> > +    parent: TYPE_SYS_BUS_DEVICE.as_ptr(),
> > +    instance_size: core::mem::size_of::<PL011State>(),
> > +    instance_align: core::mem::align_of::<PL011State>(),
> > +    instance_init: Some(pl011_init),
> > +    instance_post_init: None,
> > +    instance_finalize: None,
> > +    abstract_: false,
> > +    class_size: 0,
> > +    class_init: Some(pl011_class_init),
> > +    class_base_init: None,
> > +    class_data: core::ptr::null_mut(),
> > +    interfaces: core::ptr::null_mut(),
> > +};
>
> QOM is certainly a major part of what we need to do for idiomatic
> bindings. This series includes both using QOM objects (chardev) and
> defining them.
>
> For using QOM objects, there is only one strategy that we need to
> implement for both Chardev and DeviceState/SysBusDevice which is nice.
> We can probably take inspiration from glib-rs, see for example
> - https://docs.rs/glib/latest/glib/object/trait.Cast.html
> - https://docs.rs/glib/latest/glib/object/trait.ObjectType.html
> - https://docs.rs/glib/latest/glib/object/struct.ObjectRef.html


There was consensus in the community call that we won't be writing Rust 
APIs for internal C QEMU interfaces; or at least, that's not the goal

It's not necessary to make bindings to write idiomatic Rust. If you 
notice, most callbacks QEMU calls cast the pointer into the Rust struct 
which then calls its idiomatic type methods. I like abstraction a lot, 
but it has diminishing returns. We'll see how future Rust devices look 
like and we can then decide if extra code for abstractions is a good 
trade-off.

> For definining new classes I think it's okay if Rust does not support
> writing superclasses yet, only leaves.
>
> I would make a QOM class written in Rust a struct that only contains
> the new fields. The struct must implement Default and possibly Drop
> (for finalize).

The object is allocated and freed from C, hence it is not Dropped. We're 
only ever accessing it from a reference retrieved from a QEMU provided 
raw pointer. If the struct gains heap object fields like Box or Vec or 
String, they'd have to be dropped manually on _unrealize.


>  struct PL011_Inner {
>    iomem: MemoryRegion,
>    readbuff: u32.
>    ...
>  }
>
> and then a macro defines a wrapper struct that includes just two
> fields, one for the superclass and one for the Rust struct.
> instance_init can initialize the latter with Default::default().
>
>  struct PL011 {
>    parent_obj: qemu::bindings::SysBusDevice,
>    private: PL011_Inner,
>  }

a nested struct is not necessary for using the Default trait. Consider a 
Default impl that sets parent_obj as a field memset to zero; on instance 
initialization we can do

 *self = Self {
   parent_obj: self.parent_obj,
   ..Self::default(),
 };

> "private" probably should be RefCell<PL011_Inner>, avoiding the unsafe
>
>    state.as_mut().read(addr, size)


RefCell etc are not FFI safe. Also, nested fields must be visible so 
that the offset_of! macro works, for QOM properties. Finally, 

    state.as_mut().read(addr, size)

Is safe since we receive a valid pointer from QEMU. This fact cannot be 
derived by the compiler, which is why it has an `unsafe` keyword. That 
does not mean that the use here is unsafe.

> There should also be macros to define the wrappers for MMIO MemoryRegions.


Do you mean the MemoryRegionOps?


> > +    pub fn realize(&mut self) {
> > +        unsafe {
> > +            qemu_chr_fe_set_handlers(
> > +                addr_of_mut!(self.char_backend),
> > +                Some(pl011_can_receive),
> > +                Some(pl011_receive),
> > +                Some(pl011_event),
> > +                None,
> > +                addr_of_mut!(*self).cast::<c_void>(),
> > +                core::ptr::null_mut(),
> > +                true,
> > +            );
> > +        }
>
> Probably each set of callbacks (MemoryRegion, Chardev) needs to be a
> struct that implements a trait.
>
> > +#[macro_export]
> > +macro_rules! define_property {
> > +    ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = 
> > $defval:expr) => {
> > +        $crate::generated::Property {
> > +            name: $name,
> > +            info: $prop,
> > +            offset: ::core::mem::offset_of!($state, $field) as _,
> > +            bitnr: 0,
> > +            bitmask: 0,
> > +            set_default: true,
> > +            defval: $crate::generated::Property__bindgen_ty_1 { u: 
> > $defval.into() },
> > +            arrayoffset: 0,
> > +            arrayinfo: ::core::ptr::null(),
> > +            arrayfieldsize: 0,
> > +            link_type: ::core::ptr::null(),
> > +        }
> > +    };
>
> Perhaps we can define macros similar to the C DEFINE_PROP_*,

Hopefully if I end up doing something like this, it'd be in a standalone 
crate for other devices to use

> and const
> functions can be used to enforce some kind of type safety.
>
> pub const fn check_type<T>(_x: &T, y: T) -> T { y }
>
> static VAR: i16 = 42i16;
> static TEST: i8 = check_type(&VAR, 43i8);
>
> pub fn main() { println!("{}", TEST); }
>
> error[E0308]: mismatched types
> --> ff.rs:4:30
>  |
> 4 | static TEST: i8 = check_type(&VAR, 43i8);
>  |                   ---------- ^^^^ expected `&i8`, found `&i16`
>  |                   |
>  |                   arguments to this function are incorrect
>  |
>  = note: expected reference `&i8`
>             found reference `&i16`


Yes this kind of type safety trick is easy to do (already done for a 
register macro in src/lib.rs).

I wanted to focus on the build system integration for the first RFC 
which is why there are some macros but not in every place it makes 
sense.

Thanks Paolo,
Manos