[PATCH 0/5] rust/hpet: complete moving state out of HPETTimer

[PATCH 0/5] rust/hpet: complete moving state out of HPETTimer

[Paolo Bonzini]

This state continues the cleanups of the HPET state, moving fields out of
BqlCells and into HPETRegisters and HPETTimerRegisters.  It also restores
the old migration format and shows an interesting trick: HPETTimer is now
a very simple object that handles the "unsafe" backreference from the
timer to the HPETState, but it also implements ToMigrationStateShared
and is stored in the HPETState as Migratable<[HPETTimer; N]>.  I find
it pretty cool that the composition works naturally.

The less beautiful part is that I had to modify Timer::init_full for
this to compile.  It's probably time to work on the final design for
initialization, because this is becoming very ad hoc and the differences
between timer, MemoryRegion and Clock initialization have no real
justification.

I'm leaving out the conversion to Mutex because, as Zhao noticed, it
has a deadlock - the timer callback tries to grab the HPET mutex inside
the BQL, where as the vCPU tries to grab the BQL inside the HPET mutex.
This is not present in the C code only because... it doesn't take the
lock at all in places where it should.  In particular hpet_timer() reads
and writes t->cmp and t->cmp64 outside the lock, while hpet_ram_write()
does so within the lock via hpet_set_timer().

Patch 4 ("rust: migration: implement ToMigrationState for Timer") is
still incomplete, and provided here as a starting point.

Please review!

Paolo

Paolo Bonzini (4):
  rust/hpet: move hidden registers to HPETTimerRegisters
  rust/hpet: move hpet_offset to HPETRegisters
  rust/hpet: remove BqlRefCell around HPETTimer
  rust: migration: implement ToMigrationState for Timer

Zhao Liu (1):
  rust/hpet: Apply Migratable<> wrapper and ToMigrationState

 rust/hw/timer/hpet/src/device.rs | 377 ++++++++++++++++++-------------
 rust/migration/src/migratable.rs |  31 +++
 rust/util/src/timer.rs           |  22 +-
 3 files changed, 264 insertions(+), 166 deletions(-)

-- 
2.51.1

[PATCH 1/5] rust/hpet: move hidden registers to HPETTimerRegisters

[Paolo Bonzini]

Do not separate visible and hidden state; both of them are used in the
same circumstances and it's easiest to place both of them under the
same BqlRefCell.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/device.rs | 157 ++++++++++++++-----------------
 1 file changed, 71 insertions(+), 86 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index a1deef5a467..79d818b43da 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -171,9 +171,9 @@ 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 regs = &mut unsafe { t.state.as_mut() }.regs.borrow_mut();
+    let regs = &mut unsafe { t.state.as_ref() }.regs.borrow_mut();
     t.callback(regs)
 }
 
@@ -187,9 +187,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
     }
@@ -234,17 +260,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,
@@ -259,10 +274,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,
         }
     }
 
@@ -290,28 +301,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: &HPETRegisters, cur_tick: u64, target: u64) -> u64 {
-        // &HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
-        // but there's no lock guard to guarantee this. So we have to check BQL
-        // context explicitly. This check should be removed when we switch to
-        // Mutex<HPETRegisters>.
-        assert!(bql::is_locked());
-
-        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: &HPETRegisters) -> usize {
         // &HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
@@ -394,47 +383,46 @@ fn update_irq(&self, regs: &mut HPETRegisters, set: bool) {
         self.set_irq(regs, set);
     }
 
-    fn arm_timer(&mut self, regs: &HPETRegisters, tick: u64) {
+    fn arm_timer(&self, tn_regs: &mut HPETTimerRegisters, tick: u64) {
         // &HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
         // context explicitly. This check should be removed when we switch to
         // Mutex<HPETRegisters>.
         assert!(bql::is_locked());
 
-        let tn_regs = &regs.tn_regs[self.index as usize];
         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: &HPETRegisters) {
+    fn set_timer(&self, regs: &mut HPETRegisters) {
         // &HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
         // context explicitly. This check should be removed when we switch to
         // Mutex<HPETRegisters>.
         assert!(bql::is_locked());
 
-        let tn_regs = &regs.tn_regs[self.index as usize];
+        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 HPETRegisters) {
@@ -485,7 +473,7 @@ fn prepare_tn_cfg_reg_new(
     }
 
     /// Configuration and Capability Register
-    fn set_tn_cfg_reg(&mut self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64) {
+    fn set_tn_cfg_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64) {
         // &mut HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
         // context explicitly. This check should be removed when we switch to
@@ -508,7 +496,7 @@ fn set_tn_cfg_reg(&mut self, regs: &mut HPETRegisters, shift: u32, len: u32, val
         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() {
@@ -517,7 +505,7 @@ fn set_tn_cfg_reg(&mut self, regs: &mut HPETRegisters, shift: u32, len: u32, val
     }
 
     /// Comparator Value Register
-    fn set_tn_cmp_reg(&mut self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64) {
+    fn set_tn_cmp_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64) {
         // &mut HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
         // context explicitly. This check should be removed when we switch to
@@ -545,7 +533,7 @@ fn set_tn_cmp_reg(&mut self, regs: &mut HPETRegisters, shift: u32, len: u32, val
         }
 
         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();
@@ -566,7 +554,7 @@ fn set_tn_fsb_route_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, v
         tn_regs.fsb = tn_regs.fsb.deposit(shift, len, val);
     }
 
-    fn reset(&mut self, regs: &mut HPETRegisters) {
+    fn reset(&self, regs: &mut HPETRegisters) {
         // &mut HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
         // context explicitly. This check should be removed when we switch to
@@ -584,12 +572,12 @@ fn reset(&mut self, regs: &mut 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 HPETRegisters) {
+    fn callback(&self, regs: &mut HPETRegisters) {
         // &mut HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
         // context explicitly. This check should be removed when we switch to
@@ -597,22 +585,21 @@ fn callback(&mut self, regs: &mut HPETRegisters) {
         assert!(bql::is_locked());
 
         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);
     }
@@ -635,7 +622,7 @@ fn read(&self, target: TimerRegister, regs: &HPETRegisters) -> u64 {
     }
 
     fn write(
-        &mut self,
+        &self,
         target: TimerRegister,
         regs: &mut HPETRegisters,
         value: u64,
@@ -796,7 +783,7 @@ fn set_cfg_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64)
                 .set(ticks_to_ns(regs.counter) - CLOCK_VIRTUAL.get_ns());
 
             for timer in self.timers.iter().take(self.num_timers) {
-                let mut t = timer.borrow_mut();
+                let t = timer.borrow();
                 let id = t.index as usize;
                 let tn_regs = &regs.tn_regs[id];
 
@@ -937,7 +924,7 @@ fn reset_hold(&self, _type: ResetType) {
             let mut regs = self.regs.borrow_mut();
 
             for timer in self.timers.iter().take(self.num_timers) {
-                timer.borrow_mut().reset(&mut regs);
+                timer.borrow().reset(&mut regs);
             }
 
             regs.counter = 0;
@@ -989,7 +976,7 @@ fn read(&self, addr: hwaddr, size: u32) -> u64 {
         use DecodedRegister::*;
         use GlobalRegister::*;
         (match target {
-            Timer(timer, tn_target) => timer.borrow_mut().read(tn_target, regs),
+            Timer(timer, tn_target) => timer.borrow().read(tn_target, regs),
             Global(CAP) => regs.capability, /* including HPET_PERIOD 0x004 */
             Global(CFG) => regs.config,
             Global(INT_STATUS) => regs.int_status,
@@ -1020,7 +1007,7 @@ fn write(&self, addr: hwaddr, value: u64, size: u32) {
         use DecodedRegister::*;
         use GlobalRegister::*;
         match target {
-            Timer(timer, tn_target) => timer.borrow_mut().write(tn_target, regs, value, shift, len),
+            Timer(timer, tn_target) => timer.borrow().write(tn_target, regs, value, shift, len),
             Global(CAP) => {} // General Capabilities and ID Register: Read Only
             Global(CFG) => self.set_cfg_reg(regs, shift, len, value),
             Global(INT_STATUS) => self.set_int_status_reg(regs, shift, len, value),
@@ -1045,15 +1032,14 @@ fn pre_save(&self) -> Result<(), migration::Infallible> {
     }
 
     fn post_load(&self, _version_id: u8) -> Result<(), migration::Infallible> {
-        let regs = self.regs.borrow();
+        let mut regs = self.regs.borrow_mut();
+        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
@@ -1126,6 +1112,8 @@ impl ObjectImpl for HPETState {
             vmstate_of!(HPETTimerRegisters, config),
             vmstate_of!(HPETTimerRegisters, cmp),
             vmstate_of!(HPETTimerRegisters, fsb),
+            vmstate_of!(HPETTimerRegisters, period),
+            vmstate_of!(HPETTimerRegisters, wrap_flag),
         })
         .build()
 );
@@ -1136,9 +1124,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();
-- 
2.51.1

[PATCH 2/5] rust/hpet: move hpet_offset to HPETRegisters

[Paolo Bonzini]

Likewise, do not separate hpet_offset from the other registers.
However, because it is migrated in a subsection it is necessary
to copy it out of HPETRegisters and into a BqlCell<>.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/device.rs | 63 ++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 79d818b43da..19676af74bc 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -383,14 +383,15 @@ fn update_irq(&self, regs: &mut HPETRegisters, set: bool) {
         self.set_irq(regs, set);
     }
 
-    fn arm_timer(&self, tn_regs: &mut HPETTimerRegisters, tick: u64) {
+    fn arm_timer(&self, regs: &mut HPETRegisters, tick: u64) {
         // &HPETRegisters should be gotten from BqlRefCell<HPETRegisters>,
         // but there's no lock guard to guarantee this. So we have to check BQL
         // context explicitly. This check should be removed when we switch to
         // Mutex<HPETRegisters>.
         assert!(bql::is_locked());
 
-        let mut ns = self.get_state().get_ns(tick);
+        let mut ns = regs.get_ns(tick);
+        let tn_regs = &mut regs.tn_regs[self.index as usize];
 
         // Clamp period to reasonable min value (1 us)
         if tn_regs.is_periodic() && ns - tn_regs.last < 1000 {
@@ -408,21 +409,22 @@ fn set_timer(&self, regs: &mut HPETRegisters) {
         // Mutex<HPETRegisters>.
         assert!(bql::is_locked());
 
+        let cur_tick: u64 = regs.get_ticks();
         let tn_regs = &mut regs.tn_regs[self.index as usize];
-        let cur_tick: u64 = self.get_state().get_ticks();
 
         tn_regs.wrap_flag = 0;
         tn_regs.update_cmp64(cur_tick);
+
+        let mut next_tick: u64 = tn_regs.cmp64;
         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() && tn_regs.cmp64 > hpet_next_wrap(cur_tick) {
                 tn_regs.wrap_flag = 1;
-                self.arm_timer(tn_regs, hpet_next_wrap(cur_tick));
-                return;
+                next_tick = hpet_next_wrap(cur_tick);
             }
         }
-        self.arm_timer(tn_regs, tn_regs.cmp64);
+        self.arm_timer(regs, next_tick);
     }
 
     fn del_timer(&self, regs: &mut HPETRegisters) {
@@ -584,8 +586,8 @@ fn callback(&self, regs: &mut HPETRegisters) {
         // Mutex<HPETRegisters>.
         assert!(bql::is_locked());
 
+        let cur_tick: u64 = regs.get_ticks();
         let tn_regs = &mut regs.tn_regs[self.index as usize];
-        let cur_tick: u64 = self.get_state().get_ticks();
 
         if tn_regs.is_periodic() && tn_regs.period != 0 {
             while hpet_time_after(cur_tick, tn_regs.cmp64) {
@@ -596,11 +598,11 @@ fn callback(&self, regs: &mut HPETRegisters) {
             } else {
                 tn_regs.cmp = tn_regs.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);
         }
+        let next_tick = tn_regs.cmp64;
+        self.arm_timer(regs, next_tick);
         self.update_irq(regs, true);
     }
 
@@ -663,9 +665,22 @@ pub struct HPETRegisters {
 
     /// HPET Timer N Registers
     tn_regs: [HPETTimerRegisters; HPET_MAX_TIMERS],
+
+    /// Offset of main counter relative to qemu clock.
+    pub hpet_offset: u64,
 }
 
 impl HPETRegisters {
+    fn get_ticks(&self) -> u64 {
+        // Protect hpet_offset in lockless IO case which would not lock BQL.
+        ns_to_ticks(CLOCK_VIRTUAL.get_ns() + self.hpet_offset)
+    }
+
+    fn get_ns(&self, tick: u64) -> u64 {
+        // Protect hpet_offset in lockless IO case which would not lock BQL.
+        ticks_to_ns(tick) - self.hpet_offset
+    }
+
     fn is_legacy_mode(&self) -> bool {
         self.config & (1 << HPET_CFG_LEG_RT_SHIFT) != 0
     }
@@ -693,8 +708,7 @@ pub struct HPETState {
     #[property(rename = "msi", bit = HPET_FLAG_MSI_SUPPORT_SHIFT, default = false)]
     flags: u32,
 
-    /// Offset of main counter relative to qemu clock.
-    hpet_offset: BqlCell<u64>,
+    hpet_offset_migration: BqlCell<u64>,
     #[property(rename = "hpet-offset-saved", default = true)]
     hpet_offset_saved: bool,
 
@@ -726,14 +740,6 @@ const fn has_msi_flag(&self) -> bool {
         self.flags & (1 << HPET_FLAG_MSI_SUPPORT_SHIFT) != 0
     }
 
-    fn get_ticks(&self) -> u64 {
-        ns_to_ticks(CLOCK_VIRTUAL.get_ns() + self.hpet_offset.get())
-    }
-
-    fn get_ns(&self, tick: u64) -> u64 {
-        ticks_to_ns(tick) - self.hpet_offset.get()
-    }
-
     fn handle_legacy_irq(&self, irq: u32, level: u32) {
         let regs = self.regs.borrow();
         if irq == HPET_LEGACY_PIT_INT {
@@ -779,8 +785,7 @@ fn set_cfg_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64)
 
         if activating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
             // Enable main counter and interrupt generation.
-            self.hpet_offset
-                .set(ticks_to_ns(regs.counter) - CLOCK_VIRTUAL.get_ns());
+            regs.hpet_offset = ticks_to_ns(regs.counter) - CLOCK_VIRTUAL.get_ns();
 
             for timer in self.timers.iter().take(self.num_timers) {
                 let t = timer.borrow();
@@ -794,7 +799,7 @@ fn set_cfg_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64)
             }
         } else if deactivating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
             // Halt main counter and disable interrupt generation.
-            regs.counter = self.get_ticks();
+            regs.counter = regs.get_ticks();
 
             for timer in self.timers.iter().take(self.num_timers) {
                 timer.borrow().del_timer(regs);
@@ -873,7 +878,7 @@ unsafe fn init(mut this: ParentInit<Self>) {
         // initialized memory to all zeros - simple types (bool/u32/usize) can
         // rely on this without explicit initialization.
         uninit_field_mut!(*this, regs).write(Default::default());
-        uninit_field_mut!(*this, hpet_offset).write(Default::default());
+        uninit_field_mut!(*this, hpet_offset_migration).write(Default::default());
         // Set null_mut for now and post_init() will fill it.
         uninit_field_mut!(*this, irqs).write(Default::default());
         uninit_field_mut!(*this, rtc_irq_level).write(Default::default());
@@ -929,6 +934,7 @@ fn reset_hold(&self, _type: ResetType) {
 
             regs.counter = 0;
             regs.config = 0;
+            regs.hpet_offset = 0;
             HPETFwConfig::update_hpet_cfg(
                 self.hpet_id.get(),
                 regs.capability as u32,
@@ -937,7 +943,6 @@ fn reset_hold(&self, _type: ResetType) {
         }
 
         self.pit_enabled.set(true);
-        self.hpet_offset.set(0);
 
         // to document that the RTC lowers its output on reset as well
         self.rtc_irq_level.set(0);
@@ -982,7 +987,7 @@ fn read(&self, addr: hwaddr, size: u32) -> u64 {
             Global(INT_STATUS) => regs.int_status,
             Global(COUNTER) => {
                 let cur_tick = if regs.is_hpet_enabled() {
-                    self.get_ticks()
+                    regs.get_ticks()
                 } else {
                     regs.counter
                 };
@@ -1018,8 +1023,9 @@ fn write(&self, addr: hwaddr, value: u64, size: u32) {
 
     fn pre_save(&self) -> Result<(), migration::Infallible> {
         let mut regs = self.regs.borrow_mut();
+        self.hpet_offset_migration.set(regs.hpet_offset);
         if regs.is_hpet_enabled() {
-            regs.counter = self.get_ticks();
+            regs.counter = regs.get_ticks();
         }
 
         /*
@@ -1044,9 +1050,10 @@ fn post_load(&self, _version_id: u8) -> Result<(), migration::Infallible> {
 
         // Recalculate the offset between the main counter and guest time
         if !self.hpet_offset_saved {
-            self.hpet_offset
+            self.hpet_offset_migration
                 .set(ticks_to_ns(regs.counter) - CLOCK_VIRTUAL.get_ns());
         }
+        regs.hpet_offset = self.hpet_offset_migration.get();
 
         Ok(())
     }
@@ -1098,7 +1105,7 @@ impl ObjectImpl for HPETState {
         .minimum_version_id(1)
         .needed(&HPETState::is_offset_needed)
         .fields(vmstate_fields! {
-            vmstate_of!(HPETState, hpet_offset),
+            vmstate_of!(HPETState, hpet_offset_migration),
         })
         .build();
 
-- 
2.51.1

[PATCH 3/5] rust/hpet: remove BqlRefCell around HPETTimer

[Paolo Bonzini]

HPETTimer now has all of its state stored in HPETRegisters, so it does not
need its own BqlRefCell anymore.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/device.rs | 53 ++++++++++++++++----------------
 rust/util/src/timer.rs           | 12 +++++---
 2 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 19676af74bc..5bcf151a680 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -126,7 +126,7 @@ enum DecodedRegister<'a> {
     Global(GlobalRegister),
 
     /// Register in the timer block `0x100`...`0x3ff`
-    Timer(&'a BqlRefCell<HPETTimer>, TimerRegister),
+    Timer(&'a HPETTimer, TimerRegister),
 
     /// Invalid address
     #[allow(dead_code)]
@@ -170,8 +170,7 @@ const fn deactivating_bit(old: u64, new: u64, shift: usize) -> bool {
     (old & mask != 0) && (new & mask == 0)
 }
 
-fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
-    let t = timer_cell.borrow();
+fn timer_handler(t: &HPETTimer) {
     // SFAETY: state field is valid after timer initialization.
     let regs = &mut unsafe { t.state.as_ref() }.regs.borrow_mut();
     t.callback(regs)
@@ -277,12 +276,16 @@ fn new(index: u8, state: *const HPETState) -> HPETTimer {
         }
     }
 
-    fn init_timer_with_cell(cell: &BqlRefCell<Self>) {
-        let mut timer = cell.borrow_mut();
-        // SAFETY: HPETTimer is only used as part of HPETState, which is
-        // always pinned.
-        let qemu_timer = unsafe { Pin::new_unchecked(&mut timer.qemu_timer) };
-        qemu_timer.init_full(None, CLOCK_VIRTUAL, Timer::NS, 0, timer_handler, cell);
+    fn init_timer(timer: Pin<&mut Self>) {
+        Timer::init_full(
+            timer,
+            None,
+            CLOCK_VIRTUAL,
+            Timer::NS,
+            0,
+            timer_handler,
+            |t| &mut t.qemu_timer,
+        );
     }
 
     fn get_state(&self) -> &HPETState {
@@ -726,7 +729,7 @@ pub struct HPETState {
 
     /// HPET timer array managed by this timer block.
     #[doc(alias = "timer")]
-    timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS],
+    timers: [HPETTimer; HPET_MAX_TIMERS],
     #[property(rename = "timers", default = HPET_MIN_TIMERS)]
     num_timers: usize,
     num_timers_save: BqlCell<u8>,
@@ -761,11 +764,10 @@ fn init_timers(this: &mut MaybeUninit<Self>) {
 
             // Initialize in two steps, to avoid calling Timer::init_full on a
             // temporary that can be moved.
-            let timer = timer.write(BqlRefCell::new(HPETTimer::new(
-                index.try_into().unwrap(),
-                state,
-            )));
-            HPETTimer::init_timer_with_cell(timer);
+            let timer = timer.write(HPETTimer::new(index.try_into().unwrap(), state));
+            // SAFETY: HPETState is pinned
+            let timer = unsafe { Pin::new_unchecked(timer) };
+            HPETTimer::init_timer(timer);
         }
     }
 
@@ -787,8 +789,7 @@ fn set_cfg_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64)
             // Enable main counter and interrupt generation.
             regs.hpet_offset = ticks_to_ns(regs.counter) - CLOCK_VIRTUAL.get_ns();
 
-            for timer in self.timers.iter().take(self.num_timers) {
-                let t = timer.borrow();
+            for t in self.timers.iter().take(self.num_timers) {
                 let id = t.index as usize;
                 let tn_regs = &regs.tn_regs[id];
 
@@ -801,8 +802,8 @@ fn set_cfg_reg(&self, regs: &mut HPETRegisters, shift: u32, len: u32, val: u64)
             // Halt main counter and disable interrupt generation.
             regs.counter = regs.get_ticks();
 
-            for timer in self.timers.iter().take(self.num_timers) {
-                timer.borrow().del_timer(regs);
+            for t in self.timers.iter().take(self.num_timers) {
+                t.del_timer(regs);
             }
         }
 
@@ -830,9 +831,9 @@ fn set_int_status_reg(&self, regs: &mut HPETRegisters, shift: u32, _len: u32, va
         let new_val = val << shift;
         let cleared = new_val & regs.int_status;
 
-        for (index, timer) in self.timers.iter().take(self.num_timers).enumerate() {
-            if cleared & (1 << index) != 0 {
-                timer.borrow().update_irq(regs, false);
+        for t in self.timers.iter().take(self.num_timers) {
+            if cleared & (1 << t.index) != 0 {
+                t.update_irq(regs, false);
             }
         }
     }
@@ -928,8 +929,8 @@ fn reset_hold(&self, _type: ResetType) {
         {
             let mut regs = self.regs.borrow_mut();
 
-            for timer in self.timers.iter().take(self.num_timers) {
-                timer.borrow().reset(&mut regs);
+            for t in self.timers.iter().take(self.num_timers) {
+                t.reset(&mut regs);
             }
 
             regs.counter = 0;
@@ -981,7 +982,7 @@ fn read(&self, addr: hwaddr, size: u32) -> u64 {
         use DecodedRegister::*;
         use GlobalRegister::*;
         (match target {
-            Timer(timer, tn_target) => timer.borrow().read(tn_target, regs),
+            Timer(t, tn_target) => t.read(tn_target, regs),
             Global(CAP) => regs.capability, /* including HPET_PERIOD 0x004 */
             Global(CFG) => regs.config,
             Global(INT_STATUS) => regs.int_status,
@@ -1012,7 +1013,7 @@ fn write(&self, addr: hwaddr, value: u64, size: u32) {
         use DecodedRegister::*;
         use GlobalRegister::*;
         match target {
-            Timer(timer, tn_target) => timer.borrow().write(tn_target, regs, value, shift, len),
+            Timer(t, tn_target) => t.write(tn_target, regs, value, shift, len),
             Global(CAP) => {} // General Capabilities and ID Register: Read Only
             Global(CFG) => self.set_cfg_reg(regs, shift, len, value),
             Global(INT_STATUS) => self.set_int_status_reg(regs, shift, len, value),
diff --git a/rust/util/src/timer.rs b/rust/util/src/timer.rs
index c6b3e4088ec..829f52d111e 100644
--- a/rust/util/src/timer.rs
+++ b/rust/util/src/timer.rs
@@ -45,14 +45,14 @@ impl Timer {
     }
 
     /// Create a new timer with the given attributes.
-    pub fn init_full<'timer, 'opaque: 'timer, T, F>(
-        self: Pin<&'timer mut Self>,
+    pub fn init_full<T, F>(
+        opaque: Pin<&mut T>,
         timer_list_group: Option<&TimerListGroup>,
         clk_type: ClockType,
         scale: u32,
         attributes: u32,
         _cb: F,
-        opaque: &'opaque T,
+        field: impl FnOnce(&mut T) -> &mut Self,
     ) where
         F: for<'a> FnCall<(&'a T,)>,
     {
@@ -70,8 +70,10 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
 
         // SAFETY: the opaque outlives the timer
         unsafe {
+            let opaque = Pin::into_inner_unchecked(opaque);
+            let timer = field(opaque).as_mut_ptr();
             timer_init_full(
-                self.as_mut_ptr(),
+                timer,
                 if let Some(g) = timer_list_group {
                     g as *const TimerListGroup as *mut _
                 } else {
@@ -81,7 +83,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
                 scale as c_int,
                 attributes as c_int,
                 Some(timer_cb),
-                (opaque as *const T).cast::<c_void>().cast_mut(),
+                (opaque as *mut T).cast::<c_void>(),
             )
         }
     }
-- 
2.51.1

[PATCH 4/5] rust: migration: implement ToMigrationState for Timer

[Paolo Bonzini]

Timer is a complex struct, allow adding it to a struct that
uses #[derive(ToMigrationState)]; similar to vmstate_timer, only
the expiration time has to be preserved.

In fact, because it is thread-safe, ToMigrationStateShared can
also be implemented without needing a cell or mutex that wraps
the timer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/device.rs |  1 -
 rust/migration/src/migratable.rs | 31 +++++++++++++++++++++++++++++++
 rust/util/src/timer.rs           | 10 +++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 5bcf151a680..373ec37bbd3 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -250,7 +250,6 @@ const fn get_individual_route(&self) -> usize {
 }
 
 /// HPET Timer Abstraction
-#[repr(C)]
 #[derive(Debug)]
 pub struct HPETTimer {
     /// timer N index within the timer block (`HPETState`)
diff --git a/rust/migration/src/migratable.rs b/rust/migration/src/migratable.rs
index 02efe31d72c..c82a6b9a7cf 100644
--- a/rust/migration/src/migratable.rs
+++ b/rust/migration/src/migratable.rs
@@ -140,6 +140,26 @@ fn restore_migrated_state_mut(
 
 impl_for_primitive!(u8, u16, u32, u64, i8, i16, i32, i64, bool);
 
+impl ToMigrationState for util::timer::Timer {
+    type Migrated = i64;
+
+    fn snapshot_migration_state(&self, target: &mut i64) -> Result<(), InvalidError> {
+        // SAFETY: as_ptr() is unsafe to ensure that the caller reasons about
+        // the pinning of the data inside the Opaque<>.  Here all we do is
+        // access a field.
+        *target = self.expire_time_ns().unwrap_or(-1);
+        Ok(())
+    }
+
+    fn restore_migrated_state_mut(
+        &mut self,
+        source: Self::Migrated,
+        version_id: u8,
+    ) -> Result<(), InvalidError> {
+        self.restore_migrated_state(source, version_id)
+    }
+}
+
 impl<T: ToMigrationState, const N: usize> ToMigrationState for [T; N]
 where
     [T::Migrated; N]: Default,
@@ -237,6 +257,17 @@ fn restore_migrated_state(
     ) -> Result<(), InvalidError>;
 }
 
+impl ToMigrationStateShared for util::timer::Timer {
+    fn restore_migrated_state(&self, source: i64, _version_id: u8) -> Result<(), InvalidError> {
+        if source >= 0 {
+            self.modify(source as u64);
+        } else {
+            self.delete();
+        }
+        Ok(())
+    }
+}
+
 impl<T: ToMigrationStateShared, const N: usize> ToMigrationStateShared for [T; N]
 where
     [T::Migrated; N]: Default,
diff --git a/rust/util/src/timer.rs b/rust/util/src/timer.rs
index 829f52d111e..4109d84c398 100644
--- a/rust/util/src/timer.rs
+++ b/rust/util/src/timer.rs
@@ -10,7 +10,8 @@
 use common::{callbacks::FnCall, Opaque};
 
 use crate::bindings::{
-    self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType,
+    self, qemu_clock_get_ns, timer_del, timer_expire_time_ns, timer_init_full, timer_mod,
+    QEMUClockType,
 };
 
 /// A safe wrapper around [`bindings::QEMUTimer`].
@@ -88,6 +89,13 @@ pub fn init_full<T, F>(
         }
     }
 
+    pub fn expire_time_ns(&self) -> Option<i64> {
+        // SAFETY: the only way to obtain a Timer safely is via methods that
+        // take a Pin<&mut Self>, therefore the timer is pinned
+        let ret = unsafe { timer_expire_time_ns(self.as_ptr()) };
+        i64::try_from(ret).ok()
+    }
+
     pub fn modify(&self, expire_time: u64) {
         // SAFETY: the only way to obtain a Timer safely is via methods that
         // take a Pin<&mut Self>, therefore the timer is pinned
-- 
2.51.1

[PATCH 5/5] rust/hpet: Apply Migratable<> wrapper and ToMigrationState

[Paolo Bonzini]

From: Zhao Liu <zhao1.liu@intel.com>

Before using Mutex<> to protect HPETRegisters, it's necessary to apply
Migratable<> wrapper and ToMigrationState first since there's no
pre-defined VMState for Mutex<>.

In addition, this allows to move data from HPETTimerRegisters to
HPETTimer, so as to preserve the original migration format of the C
implementation.  To do that, HPETTimer is wrapped with Migratable<>
as well but the implementation of ToMigrationStateShared is
hand-written.

Note that even though the HPETRegistersMigration struct is
generated by ToMigrationState macro, its VMState still needs to be
implemented by hand.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20251113051937.4017675-21-zhao1.liu@intel.com
[Added HPETTimer implementation and restored compatible migration format. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/device.rs | 139 +++++++++++++++++++++++--------
 1 file changed, 102 insertions(+), 37 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 373ec37bbd3..fde4469ec16 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -13,7 +13,7 @@
 use bql::prelude::*;
 use common::prelude::*;
 use hwcore::prelude::*;
-use migration::{self, prelude::*};
+use migration::{self, prelude::*, ToMigrationStateShared};
 use qom::prelude::*;
 use system::{
     bindings::{address_space_memory, address_space_stl_le},
@@ -176,7 +176,6 @@ fn timer_handler(t: &HPETTimer) {
     t.callback(regs)
 }
 
-#[repr(C)]
 #[derive(Debug, Default)]
 pub struct HPETTimerRegisters {
     // Memory-mapped, software visible timer registers
@@ -650,11 +649,13 @@ fn write(
     }
 }
 
-#[repr(C)]
-#[derive(Default)]
+#[derive(Default, ToMigrationState)]
 pub struct HPETRegisters {
     // HPET block Registers: Memory-mapped, software visible registers
     /// General Capabilities and ID Register
+    ///
+    /// Constant and therefore not migrated.
+    #[migration_state(omit)]
     capability: u64,
     ///  General Configuration Register
     config: u64,
@@ -666,9 +667,15 @@ pub struct HPETRegisters {
     counter: u64,
 
     /// HPET Timer N Registers
+    ///
+    /// Migrated as part of `Migratable<HPETTimer>`
+    #[migration_state(omit)]
     tn_regs: [HPETTimerRegisters; HPET_MAX_TIMERS],
 
     /// Offset of main counter relative to qemu clock.
+    ///
+    /// Migrated as a subsection and therefore snapshotted into [`HPETState`]
+    #[migration_state(omit)]
     pub hpet_offset: u64,
 }
 
@@ -702,7 +709,7 @@ fn is_timer_int_active(&self, index: usize) -> bool {
 pub struct HPETState {
     parent_obj: ParentField<SysBusDevice>,
     iomem: MemoryRegion,
-    regs: BqlRefCell<HPETRegisters>,
+    regs: Migratable<BqlRefCell<HPETRegisters>>,
 
     // Internal state
     /// Capabilities that QEMU HPET supports.
@@ -728,7 +735,7 @@ pub struct HPETState {
 
     /// HPET timer array managed by this timer block.
     #[doc(alias = "timer")]
-    timers: [HPETTimer; HPET_MAX_TIMERS],
+    timers: [Migratable<HPETTimer>; HPET_MAX_TIMERS],
     #[property(rename = "timers", default = HPET_MIN_TIMERS)]
     num_timers: usize,
     num_timers_save: BqlCell<u8>,
@@ -763,9 +770,12 @@ fn init_timers(this: &mut MaybeUninit<Self>) {
 
             // Initialize in two steps, to avoid calling Timer::init_full on a
             // temporary that can be moved.
-            let timer = timer.write(HPETTimer::new(index.try_into().unwrap(), state));
+            let timer = timer.write(Migratable::new(HPETTimer::new(
+                index.try_into().unwrap(),
+                state,
+            )));
             // SAFETY: HPETState is pinned
-            let timer = unsafe { Pin::new_unchecked(timer) };
+            let timer = unsafe { Pin::new_unchecked(&mut **timer) };
             HPETTimer::init_timer(timer);
         }
     }
@@ -1109,47 +1119,102 @@ impl ObjectImpl for HPETState {
         })
         .build();
 
-impl_vmstate_struct!(
-    HPETTimerRegisters,
-    VMStateDescriptionBuilder::<HPETTimerRegisters>::new()
-        .name(c"hpet_timer/regs")
+#[derive(Default)]
+pub struct HPETTimerMigration {
+    index: u8,
+    config: u64,
+    cmp: u64,
+    fsb: u64,
+    period: u64,
+    wrap_flag: u8,
+    qemu_timer: i64,
+}
+
+impl ToMigrationState for HPETTimer {
+    type Migrated = HPETTimerMigration;
+
+    fn snapshot_migration_state(
+        &self,
+        target: &mut Self::Migrated,
+    ) -> Result<(), migration::Infallible> {
+        let state = self.get_state();
+        let regs = state.regs.borrow_mut();
+        let tn_regs = &regs.tn_regs[self.index as usize];
+
+        target.index = self.index;
+        target.config = tn_regs.config;
+        target.cmp = tn_regs.cmp;
+        target.fsb = tn_regs.fsb;
+        target.period = tn_regs.period;
+        target.wrap_flag = tn_regs.wrap_flag;
+        self.qemu_timer
+            .snapshot_migration_state(&mut target.qemu_timer)?;
+
+        Ok(())
+    }
+
+    fn restore_migrated_state_mut(
+        &mut self,
+        source: Self::Migrated,
+        version_id: u8,
+    ) -> Result<(), migration::Infallible> {
+        self.restore_migrated_state(source, version_id)
+    }
+}
+
+impl ToMigrationStateShared for HPETTimer {
+    fn restore_migrated_state(
+        &self,
+        source: Self::Migrated,
+        version_id: u8,
+    ) -> Result<(), migration::Infallible> {
+        let state = self.get_state();
+        let mut regs = state.regs.borrow_mut();
+        let tn_regs = &mut regs.tn_regs[self.index as usize];
+
+        tn_regs.config = source.config;
+        tn_regs.cmp = source.cmp;
+        tn_regs.fsb = source.fsb;
+        tn_regs.period = source.period;
+        tn_regs.wrap_flag = source.wrap_flag;
+        self.qemu_timer
+            .restore_migrated_state(source.qemu_timer, version_id)?;
+
+        Ok(())
+    }
+}
+
+const VMSTATE_HPET_TIMER: VMStateDescription<HPETTimerMigration> =
+    VMStateDescriptionBuilder::<HPETTimerMigration>::new()
+        .name(c"hpet_timer")
         .version_id(1)
         .minimum_version_id(1)
         .fields(vmstate_fields! {
-            vmstate_of!(HPETTimerRegisters, config),
-            vmstate_of!(HPETTimerRegisters, cmp),
-            vmstate_of!(HPETTimerRegisters, fsb),
-            vmstate_of!(HPETTimerRegisters, period),
-            vmstate_of!(HPETTimerRegisters, wrap_flag),
-        })
-        .build()
-);
-
-const VMSTATE_HPET_TIMER: VMStateDescription<HPETTimer> =
-    VMStateDescriptionBuilder::<HPETTimer>::new()
-        .name(c"hpet_timer")
-        .version_id(2)
-        .minimum_version_id(2)
-        .fields(vmstate_fields! {
-            vmstate_of!(HPETTimer, qemu_timer),
+            vmstate_of!(HPETTimerMigration, index),
+            vmstate_of!(HPETTimerMigration, config),
+            vmstate_of!(HPETTimerMigration, cmp),
+            vmstate_of!(HPETTimerMigration, fsb),
+            vmstate_of!(HPETTimerMigration, period),
+            vmstate_of!(HPETTimerMigration, wrap_flag),
+            vmstate_of!(HPETTimerMigration, qemu_timer),
         })
         .build();
 
-impl_vmstate_struct!(HPETTimer, VMSTATE_HPET_TIMER);
+impl_vmstate_struct!(HPETTimerMigration, VMSTATE_HPET_TIMER);
 
 const VALIDATE_TIMERS_NAME: &CStr = c"num_timers must match";
 
+// HPETRegistersMigration is generated by ToMigrationState macro.
 impl_vmstate_struct!(
-    HPETRegisters,
-    VMStateDescriptionBuilder::<HPETRegisters>::new()
+    HPETRegistersMigration,
+    VMStateDescriptionBuilder::<HPETRegistersMigration>::new()
         .name(c"hpet/regs")
         .version_id(2)
         .minimum_version_id(2)
         .fields(vmstate_fields! {
-            vmstate_of!(HPETRegisters, config),
-            vmstate_of!(HPETRegisters, int_status),
-            vmstate_of!(HPETRegisters, counter),
-            vmstate_of!(HPETRegisters, tn_regs),
+            vmstate_of!(HPETRegistersMigration, config),
+            vmstate_of!(HPETRegistersMigration, int_status),
+            vmstate_of!(HPETRegistersMigration, counter),
         })
         .build()
 );
@@ -1157,8 +1222,8 @@ impl ObjectImpl for HPETState {
 const VMSTATE_HPET: VMStateDescription<HPETState> =
     VMStateDescriptionBuilder::<HPETState>::new()
         .name(c"hpet")
-        .version_id(3)
-        .minimum_version_id(3)
+        .version_id(2)
+        .minimum_version_id(2)
         .pre_save(&HPETState::pre_save)
         .post_load(&HPETState::post_load)
         .fields(vmstate_fields! {
-- 
2.51.1

Re: [PATCH 1/5] rust/hpet: move hidden registers to HPETTimerRegisters

[Zhao Liu]

On Mon, Nov 17, 2025 at 09:47:48AM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Nov 2025 09:47:48 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/5] rust/hpet: move hidden registers to HPETTimerRegisters
> X-Mailer: git-send-email 2.51.1
> 
> Do not separate visible and hidden state; both of them are used in the
> same circumstances and it's easiest to place both of them under the
> same BqlRefCell.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/device.rs | 157 ++++++++++++++-----------------
>  1 file changed, 71 insertions(+), 86 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 2/5] rust/hpet: move hpet_offset to HPETRegisters

[Zhao Liu]

On Mon, Nov 17, 2025 at 09:47:49AM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Nov 2025 09:47:49 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 2/5] rust/hpet: move hpet_offset to HPETRegisters
> X-Mailer: git-send-email 2.51.1
> 
> Likewise, do not separate hpet_offset from the other registers.
> However, because it is migrated in a subsection it is necessary
> to copy it out of HPETRegisters and into a BqlCell<>.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/device.rs | 63 ++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 28 deletions(-)

,,,

> +        let mut next_tick: u64 = tn_regs.cmp64;
>          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() && tn_regs.cmp64 > hpet_next_wrap(cur_tick) {
>                  tn_regs.wrap_flag = 1;
> -                self.arm_timer(tn_regs, hpet_next_wrap(cur_tick));
> -                return;
> +                next_tick = hpet_next_wrap(cur_tick);
>              }
>          }
> -        self.arm_timer(tn_regs, tn_regs.cmp64);
> +        self.arm_timer(regs, next_tick);
>      }

Good! This saves a arm_timer().

...

>  impl HPETRegisters {
> +    fn get_ticks(&self) -> u64 {
> +        // Protect hpet_offset in lockless IO case which would not lock BQL.

Just nit, this comment seems not much necessary, since currently there's
no Mutex lock to represent "Protect". But it's up to you to keep it or
not.

> +        ns_to_ticks(CLOCK_VIRTUAL.get_ns() + self.hpet_offset)
> +    }
> +
> +    fn get_ns(&self, tick: u64) -> u64 {
> +        // Protect hpet_offset in lockless IO case which would not lock BQL.

Ditto.

> +        ticks_to_ns(tick) - self.hpet_offset
> +    }

LGTM,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 3/5] rust/hpet: remove BqlRefCell around HPETTimer

[Zhao Liu]

> -    fn init_timer_with_cell(cell: &BqlRefCell<Self>) {
> -        let mut timer = cell.borrow_mut();
> -        // SAFETY: HPETTimer is only used as part of HPETState, which is
> -        // always pinned.
> -        let qemu_timer = unsafe { Pin::new_unchecked(&mut timer.qemu_timer) };
> -        qemu_timer.init_full(None, CLOCK_VIRTUAL, Timer::NS, 0, timer_handler, cell);
> +    fn init_timer(timer: Pin<&mut Self>) {
> +        Timer::init_full(
> +            timer,
> +            None,
> +            CLOCK_VIRTUAL,
> +            Timer::NS,
> +            0,
> +            timer_handler,
> +            |t| &mut t.qemu_timer,
> +        );
>      }

I find this way could also work for BqlRefCell case:

    fn init_timer_with_cell(cell: &mut BqlRefCell<Self>) {
        // SAFETY: HPETTimer is only used as part of HPETState, which is
        // always pinned.
        let timer = unsafe { Pin::new_unchecked(cell) };
        Timer::init_full(
            timer,
            None,
            CLOCK_VIRTUAL,
            Timer::NS,
            0,
            timer_handler,
            |t| {
                assert!(bql::is_locked());
                &mut t.get_mut().qemu_timer
            },
        );
    }


So any other non-lockless timer can also use this interface to
initialize their BqlRefCell<>.

(BTW, I find BqlRefCell::get_mut() / as_ref() missed bql::is_locked().
 right?)

...

> diff --git a/rust/util/src/timer.rs b/rust/util/src/timer.rs
> index c6b3e4088ec..829f52d111e 100644
> --- a/rust/util/src/timer.rs
> +++ b/rust/util/src/timer.rs
> @@ -45,14 +45,14 @@ impl Timer {
>      }
>  
>      /// Create a new timer with the given attributes.
> -    pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> -        self: Pin<&'timer mut Self>,
> +    pub fn init_full<T, F>(
> +        opaque: Pin<&mut T>,
>          timer_list_group: Option<&TimerListGroup>,
>          clk_type: ClockType,
>          scale: u32,
>          attributes: u32,
>          _cb: F,
> -        opaque: &'opaque T,
> +        field: impl FnOnce(&mut T) -> &mut Self,
>      ) where
>          F: for<'a> FnCall<(&'a T,)>,


This is more tricky than I previously imaged. Good solution!

Another way to handle this kind of 'self reference' issue would probably
be to consider passing raw pointers as opaque parameters... but that's
definitely not as clean/idiomatic as the current approach.

I think it may be better to add doc about how to use this for
BqlRefCell<> case since there'll be no example after this patch.


Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 5/5] rust/hpet: Apply Migratable<> wrapper and ToMigrationState

[Zhao Liu]

On Mon, Nov 17, 2025 at 09:47:52AM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Nov 2025 09:47:52 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 5/5] rust/hpet: Apply Migratable<> wrapper and
>  ToMigrationState
> X-Mailer: git-send-email 2.51.1
> 
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Before using Mutex<> to protect HPETRegisters, it's necessary to apply
> Migratable<> wrapper and ToMigrationState first since there's no
> pre-defined VMState for Mutex<>.
> 
> In addition, this allows to move data from HPETTimerRegisters to
> HPETTimer,

Typo? move data from HPETTimerRegisters' vmstate to HPETTimer's vmstate

> so as to preserve the original migration format of the C
> implementation.  To do that, HPETTimer is wrapped with Migratable<>
> as well but the implementation of ToMigrationStateShared is
> hand-written.
> 
> Note that even though the HPETRegistersMigration struct is
> generated by ToMigrationState macro, its VMState still needs to be
> implemented by hand.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Link: https://lore.kernel.org/r/20251113051937.4017675-21-zhao1.liu@intel.com
> [Added HPETTimer implementation and restored compatible migration format. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/device.rs | 139 +++++++++++++++++++++++--------
>  1 file changed, 102 insertions(+), 37 deletions(-)

yes, the fully hand-written ToMigrationStateShared is really powerful -
it can keep the vmstate layout unchanged even when the struct (
HPETTimerRegisters) has such big changes.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 0/5] rust/hpet: complete moving state out of HPETTimer

[Zhao Liu]

On Mon, Nov 17, 2025 at 09:47:47AM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Nov 2025 09:47:47 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 0/5] rust/hpet: complete moving state out of HPETTimer
> X-Mailer: git-send-email 2.51.1
> 
> This state continues the cleanups of the HPET state, moving fields out of
> BqlCells and into HPETRegisters and HPETTimerRegisters.  It also restores
> the old migration format and shows an interesting trick: HPETTimer is now
> a very simple object that handles the "unsafe" backreference from the
> timer to the HPETState, but it also implements ToMigrationStateShared
> and is stored in the HPETState as Migratable<[HPETTimer; N]>.  I find
> it pretty cool that the composition works naturally.
> 
> The less beautiful part is that I had to modify Timer::init_full for
> this to compile.  It's probably time to work on the final design for
> initialization, because this is becoming very ad hoc and the differences
> between timer, MemoryRegion and Clock initialization have no real
> justification.

<Just some rough thoughts/understanding>

Yes, Timer requires Pin<> and it seems MemoryRegion also should requires
it because of callback...

MemoryRegion requires MaybeUninitField, but it would be not necessary
for HPET, since HPETTimer has already initialized its other field before
calling init_full(). But as a general interface, MaybeUninitField could
be also useful for Timer, as we can't require how device initialize its
field.

Clock's ParentInit<> is also different. We don't need to require
Timer::init_full must be called in init(). MemoryRegion may also don't
need it And some C device even calls memory_region_init_io() in
realize().

Regards,
Zhao

Re: [PATCH 3/5] rust/hpet: remove BqlRefCell around HPETTimer

[Paolo Bonzini]

On 11/19/25 16:17, Zhao Liu wrote:
>> -    fn init_timer_with_cell(cell: &BqlRefCell<Self>) {
>> -        let mut timer = cell.borrow_mut();
>> -        // SAFETY: HPETTimer is only used as part of HPETState, which is
>> -        // always pinned.
>> -        let qemu_timer = unsafe { Pin::new_unchecked(&mut timer.qemu_timer) };
>> -        qemu_timer.init_full(None, CLOCK_VIRTUAL, Timer::NS, 0, timer_handler, cell);
>> +    fn init_timer(timer: Pin<&mut Self>) {
>> +        Timer::init_full(
>> +            timer,
>> +            None,
>> +            CLOCK_VIRTUAL,
>> +            Timer::NS,
>> +            0,
>> +            timer_handler,
>> +            |t| &mut t.qemu_timer,
>> +        );
>>       }
> 
> I find this way could also work for BqlRefCell case:
> 
>      fn init_timer_with_cell(cell: &mut BqlRefCell<Self>) {
>          // SAFETY: HPETTimer is only used as part of HPETState, which is
>          // always pinned.
>          let timer = unsafe { Pin::new_unchecked(cell) };
>          Timer::init_full(
>              timer,
>              None,
>              CLOCK_VIRTUAL,
>              Timer::NS,
>              0,
>              timer_handler,
>              |t| {
>                  assert!(bql::is_locked());
>                  &mut t.get_mut().qemu_timer
>              },
>          );
>      }

Yes, but I'm still not sure what the final shape will be... I don't like 
this too much.

> 
> So any other non-lockless timer can also use this interface to
> initialize their BqlRefCell<>.
> 
> (BTW, I find BqlRefCell::get_mut() / as_ref() missed bql::is_locked().
>   right?)

as_ptr() doesn't need it because the pointer can be dereferenced later.

As to get_mut()... I think if you are the only owner, you should have 
the guarantee of being able to modify the content freely.  This is true 
even if your &mut came from interior mutability.

So, in the above case you only need &mut t.get_mut().qemu_timer.

> I think it may be better to add doc about how to use this for
> BqlRefCell<> case since there'll be no example after this patch.

Re: [PATCH 4/5] rust: migration: implement ToMigrationState for Timer

[Zhao Liu]

On Mon, Nov 17, 2025 at 09:47:51AM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Nov 2025 09:47:51 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 4/5] rust: migration: implement ToMigrationState for Timer
> X-Mailer: git-send-email 2.51.1
> 
> Timer is a complex struct, allow adding it to a struct that
> uses #[derive(ToMigrationState)]; similar to vmstate_timer, only
> the expiration time has to be preserved.
> 
> In fact, because it is thread-safe, ToMigrationStateShared can
> also be implemented without needing a cell or mutex that wraps
> the timer.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/device.rs |  1 -
>  rust/migration/src/migratable.rs | 31 +++++++++++++++++++++++++++++++
>  rust/util/src/timer.rs           | 10 +++++++++-
>  3 files changed, 40 insertions(+), 2 deletions(-)

I just, based on previous discussion, try to complete the timer's
ToMigrationState - use modify_ns() instead of modify(). This is on top
of this series.

From eb8b99a45ffccba7e0508141553c2c24c5efa410 Mon Sep 17 00:00:00 2001
From: Zhao Liu <zhao1.liu@intel.com>
Date: Thu, 20 Nov 2025 22:26:35 +0800
Subject: [PATCH] rust/timer: Use modify_ns() in restore_migrated_state()

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 include/qemu/timer.h             | 11 +++++++++++
 rust/migration/src/migratable.rs |  2 +-
 rust/util/src/timer.rs           | 21 +++++++++++++++++----
 util/qemu-timer.c                |  4 ++++
 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 8b561cd6960b..4c6a51d600fb 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -392,6 +392,17 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup *tlg);
  * QEMUTimer
  */

+/**
+ * timer_get_scale
+ * @ts: the timer to be accessed
+ *
+ * Get the scale value of the specified timer. The scale represents
+ * the number of nanoseconds per unit of time for this timer.
+ *
+ * Returns: the scale of the timer (nanoseconds per unit)
+ */
+int timer_get_scale(QEMUTimer *ts);
+
 /**
  * timer_init_full:
  * @ts: the timer to be initialised
diff --git a/rust/migration/src/migratable.rs b/rust/migration/src/migratable.rs
index c82a6b9a7cf2..7748aac2f27d 100644
--- a/rust/migration/src/migratable.rs
+++ b/rust/migration/src/migratable.rs
@@ -260,7 +260,7 @@ fn restore_migrated_state(
 impl ToMigrationStateShared for util::timer::Timer {
     fn restore_migrated_state(&self, source: i64, _version_id: u8) -> Result<(), InvalidError> {
         if source >= 0 {
-            self.modify(source as u64);
+            self.modify_ns(source as u64);
         } else {
             self.delete();
         }
diff --git a/rust/util/src/timer.rs b/rust/util/src/timer.rs
index 4109d84c398a..6114892f084f 100644
--- a/rust/util/src/timer.rs
+++ b/rust/util/src/timer.rs
@@ -10,8 +10,8 @@
 use common::{callbacks::FnCall, Opaque};

 use crate::bindings::{
-    self, qemu_clock_get_ns, timer_del, timer_expire_time_ns, timer_init_full, timer_mod,
-    QEMUClockType,
+    self, qemu_clock_get_ns, timer_del, timer_expire_time_ns, timer_get_scale, timer_init_full,
+    timer_mod_ns, QEMUClockType,
 };

 /// A safe wrapper around [`bindings::QEMUTimer`].
@@ -96,10 +96,23 @@ pub fn expire_time_ns(&self) -> Option<i64> {
         i64::try_from(ret).ok()
     }

-    pub fn modify(&self, expire_time: u64) {
+    fn scale(&self) -> u32 {
+        // SAFETY: the only way to obtain a Timer safely is via methods that
+        // take a Pin<&mut Self>, therefore the timer is pinned. And when Timer
+        // is created, its fields (including scale) are initialized to zero.
+        unsafe { timer_get_scale(self.as_mut_ptr()) }
+            .try_into()
+            .unwrap()
+    }
+
+    pub fn modify_ns(&self, expire_time: u64) {
         // SAFETY: the only way to obtain a Timer safely is via methods that
         // take a Pin<&mut Self>, therefore the timer is pinned
-        unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
+        unsafe { timer_mod_ns(self.as_mut_ptr(), expire_time.try_into().unwrap()) }
+    }
+
+    pub fn modify(&self, expire_time: u64) {
+        self.modify_ns(expire_time * u64::from(self.scale()))
     }

     pub fn delete(&self) {
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 2a6be4c7f958..233fdb2aa847 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -346,6 +346,10 @@ int qemu_poll_ns(GPollFD *fds, guint nfds, int64_t timeout)
 #endif
 }

+int timer_get_scale(QEMUTimer *ts)
+{
+    return ts->scale;
+}

 void timer_init_full(QEMUTimer *ts,
                      QEMUTimerListGroup *timer_list_group, QEMUClockType type,
--
2.34.1