Re: [PATCH 2/5] rust/hpet: move hpet_offset to HPETRegisters

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

On Mon, Nov 17, 2025 at 09:47:49AM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Nov 2025 09:47:49 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 2/5] rust/hpet: move hpet_offset to HPETRegisters
> X-Mailer: git-send-email 2.51.1
> 
> Likewise, do not separate hpet_offset from the other registers.
> However, because it is migrated in a subsection it is necessary
> to copy it out of HPETRegisters and into a BqlCell<>.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/device.rs | 63 ++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 28 deletions(-)

,,,

> +        let mut next_tick: u64 = tn_regs.cmp64;
>          if tn_regs.is_32bit_mod() {
>              // HPET spec says in one-shot 32-bit mode, generate an interrupt when
>              // counter wraps in addition to an interrupt with comparator match.
>              if !tn_regs.is_periodic() && tn_regs.cmp64 > hpet_next_wrap(cur_tick) {
>                  tn_regs.wrap_flag = 1;
> -                self.arm_timer(tn_regs, hpet_next_wrap(cur_tick));
> -                return;
> +                next_tick = hpet_next_wrap(cur_tick);
>              }
>          }
> -        self.arm_timer(tn_regs, tn_regs.cmp64);
> +        self.arm_timer(regs, next_tick);
>      }

Good! This saves a arm_timer().

...

>  impl HPETRegisters {
> +    fn get_ticks(&self) -> u64 {
> +        // Protect hpet_offset in lockless IO case which would not lock BQL.

Just nit, this comment seems not much necessary, since currently there's
no Mutex lock to represent "Protect". But it's up to you to keep it or
not.

> +        ns_to_ticks(CLOCK_VIRTUAL.get_ns() + self.hpet_offset)
> +    }
> +
> +    fn get_ns(&self, tick: u64) -> u64 {
> +        // Protect hpet_offset in lockless IO case which would not lock BQL.

Ditto.

> +        ticks_to_ns(tick) - self.hpet_offset
> +    }

LGTM,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>