[PATCH] rust: add --rust-target option for bindgen

[PATCH] rust: add --rust-target option for bindgen

[Paolo Bonzini]

Without it, recent bindgen will give an error

   error: extern block cannot be declared unsafe

if rustc is not new enough to support the "unsafe extern" construct.

Cc: qemu-rust@nongnu.org
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/meson.build b/meson.build
index 2c9ac9cfe1e..131b2225ab6 100644
--- a/meson.build
+++ b/meson.build
@@ -4054,6 +4054,9 @@ if have_rust
       bindgen_args += ['--formatter', 'none']
     endif
   endif
+  if bindgen.version().version_compare('>=0.66.0')
+    bindgen_args += ['--rust-target', '1.59']
+  endif
   if bindgen.version().version_compare('<0.61.0')
     # default in 0.61+
     bindgen_args += ['--size_t-is-usize']
-- 
2.48.1

[PATCH] rust: pl011: convert pl011_create to safe Rust

[Paolo Bonzini]

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

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> {}
-- 
2.48.1

Re: [PATCH] rust: add --rust-target option for bindgen

[Philippe Mathieu-Daudé]

On 6/2/25 12:15, Paolo Bonzini wrote:
> Without it, recent bindgen will give an error
> 
>     error: extern block cannot be declared unsafe
> 
> if rustc is not new enough to support the "unsafe extern" construct.
> 
> Cc: qemu-rust@nongnu.org
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   meson.build | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 2c9ac9cfe1e..131b2225ab6 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -4054,6 +4054,9 @@ if have_rust
>         bindgen_args += ['--formatter', 'none']
>       endif
>     endif
> +  if bindgen.version().version_compare('>=0.66.0')
> +    bindgen_args += ['--rust-target', '1.59']
> +  endif
>     if bindgen.version().version_compare('<0.61.0')
>       # default in 0.61+
>       bindgen_args += ['--size_t-is-usize']

Should this be merged directly on master as build-fix?

Re: [PATCH] rust: add --rust-target option for bindgen

[Paolo Bonzini]

On Thu, Feb 6, 2025 at 12:37 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
> >     if bindgen.version().version_compare('<0.61.0')
> >       # default in 0.61+
> >       bindgen_args += ['--size_t-is-usize']
>
> Should this be merged directly on master as build-fix?

If it's breaking CI I can send a pull request later. I wasn't sure if
it was just my branch with other Rust changes.

Paolo

Re: [PATCH] rust: add --rust-target option for bindgen

[Philippe Mathieu-Daudé]

On 6/2/25 12:38, Paolo Bonzini wrote:
> On Thu, Feb 6, 2025 at 12:37 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>>      if bindgen.version().version_compare('<0.61.0')
>>>        # default in 0.61+
>>>        bindgen_args += ['--size_t-is-usize']
>>
>> Should this be merged directly on master as build-fix?
> 
> If it's breaking CI I can send a pull request later. I wasn't sure if
> it was just my branch with other Rust changes.

No, this is now affecting the main repository, so also all forks.

Re: [PATCH] rust: add --rust-target option for bindgen

[Stefan Hajnoczi]

I merged this directly into qemu.git/master.

Stefan

Re: [PATCH] rust: pl011: convert pl011_create to safe Rust

[Zhao Liu]

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>

vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Paolo Bonzini]

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

Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Junjie Mao]

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

Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Paolo Bonzini]

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

Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Junjie Mao]

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

Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Junjie Mao]

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

Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Paolo Bonzini]

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

Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Junjie Mao]

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