Re: Rust in QEMU update, April 2025

[Paolo Bonzini <bonzini@gnu.org>]

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