Re: [RFC 2/5] rust: hw: core: Add rust bindings/funcs for i2c bus

["Chen Miao" <chenmiao@openatom.club>]

On 10/27/2025 3:17 PM, Zhao Liu wrote:
> On Sat, Oct 25, 2025 at 12:38:50PM +0000, chenmiao wrote:
>> Date: Sat, 25 Oct 2025 12:38:50 +0000
>> From: chenmiao <chenmiao@openatom.club>
>> Subject: [RFC 2/5] rust: hw: core: Add rust bindings/funcs for i2c bus
>> X-Mailer: git-send-email 2.43.0
>>
>> We have implemented the I2CBus and I2CSlave infrastructure in Rust by referring
>> to the SysBus device model.
>>
>> Initially, we assumed that the I2CBus was at the same hierarchical level as the
>> PL011 device. Therefore, we followed the implementation paradigm of the PL011
>> device as a reference. However, in the end, we discovered that the I2CBus is
>> actually at the same level as the SysBus. As a result, we adopted the binding
>> implementation paradigm used for SysBus devices. With this adjustment, we
>> successfully compiled the code locally.
>>
>> During the implementation process, we found that the current two paradigms in
>> Rust — bindings and impl — are extremely complex and lack comprehensive
>> documentation. There is no clear explanation as to why Bus and Device models
>> need to be implemented using different approaches. Furthermore, the
>> implementation of Bus and Device following these paradigms still has many
>> limitations. At present, at least vmstate is not easily supported.
>>
>> Signed-off-by: Chao Liu <chao.liu@openatom.club>
>> Signed-off-by: chenmiao <chenmiao@openatom.club>
>> ---
>>   rust/hw/core/meson.build   |   1 +
>>   rust/hw/core/src/i2cbus.rs | 291 +++++++++++++++++++++++++++++++++++++
>>   rust/hw/core/src/lib.rs    |   3 +
>>   rust/hw/core/wrapper.h     |   1 +
>>   4 files changed, 296 insertions(+)
>>   create mode 100644 rust/hw/core/src/i2cbus.rs
>>
>> diff --git a/rust/hw/core/meson.build b/rust/hw/core/meson.build
>> index efcda50..7c44786 100644
>> --- a/rust/hw/core/meson.build
>> +++ b/rust/hw/core/meson.build
>> @@ -52,6 +52,7 @@ _hwcore_rs = static_library(
>>         'src/bindings.rs',
>>         'src/bus.rs',
>>         'src/irq.rs',
>> +      'src/i2cbus.rs',
>>         'src/qdev.rs',
>>         'src/sysbus.rs',
>>       ],
>> diff --git a/rust/hw/core/src/i2cbus.rs b/rust/hw/core/src/i2cbus.rs
>> new file mode 100644
>> index 0000000..c026955
>> --- /dev/null
>> +++ b/rust/hw/core/src/i2cbus.rs
> This file implements both I2CBus and I2C device (Slave).
>
> To be general, I think it's better to name it as "i2c.rs".
>
>> @@ -0,0 +1,291 @@
>> +// Copyright 2025 HUST OpenAtom Open Source Club.
>> +// Author(s): Chao Liu <chao.liu@openatom.club>
>> +// Author(s): Chen Miao <chenmiao@openatom.club>
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +//! Bindings to access `i2c` functionality from Rust.
>> +
>> +use std::ffi::CStr;
>> +
>> +pub use bindings::I2CSlaveClass;
>> +use common::Opaque;
>> +use qom::{prelude::*, Owned};
>> +use util::Result;
>> +
>> +use crate::{
>> +    bindings,
>> +    bus::{BusClass, BusState},
>> +    qdev::{DeviceImpl, DeviceState},
>> +};
>> +
>> +/// A safe wrapper around [`bindings::I2CBus`].
>> +#[repr(transparent)]
>> +#[derive(Debug, common::Wrapper)]
>> +pub struct I2CBus(Opaque<bindings::I2CBus>);
>> +
>> +unsafe impl Send for I2CBus {}
>> +unsafe impl Sync for I2CBus {}
>> +
>> +unsafe impl ObjectType for I2CBus {
>> +    type Class = BusClass;
>> +    const TYPE_NAME: &'static CStr =
>> +        unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_I2C_BUS) };
>> +}
>> +
>> +qom_isa!(I2CBus: BusState, Object);
>> +
>> +// TODO: add virtual methods
>> +pub trait I2CBusImpl: DeviceImpl + IsA<I2CBus> {}
>> +
>> +/// Trait for methods of [`I2CBus`] and its subclasses.
>> +pub trait I2CBusMethods: ObjectDeref
>> +where
>> +    Self::Target: IsA<I2CBus>,
>> +{
>> +    /// Initialize an I2C bus
>> +    fn init_bus(&self, parent: &DeviceState, name: &str) -> *mut bindings::I2CBus {
>> +        assert!(bql::is_locked());
> It's better to add # SFAETY comment (others methods below also need it).
>
>> +        unsafe { bindings::i2c_init_bus(parent.as_mut_ptr(), name.as_ptr().cast()) }
>> +    }
> ...
>
>> +    /// Sets the I2C bus master.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// This function is unsafe because:
>> +    /// - `bh` must be a valid pointer to a `QEMUBH`.
>> +    /// - The caller must ensure that `self` is in a valid state.
>> +    /// - The caller must guarantee no data races occur during execution.
>> +    unsafe fn set_master(&self, bh: *mut bindings::QEMUBH) {
> I think it's better to have a Opaque<> wrapper over bindings::QEMUBH.
> However, async has not yet been fully considered.
>
> IMO, It seems i2c_bus_master() is only used by i2c-echo, which is not
> implememnted in this series. Perhaps this series could initially omit
> QEMUBH (with a TODO comment) until QEMUBH binding is introduced.
I think our team is also interested in the implementation of QEMUBH, and we 
will try to implement this part as well. Regarding the omission of QEMUBH, I 
believe we can add a comment here for now!
>> +        assert!(bql::is_locked());
>> +        unsafe { bindings::i2c_bus_master(self.upcast().as_mut_ptr(), bh) }
>> +    }
>> +
>> +    /// Release an I2C bus
>> +    fn release(&self) {
>> +        assert!(bql::is_locked());
>> +        unsafe { bindings::i2c_bus_release(self.upcast().as_mut_ptr()) }
>> +    }
>> +}
>> +
>> +impl<R: ObjectDeref> I2CBusMethods for R where R::Target: IsA<I2CBus> {}
>> +
>> +/// A safe wrapper around [`bindings::I2CSlave`].
>> +#[repr(transparent)]
>> +#[derive(Debug, common::Wrapper)]
>> +pub struct I2CSlave(Opaque<bindings::I2CSlave>);
>> +
>> +unsafe impl Send for I2CSlave {}
>> +unsafe impl Sync for I2CSlave {}
>> +
>> +unsafe impl ObjectType for I2CSlave {
>> +    type Class = I2CSlaveClass;
>> +    const TYPE_NAME: &'static CStr =
>> +        unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_I2C_SLAVE) };
>> +}
>> +
>> +qom_isa!(I2CSlave: DeviceState, Object);
>> +
>> +// TODO: add virtual methods
>> +pub trait I2CSlaveImpl: DeviceImpl + IsA<I2CSlave> {
>> +    /// Master to slave. Returns non-zero for a NAK, 0 for success.
>> +    const SEND: Option<fn(&Self, data: u8) -> Result<bool>> = None;
>> +
>> +    /// Master to slave (asynchronous). Receiving slave must call `i2c_ack()`.
>> +    const SEND_ASYNC: Option<fn(&Self, data: u8) -> Result<()>> = None;
>> +
>> +    /// Slave to master. This cannot fail, the device should always return something here.
>> +    const RECV: Option<fn(&Self) -> Result<bool>> = None;
>> +
>> +    /// Notify the slave of a bus state change. For start event,
>> +    /// returns non-zero to NAK an operation. For other events the
>> +    /// return code is not used and should be zero.
>> +    const EVENT: Option<fn(&Self, event: I2CEvent) -> Result<I2CEvent>> = None;
>> +
>> +    /// Check if this device matches the address provided. Returns bool of
>> +    /// true if it matches (or broadcast), and updates the device list, false
>> +    /// otherwise.
>> +    ///
>> +    /// If broadcast is true, match should add the device and return true.
>> +    type AddressMatcher;
>> +    const MATCH_AND_ADD: Option<Self::AddressMatcher> = None;
>> +}
>> +
>> +impl I2CSlaveClass {
>> +    /// Fill in the virtual methods of `I2CSlaveClass` based on the
>> +    /// definitions in the `I2CSlaveImpl` trait.
>> +    pub fn class_init<T: I2CSlaveImpl>(self: &mut I2CSlaveClass) {
> It seems the callbacks (SEND/SEND_ASYNC/...) are missing to set here.
>
> For example, add a callback wrapper first:
>
> unsafe extern "C" fn rust_i2c_slave_send_fn<T: I2CSlaveImpl>(
>      slave: *mut bindings::I2CSlave,
>      date: u8,
> ) {
>      let state = NonNull::new(slave).unwrap().cast::<T>();
>      T::SEND.unwrap()(unsafe { state.as_ref() }, data);
> }
>
> Then create C callback like:
>
> pub fn class_init<T: I2CSlaveImpl>(self: &mut I2CSlaveClass) {
>      ...
>      if <T as I2CSlaveImpl>::SEND.is_some() {
>          self.send = Some(rust_i2c_slave_send_fn::<T>);
>      }
>      ...
> }
>
> Similarly, you can refer to ResettableClass::class_init and
> DeviceClass::class_init.

Ok, I'll complete it strictly according to your instructions.

>
>> +        self.parent_class.class_init::<T>();
>> +    }
>> +}
>> +
> Regards,
> Zhao

Waiting for your next review for other patch.

Thanks,

Chen Miao

>