On 10/28/2025 8:14 PM, Paolo Bonzini wrote:
> 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
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
>> 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
