Re: [RFC PATCH v4 4/7] rust: add PL011 device model

[Paolo Bonzini]

On Thu, Jul 4, 2024 at 2:16 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> +ARM PL011 Rust device
> +M: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> +S: Maintained
> +F: rust/pl011/

No need for this, since it's covered by rust/. If (when) it replaces
the main one, the PL011-specific stanza will be assigned to ARM
maintainers (while you keep covering it via rust/).


> +if with_rust
> +  subdir('rust')
> +endif

Should be in patch 3.

> +subdir('pl011')

As I said before it should be handled via Kconfig, but let's do that
after the initial merge. However...

> +correctness = { level = "deny", priority = -1 }
> +suspicious = { level = "deny", priority = -1 }
> +complexity = { level = "deny", priority = -1 }
> +perf = { level = "deny", priority = -1 }
> +cargo = { level = "deny", priority = -1 }
> +nursery = { level = "deny", priority = -1 }
> +style = { level = "deny", priority = -1 }
> +# restriction group
> +dbg_macro = "deny"
> +rc_buffer = "deny"
> +as_underscore = "deny"

... repeated lints really suggest that you should use a workspace and
a single cargo invocation to build both the rust-qapi and pl011
crates, which I think is doable if you can run bindgen just once.

> +use core::{mem::MaybeUninit, ptr::NonNull};

Let's remove at least this unsafety.

> +#[used]
> +pub static VMSTATE_PL011: VMStateDescription = VMStateDescription {
> +    name: PL011_ARM_INFO.name,
> +    unmigratable: true,
> +    ..unsafe { MaybeUninit::<VMStateDescription>::zeroed().assume_init() }
> +};
> +
> +#[no_mangle]
> +pub unsafe extern "C" fn pl011_init(obj: *mut Object) {
> +    assert!(!obj.is_null());
> +    let mut state = NonNull::new_unchecked(obj.cast::<PL011State>());
> +    state.as_mut().init();

This is fine for now, but please add a

// TODO: this assumes that "all zeroes" is a valid state for all fields of
// PL011State. This is not necessarily true of any #[repr(Rust)] structs,
// including bilge-generated types. It should instead use MaybeUninit.

> +}
> +
> +qemu_api::module_init! {
> +    qom: register_type => {
> +        type_register_static(&PL011_ARM_INFO);
> +    }

Can you make the macro look like

   MODULE_INIT_QOM: fn register_type() {
     ...
   }

so that it's clear what "register_type" is, and so that it's easier to
extend it to more values?

> +    #[doc(alias = "clk")]
> +    pub clock: NonNull<Clock>,

It's null when init() runs, so please use *mut Clock.

> +    #[doc(alias = "migrate_clk")]
> +    pub migrate_clock: bool,

Please put all properties together in the struct for readability.

> +}
> +
> +#[used]
> +pub static CLK_NAME: &CStr = c"clk";
> +
> +impl PL011State {
> +    pub fn init(&mut self) {
> +        unsafe {
> +            memory_region_init_io(
> +                addr_of_mut!(self.iomem),
> +                addr_of_mut!(*self).cast::<Object>(),
> +                &PL011_OPS,
> +                addr_of_mut!(*self).cast::<c_void>(),
> +                PL011_ARM_INFO.name,
> +                0x1000,
> +            );
> +            let sbd = addr_of_mut!(*self).cast::<SysBusDevice>();
> +            let dev = addr_of_mut!(*self).cast::<DeviceState>();
> +            sysbus_init_mmio(sbd, addr_of_mut!(self.iomem));
> +            for irq in self.interrupts.iter_mut() {
> +                sysbus_init_irq(sbd, irq);
> +            }
> +            self.clock = NonNull::new(qdev_init_clock_in(
> +                dev,
> +                CLK_NAME.as_ptr(),
> +                None, /* pl011_clock_update */
> +                addr_of_mut!(*self).cast::<c_void>(),
> +                ClockEvent_ClockUpdate,
> +            ))
> +            .unwrap();
> +        }
> +    }
> +
> +    pub fn read(&mut self, offset: hwaddr, _size: core::ffi::c_uint) -> u64 {
> +        use RegisterOffset::*;
> +
> +        match RegisterOffset::try_from(offset) {
> +            Err(v) if (0x3f8..0x400).contains(&v) => {
> +                u64::from(PL011_ID_ARM[((offset - 0xfe0) >> 2) as usize])
> +            }
> +            Err(_) => {
> +                // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 
> 0x%x\n", (int)offset);
> +                0
> +            }
> +            Ok(DR) => {
> +                // s->flags &= ~PL011_FLAG_RXFF;
> +                self.flags.set_receive_fifo_full(false);
> +                let c = self.read_fifo[self.read_pos];
> +                if self.read_count > 0 {
> +                    self.read_count -= 1;
> +                    self.read_pos = (self.read_pos + 1) & (self.fifo_depth() 
> - 1);
> +                }
> +                if self.read_count == 0 {
> +                    // self.flags |= PL011_FLAG_RXFE;
> +                    self.flags.set_receive_fifo_empty(true);
> +                }
> +                if self.read_count + 1 == self.read_trigger {
> +                    //self.int_level &= ~ INT_RX;
> +                    self.int_level &= !registers::INT_RX;
> +                }
> +                // Update error bits.
> +                self.receive_status_error_clear = c.to_be_bytes()[3].into();
> +                self.update();
> +                unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) };

Please add a comment here like

// TODO: this causes a callback that creates another "&mut self".
// This is forbidden by Rust aliasing rules and has to be fixed
// using interior mutability.

Paolo