On 5/5/25 14:26, Manos Pitsidianakis wrote:
Something I do notice is that there's some inconsistency in
how we've structured things between the two devices, e.g.:
* the pl011 main source file is device.rs, but the hpet one
is hpet.rs
* some places we use the actual names of bits in registers
(eg Interrupt's OE, BE, etc consts), and some places we
seem to have renamed them (e.g. pl011 Flags has clear_to_send
not CTS, etc)
* pl011 has defined named fields for its registers, but hpet does
things like::
self.config.get() & (1 << HPET_CFG_LEG_RT_SHIFT) != 0
* pl011 has a split between PL011State and PL011Registers,
but HPET does not. As I mentioned in an email thread a
little while back, I feel like the State/Registers split
is something we should either make a more clear deliberate
formalised separation that's part of how we recommend
device models should be designed
[...]
I think it would be good to figure out what we think is the
right, best style, for writing this kind of thing, and be
consistent. We have long standing problems in the C device
models where there are multiple different styles for how
we write them, and it would be good to try to aim for
more uniformity on the Rust side.
The pl011 stuff was deliberate decisions:
- device.rs vs pl011.rs: the device was written as a crate, so it's
essentially its own library, plus pl011/src/pl011.rs would be
redundant :)
Right, I think Peter's comment was more about moving hpet.rs to
device.rs, and merging PL011's device_class.rs into its device.rs.
That said, it's not important, we can choose either convention. I
like the less redundancy and separation of concerns: if pl011 gets
converted into a module in a future refactor, it could keep its
functionality split into different submodules and `pl011.rs` or
`pl011/mod.rs` would be the device module.
I think it's okay to decide that Rust devices will have mini
directories: it's just the style of the language and Cargo more or less
relies on having lib.rs.
In a vacuum I would prefer to have hw/char/pl011.rs for what is now
rust/hw/char/pl011/lib.rs, and place the other files in hw/char/pl011;
IIRC rustc accepts that style. However we still rely on Cargo for some
things(*), and as long as we do there's not much we can do about it.
(*) notably "cargo fmt". Everything else is more or less handled
by Meson starting with 1.8.0.
- Using typed registers instead of constants: yes coming from C I can
understand it can feel unfamiliar. I specifically wanted to make the
register fields typed to avoid making the implementation a "C port",
and I think it's worthwhile to use the type system as much as
possible.
Peter's comments (especially the second and third) were about two kinds
of inconsistencies:
1) HPET not using bilge. This was because Zhao looked at HPET from the
opposite direction compared to what you did on pl011, namely avoiding
unsafe and modeling the BQL properly, and sacrificed a bit the usage of
idiomatic code like what bilge provides.
I think that you made the right choice for the first device and he made
the right choice for the second device. But now someone should look at
HPET and do the work that you did to adopt bilge.
2) The choice between bilge on one side, and bitflags or integers on the
other. For pl011 you kept interrupt bits as integers for example, and
this is related to the topic of (non-)availability of const in traits...
A straight C port would result into integer constants with integer
typed fields everywhere for registers/flags.
Yes, From/Into aren't const, at least yet, but it's not really a
hotpath performance wise. I think non-dynamically dispatched trait
methods can be inlined by annotating the `fn from(..)` impl with a
`#[inline(always)]` hint but I haven't confirmed this, just
speculation.
It's not about hot paths, it's more that 1) you cannot use From/Into in
a "static"'s initializer 2) bilge relies a lot on non-const methods in
its internal implementation, which makes it quite messy to use it in
some places. See for example this thing for which I take all the blame:
impl Data {
// bilge is not very const-friendly, unfortunately
pub const BREAK: Self = Self { value: 1 << 10 };
}
and the same would be true of interrupt constants and the IRQMASK array.
The separate bilge and bitflags worlds are what bothers me the most in
the experimental devices. I can see why they would be very confusing
for someone who's not had much experience with Rust, and therefore
doesn't know *why* they are separate.
Again, no strong opinions here. I like the "everything is typed"
approach and I think it's worth it to explore it because it allows us
to "make invalid/illegal states unrepresentable" as one sage article
goes.
I agree, and it's why I think you made the right choice using it for
pl011. With all the unsafe code that you had to use, strong-typing at
least showed *something* that Rust could provide compared to C(*). And
I like the strong typing too, even if I'm not sure I like bilge's
*implementation* that much anymore.
I'm not really up for rewriting it, but then I've also done more stupid
rewrites in the past. :)
(*) when we were discussing safety vs. unsafety last summer, I may
have sounded dismissive of this kind of benefit. My point at the
time was that unsafe code was so much more complex than C, that
the benefit of strong-typing wasn't enough to *offset* the
complexity of unsafe code. But it is absolutely present.
Related to this I have found recently the `attrs crate
<https://docs.rs/attrs/>`__, which provides an easy way to parse the
contents of attributes in a procedural macro.
I actually have some WIP patches for this I put a pause on and can
continue e.g.
https://gitlab.com/epilys/rust-for-qemu/-/commit/c2c97caaaf03273fabc14aee5a4d1499668ddbe3
The repository is private, but I look forward to seeing it! If you want
to post an RFC without even any code, just to show what the device code
looks like, that would be helpful as it will catch stuff like lack of
type safety.
BTW, if you need it to model reflection better I think it is acceptable
to assume const_refs_to_static is present.
Paolo
