On Mon, Jan 27, 2025 at 12:53 PM Zhao Liu <zhao1....@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....@intel.com>
>
>
