Re: [RFC 11/13] rust/timer/hpet: add basic HPET timer & state

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

> > +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