[PATCH 16/22] rust/hpet: Pass &BqlRefCell<HPETRegisters> as argument during MMIO access

[Zhao Liu <zhao1.liu@intel.com>]

Currently in HPETTimer context, the global registers are accessed by
dereferring a HPETState raw pointer stored in NonNull<>, and then
borrowing the BqlRefCel<>.

This blocks borrowing HPETRegisters once during MMIO access, and
furthermore prevents replacing BqlRefCell<> with Mutex<>.

Therefore, do not access global registers through NonNull<HPETState>
and instead passing &BqlRefCell<HPETRegisters> as argument in
function calls within MMIO access.

But there's one special case that is timer handler, which still needs
to access HPETRegistsers through NonNull<HPETState>. It's okay for now
since this case doesn't have any repeated borrow() or lock reentrancy
issues.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 rust/hw/timer/hpet/src/device.rs | 160 +++++++++++++++++++------------
 1 file changed, 97 insertions(+), 63 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 1bef94e560f6..13e88df66d6f 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -177,7 +177,10 @@ const fn deactivating_bit(old: u64, new: u64, shift: usize) -> bool {
 }
 
 fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
-    timer_cell.borrow_mut().callback()
+    let mut t = timer_cell.borrow_mut();
+    // SFAETY: state field is valid after timer initialization.
+    let hpet_regs = &mut unsafe { t.state.as_mut() }.regs;
+    t.callback(hpet_regs)
 }
 
 #[repr(C)]
@@ -285,11 +288,8 @@ fn get_state(&self) -> &HPETState {
         unsafe { self.state.as_ref() }
     }
 
-    fn is_int_active(&self) -> bool {
-        self.get_state()
-            .regs
-            .borrow()
-            .is_timer_int_active(self.index.into())
+    fn is_int_active(&self, hpet_regs: &BqlRefCell<HPETRegisters>) -> bool {
+        hpet_regs.borrow().is_timer_int_active(self.index.into())
     }
 
     /// calculate next value of the general counter that matches the
@@ -307,8 +307,8 @@ fn calculate_cmp64(&self, cur_tick: u64, target: u64) -> u64 {
         }
     }
 
-    fn get_int_route(&self) -> usize {
-        if self.index <= 1 && self.get_state().regs.borrow().is_legacy_mode() {
+    fn get_int_route(&self, hpet_regs: &BqlRefCell<HPETRegisters>) -> usize {
+        if self.index <= 1 && hpet_regs.borrow().is_legacy_mode() {
             // If LegacyReplacement Route bit is set, HPET specification requires
             // timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
             // timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
@@ -332,10 +332,10 @@ fn get_int_route(&self) -> usize {
         }
     }
 
-    fn set_irq(&self, set: bool) {
-        let route = self.get_int_route();
+    fn set_irq(&self, hpet_regs: &BqlRefCell<HPETRegisters>, set: bool) {
+        let route = self.get_int_route(hpet_regs);
 
-        if set && self.regs.is_int_enabled() && self.get_state().regs.borrow().is_hpet_enabled() {
+        if set && self.regs.is_int_enabled() && hpet_regs.borrow().is_hpet_enabled() {
             if self.regs.is_fsb_route_enabled() {
                 // SAFETY:
                 // the parameters are valid.
@@ -358,13 +358,17 @@ fn set_irq(&self, set: bool) {
         }
     }
 
-    fn update_irq(&self, set: bool) {
+    fn update_irq(&self, hpet_regs: &BqlRefCell<HPETRegisters>, set: bool) {
+        let mut regs = hpet_regs.borrow_mut();
         // If Timer N Interrupt Enable bit is 0, "the timer will
         // still operate and generate appropriate status bits, but
         // will not cause an interrupt"
-        self.get_state()
-            .update_int_status(self.index.into(), set && self.regs.is_int_level_triggered());
-        self.set_irq(set);
+        regs.int_status = regs.int_status.deposit(
+            self.index.into(),
+            1,
+            u64::from(set && self.regs.is_int_level_triggered()),
+        );
+        self.set_irq(hpet_regs, set);
     }
 
     fn arm_timer(&mut self, tick: u64) {
@@ -396,20 +400,26 @@ fn set_timer(&mut self) {
         self.arm_timer(self.cmp64);
     }
 
-    fn del_timer(&self) {
+    fn del_timer(&self, hpet_regs: &BqlRefCell<HPETRegisters>) {
         // Just remove the timer from the timer_list without destroying
         // this timer instance.
         self.qemu_timer.delete();
 
-        if self.is_int_active() {
+        if self.is_int_active(hpet_regs) {
             // For level-triggered interrupt, this leaves interrupt status
             // register set but lowers irq.
-            self.update_irq(true);
+            self.update_irq(hpet_regs, true);
         }
     }
 
     /// Configuration and Capability Register
-    fn set_tn_cfg_reg(&mut self, shift: u32, len: u32, val: u64) {
+    fn set_tn_cfg_reg(
+        &mut self,
+        hpet_regs: &BqlRefCell<HPETRegisters>,
+        shift: u32,
+        len: u32,
+        val: u64,
+    ) {
         // TODO: Add trace point - trace_hpet_ram_write_tn_cfg(addr & 4)
         let old_val: u64 = self.regs.config;
         let mut new_val: u64 = old_val.deposit(shift, len, val);
@@ -419,13 +429,15 @@ fn set_tn_cfg_reg(&mut self, shift: u32, len: u32, val: u64) {
         if deactivating_bit(old_val, new_val, HPET_TN_CFG_INT_TYPE_SHIFT) {
             // Do this before changing timer.regs.config; otherwise, if
             // HPET_TN_FSB is set, update_irq will not lower the qemu_irq.
-            self.update_irq(false);
+            self.update_irq(hpet_regs, false);
         }
 
         self.regs.config = new_val;
 
-        if activating_bit(old_val, new_val, HPET_TN_CFG_INT_ENABLE_SHIFT) && self.is_int_active() {
-            self.update_irq(true);
+        if activating_bit(old_val, new_val, HPET_TN_CFG_INT_ENABLE_SHIFT)
+            && self.is_int_active(hpet_regs)
+        {
+            self.update_irq(hpet_regs, true);
         }
 
         if self.regs.is_32bit_mod() {
@@ -433,13 +445,19 @@ fn set_tn_cfg_reg(&mut self, shift: u32, len: u32, val: u64) {
             self.period = u64::from(self.period as u32); // truncate!
         }
 
-        if self.get_state().regs.borrow().is_hpet_enabled() {
+        if hpet_regs.borrow().is_hpet_enabled() {
             self.set_timer();
         }
     }
 
     /// Comparator Value Register
-    fn set_tn_cmp_reg(&mut self, shift: u32, len: u32, val: u64) {
+    fn set_tn_cmp_reg(
+        &mut self,
+        hpet_regs: &BqlRefCell<HPETRegisters>,
+        shift: u32,
+        len: u32,
+        val: u64,
+    ) {
         let mut length = len;
         let mut value = val;
 
@@ -463,18 +481,24 @@ fn set_tn_cmp_reg(&mut self, shift: u32, len: u32, val: u64) {
         }
 
         self.regs.clear_valset();
-        if self.get_state().regs.borrow().is_hpet_enabled() {
+        if hpet_regs.borrow().is_hpet_enabled() {
             self.set_timer();
         }
     }
 
     /// FSB Interrupt Route Register
-    fn set_tn_fsb_route_reg(&mut self, shift: u32, len: u32, val: u64) {
+    fn set_tn_fsb_route_reg(
+        &mut self,
+        _hpet_regs: &BqlRefCell<HPETRegisters>,
+        shift: u32,
+        len: u32,
+        val: u64,
+    ) {
         self.regs.fsb = self.regs.fsb.deposit(shift, len, val);
     }
 
-    fn reset(&mut self) {
-        self.del_timer();
+    fn reset(&mut self, hpet_regs: &BqlRefCell<HPETRegisters>) {
+        self.del_timer(hpet_regs);
         self.regs.cmp = u64::MAX; // Comparator Match Registers reset to all 1's.
         self.regs.config =
             (1 << HPET_TN_CFG_PERIODIC_CAP_SHIFT) | (1 << HPET_TN_CFG_SIZE_CAP_SHIFT);
@@ -489,7 +513,7 @@ fn reset(&mut self) {
     }
 
     /// timer expiration callback
-    fn callback(&mut self) {
+    fn callback(&mut self, hpet_regs: &BqlRefCell<HPETRegisters>) {
         let period: u64 = self.period;
         let cur_tick: u64 = self.get_state().get_ticks();
 
@@ -507,10 +531,10 @@ fn callback(&mut self) {
             self.wrap_flag = 0;
             self.arm_timer(self.cmp64);
         }
-        self.update_irq(true);
+        self.update_irq(hpet_regs, true);
     }
 
-    const fn read(&self, target: TimerRegister) -> u64 {
+    const fn read(&self, target: TimerRegister, _hpet_regs: &BqlRefCell<HPETRegisters>) -> u64 {
         use TimerRegister::*;
         match target {
             CFG => self.regs.config, // including interrupt capabilities
@@ -519,12 +543,19 @@ const fn read(&self, target: TimerRegister) -> u64 {
         }
     }
 
-    fn write(&mut self, target: TimerRegister, value: u64, shift: u32, len: u32) {
+    fn write(
+        &mut self,
+        target: TimerRegister,
+        hpet_regs: &BqlRefCell<HPETRegisters>,
+        value: u64,
+        shift: u32,
+        len: u32,
+    ) {
         use TimerRegister::*;
         match target {
-            CFG => self.set_tn_cfg_reg(shift, len, value),
-            CMP => self.set_tn_cmp_reg(shift, len, value),
-            ROUTE => self.set_tn_fsb_route_reg(shift, len, value),
+            CFG => self.set_tn_cfg_reg(hpet_regs, shift, len, value),
+            CMP => self.set_tn_cmp_reg(hpet_regs, shift, len, value),
+            ROUTE => self.set_tn_fsb_route_reg(hpet_regs, shift, len, value),
         }
     }
 }
@@ -643,38 +674,33 @@ fn init_timers(this: &mut MaybeUninit<Self>) {
         }
     }
 
-    fn update_int_status(&self, index: u32, level: bool) {
-        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.regs.borrow().config;
+    fn set_cfg_reg(&self, regs: &BqlRefCell<HPETRegisters>, shift: u32, len: u32, val: u64) {
+        let old_val = 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.regs.borrow_mut().config = new_val;
+        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.regs.borrow().counter) - CLOCK_VIRTUAL.get_ns());
+                .set(ticks_to_ns(regs.borrow().counter) - CLOCK_VIRTUAL.get_ns());
 
             for timer in self.timers.iter().take(self.num_timers) {
                 let mut t = timer.borrow_mut();
 
-                if t.regs.is_int_enabled() && t.is_int_active() {
-                    t.update_irq(true);
+                if t.regs.is_int_enabled() && t.is_int_active(regs) {
+                    t.update_irq(regs, true);
                 }
                 t.set_timer();
             }
         } else if deactivating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
             // Halt main counter and disable interrupt generation.
-            self.regs.borrow_mut().counter = self.get_ticks();
+            regs.borrow_mut().counter = self.get_ticks();
 
             for timer in self.timers.iter().take(self.num_timers) {
-                timer.borrow().del_timer();
+                timer.borrow().del_timer(regs);
             }
         }
 
@@ -692,20 +718,26 @@ 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) {
+    fn set_int_status_reg(
+        &self,
+        regs: &BqlRefCell<HPETRegisters>,
+        shift: u32,
+        _len: u32,
+        val: u64,
+    ) {
         let new_val = val << shift;
-        let cleared = new_val & self.regs.borrow().int_status;
+        let cleared = new_val & regs.borrow().int_status;
 
         for (index, timer) in self.timers.iter().take(self.num_timers).enumerate() {
             if cleared & (1 << index) != 0 {
-                timer.borrow().update_irq(false);
+                timer.borrow().update_irq(regs, false);
             }
         }
     }
 
     /// Main Counter Value Register
-    fn set_counter_reg(&self, shift: u32, len: u32, val: u64) {
-        let mut regs = self.regs.borrow_mut();
+    fn set_counter_reg(&self, regs: &BqlRefCell<HPETRegisters>, shift: u32, len: u32, val: u64) {
+        let mut regs = regs.borrow_mut();
         if regs.is_hpet_enabled() {
             // TODO: Add trace point -
             // trace_hpet_ram_write_counter_write_while_enabled()
@@ -786,7 +818,7 @@ fn realize(&self) -> util::Result<()> {
 
     fn reset_hold(&self, _type: ResetType) {
         for timer in self.timers.iter().take(self.num_timers) {
-            timer.borrow_mut().reset();
+            timer.borrow_mut().reset(&self.regs);
         }
 
         // pit_enabled.set(true) will call irq handler and access regs
@@ -838,16 +870,17 @@ fn decode(&self, mut addr: hwaddr, size: u32) -> HPETAddrDecode<'_> {
     fn read(&self, addr: hwaddr, size: u32) -> u64 {
         // TODO: Add trace point - trace_hpet_ram_read(addr)
         let HPETAddrDecode { shift, target, .. } = self.decode(addr, size);
+        let regs = &self.regs;
 
         use DecodedRegister::*;
         use GlobalRegister::*;
         (match target {
-            Timer(timer, tn_target) => timer.borrow_mut().read(tn_target),
-            Global(CAP) => self.regs.borrow().capability, /* including HPET_PERIOD 0x004 */
-            Global(CFG) => self.regs.borrow().config,
-            Global(INT_STATUS) => self.regs.borrow().int_status,
+            Timer(timer, tn_target) => timer.borrow_mut().read(tn_target, regs),
+            Global(CAP) => regs.borrow().capability, /* including HPET_PERIOD 0x004 */
+            Global(CFG) => regs.borrow().config,
+            Global(INT_STATUS) => regs.borrow().int_status,
             Global(COUNTER) => {
-                let regs = self.regs.borrow();
+                let regs = regs.borrow();
                 // TODO: Add trace point
                 // trace_hpet_ram_read_reading_counter(addr & 4, cur_tick)
                 if regs.is_hpet_enabled() {
@@ -865,16 +898,17 @@ fn read(&self, addr: hwaddr, size: u32) -> u64 {
 
     fn write(&self, addr: hwaddr, value: u64, size: u32) {
         let HPETAddrDecode { shift, len, target } = self.decode(addr, size);
+        let regs = &self.regs;
 
         // TODO: Add trace point - trace_hpet_ram_write(addr, value)
         use DecodedRegister::*;
         use GlobalRegister::*;
         match target {
-            Timer(timer, tn_target) => timer.borrow_mut().write(tn_target, value, shift, len),
+            Timer(timer, tn_target) => timer.borrow_mut().write(tn_target, regs, value, shift, len),
             Global(CAP) => {} // General Capabilities and ID Register: Read Only
-            Global(CFG) => self.set_cfg_reg(shift, len, value),
-            Global(INT_STATUS) => self.set_int_status_reg(shift, len, value),
-            Global(COUNTER) => self.set_counter_reg(shift, len, value),
+            Global(CFG) => self.set_cfg_reg(regs, shift, len, value),
+            Global(INT_STATUS) => self.set_int_status_reg(regs, shift, len, value),
+            Global(COUNTER) => self.set_counter_reg(regs, shift, len, value),
             Unknown(_) => {
                 // TODO: Add trace point - trace_hpet_ram_write_invalid()
             }
-- 
2.34.1