On Tue, Oct 28, 2025 at 11:19 AM chenmiao <[email protected]> wrote:
>
> After implementing the I2CBus and I2CSlave, we proceeded to implement a basic
> GPIO device — the PCF8574 — which depends on I2CSlave.
Yes, this is a good choice.
> The most significant insight I gained during this process is that the current
> approach in Rust appears to be more complex and harder to understand than its
> C counterpart. This trend poses a substantial barrier for developers
> interested
> in Rust for QEMU, preventing them from efficiently, quickly, and safely
> modeling
> QEMU devices in Rust. In my opinion, this deviates from the original intent of
> adopting Rust.
If you were implementing a new serial device, for example, you would
just write a new directory in rust/hw/char.
Even in that case, creating the meson.build file is more complex than
in C; I'm working on simplifying that by adding new features to Meson.
In other words, you're not wrong at all but note that you are not
seeing the code from the point of view of someone implementing the
Rust-to-C bridges, not someone implementing a new device, because
there are no I2C devices yet in QEMU that are written in Rust. I agree
that this is a relatively large work, on the other it only has to be
done once.
> Moreover, the mechanisms of bindings and wrappers are overly redundant, and
> there is no clear indication that they are mandatory. These constructs
> generate
> a large number of similar structures in the final compilation artifacts. Could
> we design a more generic approach to bindings and wrappers?
Bindings are redundant across crates, indeed. It is probably possible
to improve how they work.
> Additionally, vmstate has not been fully implemented for the PCF8574 (to be
> precise, it is not yet complete). During the implementation, I encountered an
> issue: initially, I tried to directly transliterate the C struct — that is, I
> attempted to configure vmstate and handle other aspects directly via field
> mappings. However, I eventually found that some internal APIs were
> incompatible.
> To make it work, I had to isolate those "potentially mutable values" into a
> separate structure, akin to something like a "PL011Register", in order for it
> to function correctly.
This is correct and intended. It isolates the interior-mutable parts
of the struct and in the future it can be used to support
multi-threaded devices too.
> +// impl_vmstate_struct!(
> +// I2CSlave,
> +// VMStateDescriptionBuilder::<I2CSlave>::new()
> +// .name(c"I2CSlave")
> +// .version_id(1)
> +// .minimum_version_id(1)
> +// .post_load(&I2CSlave::post_load)
> +// .fields(vmstate_fields! {
> +// vmstate_of!(I2CSlave, address),
> +// })
> +// .build()
> +// );
This is not needed, instead you can implement VMState for I2CSlave:
impl_vmstate_c_struct!(I2CSlave, bindings::vmstate_i2c_slave);
> +bilge = { version = "0.2.0" }
> +bilge-impl = { version = "0.2.0" }
> +chardev = { path = "../../../chardev" }
You don't need these.
> +_libpcf8574_bindings_inc_rs = rust.bindgen(
> + input: 'wrapper.h',
> + dependencies: common_ss.all_dependencies(),
> + output: 'bindings.inc.rs',
> + include_directories: bindings_incdir,
> + bindgen_version: ['>=0.60.0'],
> + args: bindgen_args_common,
> + c_args: bindgen_c_args,
> +)
You also don't need bindgen for this simple device...
> +_libpcf8574_rs = static_library(
> + 'pcf8574',
> + structured_sources(
> + [
> + 'src/lib.rs',
> + 'src/bindings.rs',
> + 'src/device.rs',
> + ],
> + {'.' : _libpcf8574_bindings_inc_rs},
... and therefore you don't need structured_sources, just 'src/lib.rs'.
> + bilge_rs,
> + bilge_impl_rs,
> + chardev_rs,
Also remove unneeded crates from here.
> --- /dev/null
> +++ b/rust/hw/gpio/pcf8574/src/device.rs
> @@ -0,0 +1,180 @@
> +// Copyright 2025 HUST OpenAtom Open Source Club.
> +// Author(s): Chen Miao <[email protected]>
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +use std::slice::from_ref;
> +
> +// use common::static_assert;
> +use bql::BqlRefCell;
> +use hwcore::{
> + bindings, DeviceClass, DeviceImpl, DeviceMethods, DeviceState, I2CSlave,
> I2CSlaveImpl,
> + InterruptSource, ResetType, ResettablePhasesImpl,
> +};
> +use migration::{
> + self, impl_vmstate_struct, vmstate_fields, vmstate_of,
> VMStateDescription,
> + VMStateDescriptionBuilder,
> +};
> +use qom::{qom_isa, IsA, Object, ObjectImpl, ObjectType, ParentField};
> +use util::Result;
> +
> +const PORTS_COUNT: usize = 8;
> +
> +#[repr(C)]
> +#[derive(Debug, Default)]
> +pub struct PCF8574StateInner {
> + pub lastrq: u8,
> + pub input: u8,
> + pub output: u8,
> +}
This is small and simple enough that it can be Copy.
> +#[repr(C)]
> +#[derive(qom::Object, hwcore::Device)]
> +pub struct PCF8574State {
> + pub parent_obj: ParentField<I2CSlave>,
> + pub inner: BqlRefCell<PCF8574StateInner>,
> + pub handler: [InterruptSource; PORTS_COUNT],
> + pub intrq: InterruptSource,
> +}
> +
> +// static_assert!(size_of::<PCF8574State>() <=
> size_of::<crate::bindings::PCF8574State>());
> +
> +qom_isa!(PCF8574State: I2CSlave, DeviceState, Object);
> +
> +#[allow(dead_code)]
> +trait PCF8574Impl: I2CSlaveImpl + IsA<PCF8574State> {}
> +
> +unsafe impl ObjectType for PCF8574State {
> + type Class = DeviceClass;
> + const TYPE_NAME: &'static std::ffi::CStr = crate::TYPE_PCF8574;
> +}
> +
> +impl PCF8574Impl for PCF8574State {}
> +
> +impl ObjectImpl for PCF8574State {
> + type ParentType = I2CSlave;
> + const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
> +}
> +
> +impl DeviceImpl for PCF8574State {
> + const VMSTATE: Option<migration::VMStateDescription<Self>> =
> Some(VMSTATE_PCF8574);
> + const REALIZE: Option<fn(&Self) -> util::Result<()>> =
> Some(Self::realize);
> +}
> +
> +impl ResettablePhasesImpl for PCF8574State {
> + const HOLD: Option<fn(&Self, ResetType)> = Some(Self::reset_hold);
> +}
> +
> +impl I2CSlaveImpl for PCF8574State {
> + const SEND: Option<fn(&Self, data: u8) -> util::Result<bool>> =
> Some(Self::send);
> + const RECV: Option<fn(&Self) -> util::Result<bool>> = Some(Self::recv);
> + const SEND_ASYNC: Option<fn(&Self, data: u8)> = None;
> + const EVENT: Option<fn(&Self, event: hwcore::I2CEvent) ->
> util::Result<hwcore::I2CEvent>> =
> + None;
> + const MATCH_AND_ADD: Option<
> + fn(
> + &Self,
> + address: u8,
> + broadcast: bool,
> + current_devs: *mut bindings::I2CNodeList,
> + ) -> Result<bool>,
> + > = None;
You don't need to redeclare the ones that are "None".
> +}
> +
> +impl PCF8574State {
> + fn send(&self, data: u8) -> Result<bool> {
> + let prev = self.line_state();
> + self.inner.borrow_mut().output = data;
I think you can use a function like
impl PCF8574Inner {
pub fn line_state(&self) {
self.input & self.output
}
pub fn set_outputs(&mut self, data: u32) -> PCF8574Inner {
let prev = self.line_state();
self.output = data;
let actual = self.line_state();
self
}
}
by making PCF8574Inner "Copy". This removes a lot of the borrows you need below.
> + let actual = self.line_state();
> +
> + let mut diff = actual ^ prev;
> + while diff != 0 {
> + let line = diff.trailing_zeros() as u8;
> + if let Some(handler) = self.handler.get(line as usize) {
> + if handler.get() {
> + handler.set((actual >> line) & 1 == 1);
> + }
There's no need to check if a GPIO line is connected, InterruptSource
handles it. (This can also be changed in the C version).
> + fn recv(&self) -> Result<bool> {
> + let line_state = self.line_state();
> + if self.inner.borrow().lastrq != line_state {
> + self.inner.borrow_mut().lastrq = line_state;
Same here, part of the logic can be moved to PCF8574Inner:
pub fn receive(&mut self, data: u32) -> (bool, u32) {
let prev = self.lastrq;
let actual = self.line_state();
self.lastrq = actual;
(prev != actual, actual)
}
The second element of the tuple is needed because...
> + if self.intrq.get() {
> + self.intrq.raise();
> + }
> + }
> +
> + Ok(true)
... RECV should return a byte, not a bool.
> + }
> +
> + fn realize(&self) -> util::Result<()> {
> + self.init_gpio_in(self.handler_size(), PCF8574State::gpio_set);
> + self.init_gpio_out(from_ref(&self.handler[0]));
> + self.init_gpio_out_named(from_ref(&self.intrq), "nINT", 1);
> + Ok(())
> + // unsafe {
> + // bindings::qdev_init_gpio_in(addr_of_mut!(*self),
> Self::gpio_set, Self::handler_size as c_int);
> + // bindings::qdev_init_gpio_out(addr_of_mut!(*self),
> InterruptSource::slice_as_ptr(self.handler), Self::handler_size as c_int);
> + // bindings::qdev_init_gpio_out_named(addr_of_mut!(*self),
> self.intrq, c"nINT", 1);
> + // }
> + }
> +
> + fn gpio_set(&self, line: u32, level: u32) {
> + assert!(line < self.handler_size());
> +
> + match level {
> + 0 => self.inner.borrow_mut().input &= !(1 << line),
> + _ => self.inner.borrow_mut().input |= 1 << line,
> + }
Again, move this to PCF8574Inner. deposit is in
common::bitops::IntegerExt and lets you write this:
pub fn set_input(&mut self, n: u32, value: u32) -> bool {
self.input = self.input.deposit(n, 1, value);
self.line_state() != self.lastrq
}
> + if self.line_state() != self.inner.borrow().lastrq &&
> self.intrq.get() {
> + self.intrq.raise();
> + }
> + }
> +
> + fn handler_size(&self) -> u32 {
> + self.handler.len() as u32
> + }
> +!(
> + fn line_state(&self) -> u8 {
> + let inner = self.inner.borrow();
> + inner.input & inner.output
> + }
> +
> + fn reset_hold(&self, _type: ResetType) {}
This should clear PCF8574Inner (it can be another method in there).
> +}
> +
> +impl_vmstate_struct!(
> + PCF8574StateInner,
> + VMStateDescriptionBuilder::<PCF8574StateInner>::new()
> + .name(c"pcf8574/inner")
> + .version_id(0)
> + .minimum_version_id(0)
> + .fields(vmstate_fields! {
> + vmstate_of!(PCF8574StateInner, lastrq),
> + vmstate_of!(PCF8574StateInner, input),
> + vmstate_of!(PCF8574StateInner, output),
> + })
> + .build()
> +);
> +
> +pub const VMSTATE_PCF8574: VMStateDescription<PCF8574State> =
> + VMStateDescriptionBuilder::<PCF8574State>::new()
> + .name(c"pcf8574")
> + .version_id(0)
> + .minimum_version_id(0)
> + .fields(vmstate_fields! {
> + // TODO
> + // vmstate_of!(PCF8574State, parent_obj),
You're right, for this to work you'll need to add
impl_vmstate_transparent!(ParentField<T> where T: VMState);
to rust/qom/src/qom.rs and
impl_vmstate_transparent!(std::mem::ManuallyDrop<T> where T: VMState);
to rust/migration/src/vmstate.rs. I'll send a patch for this.
But really -- not a bad job at all! Thanks very much for trying,
remember that Zhao and Manos and myself have been working on this for
over a year. Your experience and your suggestions will be precious,
so please let us know what can be done better.
Paolo
