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

Zhao Liu <[email protected]>
Newsgroups org.nongnu.qemu-rust,org.nongnu.qemu-devel
Message-ID <[email protected]>
> @@ -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);
     }
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.