| Message ID | 20250206111514.2134895-2-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rust: pl011: convert pl011_create to safe Rust | expand |
On Thu, Feb 06, 2025 at 12:15:14PM +0100, Paolo Bonzini wrote: > Date: Thu, 6 Feb 2025 12:15:14 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH] rust: pl011: convert pl011_create to safe Rust > X-Mailer: git-send-email 2.48.1 > > Not a major change but, as a small but significant step in creating > qdev bindings, show how pl011_create can be written without "unsafe" > calls (apart from converting pointers to references). > > This also provides a starting point for creating Error** bindings. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++---------------- > rust/qemu-api/src/sysbus.rs | 34 +++++++++++++++++++++++++--- > 2 files changed, 50 insertions(+), 23 deletions(-) ... > + fn realize(&self) { What about renaming this as "realize_with_sysbus"? Elsewhere, the device's own realize method is often used to set DeviceImpl::REALIZE. And this realize here is meant to call the realize() method defined on the C side, so to avoid confusion we can avoid the same name? It's up to you. > + // TODO: return an Error > + assert!(bql_locked()); > + unsafe { > + bindings::sysbus_realize(self.as_mut_ptr(), addr_of_mut!(bindings::error_fatal)); > + } > + } This is a nice patch that shows more about how to use Owned<>! (BTW, I guess this patch is not the stable material. :-) ) Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
On 2/10/25 10:59, Zhao Liu wrote: > On Thu, Feb 06, 2025 at 12:15:14PM +0100, Paolo Bonzini wrote: >> Date: Thu, 6 Feb 2025 12:15:14 +0100 >> From: Paolo Bonzini <pbonzini@redhat.com> >> Subject: [PATCH] rust: pl011: convert pl011_create to safe Rust >> X-Mailer: git-send-email 2.48.1 >> >> Not a major change but, as a small but significant step in creating >> qdev bindings, show how pl011_create can be written without "unsafe" >> calls (apart from converting pointers to references). >> >> This also provides a starting point for creating Error** bindings. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++---------------- >> rust/qemu-api/src/sysbus.rs | 34 +++++++++++++++++++++++++--- >> 2 files changed, 50 insertions(+), 23 deletions(-) > > ... > >> + fn realize(&self) { > > What about renaming this as "realize_with_sysbus"? > > Elsewhere, the device's own realize method is often used to set > DeviceImpl::REALIZE. I *think* this is not a problem in practice because trait methods are public (if the trait is in scope) whereas the device's realize method if private. I agree that the naming conflict is unfortunate though, if only because it can cause confusion. I don't know if this can be solved by procedural macros; for example a #[vtable] attribute that changes #[qemu_api_macros::vtable] fn realize(...) { } into const fn REALIZE: ... = Some({ fn realize(...) { } realize } This way, the REALIZE item would be included in the "impl DeviceImpl for PL011State" block instead of "impl PL011State". It's always a fine line between procedural macros cleaning vs. messing things up, which is why until now I wanted to see what things look like with pure Rust code; but I guess now it's the time to evaluate this kind of thing. Adding Junjie since he contributed the initial proc macro infrastructure and may have opinions on this. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 2/10/25 10:59, Zhao Liu wrote: >> On Thu, Feb 06, 2025 at 12:15:14PM +0100, Paolo Bonzini wrote: >>> Not a major change but, as a small but significant step in creating >>> qdev bindings, show how pl011_create can be written without "unsafe" >>> calls (apart from converting pointers to references). >>> >>> This also provides a starting point for creating Error** bindings. >>> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++---------------- >>> rust/qemu-api/src/sysbus.rs | 34 +++++++++++++++++++++++++--- >>> 2 files changed, 50 insertions(+), 23 deletions(-) >> ... >> >>> + fn realize(&self) { >> What about renaming this as "realize_with_sysbus"? >> Elsewhere, the device's own realize method is often used to set >> DeviceImpl::REALIZE. > > I *think* this is not a problem in practice because trait methods are public (if > the trait is in scope) whereas the device's realize method if private. I would suggest to keep the "sysbus" prefix in the method name, or in general, keep the class prefix in the method names in XXXClassMethods traits. Otherwise APIs from different parent classes may also conflict. As an example, in the following class hierarchy: Object -> DeviceState -> PCIDevice -> PCIBridge -> ... For PCIDevice instances there is an API pci_device_reset(), and for PCIBridge ones pci_bridge_reset(). Without class prefixes both will be wrapped (as blanket impl in two traits) as reset() and a dev.reset() call will lead to a "multiple applicable items in scope" build error. In addition, it is quite common to see static functions named xxx_device_type_reset() in C today. Thus, it is quite natural to expect (private) methods named XXXDeviceState::reset() in future Rust devices, which will cause even more trouble as the compiler will no longer complain and always pick that method for dev.reset() calls. More general names can be found in multiple device classes, such as write_config (pci_default_write_config() and pci_bridge_write_config()). There are alternatives to solve such conflicts, such as requiring proc macro attributes above methods with such general names, and requiring ambiguous parent class API call to use fully qualified syntax, e.g., SysBusDeviceMethods::realize(self). But I think the former can be error-prone because the list of general names vary among different device types and required attributes can be easily neglected since no build error is triggered without them, and the later is more tedious than self.sysbus_realize(). > > I agree that the naming conflict is unfortunate though, if only because it can > cause confusion. I don't know if this can be solved by procedural macros; for > example a #[vtable] attribute that changes > > #[qemu_api_macros::vtable] > fn realize(...) { > } > > into > > const fn REALIZE: ... = Some({ > fn realize(...) { > } > realize > } > > This way, the REALIZE item would be included in the "impl DeviceImpl for > PL011State" block instead of "impl PL011State". It's always a fine line > between procedural macros cleaning vs. messing things up, which is why until now > I wanted to see what things look like with pure Rust code; but I guess now it's > the time to evaluate this kind of thing. > > Adding Junjie since he contributed the initial proc macro infrastructure and may > have opinions on this. I agree that uses of proc macros should be carefully evaluated to see if they really help or hurt. In this specific case, I'm not sure if using attributes solves the root cause, though. -- Best Regards Junjie Mao > > Paolo
On Tue, Feb 11, 2025 at 7:47 AM Junjie Mao <junjie.mao@hotmail.com> wrote: > I would suggest to keep the "sysbus" prefix in the method name, or in > general, keep the class prefix in the method names in XXXClassMethods > traits. Otherwise APIs from different parent classes may also > conflict. As an example, in the following class hierarchy: > > Object -> DeviceState -> PCIDevice -> PCIBridge -> ... > > For PCIDevice instances there is an API pci_device_reset(), and for > PCIBridge ones pci_bridge_reset(). Without class prefixes both will be > wrapped (as blanket impl in two traits) as reset() and a dev.reset() > call will lead to a "multiple applicable items in scope" build error. Yes, reset is definitely a problem. I will wrap qdev_realize() in DeviceMethods to check if you get "multiple applicable items" from that as well. I can also go back and add "sysbus_" in front of init_mmio, init_irq, etc.; but on the other hand we have Timer::{modify, delete} and DeviceMethods::init_{clock,qdev}_{in,out}, should they be changed as well? I'm not sure there is a single solution that always works. > I agree that uses of proc macros should be carefully evaluated to see if > they really help or hurt. In this specific case, I'm not sure if using > attributes solves the root cause, though. Yes, it doesn't help if you have multiple similarly-named methods across the "superclasses". Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On Tue, Feb 11, 2025 at 7:47 AM Junjie Mao <junjie.mao@hotmail.com> wrote: >> I would suggest to keep the "sysbus" prefix in the method name, or in >> general, keep the class prefix in the method names in XXXClassMethods >> traits. Otherwise APIs from different parent classes may also >> conflict. As an example, in the following class hierarchy: >> >> Object -> DeviceState -> PCIDevice -> PCIBridge -> ... >> >> For PCIDevice instances there is an API pci_device_reset(), and for >> PCIBridge ones pci_bridge_reset(). Without class prefixes both will be >> wrapped (as blanket impl in two traits) as reset() and a dev.reset() >> call will lead to a "multiple applicable items in scope" build error. > > Yes, reset is definitely a problem. > > I will wrap qdev_realize() in DeviceMethods to check if you get > "multiple applicable items" from that as well. > > I can also go back and add "sysbus_" in front of init_mmio, init_irq, > etc.; but on the other hand we have Timer::{modify, delete} and > DeviceMethods::init_{clock,qdev}_{in,out}, should they be changed as > well? I'm not sure there is a single solution that always works. > The DeviceMethods::init_* wrappers may need a similar prefix for the same reason, though it seems unlikely to suffer from such name conflicts. Meanwhile, adding prefixes to timer::* seems redundant. Essentially we want a naming convention that (1) avoids any potential name conflicts, and (2) applies consistently to (ideally) all APIs. For goal (1), we need something at API call sites that can resolve the potential ambiguity. So instead of dev.realize(), we may write: a) dev.sysbus_realize() b) SysBusDeviceMethods::realize(&dev); dev.realize() is still ok if there is no ambiguity c) dev.as_ref::<SysBusDevice>().realize() (any more options?) None looks perfect, unfortunately. Option (a) introduces inconsistent naming conventions as mentioned earlier; (b) cannot prevent confusions when a device has both a "reset()" method and "dev.reset()" calls; (c) deviates from how wrappers are auto-delegated to child classes today and is the longest to write. Option (b) + a lint that warns same method names looks a good tradeoff to me. I tried to find some clippy lints for that purpose, but have not found any yet. A similar issue exists [1], but has been kept open for >6 years and is not exactly what we want. [1] https://github.com/rust-lang/rust-clippy/issues/3117 -- Best Regards Junjie Mao >> I agree that uses of proc macros should be carefully evaluated to see if >> they really help or hurt. In this specific case, I'm not sure if using >> attributes solves the root cause, though. > > Yes, it doesn't help if you have multiple similarly-named methods > across the "superclasses". > > Paolo
Junjie Mao <junjie.mao@hotmail.com> writes: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On Tue, Feb 11, 2025 at 7:47 AM Junjie Mao <junjie.mao@hotmail.com> wrote: >>> I would suggest to keep the "sysbus" prefix in the method name, or in >>> general, keep the class prefix in the method names in XXXClassMethods >>> traits. Otherwise APIs from different parent classes may also >>> conflict. As an example, in the following class hierarchy: >>> >>> Object -> DeviceState -> PCIDevice -> PCIBridge -> ... >>> >>> For PCIDevice instances there is an API pci_device_reset(), and for >>> PCIBridge ones pci_bridge_reset(). Without class prefixes both will be >>> wrapped (as blanket impl in two traits) as reset() and a dev.reset() >>> call will lead to a "multiple applicable items in scope" build error. >> >> Yes, reset is definitely a problem. >> >> I will wrap qdev_realize() in DeviceMethods to check if you get >> "multiple applicable items" from that as well. >> >> I can also go back and add "sysbus_" in front of init_mmio, init_irq, >> etc.; but on the other hand we have Timer::{modify, delete} and >> DeviceMethods::init_{clock,qdev}_{in,out}, should they be changed as >> well? I'm not sure there is a single solution that always works. >> > > The DeviceMethods::init_* wrappers may need a similar prefix for the > same reason, though it seems unlikely to suffer from such name > conflicts. Meanwhile, adding prefixes to timer::* seems redundant. > > Essentially we want a naming convention that (1) avoids any potential > name conflicts, and (2) applies consistently to (ideally) all APIs. For > goal (1), we need something at API call sites that can resolve the > potential ambiguity. So instead of dev.realize(), we may write: > > a) dev.sysbus_realize() > > b) SysBusDeviceMethods::realize(&dev); dev.realize() is still ok if > there is no ambiguity > > c) dev.as_ref::<SysBusDevice>().realize() > > (any more options?) > > None looks perfect, unfortunately. Option (a) introduces inconsistent > naming conventions as mentioned earlier; (b) cannot prevent confusions > when a device has both a "reset()" method and "dev.reset()" calls; (c) > deviates from how wrappers are auto-delegated to child classes today and > is the longest to write. > > Option (b) + a lint that warns same method names looks a good tradeoff > to me. I tried to find some clippy lints for that purpose, but have not > found any yet. A similar issue exists [1], but has been kept open for >6 > years and is not exactly what we want. Just found the lint: https://rust-lang.github.io/rust-clippy/master/index.html#same_name_method > > [1] https://github.com/rust-lang/rust-clippy/issues/3117 -- Best Regards Junjie Mao
On Tue, Feb 11, 2025 at 12:34 PM Junjie Mao <junjie.mao@hotmail.com> wrote: > Essentially we want a naming convention that (1) avoids any potential > name conflicts, and (2) applies consistently to (ideally) all APIs. For > goal (1), we need something at API call sites that can resolve the > potential ambiguity. I now agree that (1) is more important than (2). Adding a function like fn realize(&self, bus: *mut BusState) { // TODO: return an Error assert!(bql_locked()); unsafe { bindings::qdev_realize(self.as_mut_ptr(), bus, addr_of_mut!(bindings::error_fatal)); } } to DeviceMethods is enough to cause an error: error[E0034]: multiple applicable items in scope --> hw/char/pl011/src/device.rs:714:9 | 714 | dev.realize(); | ^^^^^^^ multiple `realize` found > So instead of dev.realize(), we may write: > > a) dev.sysbus_realize() > b) SysBusDeviceMethods::realize(&dev); dev.realize() is still ok if > there is no ambiguity > c) dev.as_ref::<SysBusDevice>().realize() > > (any more options?) > > None looks perfect, unfortunately. Option (a) introduces inconsistent > naming conventions as mentioned earlier; (b) cannot prevent confusions > when a device has both a "reset()" method and "dev.reset()" calls; (c) > deviates from how wrappers are auto-delegated to child classes today and > is the longest to write. There is one more, which is a variant of (c): use Deref to delegate to the superclass, and traits for interfaces only. Then the default would always be the closest to the class you're defining, and you could override it with SysBusDevice::realize(&dev). It requires more glue code, but creating it could be delegated to #[derive(Object)]. I think we can use (a) as proposed by Zhao and yourself, and document this convention (1) whenever a name is unique in the hierarchy, do not add the prefix (2) whenever a name is not unique in the hierarchy, always add the prefix to the classes that are lower in the hierarchy; for the top class, decide on a case by case basis. For example, you'd have DeviceMethods::cold_reset() PciDeviceMethods::pci_device_reset() PciBridgeMethods::pci_bridge_reset() PciDeviceMethods adds the prefix because the three methods have different functionality. Subclasses of PciBridgeMethods may need to call either pci_device_reset() or pci_bridge_reset(). And also, because there is a similar name in DeviceMethods, PciDeviceMethods::reset() would be confusing. (As an aside, pci_device_reset() probably should be implemented at the Resettable level, e.g. RESET_TYPE_BUS, but that's a different story). Or perhaps pci_bridge_reset() becomes PciBridgeCap::reset(), which is not a trait. That's okay too, and it's not a problem for the naming of pci_device_reset(). but: DeviceMethods::realize() SysbusDeviceMethods::sysbus_realize() PciDeviceMethods::pci_realize() Here, DeviceMethods does not add the prefix because generally the lower classes only add casts and compile-time typing but not functionality. The basic realize() functionality is the same for all devices. What about confusion with the realize function on the struct? It's unlikely to be an issue because it has a different signature than DeviceMethods::realize(), which takes a BusState too. But if I'm wrong and there _is_ confusion, renaming DeviceMethods::realize() is easy. > Just found the lint: https://rust-lang.github.io/rust-clippy/master/index.html#same_name_method Almost: "It lints if a struct has two methods with the same name: one from a trait, another not from a trait." - it doesn't check two traits. Also I think in this case it doesn't fire because the trait is implemented for &PL011State, not PL011State. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On Tue, Feb 11, 2025 at 12:34 PM Junjie Mao <junjie.mao@hotmail.com> wrote: >> Essentially we want a naming convention that (1) avoids any potential >> name conflicts, and (2) applies consistently to (ideally) all APIs. For >> goal (1), we need something at API call sites that can resolve the >> potential ambiguity. > > I now agree that (1) is more important than (2). Adding a function like > > fn realize(&self, bus: *mut BusState) { > // TODO: return an Error > assert!(bql_locked()); > unsafe { > bindings::qdev_realize(self.as_mut_ptr(), > bus, addr_of_mut!(bindings::error_fatal)); > } > } > > to DeviceMethods is enough to cause an error: > > error[E0034]: multiple applicable items in scope > --> hw/char/pl011/src/device.rs:714:9 > | > 714 | dev.realize(); > | ^^^^^^^ multiple `realize` found > >> So instead of dev.realize(), we may write: >> >> a) dev.sysbus_realize() >> b) SysBusDeviceMethods::realize(&dev); dev.realize() is still ok if >> there is no ambiguity >> c) dev.as_ref::<SysBusDevice>().realize() >> >> (any more options?) >> >> None looks perfect, unfortunately. Option (a) introduces inconsistent >> naming conventions as mentioned earlier; (b) cannot prevent confusions >> when a device has both a "reset()" method and "dev.reset()" calls; (c) >> deviates from how wrappers are auto-delegated to child classes today and >> is the longest to write. > > There is one more, which is a variant of (c): use Deref to delegate to > the superclass, and traits for interfaces only. Then the default would > always be the closest to the class you're defining, and you could > override it with SysBusDevice::realize(&dev). > > It requires more glue code, but creating it could be delegated to > #[derive(Object)]. > > I think we can use (a) as proposed by Zhao and yourself, and document > this convention > > (1) whenever a name is unique in the hierarchy, do not add the prefix > > (2) whenever a name is not unique in the hierarchy, always add the > prefix to the classes that are lower in the hierarchy; for the top > class, decide on a case by case basis. > That convention looks good to me and does keep the naming simple for the vast majority. > For example, you'd have > > DeviceMethods::cold_reset() > PciDeviceMethods::pci_device_reset() > PciBridgeMethods::pci_bridge_reset() > > PciDeviceMethods adds the prefix because the three methods have > different functionality. Subclasses of PciBridgeMethods may need to > call either pci_device_reset() or pci_bridge_reset(). > > And also, because there is a similar name in DeviceMethods, > PciDeviceMethods::reset() would be confusing. > > (As an aside, pci_device_reset() probably should be implemented at the > Resettable level, e.g. RESET_TYPE_BUS, but that's a different story). > > Or perhaps pci_bridge_reset() becomes PciBridgeCap::reset(), which is > not a trait. That's okay too, and it's not a problem for the naming of > pci_device_reset(). > > but: > > DeviceMethods::realize() > SysbusDeviceMethods::sysbus_realize() > PciDeviceMethods::pci_realize() > > Here, DeviceMethods does not add the prefix because generally the > lower classes only add casts and compile-time typing but not > functionality. The basic realize() functionality is the same for all > devices. > > What about confusion with the realize function on the struct? It's > unlikely to be an issue because it has a different signature than > DeviceMethods::realize(), which takes a BusState too. But if I'm wrong > and there _is_ confusion, renaming DeviceMethods::realize() is easy. > I don't think I'm experienced enough to tell if that can confuse device writers. Perhaps we can keep it for now as renaming is easy with the support from the language server. >> Just found the lint: https://rust-lang.github.io/rust-clippy/master/index.html#same_name_method > > Almost: "It lints if a struct has two methods with the same name: one > from a trait, another not from a trait." - it doesn't check two > traits. Also I think in this case it doesn't fire because the trait is > implemented for &PL011State, not PL011State. I thought that lint can help warn early when a device writer accidentally named a method in the same way as an API. But as you have pointed out, it doesn't really help in this case. I'm still a bit worried about the potential ambiguity between API and device-defined method names. The compiler keeps silent on that, and it can eventually cause unexpected control flow at runtime. That said, I'm not sure how likely it will hit us. We may keep it as is for now and go extend that lint when we find out later that the ambiguity worths early warnings. > > Paolo -- Best Regards Junjie Mao
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 22f3ca3b4e8..7e936b50eb0 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -10,14 +10,12 @@ use qemu_api::{ bindings::{ - error_fatal, qdev_prop_set_chr, qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, - qemu_chr_fe_set_handlers, qemu_chr_fe_write_all, qemu_irq, sysbus_connect_irq, - sysbus_mmio_map, sysbus_realize, CharBackend, Chardev, QEMUChrEvent, - CHR_IOCTL_SERIAL_SET_BREAK, + qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers, + qemu_chr_fe_write_all, CharBackend, QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK, }, chardev::Chardev, - c_str, impl_vmstate_forward, - irq::InterruptSource, + impl_vmstate_forward, + irq::{IRQState, InterruptSource}, memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder}, prelude::*, qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl}, @@ -698,26 +696,27 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> { /// # Safety /// -/// We expect the FFI user of this function to pass a valid pointer for `chr`. +/// We expect the FFI user of this function to pass a valid pointer for `chr` +/// and `irq`. #[no_mangle] pub unsafe extern "C" fn pl011_create( addr: u64, - irq: qemu_irq, + irq: *mut IRQState, chr: *mut Chardev, ) -> *mut DeviceState { - let pl011 = PL011State::new(); - unsafe { - let dev = pl011.as_mut_ptr::<DeviceState>(); - qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr); + // SAFETY: The callers promise that they have owned references. + // They do not gift them to pl011_create, so use `Owned::from`. + let irq = unsafe { Owned::<IRQState>::from(&*irq) }; + let chr = unsafe { Owned::<Chardev>::from(&*chr) }; - let sysbus = pl011.as_mut_ptr::<SysBusDevice>(); - sysbus_realize(sysbus, addr_of_mut!(error_fatal)); - sysbus_mmio_map(sysbus, 0, addr); - sysbus_connect_irq(sysbus, 0, irq); - - // return the pointer, which is kept alive by the QOM tree; drop owned ref - pl011.as_mut_ptr() - } + let dev = PL011State::new(); + dev.prop_set_chr("chardev", &chr); + dev.realize(); + dev.mmio_map(0, addr); + dev.connect_irq(0, &irq); + // SAFETY: return the pointer, which has to be mutable and is kept alive + // by the QOM tree; drop owned ref + unsafe { dev.as_mut_ptr() } } #[repr(C)] diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs index c27dbf79e43..1f071706ce8 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -2,18 +2,18 @@ // Author(s): Paolo Bonzini <pbonzini@redhat.com> // SPDX-License-Identifier: GPL-2.0-or-later -use std::ffi::CStr; +use std::{ffi::CStr, ptr::addr_of_mut}; pub use bindings::{SysBusDevice, SysBusDeviceClass}; use crate::{ bindings, cell::bql_locked, - irq::InterruptSource, + irq::{IRQState, InterruptSource}, memory::MemoryRegion, prelude::*, qdev::{DeviceClass, DeviceState}, - qom::ClassInitImpl, + qom::{ClassInitImpl, Owned}, }; unsafe impl ObjectType for SysBusDevice { @@ -60,6 +60,34 @@ fn init_irq(&self, irq: &InterruptSource) { bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr()); } } + + // TODO: do we want a type like GuestAddress here? + fn mmio_map(&self, id: u32, addr: u64) { + assert!(bql_locked()); + let id: i32 = id.try_into().unwrap(); + unsafe { + bindings::sysbus_mmio_map(self.as_mut_ptr(), id, addr); + } + } + + // Owned<> is used here because sysbus_connect_irq (via + // object_property_set_link) adds a reference to the IRQState, + // which can prolong its life + fn connect_irq(&self, id: u32, irq: &Owned<IRQState>) { + assert!(bql_locked()); + let id: i32 = id.try_into().unwrap(); + unsafe { + bindings::sysbus_connect_irq(self.as_mut_ptr(), id, irq.as_mut_ptr()); + } + } + + fn realize(&self) { + // TODO: return an Error + assert!(bql_locked()); + unsafe { + bindings::sysbus_realize(self.as_mut_ptr(), addr_of_mut!(bindings::error_fatal)); + } + } } impl<R: ObjectDeref> SysBusDeviceMethods for R where R::Target: IsA<SysBusDevice> {}
Not a major change but, as a small but significant step in creating qdev bindings, show how pl011_create can be written without "unsafe" calls (apart from converting pointers to references). This also provides a starting point for creating Error** bindings. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++---------------- rust/qemu-api/src/sysbus.rs | 34 +++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 23 deletions(-)