Re: [PATCH 4/5] rust: pl011: switch to safe chardev operation

[Peter Maydell]

On Thu, 27 Feb 2025 at 16:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Switch bindings::CharBackend with chardev::CharBackend.  This removes
> occurrences of "unsafe" due to FFI and switches the wrappers for receive,
> can_receive and event callbacks to the common ones implemented by
> chardev::CharBackend.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


> @@ -567,21 +552,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) 
> {

> -            update_irq = self.regs.borrow_mut().write(
> -                field,
> -                value as u32,
> -                addr_of!(self.char_backend) as *mut _,
> -            );
> +            update_irq = self
> +                .regs
> +                .borrow_mut()
> +                .write(field, value as u32, &self.char_backend);
>          } else {
>              eprintln!("write bad offset {offset} value {value}");
>          }

Entirely unrelated to this patch, but seeing this go past
reminded me that I had a question I didn't get round to
asking in the community call the other day. In this
PL011State::write function, we delegate the job of
updating the register state to PL011Registers::write,
which returns a bool to tell us whether to call update().

I guess the underlying design idea here is "the register
object updates itself and tells the device object what
kinds of updates to the outside world it needs to do" ?
But then, why is the irq output something that PL011State
needs to handle itself whereas the chardev backend is
something we can pass into PL011Registers ?

In the C version, we just call pl011_update() where we
need to; we could validly call it unconditionally for any
write, we're just being (possibly prematurely) efficient
by avoiding a call when we happen to know that the register
write didn't touch any of the state that pl011_update()
cares about. So it feels a bit odd to me that in the Rust
version this "we happen to know that sometimes it would be
unnecessary to call the update function" has been kind of
promoted to being part of an interface between the two
different types PL011Registers and PL011State.

Thinking about other devices, presumably for more complex
devices we might need to pass more than just a single 'bool'
back from PL011Registers::write. What other kinds of thing
might we need to do in the FooState function, and (since
the pl011 code is presumably going to be used as a template
for those other devices) is it worth having something that
expresses that better than just a raw 'bool' return ?

thanks
-- PMM