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

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 27 Feb 2025 at 18:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Thu, Feb 27, 2025 at 6:25 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > 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.

I think the idea I vaguely have in the back of my mind is that
maybe it's a nice idea to have a coding structure that enforces
"you update your own internal state, and only then do things that
might involve calling out to the outside world, and if there's
something that you need to do with the result of that callout,
there's an easy mechanism for 'this is what I will want to do
next after that' continuations".

The fact that our C device implementations don't do that is
kind of the underlying cause of all the "recursive entry back
into the device via DMA" problems that we squashed with the
big hammer of "just forbid it entirely". It's also in a way
the problem underlying the race condition segfault here:
https://lore.kernel.org/qemu-devel/CAFEAcA9odnPo2LPip295Uztri7JfoVnQbkJ=Wn+k8dQneB_ynQ@mail.gmail.com/T/#u
(memory_region_snapshot_and_clear_dirty() drops the BKL, no
callers expect that, segfaults in the calling code in the
framebuffer device models if something else gets in and
e.g. resizes the framebuffer in the middle of a display update).

So I was sort of wondering if the pl011 structure was aiming
at providing that kind of separation of "internal state" and
"external interactions", such that the compiler would complain
if you tried to do an externally-interacting thing while your
internal state was not consistent.

thanks
-- PMM