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

[Paolo Bonzini <pbonzini@redhat.com>]

On 3/4/25 10:13, Zhao Liu wrote:
>> -    const fn as_mut_ptr(&self) -> *mut Self {
>> -        self as *const Timer as *mut _
>> +    /// Create a `Timer` struct without initializing it.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The timer must be initialized before it is armed with
>> +    /// [`modify`](Self::modify).
>> +    pub unsafe fn new() -> Self {
>> +        // SAFETY: requirements relayed to callers of Timer::new
>> +        Self(unsafe { Opaque::zeroed() })
> 
> We should use Opaque::uninit()? Because MaybeUninit::<bindings::QEMUTimer>::zeroed()
> marks the timer as initialized, which disables MaybeUninit's ability to check for
> initialization. e.g.,

While neither is good, a zeroed area of memory behaves better than an 
uninitialized one...  In particular, Drop calls timer_del() which works 
fine with a zeroed QEMUTimer.  With Opaque::uninit() you could have a 
crash just with

     drop(Timer::new());

> // No compiling error or runtime panic
> let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
> let _t = unsafe { t.assume_init() };
> 
> Further more, I spent some time trying to figure out if MaybeUninit in
> Opaque<> could help identify UB caused by uninitialized Timer, but I found
> it doesn't work. :-(
> 
> // No compiling error or runtime panic
> let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
> let _v = unsafe { v.get_mut().assume_init() };
> 
> But when I adjust MaybeUninit as the outer wrapper, the UB check can
> work:
> 
> // Runtime panic: Illegal instruction
> let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
> let _v = unsafe { v.assume_init() };
> 
> Compared with linux's Opaque, it also puts MaybeUninit on the outermost
> layer.

Yes, I admit I just copied what Linux does. :)

> And there's another example:
> 
> https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get
> 
> Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
> mutability is expected... but this layout breaks MaybeUninit's functionality.

Thanks for the example from the documentation!  Indeed it should be 
possible to do

     /// Returns a raw mutable pointer to the opaque data.
     pub const fn as_mut_ptr(&self) -> *mut T {
         UnsafeCell::raw_get(self.value.as_ptr())
     }

     /// Returns a raw pointer to the opaque data that can be passed to a
     /// C function as `void *`.
     pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
         self.as_mut_ptr().cast()
     }

     pub const fn raw_get(slot: *const Self) -> *mut T {
         // SAFETY: even if uninitialized, slot points to a MaybeUninit
         let slot = slot.cast::<MaybeUninit<UnsafeCell<T>>>;
         UnsafeCell::raw_get(slot.as_ptr())
     }

if Opaque<> uses a MaybeUninit<UnsafeCell<T>>.  I'm a bit worried of 
deviating from what Linux does though...

Paolo