On 10/28/2025 6:45 PM, Paolo Bonzini wrote:
> On Tue, Oct 28, 2025 at 11:18 AM chenmiao <[email protected]> wrote:
>> 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.
> I don't think they need to be implemented using different approaches.
> The difference between the two is that:
>
> - the currently implemented devices do not expose any bus, they stay
> on a bus. This means the bus is never a child of the device
>
> - the currently implemented buses are all in C code, whereas there are
> devices implemented in Rust.
>
> I agree that the Rust-to-C bridge code is complex, but it does have
> documentation, much more so than the C versions in fact. If there are
> specific aspects of the documentation that you would like to see
> improved, you can help by explaining what problems and sources of
> confusion you encountered.
That makes sense to me. Since buses are mainly implemented in C, the current
focus of Rust is on porting the device layer. Rust-implemented devices are
consumers and do not create any child objects.
I think our previous confusion stemmed from the assumption that Rust was
porting the entire hierarchy, when in fact Rust is currently only implementing
the device layer.
>> +/// 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>
>
> There are no virtual methods, and therefore I2CBus does not have
> subclasses. Therefore you don't need these traits and you can
> implement the functions directly on I2CBus.
For this part, we directly referred to the implementation of SysBus.
>> +{
>> + /// # Safety
>> + ///
>> + /// Initialize an I2C bus
>> + fn init_bus(&self, parent: &DeviceState, name: &str) -> *mut
>> bindings::I2CBus {
>> + assert!(bql::is_locked());
>> + unsafe { bindings::i2c_init_bus(parent.as_mut_ptr(),
>> name.as_ptr().cast()) }
>> + }
> This should return an Owned<I2CBus>.
Yes, I'll fix it later.
>
>> + /// 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.
>> + ///
>> + /// TODO: `bindings:QEMUBH` should be wrapped by Opaque<>.
>> + unsafe fn set_master(&self, bh: *mut bindings::QEMUBH) {
>> + assert!(bql::is_locked());
>> + unsafe { bindings::i2c_bus_master(self.upcast().as_mut_ptr(), bh) }
>> + }
> I agree with Zhao, I would leave out this completely. You can add a
> TODO ("i2c_bus_master missing until QEMUBH is wrapped").
>
>> + /// # Safety
>> + ///
>> + /// Release an I2C bus
>> + fn release(&self) {
>> + assert!(bql::is_locked());
>> + unsafe { bindings::i2c_bus_release(self.upcast().as_mut_ptr()) }
>> + }
> Same for this, which is the counterpart of i2c_bus_master. In Rust, in
> fact, release() should be done with an RAII guard returned by
> set_master.
>
>> +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;
> Make it return an "enum I2CResult { Ack, Nack };" instead of a Result.
> Likewise other function pointers here do not need a Result<>.
>> + /// Master to slave (asynchronous). Receiving slave must call
>> `i2c_ack()`.
>> + const SEND_ASYNC: Option<fn(&Self, data: u8)> = None;
> This requires the bottom half machinery. Leave it out.
>> +/// Trait for methods of [`I2CSlave`] and its subclasses.
>> +pub trait I2CSlaveMethods: ObjectDeref
>> +where
>> + Self::Target: IsA<I2CSlave>,
>> +{
>> + /// Create an I2C slave device on the heap.
>> + ///
>> + /// # Arguments
>> + /// * `name` - a device type name
>> + /// * `addr` - I2C address of the slave when put on a bus
>> + ///
>> + /// This only initializes the device state structure and allows
>> + /// properties to be set. Type `name` must exist. The device still
>> + /// needs to be realized.
>> + fn init_new(name: &str, addr: u8) -> Owned<I2CSlave> {
>> + assert!(bql::is_locked());
>> + unsafe {
>> + let slave = bindings::i2c_slave_new(name.as_ptr().cast(), addr);
>> + Owned::from(I2CSlave::from_raw(slave))
>> + }
>> + }
>> +
>> + /// Create and realize an I2C slave device on the heap.
>> + ///
>> + /// # Arguments
>> + /// * `bus` - I2C bus to put it on
>> + /// * `name` - I2C slave device type name
>> + /// * `addr` - I2C address of the slave when put on a bus
>> + ///
>> + /// Create the device state structure, initialize it, put it on the
>> + /// specified `bus`, and drop the reference to it (the device is
>> realized).
>> + fn create_simple(&self, bus: &I2CBus, name: &str, addr: u8) ->
>> Owned<I2CSlave> {
>> + assert!(bql::is_locked());
>> + unsafe {
>> + let slave =
>> + bindings::i2c_slave_create_simple(bus.as_mut_ptr(),
>> name.as_ptr().cast(), addr);
>> + Owned::from(I2CSlave::from_raw(slave))
>> + }
>> + }
>> +
>> + /// Set the I2C bus address of a slave device
>> + ///
>> + /// # Arguments
>> + /// * `address` - I2C address of the slave when put on a bus
>> + fn set_address(&self, address: u8) {
>> + assert!(bql::is_locked());
>> + unsafe {
>> bindings::i2c_slave_set_address(self.upcast().as_mut_ptr(), address) }
>> + }
> These three are used by boards, which we don't model. We can keep the
> code simple by leaving them off (in addition, init_new and
> create_simple would be *class* methods, as visible from the fact that
> they don't use self at all).
>
>> + /// Get the I2C bus address of a slave device
>> + fn get_address(&self) -> u8 {
>> + assert!(bql::is_locked());
>> + // SAFETY: the BQL ensures that no one else writes to the I2CSlave
>> structure,
>> + // and the I2CSlave must be initialized to get an IsA<I2CSlave>.
>> + let slave = unsafe { *self.upcast().as_ptr() };
>> + slave.address
>> + }
>> +}
>> +
>> +impl<R: ObjectDeref> I2CSlaveMethods for R where R::Target: IsA<I2CSlave> {}
>> +
>> +/// Enum representing I2C events
>> +#[repr(i32)]
>> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> +pub enum I2CEvent {
>> + StartRecv = 0,
>> + StartSend = 1,
>> + StartSendAsync = 2,
>> + Finish = 3,
>> + Nack = 4,
>> +}
> Make it "= bindings::I2C_START_RECV" etc. You can then use
> #[derive(common::TryInto)] instead of implementing by hand the From
> traits.
>
> Paolo
I'll revise all the points you have raised later.
Thanks,
Chen Miao
