On Wed, Jan 29, 2025 at 11:57:18AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:57:18 +0100
> From: Paolo Bonzini <pbonz...@redhat.com>
> Subject: Re: [PATCH 08/10] rust/timer/hpet: add basic HPET timer and
> HPETState
>
>
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1....@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 "///"...
OK,
> > +// Timer N FSB Interrupt Route Register (masked by 0x18)
> > +const HPET_TN_FSB_ROUTE_REG: u64 = 0x010;
>
> ... all the way to here.
Done.
> > +/// 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
Fixed. Thanks!
> > + 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.
Yes, and the second use would be a common case in practice. So maybe we
need some wrapper on this, to provide someting like HPETTimer::get_state(),
which returns a reference of the parent structure.
In addition, for underlying library, NonNull<> is nice thinks to its low
overhead. But for device implementation, I'm not sure if it's the good
choice. Rust uses RefCell<Weak<>> (for QEMU, it could be BqlRefCell<Weak<>>)
to handle this cycle reference case.
As I mentioned in RFC,
The device instance is not created in Rust side, and there's
only init() method to initialize created device instance. This
way, it's hard to be compatible with the pattern of creating
weak references (at least I failed).
To address this issue, the method `from` or `from_raw` seems necessary,
like `Owned::from`, but it's tricky... So I don't have a good idea to
replace NonNull<> yet!
> 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).
>
Yes, it needs as_ref(). A direct unwrap() consumes the Some() since
inter type of qemu_timer (Box<Timer>) is non-`Copy`.
Another example is
fn get_state(&self) -> &HPETState {
// SAFETY:
// the pointer is convertible to a reference
unsafe { self.state.unwrap().as_ref() }
}
Because the state (Option<NonNull<HPETState>>) has `Copy` so that this
case doesn't need as_ref before unwrap.
Thanks,
Zhao
