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
