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 = ®s.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> - 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>
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.
© 2016 - 2025 Red Hat, Inc.