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 <[email protected]>
> 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
