Re: [PATCH 10/10] rust: bindings for MemoryRegionOps

[Paolo Bonzini]

On Mon, Jan 27, 2025 at 12:53 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > @@ -490,20 +490,24 @@ impl PL011State {
> >      /// location/instance. All its fields are expected to hold unitialized
> >      /// values with the sole exception of `parent_obj`.
> >      unsafe fn init(&mut self) {
> > +        static PL011_OPS: MemoryRegionOps<PL011State> = 
> > MemoryRegionOpsBuilder::<PL011State>::new()
> > +            .read(&PL011State::read)
> > +            .write(&PL011State::write)
> > +            .native_endian()
> > +            .impl_sizes(4, 4)
> > +            .build();
> > +
>
> Nice design. Everything was done smoothly in one go.

I hope something similar can be done with VMStateDescription too...

> > +pub struct MemoryRegionOps<T>(
> > +    bindings::MemoryRegionOps,
> > +    // Note: quite often you'll see PhantomData<fn(&T)> mentioned when 
> > discussing
> > +    // covariance and contravariance; you don't need any of those to 
> > understand
> > +    // this usage of PhantomData.  Quite simply, MemoryRegionOps<T> 
> > *logically*
> > +    // holds callbacks that take an argument of type &T, except the type 
> > is erased
> > +    // before the callback is stored in the bindings::MemoryRegionOps 
> > field.
> > +    // The argument of PhantomData is a function pointer in order to 
> > represent
> > +    // that relationship; while that will also provide desirable and safe 
> > variance
> > +    // for T, variance is not the point but just a consequence.
> > +    PhantomData<fn(&T)>,
> > +);
>
> Wow, it can be wrapped like this!

I like your enthusiasm but I'm not sure what you refer to. ;) Maybe
it's worth documenting this pattern, so please tell me more (after
your holidays).

> > +impl MemoryRegion {
> > +    // inline to ensure that it is not included in tests, which only
> > +    // link to hwcore and qom.  FIXME: inlining is actually the opposite
> > +    // of what we want, since this is the type-erased version of the
> > +    // init_io function below.  Look into splitting the qemu_api crate.
>
> Ah, I didn't understand the issue described in this comment. Why would
> inlining affect the linking of tests?

If you don't inline it, do_init_io will always be linked into the
tests because it is a non-generic function. The tests then fail to
link, because memory_region_init_io is undefined.

This is ugly because do_init_io exists *exactly* to extract the part
that is not generic. (See
https://users.rust-lang.org/t/soft-question-significantly-improve-rust-compile-time-via-minimizing-generics/103632/8
for an example of this; I think there's even a procedural macro crate
that does that for you, but I can't find it right now).

> > +    pub fn init_io<T: IsA<Object>>(
> > +        &mut self,
> > +        owner: *mut T,
> > +        ops: &'static MemoryRegionOps<T>,
> > +        name: &'static str,
>
> What about &'static CStr?
>
> Then pl011 could pass `c_str!("pl011")` or `Self::TYPE_NAME`.

I think it's better to use a Rust string; there's no reason why the
name of the memory region has to match Self::TYPE_NAME; unlike the
name of the device, the name of the memory region is not visible on
the command line for example.

Thanks,

Paolo

> Otherwise,
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>
>