Re: [PATCH 08/10] rust/timer/hpet: add basic HPET timer and HPETState

[Paolo Bonzini <pbonzini@redhat.com>]

On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> +// Register space for each timer block. (HPET_BASE isn't defined here.)
> +const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes

Use doc comments "///"...

> +// Timer N FSB Interrupt Route Register (masked by 0x18)
> +const HPET_TN_FSB_ROUTE_REG: u64 = 0x010;

... all the way to here.

> +/// HPET Timer Abstraction
> +#[repr(C)]
> +#[derive(Debug, Default, qemu_api_macros::offsets)]
> +pub struct HPETTimer {
> +    /// timer N index within the timer block (HPETState)
> +    #[doc(alias = "tn")]
> +    index: usize,
> +    qemu_timer: Option<Box<QEMUTimer>>,
> +    /// timer block abstraction containing this timer
> +    state: Option<NonNull<HPETState>>,
> +
> +    /// Memory-mapped, software visible timer registers

These "headings" cannot be doc comments, because they would be attached
to the field after.  Same below:

-    /// Hidden register state
+    // Hidden register state

-    /// HPET block Registers: Memory-mapped, software visible registers
+    // HPET block Registers: Memory-mapped, software visible registers

-    /// Internal state
+    // Internal state

> +    fn get_state(&self) -> &HPETState {
> +        // SAFETY:
> +        // the pointer is convertible to a reference
> +        unsafe { self.state.unwrap().as_ref() }
> +    }

Note for the future: I looked briefly into why a NonNull<> is needed
and the reasons are two.  First, the offset_of! replacement does not
support lifetime parameters.  Second, it's messy to pass a &HPETState
to the &HPETTimer, because the HPETTimer still has to be written into
the HPETState.  This may be worth revisiting when QEMU starts using
pinned-init is added, but for now is self-contained.

> +        if let Some(timer) = self.qemu_timer.as_mut() {
> +            timer.timer_mod(self.last);
> +        }

I think this can be just "timer.unwrap().timer_mod(self.last)" (not
sure if it needs an as_ref() too).

Paolo