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

[Paolo Bonzini <pbonzini@redhat.com>]

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,
}

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.

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)])

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.

Paolo

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 75a6fd8a050..5ff02c01539 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -172,7 +172,7 @@ const fn deactivating_bit(old: u64, new: u64, shift: usize) -> bool {
  }
  
  fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
-    let mut t = timer_cell.borrow_mut();
+    let t = timer_cell.borrow();
      // SFAETY: state field is valid after timer initialization.
      let mut regs = unsafe { t.state.as_ref() }.regs.lock().unwrap();
      t.callback(&mut regs)
@@ -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,
+
      // Memory-mapped, software visible timer registers
      /// Timer N Configuration and Capability Register
      config: u64,
@@ -188,9 +191,35 @@ pub struct HPETTimerRegisters {
      cmp: u64,
      /// Timer N FSB Interrupt Route Register
      fsb: u64,
+
+    // Hidden register state
+    /// comparator (extended to counter width)
+    cmp64: u64,
+    /// Last value written to comparator
+    period: u64,
+    /// timer pop will indicate wrap for one-shot 32-bit
+    /// mode. Next pop will be actual timer expiration.
+    wrap_flag: u8,
+    /// last value armed, to avoid timer storms
+    last: u64,
  }
  
  impl HPETTimerRegisters {
+    /// calculate next value of the general counter that matches the
+    /// target (either entirely, or the low 32-bit only depending on
+    /// the timer mode).
+    fn update_cmp64(&mut self, cur_tick: u64) {
+        self.cmp64 = if self.is_32bit_mod() {
+            let mut result: u64 = cur_tick.deposit(0, 32, self.cmp);
+            if result < cur_tick {
+                result += 0x100000000;
+            }
+            result
+        } else {
+            self.cmp
+        }
+    }
+
      const fn is_fsb_route_enabled(&self) -> bool {
          self.config & (1 << HPET_TN_CFG_FSB_ENABLE_SHIFT) != 0
      }
@@ -235,17 +264,6 @@ pub struct HPETTimer {
      qemu_timer: Timer,
      /// timer block abstraction containing this timer
      state: NonNull<HPETState>,
-
-    // Hidden register state
-    /// comparator (extended to counter width)
-    cmp64: u64,
-    /// Last value written to comparator
-    period: u64,
-    /// timer pop will indicate wrap for one-shot 32-bit
-    /// mode. Next pop will be actual timer expiration.
-    wrap_flag: u8,
-    /// last value armed, to avoid timer storms
-    last: u64,
  }
  
  // SAFETY: Sync is not automatically derived due to the `state` field,
@@ -260,10 +278,6 @@ fn new(index: u8, state: *const HPETState) -> HPETTimer {
              // is initialized below.
              qemu_timer: unsafe { Timer::new() },
              state: NonNull::new(state.cast_mut()).unwrap(),
-            cmp64: 0,
-            period: 0,
-            wrap_flag: 0,
-            last: 0,
          }
      }
  
@@ -291,22 +305,6 @@ fn is_int_active(&self, regs: &HPETRegisters) -> bool {
          regs.is_timer_int_active(self.index.into())
      }
  
-    /// calculate next value of the general counter that matches the
-    /// target (either entirely, or the low 32-bit only depending on
-    /// the timer mode).
-    fn calculate_cmp64(&self, regs: &MutexGuard<HPETRegisters>, cur_tick: u64, target: u64) -> u64 {
-        let tn_regs = &regs.tn_regs[self.index as usize];
-        if tn_regs.is_32bit_mod() {
-            let mut result: u64 = cur_tick.deposit(0, 32, target);
-            if result < cur_tick {
-                result += 0x100000000;
-            }
-            result
-        } else {
-            target
-        }
-    }
-
      fn get_int_route(&self, regs: &MutexGuard<HPETRegisters>) -> usize {
          if self.index <= 1 && regs.is_legacy_mode() {
              // If LegacyReplacement Route bit is set, HPET specification requires
@@ -371,35 +369,34 @@ fn update_irq(&self, regs: &mut MutexGuard<HPETRegisters>, set: bool) {
          self.set_irq(regs, set);
      }
  
-    fn arm_timer(&mut self, regs: &MutexGuard<HPETRegisters>, tick: u64) {
-        let tn_regs = &regs.tn_regs[self.index as usize];
+    fn arm_timer(&self, tn_regs: &mut HPETTimerRegisters, tick: u64) {
          let mut ns = self.get_state().get_ns(tick);
  
          // Clamp period to reasonable min value (1 us)
-        if tn_regs.is_periodic() && ns - self.last < 1000 {
-            ns = self.last + 1000;
+        if tn_regs.is_periodic() && ns - tn_regs.last < 1000 {
+            ns = tn_regs.last + 1000;
          }
  
-        self.last = ns;
-        self.qemu_timer.modify(self.last);
+        tn_regs.last = ns;
+        self.qemu_timer.modify(tn_regs.last);
      }
  
-    fn set_timer(&mut self, regs: &MutexGuard<HPETRegisters>) {
-        let tn_regs = &regs.tn_regs[self.index as usize];
+    fn set_timer(&self, regs: &mut MutexGuard<HPETRegisters>) {
+        let tn_regs = &mut regs.tn_regs[self.index as usize];
          let cur_tick: u64 = self.get_state().get_ticks();
  
-        self.wrap_flag = 0;
-        self.cmp64 = self.calculate_cmp64(regs, cur_tick, tn_regs.cmp);
+        tn_regs.wrap_flag = 0;
+        tn_regs.update_cmp64(cur_tick);
          if tn_regs.is_32bit_mod() {
              // HPET spec says in one-shot 32-bit mode, generate an interrupt when
              // counter wraps in addition to an interrupt with comparator match.
-            if !tn_regs.is_periodic() && self.cmp64 > hpet_next_wrap(cur_tick) {
-                self.wrap_flag = 1;
-                self.arm_timer(regs, hpet_next_wrap(cur_tick));
+            if !tn_regs.is_periodic() && tn_regs.cmp64 > hpet_next_wrap(cur_tick) {
+                tn_regs.wrap_flag = 1;
+                self.arm_timer(tn_regs, hpet_next_wrap(cur_tick));
                  return;
              }
          }
-        self.arm_timer(regs, self.cmp64);
+        self.arm_timer(tn_regs, tn_regs.cmp64);
      }
  
      fn del_timer(&self, regs: &mut MutexGuard<HPETRegisters>) {
@@ -440,7 +437,7 @@ fn prepare_tn_cfg_reg_new(
  
      /// Configuration and Capability Register
      fn set_tn_cfg_reg(
-        &mut self,
+        &self,
          regs: &mut MutexGuard<HPETRegisters>,
          shift: u32,
          len: u32,
@@ -462,7 +459,7 @@ fn set_tn_cfg_reg(
          let tn_regs = &mut regs.tn_regs[self.index as usize];
          if tn_regs.is_32bit_mod() {
              tn_regs.cmp = u64::from(tn_regs.cmp as u32); // truncate!
-            self.period = u64::from(self.period as u32); // truncate!
+            tn_regs.period = u64::from(tn_regs.period as u32); // truncate!
          }
  
          if regs.is_hpet_enabled() {
@@ -472,7 +469,7 @@ fn set_tn_cfg_reg(
  
      /// Comparator Value Register
      fn set_tn_cmp_reg(
-        &mut self,
+        &self,
          regs: &mut MutexGuard<HPETRegisters>,
          shift: u32,
          len: u32,
@@ -499,7 +496,7 @@ fn set_tn_cmp_reg(
          }
  
          if tn_regs.is_periodic() {
-            self.period = self.period.deposit(shift, length, value);
+            tn_regs.period = tn_regs.period.deposit(shift, length, value);
          }
  
          tn_regs.clear_valset();
@@ -520,10 +517,11 @@ fn set_tn_fsb_route_reg(
          tn_regs.fsb = tn_regs.fsb.deposit(shift, len, val);
      }
  
-    fn reset(&mut self, regs: &mut MutexGuard<HPETRegisters>) {
+    fn reset(&self, regs: &mut MutexGuard<HPETRegisters>) {
          self.del_timer(regs);
  
          let tn_regs = &mut regs.tn_regs[self.index as usize];
+        tn_regs.index = self.index;
          tn_regs.cmp = u64::MAX; // Comparator Match Registers reset to all 1's.
          tn_regs.config = (1 << HPET_TN_CFG_PERIODIC_CAP_SHIFT) | (1 << HPET_TN_CFG_SIZE_CAP_SHIFT);
          if self.get_state().has_msi_flag() {
@@ -532,29 +530,28 @@ fn reset(&mut self, regs: &mut MutexGuard<HPETRegisters>) {
          // advertise availability of ioapic int
          tn_regs.config |=
              (u64::from(self.get_state().int_route_cap)) << HPET_TN_CFG_INT_ROUTE_CAP_SHIFT;
-        self.period = 0;
-        self.wrap_flag = 0;
+        tn_regs.period = 0;
+        tn_regs.wrap_flag = 0;
      }
  
      /// timer expiration callback
-    fn callback(&mut self, regs: &mut MutexGuard<HPETRegisters>) {
+    fn callback(&self, regs: &mut MutexGuard<HPETRegisters>) {
          let tn_regs = &mut regs.tn_regs[self.index as usize];
-        let period: u64 = self.period;
          let cur_tick: u64 = self.get_state().get_ticks();
  
-        if tn_regs.is_periodic() && period != 0 {
-            while hpet_time_after(cur_tick, self.cmp64) {
-                self.cmp64 += period;
+        if tn_regs.is_periodic() && tn_regs.period != 0 {
+            while hpet_time_after(cur_tick, tn_regs.cmp64) {
+                tn_regs.cmp64 += tn_regs.period;
              }
              if tn_regs.is_32bit_mod() {
-                tn_regs.cmp = u64::from(self.cmp64 as u32); // truncate!
+                tn_regs.cmp = u64::from(tn_regs.cmp64 as u32); // truncate!
              } else {
-                tn_regs.cmp = self.cmp64;
+                tn_regs.cmp = tn_regs.cmp64;
              }
-            self.arm_timer(regs, self.cmp64);
-        } else if self.wrap_flag != 0 {
-            self.wrap_flag = 0;
-            self.arm_timer(regs, self.cmp64);
+            self.arm_timer(tn_regs, tn_regs.cmp64);
+        } else if tn_regs.wrap_flag != 0 {
+            tn_regs.wrap_flag = 0;
+            self.arm_timer(tn_regs, tn_regs.cmp64);
          }
          self.update_irq(regs, true);
      }
@@ -571,7 +568,7 @@ fn read(&self, target: TimerRegister, regs: &MutexGuard<HPETRegisters>) -> u64 {
      }
  
      fn write(
-        &mut self,
+        &self,
          target: TimerRegister,
          regs: &mut MutexGuard<HPETRegisters>,
          value: u64,
@@ -731,7 +728,7 @@ fn set_cfg_reg(&self, regs: &mut MutexGuard<HPETRegisters>, shift: u32, len: u32
              for timer in self.timers.iter().take(self.num_timers) {
                  // Protect timer in lockless IO case which would not lock BQL.
                  bql::with_guard(|| {
-                    let mut t = timer.borrow_mut();
+                    let t = timer.borrow_mut();
                      let id = t.index as usize;
                      let tn_regs = &regs.tn_regs[id];
  
@@ -992,15 +989,14 @@ fn pre_save(&self) -> Result<(), migration::Infallible> {
      }
  
      fn post_load(&self, _version_id: u8) -> Result<(), migration::Infallible> {
-        let regs = self.regs.lock().unwrap();
+        let mut regs = self.regs.lock().unwrap();
+        let cnt = regs.counter;
  
-        for timer in self.timers.iter().take(self.num_timers) {
-            let mut t = timer.borrow_mut();
-            let cnt = regs.counter;
-            let cmp = regs.tn_regs[t.index as usize].cmp;
+        for index in 0..self.num_timers {
+            let tn_regs = &mut regs.tn_regs[index];
  
-            t.cmp64 = t.calculate_cmp64(&regs, cnt, cmp);
-            t.last = CLOCK_VIRTUAL.get_ns() - NANOSECONDS_PER_SECOND;
+            tn_regs.update_cmp64(cnt);
+            tn_regs.last = CLOCK_VIRTUAL.get_ns() - NANOSECONDS_PER_SECOND;
          }
  
          // Recalculate the offset between the main counter and guest time
@@ -1072,9 +1068,12 @@ impl ObjectImpl for HPETState {
          .version_id(1)
          .minimum_version_id(1)
          .fields(vmstate_fields! {
+            vmstate_of!(HPETTimerRegistersMigration, index),
              vmstate_of!(HPETTimerRegistersMigration, config),
              vmstate_of!(HPETTimerRegistersMigration, cmp),
              vmstate_of!(HPETTimerRegistersMigration, fsb),
+            vmstate_of!(HPETTimerRegistersMigration, period),
+            vmstate_of!(HPETTimerRegistersMigration, wrap_flag),
          })
          .build()
  );
@@ -1085,9 +1084,6 @@ impl ObjectImpl for HPETState {
          .version_id(2)
          .minimum_version_id(2)
          .fields(vmstate_fields! {
-            vmstate_of!(HPETTimer, index),
-            vmstate_of!(HPETTimer, period),
-            vmstate_of!(HPETTimer, wrap_flag),
              vmstate_of!(HPETTimer, qemu_timer),
          })
          .build();