[RFC PATCH v2 5/5] rust/hw/gpio: Add the the first gpio device pcf8574

[chenmiao]

After implementing the I2CBus and I2CSlave, we proceeded to implement a basic
GPIO device — the PCF8574 — which depends on I2CSlave.

If we had directly referred to the PL011 implementation, we would have been
able to achieve a basic version of the PCF8574. However, during the
implementation, the excessive number of types and conversions led to various
comprehension conflicts and cognitive overhead.

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.

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?

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. Moreover, the vmstate configuration also needed to be
declared separately. Although I could infer its meaning, I still hope that a
more rational and streamlined process can be established.

At present, I have summarized the general workflow for device modeling in Rust
as follows:

1. Modify the configuration under hw/deviceto distinguish between C and Rust
versions.
2. Create a device crate under rust/hw.
3. Add (or copy) the necessary wrappers, build.rs, and bindings.
4. Begin the device modeling process.
5. Construct the corresponding structures in Rust that mirror those in C,
especially for “members that may change”.
6. Referencing the C implementation, define initialization functions and
establish parent-class relationships for the Rust structure.
7. Set up ObjectImpl, DeviceImpl, and ResettablePhasesImpl.
8. Configure vmstate.
9. Implement other functional methods.

Signed-off-by: chenmiao <chen...@openatom.club>
---
hw/gpio/Kconfig | 5 +
hw/gpio/meson.build | 2 +-
rust/Cargo.lock | 21 +++-
rust/Cargo.toml | 1 +
rust/hw/Kconfig | 1 +
rust/hw/core/src/i2c.rs | 22 ++++
rust/hw/gpio/Kconfig | 2 +
rust/hw/gpio/meson.build | 1 +
rust/hw/gpio/pcf8574/Cargo.toml | 31 +++++
rust/hw/gpio/pcf8574/build.rs | 1 +
rust/hw/gpio/pcf8574/meson.build | 50 ++++++++
rust/hw/gpio/pcf8574/src/bindings.rs | 29 +++++
rust/hw/gpio/pcf8574/src/device.rs | 180 +++++++++++++++++++++++++++
rust/hw/gpio/pcf8574/src/lib.rs | 4 +
rust/hw/gpio/pcf8574/wrapper.h | 51 ++++++++
rust/hw/meson.build | 1 +
16 files changed, 400 insertions(+), 2 deletions(-)
create mode 100644 rust/hw/gpio/Kconfig
create mode 100644 rust/hw/gpio/meson.build
create mode 100644 rust/hw/gpio/pcf8574/Cargo.toml
create mode 120000 rust/hw/gpio/pcf8574/build.rs
create mode 100644 rust/hw/gpio/pcf8574/meson.build
create mode 100644 rust/hw/gpio/pcf8574/src/bindings.rs
create mode 100644 rust/hw/gpio/pcf8574/src/device.rs
create mode 100644 rust/hw/gpio/pcf8574/src/lib.rs
create mode 100644 rust/hw/gpio/pcf8574/wrapper.h

diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
index a209294c20..1be534aaf6 100644
--- a/hw/gpio/Kconfig
+++ b/hw/gpio/Kconfig
@@ -27,6 +27,11 @@ config PCA9554
config PCF8574
bool
depends on I2C
+ select PCF8574_C if !HAVE_RUST
+ select X_PCF8574_RUST if HAVE_RUST
+
+config PCF8574_C
+ bool

config ZAURUS_SCOOP
bool
diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
index 74840619c0..9e4b1a8fd7 100644
--- a/hw/gpio/meson.build
+++ b/hw/gpio/meson.build
@@ -17,4 +17,4 @@ system_ss.add(when: 'CONFIG_RASPI', if_true: files(
system_ss.add(when: 'CONFIG_STM32L4X5_SOC', if_true: files('stm32l4x5_gpio.c'))
system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_gpio.c'))
system_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true: files('sifive_gpio.c'))
-system_ss.add(when: 'CONFIG_PCF8574', if_true: files('pcf8574.c'))
+system_ss.add(when: 'CONFIG_PCF8574_C', if_true: files('pcf8574.c'))
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index 0c1df625df..be9c2eb351 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -1,6 +1,6 @@
# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
-version = 3
+version = 4

[[package]]
name = "anyhow"
@@ -204,6 +204,25 @@ dependencies = [
"util",
]

+[[package]]
+name = "pcf8574"
+version = "0.1.0"
+dependencies = [
+ "bilge",
+ "bilge-impl",
+ "bits",
+ "bql",
+ "chardev",
+ "common",
+ "glib-sys",
+ "hwcore",
+ "migration",
+ "qom",
+ "system",
+ "trace",
+ "util",
+]
+
[[package]]
name = "pkg-config"
version = "0.3.32"
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 783e626802..6a17eefe49 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -10,6 +10,7 @@ members = [
"system",
"hw/core",
"hw/char/pl011",
+ "hw/gpio/pcf8574",
"hw/timer/hpet",
"trace",
"util",
diff --git a/rust/hw/Kconfig b/rust/hw/Kconfig
index 36f92ec028..ba1297ca2d 100644
--- a/rust/hw/Kconfig
+++ b/rust/hw/Kconfig
@@ -1,3 +1,4 @@
# devices Kconfig
source char/Kconfig
+source gpio/Kconfig
source timer/Kconfig
diff --git a/rust/hw/core/src/i2c.rs b/rust/hw/core/src/i2c.rs
index 2d297dbd6e..290dc56590 100644
--- a/rust/hw/core/src/i2c.rs
+++ b/rust/hw/core/src/i2c.rs
@@ -281,6 +281,15 @@ pub trait I2CSlaveImpl: DeviceImpl + IsA<I2CSlave> {
T::MATCH_AND_ADD.unwrap()(unsafe { state.as_ref() }, address, broadcast, current_devs).is_ok()
}

+// const I2C_BROADCAST: usize = 0x0;
+
+// impl I2CSlave {
+// pub unsafe fn post_load(&self, _version_id: u32) -> Result<(), migration::InvalidError> {
+// // TODO
+// Ok(())
+// }
+// }
+
impl I2CSlaveClass {
/// Fill in the virtual methods of `I2CSlaveClass` based on the
/// definitions in the `I2CSlaveImpl` trait.
@@ -397,3 +406,16 @@ fn from(event: I2CEvent) -> Self {
}
}
}
+
+// 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()
+// );
diff --git a/rust/hw/gpio/Kconfig b/rust/hw/gpio/Kconfig
new file mode 100644
index 0000000000..c47aa76f14
--- /dev/null
+++ b/rust/hw/gpio/Kconfig
@@ -0,0 +1,2 @@
+config X_PCF8574_RUST
+ bool
diff --git a/rust/hw/gpio/meson.build b/rust/hw/gpio/meson.build
new file mode 100644
index 0000000000..908991ad13
--- /dev/null
+++ b/rust/hw/gpio/meson.build
@@ -0,0 +1 @@
+subdir('pcf8574')
diff --git a/rust/hw/gpio/pcf8574/Cargo.toml b/rust/hw/gpio/pcf8574/Cargo.toml
new file mode 100644
index 0000000000..e3647f67a3
--- /dev/null
+++ b/rust/hw/gpio/pcf8574/Cargo.toml
@@ -0,0 +1,31 @@
+[package]
+name = "pcf8574"
+version = "0.1.0"
+authors = ["Chen Miao <chen...@openatom.club>"]
+description = "pcf8574 device model for QEMU"
+resolver = "2"
+publish = false
+
+edition.workspace = true
+homepage.workspace = true
+license.workspace = true
+repository.workspace = true
+rust-version.workspace = true
+
+[dependencies]
+glib-sys.workspace = true
+bilge = { version = "0.2.0" }
+bilge-impl = { version = "0.2.0" }
+bits = { path = "../../../bits" }
+common = { path = "../../../common" }
+util = { path = "../../../util" }
+bql = { path = "../../../bql" }
+migration = { path = "../../../migration" }
+qom = { path = "../../../qom" }
+chardev = { path = "../../../chardev" }
+system = { path = "../../../system" }
+hwcore = { path = "../../../hw/core" }
+trace = { path = "../../../trace" }
+
+[lints]
+workspace = true
diff --git a/rust/hw/gpio/pcf8574/build.rs b/rust/hw/gpio/pcf8574/build.rs
new file mode 120000
index 0000000000..5f5060db35
--- /dev/null
+++ b/rust/hw/gpio/pcf8574/build.rs
@@ -0,0 +1 @@
+../../../util/build.rs
\ No newline at end of file
diff --git a/rust/hw/gpio/pcf8574/meson.build b/rust/hw/gpio/pcf8574/meson.build
new file mode 100644
index 0000000000..64de86675f
--- /dev/null
+++ b/rust/hw/gpio/pcf8574/meson.build
@@ -0,0 +1,50 @@
+# TODO: Remove this comment when the clang/libclang mismatch issue is solved.
+#
+# Rust bindings generation with `bindgen` might fail in some cases where the
+# detected `libclang` does not match the expected `clang` version/target. In
+# this case you must pass the path to `clang` and `libclang` to your build
+# command invocation using the environment variables CLANG_PATH and
+# LIBCLANG_PATH
+_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,
+)
+
+_libpcf8574_rs = static_library(
+ 'pcf8574',
+ structured_sources(
+ [
+ 'src/lib.rs',
+ 'src/bindings.rs',
+ 'src/device.rs',
+ ],
+ {'.' : _libpcf8574_bindings_inc_rs},
+ ),
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'rust',
+ dependencies: [
+ bilge_rs,
+ bilge_impl_rs,
+ bits_rs,
+ common_rs,
+ glib_sys_rs,
+ util_rs,
+ migration_rs,
+ bql_rs,
+ qom_rs,
+ chardev_rs,
+ system_rs,
+ hwcore_rs,
+ trace_rs
+ ],
+)
+
+rust_devices_ss.add(when: 'CONFIG_X_PCF8574_RUST', if_true: [declare_dependency(
+ link_whole: [_libpcf8574_rs],
+ variables: {'crate': 'pcf8574'},
+)])
diff --git a/rust/hw/gpio/pcf8574/src/bindings.rs b/rust/hw/gpio/pcf8574/src/bindings.rs
new file mode 100644
index 0000000000..2c1b236f29
--- /dev/null
+++ b/rust/hw/gpio/pcf8574/src/bindings.rs
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#![allow(
+ dead_code,
+ improper_ctypes_definitions,
+ improper_ctypes,
+ non_camel_case_types,
+ non_snake_case,
+ non_upper_case_globals,
+ unnecessary_transmutes,
+ unsafe_op_in_unsafe_fn,
+ clippy::pedantic,
+ clippy::restriction,
+ clippy::style,
+ clippy::missing_const_for_fn,
+ clippy::ptr_offset_with_cast,
+ clippy::useless_transmute,
+ clippy::missing_safety_doc,
+ clippy::too_many_arguments
+)]
+
+//! `bindgen`-generated declarations.
+
+use glib_sys::{GHashTable, GHashTableIter, GList, GPtrArray, GQueue, GSList};
+
+#[cfg(MESON)]
+include!("bindings.inc.rs");
+
+#[cfg(not(MESON))]
+include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs"));
diff --git a/rust/hw/gpio/pcf8574/src/device.rs b/rust/hw/gpio/pcf8574/src/device.rs
new file mode 100644
index 0000000000..99f5f2aed9
--- /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 <chen...@openatom.club>
+// 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,
+}
+
+#[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;
+}
+
+impl PCF8574State {
+ fn send(&self, data: u8) -> Result<bool> {
+ let prev = self.line_state();
+ self.inner.borrow_mut().output = data;
+ 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);
+ }
+ }
+ diff &= !(1 << line);
+ }
+
+ if self.intrq.get() {
+ self.intrq.set(actual == self.inner.borrow().lastrq);
+ }
+
+ Ok(true)
+ }
+
+ 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;
+ if self.intrq.get() {
+ self.intrq.raise();
+ }
+ }
+
+ Ok(true)
+ }
+
+ 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,
+ }
+
+ 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) {}
+}
+
+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),
+ vmstate_of!(PCF8574State, inner),
+ })
+ .build();
diff --git a/rust/hw/gpio/pcf8574/src/lib.rs b/rust/hw/gpio/pcf8574/src/lib.rs
new file mode 100644
index 0000000000..0dec7334a6
--- /dev/null
+++ b/rust/hw/gpio/pcf8574/src/lib.rs
@@ -0,0 +1,4 @@
+mod bindings;
+mod device;
+
+pub const TYPE_PCF8574: &::std::ffi::CStr = c"pcf8574";
diff --git a/rust/hw/gpio/pcf8574/wrapper.h b/rust/hw/gpio/pcf8574/wrapper.h
new file mode 100644
index 0000000000..29c7bdf5e6
--- /dev/null
+++ b/rust/hw/gpio/pcf8574/wrapper.h
@@ -0,0 +1,51 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2024 Linaro Ltd.
+ *
+ * Authors: Manos Pitsidianakis <manos.pit...@linaro.org>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+
+/*
+ * This header file is meant to be used as input to the `bindgen` application
+ * in order to generate C FFI compatible Rust bindings.
+ */
+
+#ifndef __CLANG_STDATOMIC_H
+#define __CLANG_STDATOMIC_H
+/*
+ * Fix potential missing stdatomic.h error in case bindgen does not insert the
+ * correct libclang header paths on its own. We do not use stdatomic.h symbols
+ * in QEMU code, so it's fine to declare dummy types instead.
+ */
+typedef enum memory_order {
+ memory_order_relaxed,
+ memory_order_consume,
+ memory_order_acquire,
+ memory_order_release,
+ memory_order_acq_rel,
+ memory_order_seq_cst,
+} memory_order;
+#endif /* __CLANG_STDATOMIC_H */
+
+#include "qemu/osdep.h"
+#include "hw/gpio/pcf8574.h"
diff --git a/rust/hw/meson.build b/rust/hw/meson.build
index 9749d4adfc..d6b273170e 100644
--- a/rust/hw/meson.build
+++ b/rust/hw/meson.build
@@ -1,2 +1,3 @@
subdir('char')
+subdir('gpio')
subdir('timer')
--
2.43.0

[chenmiao]

[Chao Liu]

Reviewed-by: Chao Liu <chao...@openatom.club>

[chenmiao]

--
2.43.0

[Paolo Bonzini]

On Tue, Oct 28, 2025 at 11:19 AM chenmiao <chen...@openatom.club> 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.

This is small and simple enough that it can be Copy.

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

[Paolo Bonzini]

On Tue, Oct 28, 2025 at 12:26 PM Paolo Bonzini <pbon...@redhat.com> wrote:
> 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

Very important typo: you are *now* seeing the code from the point of
view of someone implementing the Rust-to-C bridges.

Paolo

[Chen Miao]

On 10/28/2025 8:14 PM, Paolo Bonzini wrote:
> On Tue, Oct 28, 2025 at 12:26 PM Paolo Bonzini <pbon...@redhat.com> wrote:
>> 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
> Very important typo: you are *now* seeing the code from the point of
> view of someone implementing the Rust-to-C bridges.
>
> Paolo

Yes, I' ve come to realize this very clearly; I' ve found that many of the
suggestions you gave actually stem from my failure to fully grasp the
underlying design principles!

Chen Miao

[Paolo Bonzini]

On Tue, Oct 28, 2025 at 1:19 PM Chen Miao <chen...@openatom.club> wrote:
>
> On 10/28/2025 8:14 PM, Paolo Bonzini wrote:
> > On Tue, Oct 28, 2025 at 12:26 PM Paolo Bonzini <pbon...@redhat.com> wrote:
> >> 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
> > Very important typo: you are *now* seeing the code from the point of
> > view of someone implementing the Rust-to-C bridges.
> >
> > Paolo
>
> Yes, I' ve come to realize this very clearly; I' ve found that many of the
> suggestions you gave actually stem from my failure to fully grasp the
> underlying design principles!

Even more than that, you thought that the work would be easy. :) The
good news is that you were still able to do it.

Paolo