> > +use qemu_api::{ > > + bindings::*, > > Let's avoid bindings::*.
Sure. > > + self.qemu_timer = Box::new(HPETState::timer_new_ns( > > Oh! I noticed now that while your API is called timer_new_ns, it is actually > the same as timer_init_full. Let's call it init_full() then. Sure, will use init_full() directly. > > + fn get_state_ref(&self) -> &HPETState { > > + // SAFETY: > > + // the pointer is convertible to a reference > > + unsafe { self.state.unwrap().as_ref() } > > + } > > + > > + fn get_state_mut(&mut self) -> &mut HPETState { > > + // SAFETY: > > + // the pointer is convertible to a reference > > + unsafe { self.state.unwrap().as_mut() } > > + } > > You should not need get_state_mut(), which also has the advantage of > shortening get_state_ref() to get_state(). I see now, internal mutability requires using immutable references as much as possible! ... > > + /// True if timer interrupt is level triggered; otherwise, edge > > triggered. > > + fn is_int_level_triggered(&self) -> bool { > > + self.config & 1 << HPET_TN_CFG_INT_TYPE_SHIFT != 0 > > + } > > PL011 is using bilge here. I think it's fair to show the two ways to do it. > If we have devices showing two different things: > > - PL011 shows higher-level abstractions for registers > > - HPET has a good approach to interior mutability from the beginning > > Then it gives a clearer view of the options. Yes, I am used to handling registers in a C-style way, which is more straightforward and easier for me to use. However, compared to PL011, it has less abstraction so that it might look a bit messier. The choice of different options is basically a trade-off between different styles. :-) > > + fn update_int_status(&mut self, set: bool) -> &mut Self { > > + let mask: u64 = 1 << self.index; > > + > > + if set && self.is_int_level_triggered() { > > + // 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_mut().int_status.get_mut() |= mask; > > + } else { > > + *self.get_state_mut().int_status.get_mut() &= !mask; > > + } > > + self > > + } > > See remarks elsewhere on update_int_status(), and how it uses > get_state_mut() and get_mut(). Thank you! Will change as your example. > > + unsafe { > > + address_space_stl_le( > > + &mut address_space_memory, > > + self.fsb >> 32, // Timer N FSB int addr > > + self.fsb as u32, // Timer N FSB int value, > > truncate! > > + *MEMTXATTRS_UNSPECIFIED, > > + null_mut(), > > + ); > > + } > > This is the only use of unsafe, whic is not bad at all. Not urgent, but we > should think about the AddressSpace bindings, and whether it makes sense to > use (or steal APIs from) rust-vmm's vm-memory. Good idea, I agree that this is a good "starting" point to consider introducing rust-vmm to QEMU. I'll add it to my TODO list and consider it. Once I have some ideas, I'll share them. > > + fn arm_timer(&mut self, tick: u64) { > > + let mut ns = self.get_state_ref().get_ns(tick); > > + > > + // Clamp period to reasonable min value (1 us) > > + if self.is_periodic() && ns - self.last < 1000 { > > + ns = self.last + 1000; > > + } > > + > > + self.last = ns; > > + self.qemu_timer.as_mut().timer_mod(self.last); > > + } > > No as_mut(), timer_mod is thread safe. timer_mod() need to take &self. Ok, I understand then I need to implement a as_mut_ptr to convert `&self` to `* mut QEMUTimer`, as you did for SysBusDevice. > > + fn del_timer(&mut self) { > > + self.qemu_timer.as_mut().timer_del(); > > Same as above. I see, thanks. > > +#[derive(Debug)] > > +pub struct HPETTimerInstance(BqlRefCell<HPETTimer>); > > + > > +impl HPETTimerInstance { > > + fn timer_handler(timer: &mut HPETTimerInstance) { > > + timer.0.borrow_mut().callback() > > + } > > +} > > Also not "&mut" - you don't need it, as "timer.0" is only used to borrow > from the BqlRefCell. Also with a more refined timer abstraction this > doesn't need HPETTimerInstance, it can probably be a global function like > > fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) { > timer_cell.borrow_mut().callback() > } I see, it's clearer! > > + /// General Capabilities and ID Register > > + capability: BqlCell<u64>, > > + /// General Configuration Register > > + config: BqlCell<u64>, > > + /// General Interrupt Status Register > > + int_status: BqlCell<u64>, > > + /// Main Counter Value Register > > + counter: BqlCell<u64>, > > + > > + /// Internal state > > + > > + /// Capabilities that QEMU HPET supports. > > + /// bit 0: MSI (or FSB) support. > > + pub(crate) flags: BqlCell<u32>, > > flags doesn't need to be a cell (it's just a property). I'll send a patch > for the C code. Thank you! I've reviewed that patch. > > + /// Offset of main counter relative to qemu clock. > > + hpet_offset: BqlCell<u64>, > > + pub(crate) hpet_offset_saved: bool, > > + > > + irqs: [InterruptSource; HPET_NUM_IRQ_ROUTES], > > + rtc_irq_level: BqlCell<u8>, > > + pit_enabled: InterruptSource, > > + > > + /// Interrupt Routing Capability. > > + /// This field indicates to which interrupts in the I/O (x) APIC > > + /// the timers' interrupt can be routed, and is encoded in the > > + /// bits 32:64 of timer N's config register: > > + pub(crate) int_route_cap: u32, > > + > > + /// HPET timer array managed by this timer block. > > + timer: [HPETTimerInstance; HPET_MAX_TIMERS], > > + pub(crate) num_timers: BqlCell<usize>, > > Ah, this needs to be a BqlCell because it can be clamped to > MIN_TIMERS..MAX_TIMERS by realize. Fair enough. Yes, it's a corner case. Regards, Zhao