Re: [PATCH v3] rust/pl011: Fix DeviceID reads

[Paolo Bonzini]

On Mon, Nov 18, 2024 at 3:46 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> Yep, const fns are evaluated at compile time

This is not correct; "const fn"s *can* be evaluated at compile time.
In this case they won't since "self" is only available at run time.
The match statements persist in the final compiled code.

> >Perhaps this comes down to unfamiliarity with the way macros are working
> >here but in general macros should be eliding boilerplate to allow us to
> >concisely represent the relevant data and functionality. Here it adds an
> >additional indirection when reading the code just to see what is going
> >on.
>
> Well in the previous patch versions the concern was that it was
> "verbose". But even if the registers are available as memory mapped in
> the device it's the wrong abstraction to use here; this is a higher
> level language than C and using indices is a "when you have a hammer
> everything looks like a nail" situation.
>
> We have different programming paradigms in Rust, allowing as to have
> documented code (via rustdoc), so either we take advantage of Rust or we
> don't.

Your usage of macros also is a "when you have a hammer everything
looks like a nail" situation. You're going straight from code that
might be a bit C-like to code that looks like Perl. Rust macros can be
useful to reduce boilerplate, but in this case the macro-based
solution does not make it much easier to see all values at once for
verification purposes, and makes it harder to understand the code's
behavior.

What makes code more "Rusty" would be things like using Result for
error handling, taking advantage of the type system, using traits for
polymorphism, using references to guarantee memory safety. QEMU is
hardly doing any of this, and the rustdoc for qemu_api is also just
starting, so maybe this patch is getting a bit ahead of ourselves?

It's not reasonable to use 50 lines of code just for eight
identification bytes. Even the spec says "The registers can
conceptually be treated as a 32-bit register", which to me suggests
making its contents a u32 const and calling it a day. But if you want
to make it overengineered but sensible, then do this:

const fn getPartNumber(self) -> u12 {0x11}
const fn getDesignerID(self) -> u8 {match ...}
const fn getRevision(self) -> u4 {0x1}
const fn getConfiguration(self) -> u8 {match ...}

and then a getPeriphId(idx: u8) that puts the four together in a u32
and returns ((val32 >> (8 * idx)) & 255) as u8.

For PCellID, the spec itself says the value is 0xB105F00D, so *please*
just hardcode them in the read method. Anything else is a waste of
everybody's time. And since we're in hard freeze, maybe we should
reconsider the one line fix and leave everything for later.

> I had sent patches on a previous patch series that only some patches of
> them were picked up for merging, I plan to send a new revision soon

Cool, thanks.

Paolo