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

> @@ -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);

I didn't notice this in previous review... arming timer unconditionally
is wrong, since the code block is:

if tn_regs.is_periodic() && tn_regs.period != 0 {
    ...
    self.arm_timer(tn_regs, tn_regs.cmp64);
} else if tn_regs.wrap_flag != 0 {
    ...
    self.arm_timer(tn_regs, tn_regs.cmp64);
}

So one-shot mode (with wrap_flag == 0) shouldn't arm timer again,
(otherwise, the guest will hang).

---
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 19676af74bc6..179bd18e2045 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -586,23 +586,33 @@ 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 next_tick = {
+            let cur_tick: u64 = regs.get_ticks();
+            let tn_regs = &mut regs.tn_regs[self.index as usize];

-        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(tn_regs.cmp64 as u32); // truncate!
+            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(tn_regs.cmp64 as u32); // truncate!
+                } else {
+                    tn_regs.cmp = tn_regs.cmp64;
+                }
+
+                Some(tn_regs.cmp64)
+            } else if tn_regs.wrap_flag != 0 {
+                tn_regs.wrap_flag = 0;
+
+                Some(tn_regs.cmp64)
             } else {
-                tn_regs.cmp = tn_regs.cmp64;
+                None
             }
-        } else if tn_regs.wrap_flag != 0 {
-            tn_regs.wrap_flag = 0;
+        };
+
+        if let Some(t) = next_tick {
+            self.arm_timer(regs, t);
         }
-        let next_tick = tn_regs.cmp64;
-        self.arm_timer(regs, next_tick);
         self.update_irq(regs, true);
     }

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>