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

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

> 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());

Good point.

> > // 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. :)

Thanks for pointing this! I realized I referred the old code, since this
commit, linux puts the UnsafeCell to the outer layer [2]

[2]: https://github.com/torvalds/linux/commit/35cad617df2eeef8440a38e82bb2d81ae32ca50d

It seems that, at least from the Linux view, here the role of MaybeUninit
(as the cases I tested) is not a main concern, and Rust convention is
superior...

> > 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...

Thank you, this convertion to UnsafeCell<MaybeUninit<T>> in Linux
history convinces me... I also agree that we should follow it for now :-).

Regards,
Zhao