Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support

[Paolo Bonzini]

On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>      fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {

This can be &self.

>          let shift: u64 = (addr & 4) * 8;
>
> +        match addr {
> +            HPET_TN_CFG_REG => self.config >> shift, // including interrupt 
> capabilities

This needs to be "match addr & !4".

> +            HPET_TN_CMP_REG => self.cmp >> shift,    // comparator register
> +            HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
> +            _ => {
> +                // TODO: Add trace point - trace_hpet_ram_read_invalid()
> +                // Reserved.
> +                0
> +            }
> +        }
> +    }
> +
> +    fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
> +        let shift = ((addr & 4) * 8) as u32;
> +        let len = std::cmp::min(size * 8, 64 - shift);
> +
> +        match addr {
> +            HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),

Likewise.

> +    fn write(&self, addr: hwaddr, value: u64, size: u32) {
> +        let shift = ((addr & 4) * 8) as u32;
> +        let len = std::cmp::min(size * 8, 64 - shift);
> +
> +        // TODO: Add trace point - trace_hpet_ram_write(addr, value)
> +        if (0x100..=0x3ff).contains(&addr
) {
> +            match self.timer_and_addr(addr) {
> +                None => return, // Reserved.

Clippy complains about an unnecessary return, just replace it with "()".

> +                Some((timer, addr)) => timer.borrow_mut().write(addr, value, 
> size),
> +            }

> +
> +    fn reset_hold(&self, _type: ResetType) {
> +        let sbd = self.upcast::<SysBusDevice>();
> +
> +        for timer in self.timers.iter().take(self.num_timers.get()) {
> +            timer.borrow_mut().reset();
> +        }
> +
> +        self.counter.set(0);
> +        self.config.set(0);
> +        self.pit_enabled.set(true);
> +        self.hpet_offset.set(0);
> +
> +        HPETFwConfig::update_hpet_cfg(
> +            self.hpet_id.get(),
> +            Some(self.capability.get() as u32),
> +            Some((*sbd).mmio[0].addr),
> +        );

This can be simply sbd.mmio[0].addr, without the (*...).

Also, you can change update_hpet_cfg to take arguments without the 
Option<> around them.

> +// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
> +#[repr(C)]
> +pub struct HPETClass {
> +    parent_class: <SysBusDevice as ObjectType>::Class,
> +}
> +
> +unsafe impl ObjectType for HPETState {
> +    type Class = HPETClass;
> +    const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> +}

No need for HPETClass (and for ClassInitImpl<HPETClass>), just do

unsafe impl ObjectType for HPETState {
    type Class = <SysBusDevice as ObjectType>::Class;
     const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
}

which is indeed more similar to OBJECT_DECLARE_SIMPLE_TYPE().

Paolo