On Tue, Oct 28, 2025 at 12:26 PM Paolo Bonzini <[email protected]> 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
> 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
