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

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/5/24 07:07, Zhao Liu wrote:
> Add the HPETTimer and HPETState (HPET timer block), along with their
> basic methods and register definitions.
> 
> This is in preparation for supporting the QAPI interfaces.
> 
> Note, wrap all items in HPETState that may be changed in the callback
> called by C code into the BqlCell/BqlRefCell.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   rust/hw/timer/hpet/src/hpet.rs | 638 +++++++++++++++++++++++++++++++++
>   rust/hw/timer/hpet/src/lib.rs  |   1 +
>   rust/wrapper.h                 |   1 +
>   3 files changed, 640 insertions(+)
>   create mode 100644 rust/hw/timer/hpet/src/hpet.rs
> 
> diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
> new file mode 100644
> index 000000000000..9550d8fe438a
> --- /dev/null
> +++ b/rust/hw/timer/hpet/src/hpet.rs
> @@ -0,0 +1,638 @@
> +// Copyright (C) 2024 Intel Corporation.
> +// Author(s): Zhao Liu <zhai1.liu@intel.com>
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#![allow(dead_code)]
> +
> +use core::ptr::{null_mut, NonNull};
> +use std::os::raw::c_int;
> +
> +use qemu_api::{
> +    bindings::*,

Let's avoid bindings::*.

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

> +    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().

> +
> +    fn is_int_active(&self) -> bool {
> +        self.get_state_ref().int_status.get() & (1 << self.index) != 0
> +    }
> +
> +    fn is_fsb_route_enabled(&self) -> bool {
> +        self.config & 1 << HPET_TN_CFG_FSB_ENABLE_SHIFT != 0
> +    }
> +
> +    fn is_periodic(&self) -> bool {
> +        self.config & 1 << HPET_TN_CFG_PERIODIC_SHIFT != 0
> +    }
> +
> +    fn is_int_enabled(&self) -> bool {
> +        self.config & 1 << HPET_TN_CFG_INT_ENABLE_SHIFT != 0
> +    }
> +
> +    fn is_32bit_mod(&self) -> bool {
> +        self.config & 1 << HPET_TN_CFG_32BIT_SHIFT != 0
> +    }
> +
> +    fn is_valset_enabled(&self) -> bool {
> +        self.config & 1 << HPET_TN_CFG_SETVAL_SHIFT != 0
> +    }
> +
> +    fn clear_valset(&mut self) {
> +        self.config &= !(1 << HPET_TN_CFG_SETVAL_SHIFT);
> +    }
> +
> +    /// 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.


> +    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().

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

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

> +    fn del_timer(&mut self) {
> +        self.qemu_timer.as_mut().timer_del();

Same as above.

> +#[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()
}

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

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

> +    /// Instance id (HPET timer block ID).
> +    hpet_id: BqlCell<usize>,
> +}
> +
Like flags this does not need to be a cell.

Paolo