Re: [PATCH 10/22] rust/hpet: Abstract HPETTimerRegisters struct

[Zhao Liu <zhao1.liu@intel.com>]

On Thu, Nov 13, 2025 at 12:24:07PM +0100, Paolo Bonzini wrote:
> Date: Thu, 13 Nov 2025 12:24:07 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 10/22] rust/hpet: Abstract HPETTimerRegisters struct
> 
> On 11/13/25 06:19, Zhao Liu wrote:
> > Place all timer N's registers in a HPETTimerRegisters struct.
> > 
> > This allows all Timer N registers to be grouped together with global
> > registers and managed using a single lock (BqlRefCell or Mutex) in
> > future. And this makes it easier to apply ToMigrationState macro.
> 
> This is pretty much the crucial patch in the series and it's the only
> one that needs more work, or some fixup at the end.
> 
> In particular, more fields of HPETTimer need to be moved to the
> HPETTimerRegisters. It's possible that it would be a problem to move
> the timer itself inside the mutex but, at least, the HPETTimer could be
> changed to just
> 
> pub struct HPETTimer {
>     timer: QemuTimer,
>     state: NonNull<HPETState>,
>     index: u8,
> }

Good idea!

> as in the patch included at the end (compile-tested only).  Then, the
> BqlRefCell<HPETTimer> can be changed to just HPETTimer because all the
> fields handle their interior-mutable fields.

Yes, this will reduce BQL context in lockless IO a lot. And I'll based
on your patch to extract HPETTimer from BqlRefCell.

> Preserving the old migration format can then be solved in two ways:
> 
> 1) with a handwritten ToMigrationStateShared implementation for
> HPETTimer (and marking the tn_regs array as #[migration_state(omit)])

Yes, compared with 2), I also this is the better choice, which looks
more common and could be an good example for other device.

> 2) by also adding num_timers_save and the timer's expiration to
> HPETRegisters and HPETTimerRegisters, respectively.
> 
> I think I prefer the former, but I haven't written the code so it's
> not my choice. :)
> 
> I'm okay with doing these changes on top of these patches as well.

Thank you!

The code attached looks good. Only a minor question below:
 
> @@ -181,6 +181,9 @@ fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
>  #[repr(C)]
>  #[derive(Debug, Default, ToMigrationState)]
>  pub struct HPETTimerRegisters {
> +    // Only needed for migration
> +    index: u8,

I didn't get the point why we need to migrate this "index" instead of
HPETTimer::index?

Anyway, if we continue to keep index in HPETTimerRegisters, we can make
it have usize type with a "#[migration_state(try_into(u8))]" property.

If we just HPETTimer::index for migration, I think it's possible to do
type convertion in handwritten ToMigrationStateShared.

Thanks,
Zhao