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

[Paolo Bonzini]

On Thu, Feb 27, 2025 at 6:25 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> 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 ?

Just because the IRQ update is needed in many places and the chardev
backend only in one place.

> 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.

Yeah, if I was writing from scratch I would probably call update()
unconditionally. If it turns out to be inefficient you could cache the
current value of

        let flags = regs.int_level & regs.int_enabled;

in PL011State as a BqlCell.

> 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 ?

Ideally nothing, especially considering that more modern devices have
edge-triggered interrupts like MSIs, instead of level-triggered
interrupts that need qemu_set_irq() calls. But if you have something a
lot more complex than a bool I would pass down the PL011State and do
something like pl011.schedule_update_irq() which updates a BqlCell<>.
The device could then use a bottom half or process them after
"drop(regs)".

HPET has another approach, which is to store a backpointer from
HPETTimer to the HPETState, so that it can do

    self.get_state().irqs[route].pulse();

without passing down anything. The reason for this is that it has
multiple timers on the same routine, and it assigns the timers to
separate HPETTimers. I would not use it for PL011 because all accesses
to the PL011Registers go through the PL011State.

A while ago I checked how OpenVMM does it, and basically it does not
have the PL011State/PL011Registers separation at all: the devices are
automatically wrapped with a Mutex and memory accesses take a &mut.
That removes some of the complexity, but also a lot of flexibility.

Unfortunately, before being able to reason on how to make peace with
the limitations of safe Rust, it was necessary to spend a lot of time
writing API abstractions, otherwise you don't actually experience the
limitations. But that means that the number of lines of code in
qemu_api is not representative of my experience using it. :( I am
sorry this isn't a great answer yet; certainly some aspects of the
PL011 or HPET devices could be treated as a blueprint for future
devices, but which and how to document that is something where I would
like to consult with an actual Rust maven.

Paolo