| Message ID | 20241205060714.256270-5-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Reinvent the wheel for HPET timer in Rust | expand |
On 12/5/24 07:07, Zhao Liu wrote: > The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to > wrap them as DeviceState's methods in Rust API, which could eliminate > unsafe cases in the device lib. > > Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl. > > In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust > callback into a C-style callback qemu_irq_handler, add a handler pointer > member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it > needs to define a handler. And for device which just wants to initialize > GPIO out, it can leave the GPIO_IRQ_HANDLER as None. This has the same issue as timers, in that you could have (especially once someone adds named GPIOs) multiple handlers. So we need the same kind of Fn-based thing here too. > +/// Trait that defines the irq handler for GPIO in. > +pub trait DeviceGPIOImpl { > + const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None; Ah, I see that you're placing the qdev_init_gpio_in here so that you only make that accessible for devices that did implement DeviceGPIOImpl. However you are not guaranteeing that this _is_ a DeviceState. If the handler can be passed as a function, the problem of getting the GPIO_INT_HANDLER does not exist anymore. So with the code in rust-next you can add these to a trait like /// 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) { } } impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {} > + fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) { > + unsafe { > + qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int); > + } > + } > +} Pass a slice &[InterruptSource], and get the "len" from the length of the slice. Paolo
On Thu, Dec 05, 2024 at 07:53:42PM +0100, Paolo Bonzini wrote: > Date: Thu, 5 Dec 2024 19:53:42 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization > > On 12/5/24 07:07, Zhao Liu wrote: > > The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to > > wrap them as DeviceState's methods in Rust API, which could eliminate > > unsafe cases in the device lib. > > > > Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl. > > > > In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust > > callback into a C-style callback qemu_irq_handler, add a handler pointer > > member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it > > needs to define a handler. And for device which just wants to initialize > > GPIO out, it can leave the GPIO_IRQ_HANDLER as None. > > This has the same issue as timers, in that you could have (especially once > someone adds named GPIOs) multiple handlers. So we need the same kind of > Fn-based thing here too. I will refer to the timer callback prototype you suggested and try that way. Will you rework the timer binding soon? (I'm sorry for bringing such burden to you). > > +/// Trait that defines the irq handler for GPIO in. > > +pub trait DeviceGPIOImpl { > > + const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None; > > Ah, I see that you're placing the qdev_init_gpio_in here so that you > only make that accessible for devices that did implement DeviceGPIOImpl. > However you are not guaranteeing that this _is_ a DeviceState. Thank you, I really couldn't think of a good way to implement the DeviceState method...One reason is that DeviceImpl is a bit confusing to me, and please see the comment below. > If the handler can be passed as a function, the problem of getting the > GPIO_INT_HANDLER does not exist anymore. So with the code in rust-next you > can add these to a trait like > > /// 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) { > } > } > > impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> > {} > Thank you for your idea! This is a true implementation of the DeviceState method. I'll try this way! Additionally, the current DeviceImpl trait is quite special. Although in Rust, DeviceImpl traits are implemented for device states, DeviceImpl is actually used for device classes. Semantically, it might be more natural for DeviceImpl to be a trait for device classes. However, parameters of its methods are DeviceState, so it makes sense as a trait for states in Rust. This seems to be a different design before C and Rust Qdev. > > + fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) { > > + unsafe { > > + qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int); > > + } > > + } > > +} > > Pass a slice &[InterruptSource], and get the "len" from the length of the > slice. Thanks! Will change this. Regards, Zhao
On Sun, Dec 8, 2024 at 5:09 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > This has the same issue as timers, in that you could have (especially once > > someone adds named GPIOs) multiple handlers. So we need the same kind of > > Fn-based thing here too. > > I will refer to the timer callback prototype you suggested and try that > way. Will you rework the timer binding soon? (I'm sorry for bringing such > burden to you). No, I have written a utility that can be used for all callbacks but I'll leave it to you to use it for timers. Both because you have already started the work, and because it helps if one person writes the code and one uses it. > Additionally, the current DeviceImpl trait is quite special. Although in > Rust, DeviceImpl traits are implemented for device states, DeviceImpl is > actually used for device classes. > > Semantically, it might be more natural for DeviceImpl to be a trait for > device classes. However, parameters of its methods are DeviceState, so > it makes sense as a trait for states in Rust. > > This seems to be a different design before C and Rust Qdev. I agree that there are differences in how you write the code, due to the fact that Rust supports associating functions and traits to a struct, while C only has a global namespace. Also, functions in a trait can look like both "normal" and static methods, so it's easier to place all functions in DeviceState. The DeviceClass part is mostly automatic. So if Xyz is a struct corresponding to a QOM type, it will: - include a field of type Abc corresponding to the direct superclass - implement virtual methods for all superclasses through traits such as AbcImpl or DefImpl, up to ObjectImpl - expose its virtual methods to C thorough a blanket implementation of ClassInitImpl<AbcClass> or ClassInitImpl<DefClass> - invoke methods through blanket implementations of AbcMethods, AbcClassMethods etc. for all superclasses and the structure of all the blanket implementation is always the same: pub trait DeviceClassMethods: IsA<DeviceState> {...} impl<T> DeviceClassMethods for T where T: IsA<DeviceState> {} pub trait DeviceMethods: ObjectDeref where Self::Target: IsA<DeviceState>, {...} impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {} impl<T> ClassInitImpl<DeviceClass> for T where T: ClassInitImpl<ObjectClass> + DeviceImpl {...} In the future, developers will not need to worry much about these, but for some time every new device will probably need a few new functions or even modules in qemu_api. Paolo
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs index 23a06b377b2c..5e6580b6f261 100644 --- a/rust/qemu-api/src/qdev.rs +++ b/rust/qemu-api/src/qdev.rs @@ -4,12 +4,17 @@ //! Bindings to create devices and access device functionality from Rust. -use std::ffi::CStr; +use std::{ + ffi::CStr, + os::raw::{c_int, c_void}, + ptr::{addr_of, NonNull}, +}; pub use bindings::{DeviceClass, DeviceState, Property}; use crate::{ - bindings::{self, Error}, + bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error}, + irq::InterruptSource, qom::{ClassInitImpl, Object, ObjectClass, ObjectType}, qom_isa, vmstate::VMStateDescription, @@ -144,3 +149,49 @@ unsafe impl ObjectType for DeviceState { unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) }; } qom_isa!(DeviceState: Object); + +/// # Safety +/// +/// We expect the FFI user of this function to pass a valid pointer that +/// can be downcasted to type `T`. We also expect the device is +/// readable/writeable from one thread at any time. +/// +/// Note: Always assume opaque is referred to the self DeviceState, and +/// this is also the most common case in QEMU. +unsafe extern "C" fn rust_irq_handler<T: DeviceGPIOImpl>( + opaque: *mut c_void, + lines_num: c_int, + level: c_int, +) { + // SAFETY: + // the pointer is convertible to a reference + let state = unsafe { NonNull::new(opaque.cast::<T>()).unwrap().as_mut() }; + + T::GPIO_IRQ_HANDLER.unwrap()(state, lines_num as u32, level as u32); +} + +/// Trait that defines the irq handler for GPIO in. +pub trait DeviceGPIOImpl { + const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None; + + fn init_gpio_in(&self, lines_num: u32) + where + Self: Sized, + { + assert!(Self::GPIO_IRQ_HANDLER.is_some()); + + unsafe { + qdev_init_gpio_in( + addr_of!(*self) as *mut _, + Some(rust_irq_handler::<Self>), + lines_num as c_int, + ); + } + } + + fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) { + unsafe { + qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int); + } + } +}
The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to wrap them as DeviceState's methods in Rust API, which could eliminate unsafe cases in the device lib. Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl. In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust callback into a C-style callback qemu_irq_handler, add a handler pointer member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it needs to define a handler. And for device which just wants to initialize GPIO out, it can leave the GPIO_IRQ_HANDLER as None. Then device could use init_gpio_in() and init_gpio_out() to initialize GPIO in and out, like C code. Note, for qemu_irq_handler, assume the opaque parameter refers to the self DeviceState, and this is enough as for now, as it's the most common case in QEMU. Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- rust/qemu-api/src/qdev.rs | 55 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-)