Like timer there are just a couple nits here.
On Thu, Jan 16, 2025 at 3:45 AM Zhao Liu <zhao1....@intel.com> wrote:
> * gpio_in and gpio_out:
>
> /// Trait for methods of [`DeviceState`] and its subclasses.
> pub trait DeviceMethods: ObjectDeref
> where
> Self::Target: IsA<DeviceState>,
> {
> fn init_gpio_in<F>(&self, lines_num: u32, _f: F)
num_lines :)
> where
> F: for<'a> FnCall<(&'a Self::Target, u32, u32)>,
> {
> unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T,
> u32, u32)>>(
> opaque: *mut c_void,
> lines_num: c_int,
"line" instead of lines_num.
> unsafe {
> qdev_init_gpio_in(
> self.upcast::<DeviceState>() as *const DeviceState as *mut
> DeviceState,
I think you can use self.as_mut_ptr::<DeviceState>() or something like that.
> assert!(pins.len() > 0);
!pins.is_empty(). But I am not sure it's needed...
>
> unsafe {
> qdev_init_gpio_out(
> self.upcast::<DeviceState>() as *const DeviceState as *mut
> DeviceState,
> pins[0].as_ptr(),
> pins.len() as c_int,
... if you use instead pins.as_ptr() without the initial dereference.
> impl HPETState {
> ...
>
> fn handle_legacy_irq(&self, irq: u32, level: u32) {
> if irq == HPET_LEGACY_PIT_INT {
> if !self.is_legacy_mode() {
> self.irqs[0].set(level != 0);
> }
> } else {
> self.rtc_irq_level.set(level as u8);
Any reason why you defined rtc_irq_level as InterruptSource<u8>
instead of InterruptSource<u32>?
> fn realize(&self) {
> ...
> self.init_gpio_in(2, HPETState::handle_legacy_irq);
> self.init_gpio_out(from_ref(&self.pit_enabled));
> }
> }
>
> ---
>
> I made the handler accept the inmuttable reference, but init_gpio_in()
> is called in realize(), which now accepts the `&mut self`.
>
> I think init_gpio_in() should be called in realize() so that realize()
> needs to become safe in advance (before your plan).
>
> The safe realize() mainly affects pl011, and before you formally
> introduce char binding, if you don't mind, I can make this change to
> pl011:
>
> - pub fn realize(&mut self) {
> + pub fn realize(&self) {
> // SAFETY: self.char_backend has the correct size and alignment for a
> // CharBackend object, and its callbacks are of the correct types.
> unsafe {
> qemu_chr_fe_set_handlers(
> - addr_of_mut!(self.char_backend),
> + addr_of!(self.char_backend) as *mut CharBackend,
> Some(pl011_can_receive),
> Some(pl011_receive),
> Some(pl011_event),
> None,
> - addr_of_mut!(*self).cast::<c_void>(),
> + addr_of!(*self).cast::<c_void>() as *mut c_void,
> core::ptr::null_mut(),
> true,
> );
That's fine, yes.
Paolo
