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

Place all HPET (global) timer block registers in a HPETRegisters struct,
and wrap the whole register struct with a BqlRefCell<>.

This allows to elevate the Bql check from individual register access to
register structure access, making the Bql check more coarse-grained. But
in current step, just treat BqlRefCell as BqlCell while maintaining
fine-grained BQL protection. This approach helps to use HPETRegisters
struct clearly without introducing the "already borrowed" around
BqlRefCell.

HPETRegisters struct makes it possible to take a Mutex<> to replace
BqlRefCell<>, like C side did.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20251113051937.4017675-13-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/device.rs | 116 +++++++++++++++++++------------
 1 file changed, 70 insertions(+), 46 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 8dc3cc59c98..bf9f4936718 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -524,24 +524,29 @@ fn write(&mut self, target: TimerRegister, value: u64, shift: u32, len: u32) {
     }
 }
 
+#[repr(C)]
+#[derive(Default)]
+pub struct HPETRegisters {
+    // HPET block Registers: Memory-mapped, software visible registers
+    /// General Capabilities and ID Register
+    capability: u64,
+    ///  General Configuration Register
+    config: u64,
+    /// General Interrupt Status Register
+    #[doc(alias = "isr")]
+    int_status: u64,
+    /// Main Counter Value Register
+    #[doc(alias = "hpet_counter")]
+    counter: u64,
+}
+
 /// HPET Event Timer Block Abstraction
 #[repr(C)]
 #[derive(qom::Object, hwcore::Device)]
 pub struct HPETState {
     parent_obj: ParentField<SysBusDevice>,
     iomem: MemoryRegion,
-
-    // HPET block Registers: Memory-mapped, software visible registers
-    /// General Capabilities and ID Register
-    capability: BqlCell<u64>,
-    ///  General Configuration Register
-    config: BqlCell<u64>,
-    /// General Interrupt Status Register
-    #[doc(alias = "isr")]
-    int_status: BqlCell<u64>,
-    /// Main Counter Value Register
-    #[doc(alias = "hpet_counter")]
-    counter: BqlCell<u64>,
+    regs: BqlRefCell<HPETRegisters>,
 
     // Internal state
     /// Capabilities that QEMU HPET supports.
@@ -583,15 +588,15 @@ const fn has_msi_flag(&self) -> bool {
     }
 
     fn is_legacy_mode(&self) -> bool {
-        self.config.get() & (1 << HPET_CFG_LEG_RT_SHIFT) != 0
+        self.regs.borrow().config & (1 << HPET_CFG_LEG_RT_SHIFT) != 0
     }
 
     fn is_hpet_enabled(&self) -> bool {
-        self.config.get() & (1 << HPET_CFG_ENABLE_SHIFT) != 0
+        self.regs.borrow().config & (1 << HPET_CFG_ENABLE_SHIFT) != 0
     }
 
     fn is_timer_int_active(&self, index: usize) -> bool {
-        self.int_status.get() & (1 << index) != 0
+        self.regs.borrow().int_status & (1 << index) != 0
     }
 
     fn get_ticks(&self) -> u64 {
@@ -631,22 +636,22 @@ fn init_timers(this: &mut MaybeUninit<Self>) {
     }
 
     fn update_int_status(&self, index: u32, level: bool) {
-        self.int_status
-            .set(self.int_status.get().deposit(index, 1, u64::from(level)));
+        let mut regs = self.regs.borrow_mut();
+        regs.int_status = regs.int_status.deposit(index, 1, u64::from(level));
     }
 
     /// General Configuration Register
     fn set_cfg_reg(&self, shift: u32, len: u32, val: u64) {
-        let old_val = self.config.get();
+        let old_val = self.regs.borrow().config;
         let mut new_val = old_val.deposit(shift, len, val);
 
         new_val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
-        self.config.set(new_val);
+        self.regs.borrow_mut().config = new_val;
 
         if activating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
             // Enable main counter and interrupt generation.
             self.hpet_offset
-                .set(ticks_to_ns(self.counter.get()) - CLOCK_VIRTUAL.get_ns());
+                .set(ticks_to_ns(self.regs.borrow().counter) - CLOCK_VIRTUAL.get_ns());
 
             for timer in self.timers.iter().take(self.num_timers) {
                 let mut t = timer.borrow_mut();
@@ -658,7 +663,7 @@ fn set_cfg_reg(&self, 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.
-            self.counter.set(self.get_ticks());
+            self.regs.borrow_mut().counter = self.get_ticks();
 
             for timer in self.timers.iter().take(self.num_timers) {
                 timer.borrow().del_timer();
@@ -681,7 +686,7 @@ fn set_cfg_reg(&self, shift: u32, len: u32, val: u64) {
     /// General Interrupt Status Register: Read/Write Clear
     fn set_int_status_reg(&self, shift: u32, _len: u32, val: u64) {
         let new_val = val << shift;
-        let cleared = new_val & self.int_status.get();
+        let cleared = new_val & self.regs.borrow().int_status;
 
         for (index, timer) in self.timers.iter().take(self.num_timers).enumerate() {
             if cleared & (1 << index) != 0 {
@@ -701,8 +706,8 @@ fn set_counter_reg(&self, shift: u32, len: u32, val: u64) {
             // not be changed as well).
             trace::trace_hpet_ram_write_counter_write_while_enabled();
         }
-        self.counter
-            .set(self.counter.get().deposit(shift, len, val));
+        let mut regs = self.regs.borrow_mut();
+        regs.counter = regs.counter.deposit(shift, len, val);
     }
 
     unsafe fn init(mut this: ParentInit<Self>) {
@@ -745,14 +750,12 @@ fn realize(&self) -> util::Result<()> {
         self.hpet_id.set(HPETFwConfig::assign_hpet_id()?);
 
         // 64-bit General Capabilities and ID Register; LegacyReplacementRoute.
-        self.capability.set(
-            HPET_CAP_REV_ID_VALUE << HPET_CAP_REV_ID_SHIFT |
+        self.regs.borrow_mut().capability = HPET_CAP_REV_ID_VALUE << HPET_CAP_REV_ID_SHIFT |
             1 << HPET_CAP_COUNT_SIZE_CAP_SHIFT |
             1 << HPET_CAP_LEG_RT_CAP_SHIFT |
             HPET_CAP_VENDER_ID_VALUE << HPET_CAP_VENDER_ID_SHIFT |
             ((self.num_timers - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
-            (HPET_CLK_PERIOD * FS_PER_NS) << HPET_CAP_CNT_CLK_PERIOD_SHIFT, // 10 ns
-        );
+            (HPET_CLK_PERIOD * FS_PER_NS) << HPET_CAP_CNT_CLK_PERIOD_SHIFT; // 10 ns
 
         self.init_gpio_in(2, HPETState::handle_legacy_irq);
         self.init_gpio_out(from_ref(&self.pit_enabled));
@@ -764,17 +767,23 @@ fn reset_hold(&self, _type: ResetType) {
             timer.borrow_mut().reset();
         }
 
-        self.counter.set(0);
-        self.config.set(0);
+        // pit_enabled.set(true) will call irq handler and access regs
+        // again. We cannot borrow BqlRefCell twice at once. Minimize the
+        // scope of regs to ensure it will be dropped before irq callback.
+        {
+            let mut regs = self.regs.borrow_mut();
+            regs.counter = 0;
+            regs.config = 0;
+            HPETFwConfig::update_hpet_cfg(
+                self.hpet_id.get(),
+                regs.capability as u32,
+                self.mmio_addr(0).unwrap(),
+            );
+        }
+
         self.pit_enabled.set(true);
         self.hpet_offset.set(0);
 
-        HPETFwConfig::update_hpet_cfg(
-            self.hpet_id.get(),
-            self.capability.get() as u32,
-            self.mmio_addr(0).unwrap(),
-        );
-
         // to document that the RTC lowers its output on reset as well
         self.rtc_irq_level.set(0);
     }
@@ -812,14 +821,14 @@ fn read(&self, addr: hwaddr, size: u32) -> u64 {
         use GlobalRegister::*;
         (match target {
             Timer(timer, tn_target) => timer.borrow_mut().read(tn_target),
-            Global(CAP) => self.capability.get(), /* including HPET_PERIOD 0x004 */
-            Global(CFG) => self.config.get(),
-            Global(INT_STATUS) => self.int_status.get(),
+            Global(CAP) => self.regs.borrow().capability, /* including HPET_PERIOD 0x004 */
+            Global(CFG) => self.regs.borrow().config,
+            Global(INT_STATUS) => self.regs.borrow().int_status,
             Global(COUNTER) => {
                 let cur_tick = if self.is_hpet_enabled() {
                     self.get_ticks()
                 } else {
-                    self.counter.get()
+                    self.regs.borrow().counter
                 };
 
                 trace::trace_hpet_ram_read_reading_counter((addr & 4) as u8, cur_tick);
@@ -852,7 +861,7 @@ fn write(&self, addr: hwaddr, value: u64, size: u32) {
 
     fn pre_save(&self) -> Result<(), migration::Infallible> {
         if self.is_hpet_enabled() {
-            self.counter.set(self.get_ticks());
+            self.regs.borrow_mut().counter = self.get_ticks();
         }
 
         /*
@@ -867,15 +876,16 @@ fn pre_save(&self) -> Result<(), migration::Infallible> {
     fn post_load(&self, _version_id: u8) -> Result<(), migration::Infallible> {
         for timer in self.timers.iter().take(self.num_timers) {
             let mut t = timer.borrow_mut();
+            let cnt = t.get_state().regs.borrow().counter;
 
-            t.cmp64 = t.calculate_cmp64(t.get_state().counter.get(), t.regs.cmp);
+            t.cmp64 = t.calculate_cmp64(cnt, t.regs.cmp);
             t.last = CLOCK_VIRTUAL.get_ns() - NANOSECONDS_PER_SECOND;
         }
 
         // Recalculate the offset between the main counter and guest time
         if !self.hpet_offset_saved {
             self.hpet_offset
-                .set(ticks_to_ns(self.counter.get()) - CLOCK_VIRTUAL.get_ns());
+                .set(ticks_to_ns(self.regs.borrow().counter) - CLOCK_VIRTUAL.get_ns());
         }
 
         Ok(())
@@ -966,6 +976,22 @@ impl ObjectImpl for HPETState {
 
 const VALIDATE_TIMERS_NAME: &CStr = c"num_timers must match";
 
+// In fact, version_id and minimum_version_id for HPETRegisters are
+// unrelated to HPETState's version IDs. Does not affect compatibility.
+impl_vmstate_struct!(
+    HPETRegisters,
+    VMStateDescriptionBuilder::<HPETRegisters>::new()
+        .name(c"hpet/regs")
+        .version_id(1)
+        .minimum_version_id(1)
+        .fields(vmstate_fields! {
+            vmstate_of!(HPETRegisters, config),
+            vmstate_of!(HPETRegisters, int_status),
+            vmstate_of!(HPETRegisters, counter),
+        })
+        .build()
+);
+
 const VMSTATE_HPET: VMStateDescription<HPETState> =
     VMStateDescriptionBuilder::<HPETState>::new()
         .name(c"hpet")
@@ -974,9 +1000,7 @@ impl ObjectImpl for HPETState {
         .pre_save(&HPETState::pre_save)
         .post_load(&HPETState::post_load)
         .fields(vmstate_fields! {
-            vmstate_of!(HPETState, config),
-            vmstate_of!(HPETState, int_status),
-            vmstate_of!(HPETState, counter),
+            vmstate_of!(HPETState, regs),
             vmstate_of!(HPETState, num_timers_save),
             vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
             vmstate_of!(HPETState, timers[0 .. num_timers_save], HPETState::validate_num_timers).with_version_id(0),
-- 
2.52.0