Re: [PATCH 3/5] rust/hpet: remove BqlRefCell around HPETTimer

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

> -    fn init_timer_with_cell(cell: &BqlRefCell<Self>) {
> -        let mut timer = cell.borrow_mut();
> -        // SAFETY: HPETTimer is only used as part of HPETState, which is
> -        // always pinned.
> -        let qemu_timer = unsafe { Pin::new_unchecked(&mut timer.qemu_timer) };
> -        qemu_timer.init_full(None, CLOCK_VIRTUAL, Timer::NS, 0, timer_handler, cell);
> +    fn init_timer(timer: Pin<&mut Self>) {
> +        Timer::init_full(
> +            timer,
> +            None,
> +            CLOCK_VIRTUAL,
> +            Timer::NS,
> +            0,
> +            timer_handler,
> +            |t| &mut t.qemu_timer,
> +        );
>      }

I find this way could also work for BqlRefCell case:

    fn init_timer_with_cell(cell: &mut BqlRefCell<Self>) {
        // SAFETY: HPETTimer is only used as part of HPETState, which is
        // always pinned.
        let timer = unsafe { Pin::new_unchecked(cell) };
        Timer::init_full(
            timer,
            None,
            CLOCK_VIRTUAL,
            Timer::NS,
            0,
            timer_handler,
            |t| {
                assert!(bql::is_locked());
                &mut t.get_mut().qemu_timer
            },
        );
    }


So any other non-lockless timer can also use this interface to
initialize their BqlRefCell<>.

(BTW, I find BqlRefCell::get_mut() / as_ref() missed bql::is_locked().
 right?)

...

> diff --git a/rust/util/src/timer.rs b/rust/util/src/timer.rs
> index c6b3e4088ec..829f52d111e 100644
> --- a/rust/util/src/timer.rs
> +++ b/rust/util/src/timer.rs
> @@ -45,14 +45,14 @@ impl Timer {
>      }
>  
>      /// Create a new timer with the given attributes.
> -    pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> -        self: Pin<&'timer mut Self>,
> +    pub fn init_full<T, F>(
> +        opaque: Pin<&mut T>,
>          timer_list_group: Option<&TimerListGroup>,
>          clk_type: ClockType,
>          scale: u32,
>          attributes: u32,
>          _cb: F,
> -        opaque: &'opaque T,
> +        field: impl FnOnce(&mut T) -> &mut Self,
>      ) where
>          F: for<'a> FnCall<(&'a T,)>,


This is more tricky than I previously imaged. Good solution!

Another way to handle this kind of 'self reference' issue would probably
be to consider passing raw pointers as opaque parameters... but that's
definitely not as clean/idiomatic as the current approach.

I think it may be better to add doc about how to use this for
BqlRefCell<> case since there'll be no example after this patch.


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