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

[Paolo Bonzini <pbonzini@redhat.com>]

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