On Mon, May 5, 2025 at 4:45 PM Paolo Bonzini <bonz...@gnu.org> wrote:
>
> 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...
Oh right.
>
> > 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:
Yeah it's not nice that we can't use it in static/const initializers.
>
> 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.
It has its pros and cons that's for sure... there are many crates that
let you define typed bitfields, I haven't looked into the current
state of the art lately. We should definitely move on to something
better if it exists now or in the future.
> 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.
Oops, I forgot I had archived it because I moved development to
https://gitlab.com/epilys/qemu . I made it public again. Bear in mind
this was a WIP, basically my git stash but committed.
>
> BTW, if you need it to model reflection better I think it is acceptable
> to assume const_refs_to_static is present.
>
> Paolo
