Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements

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

On Mon, Mar 03, 2025 at 03:51:25PM +0100, Paolo Bonzini wrote:
> Date: Mon, 3 Mar 2025 15:51:25 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
>  express pinning requirements
> 
> On 3/3/25 15:28, Zhao Liu wrote:
> > > -    pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> > > -        &'timer mut self,
> > > +    pub fn new_full<'opaque, T, F>(
> > >           timer_list_group: Option<&TimerListGroup>,
> > >           clk_type: ClockType,
> > >           scale: u32,
> > >           attributes: u32,
> > >           _cb: F,
> > >           opaque: &'opaque T,
> > > -    ) where
> > > +    ) -> Pin<Box<Self>>
> > >           F: for<'a> FnCall<(&'a T,)>,
> > >       {
> > 
> > Ah, the lifetime here isn't effectively bound... However, I also
> > referred to your latest code [1] :), and it seems that this issue
> > has already been fixed. (Nit: The code still has a complaint from
> > `cargo fmt`)
> 
> I am not sure if the change I have in that commit actually does anything,
> unfortunately... :( which is why I wanted to use init_full instead of
> new_full.

EMM, I tried with the below case...

diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index a440c9f4cb98..c167d69eef4c 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -190,14 +190,17 @@ fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
     }

     fn init_timer_with_state(&mut self) {
-        self.qemu_timer = Some(Timer::new_full(
-            None,
-            CLOCK_VIRTUAL,
-            Timer::NS,
-            0,
-            timer_handler,
-            &self.get_state().timers[self.index],
-        ));
+        {
+            let tmp = &self.get_state().timers[self.index];
+            self.qemu_timer = Some(Timer::new_full(
+                None,
+                CLOCK_VIRTUAL,
+                Timer::NS,
+                0,
+                timer_handler,
+                tmp,
+            ));
+        }
     }

     fn get_state(&self) -> &HPETState {

---

It can compile and seems lifetime doesn't work... I think if we want the
lifetime check, it would be necessary to store &opaque in Rust's Timer
wrapper and specify a lifetime for Timer.

Maybe we need something like (similar to MemoryRegionOps):

pub struct Timer<'timer, T> {
    Opaque<bindings::QEMUTimer>,
    PhantomData<&'timer T>,
}

> It's easiest to marked new_full() unsafe for now.

Yes, I agree.

Thanks,
Zhao