Re: [RFC PATCH 5/9] rust: vmstate: implement VMState for scalar types

[Paolo Bonzini]

On 1/8/25 07:45, Zhao Liu wrote:
> >   #[macro_export]
> >   macro_rules! vmstate_of {
> > -    ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])? 
> > $(,)?) => {
> > +    ($struct_name:ty, $field_name:ident $([0 .. $num:tt $(* $factor:expr)?])? 
> > $(,)?) => {
>
> Why change ident to tt?

Rebase mistake.  Initially I had $num:tt, however that becomes unclear 
if you have [0 .. 0] where the second 0 is a field name.

> > +impl_vmstate_scalar!(vmstate_info_bool, bool);
> > +impl_vmstate_scalar!(vmstate_info_int8, i8);
> > +impl_vmstate_scalar!(vmstate_info_int16, i16);
> > +impl_vmstate_scalar!(vmstate_info_int32, i32);
>
> missed VMS_VARRAY_INT32 :-)

I left that out intentionally, as Rust is probably going to use 
IndexMut<uNN> instead of i32.

> > +impl_vmstate_scalar!(vmstate_info_int64, i64);
> > +impl_vmstate_scalar!(vmstate_info_uint8, u8, VMS_VARRAY_UINT8);
> > +impl_vmstate_scalar!(vmstate_info_uint16, u16, VMS_VARRAY_UINT16);
> > +impl_vmstate_scalar!(vmstate_info_uint32, u32, VMS_VARRAY_UINT32);
>
> If we want to expand in the future (e.g., support vmstate_info_int32_equal
> and vmstate_info_int32_le), then introducing new macro variants will be
> straightforward. So, fair enough.
>
> > +impl_vmstate_scalar!(vmstate_info_uint64, u64);
>
> What about applying this to "usize" with vmstate_info_uint64?

There's 32-bit hosts too...  So one would have to add vmstate_info_ulong 
which is serialized as 64-bit.

We can add it later, but perhaps we could also create a derive(Index, 
IndexMut) macro that makes it possible to specify the type of the index. 
 While Rust uses usize instead of uNN for array indices, that does not 
have to be universal; using uNN is a lot better if it means you can get 
rid of casts from register values to array indices and back.  See for 
example commit 6b4f7b0705b ("rust: pl011: fix migration stream", 
2024-12-19).

That is indeed also an issue for HPET, but in that case it can be 
isolated to a couple lines,

            let timer_id: usize = ((addr - 0x100) / 0x20) as usize;

and it could even be wrapped further

    fn timer_and_addr(&self, addr: hwaddr) -> 
Option<&BqlRefCell<HPETTimer>, hwaddr> {
        let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
        if timer_id > self.num_timers.get() {
            // TODO: Add trace point - 
trace_hpet_timer_id_out_of_range(timer_id)
            None
        } else {
            Some((self.get_timer(timer_id), addr & 0x18))
        }
    }

    ...

    match self.timer_and_addr(addr) {
        None => 0 // Reserved,
        Some(timer, addr) => timer.borrow_mut().read(addr, size)
    }


So for HPET you didn't reach the threshold of having to create "pub 
struct HPETTimers([BqlRefCell<HPETTimer>; MAX_HPET_TIMERS])" and 
implement Index<>.

Paolo