On Thu, Jul 4, 2024 at 2:16 PM Manos Pitsidianakis <manos.pitsidiana...@linaro.org> wrote: > +ARM PL011 Rust device > +M: Manos Pitsidianakis <manos.pitsidiana...@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