On Wed, Feb 19, 2025 at 10:25:40AM +0100, Paolo Bonzini wrote:
> Date: Wed, 19 Feb 2025 10:25:40 +0100
> From: Paolo Bonzini <pbonz...@redhat.com>
> Subject: Re: [PATCH] hw/timer/hpet: Detect invalid access to TN registers
>
> On 2/18/25 10:07, Philippe Mathieu-Daudé wrote:
> > On 18/2/25 09:53, Paolo Bonzini wrote:
> > > On 2/18/25 08:37, Zhao Liu wrote:
> > > > "addr & 0x18" ignores invalid address, so that the trace in default
> > > > branch (trace_hpet_ram_{read|write}_invalid()) doesn't work.
> > > >
> > > > Mask addr by "0x1f & ~4", in which 0x1f means to get the complete TN
> > > > registers access and ~4 means to keep any invalid address offset.
> > >
> > > I think this is less readable.
> > >
> > > The reason to use !4 in the Rust code is because the initial AND is done
> > > in a separate function, timer_and_addr().
> >
> > Having a quick look at the model without looking at the specs:
> >
> > include/hw/timer/hpet.h:20:#define HPET_LEN 0x400
> >
> > hw/timer/hpet.c:439:static uint64_t hpet_ram_read(...,
> > hw/timer/hpet.c-441-{
> > hw/timer/hpet.c-448- /*address range of all TN regs*/
> > hw/timer/hpet.c-449- if (addr >= 0x100 && addr <= 0x3ff) {
> > hw/timer/hpet.c-450- uint8_t timer_id = (addr - 0x100) / 0x20;
> > ...
> > hw/timer/hpet.c-469- } else {
> > hw/timer/hpet.c-470- switch (addr & ~4) {
> > ...
> > hw/timer/hpet.c-488- }
> > hw/timer/hpet.c-489- }
> > hw/timer/hpet.c-490- return 0;
> > hw/timer/hpet.c-491-}
> >
> > hw/timer/hpet.c:699: memory_region_init_io(&s->iomem, obj,
> > &hpet_ram_ops, s,
> > "hpet", HPET_LEN);
> >
> > I suppose we want to register multiple timers of I/O size 0x20 at 0x100,
> > and the I/O size of 0x20 at 0x000 is a generic control region.
> >
> > Maybe split hpet_ram_ops in 2 (hpet_cfg_ops and hpet_tmr_ops), mapping
> > the first one once at 0x000 and the other 24 times at 0x100-0x3ff?
>
> You would have to come up with a way to get the index though. It seems to
> be adding churn for no particular reason.
>
> I'd rather look into how to make decoding code *easy* without making
> everything MemoryRegions.
I aslo met an register space implementation [1] which stores registers
with range (I guess for QEMU, it could map each register to a memory
region) and register specific callbacks.
I didn't choose this way since it's too complex to quickly develop...
[1]:
https://github.com/google/crosvm/blob/main/devices/src/register_space/mod.rs
> As I explained yesterday, while I'm not yet sure
> that Rust is going to stay in QEMU, I'd like to have as many examples as
> possible to help tilting the balance one way or the other. And indeed in the
> Rust version of HPET, timer_and_addr() could be extended to something like
> this:
>
> // Start with the same "enum for registers" pattern that PL011 uses:
> #[derive(qemu_api_macros::TryInto)]
> #[repr(u64)]
> enum TimerRegister {
> CFG = 0,
> CMP = 8,
> ROUTE = 16,
> }
>
> #[derive(qemu_api_macros::TryInto)]
> #[repr(u64)]
> enum GlobalRegister {
> CAP = 0,
> CFG = 0x10,
> INT_STATUS = 0x20,
> COUNTER = 0xF0,
> }
>
> // Go one step further and define types for all possible outcomes:
> #[derive(Copy)]
> enum HPETRegister {
> Timer(&BqlRefCell<HPETTimer>, TimerRegister),
> Global(GlobalRegister),
> Unknown(hwaddr),
> }
>
> struct HPETAddrDecode {
> u32 shift,
> u32 len,
> HPETRegister reg,
> }
>
> fn decode(&self, addr: hwaddr, size: u32) -> HPETAddrDecode {
> let shift = ((addr & 4) * 8) as u32;
> let len = std::cmp::min(size * 8, 64 - shift);
>
> addr &= !4;
> let reg = if (0x100..=0x3ff).contains(&addr) {
> let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
> TimerRegister::try_from(addr)
> .map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg))
> } else {
> GlobalRegister::try_from(addr)
> .map(HPETRegister::Global)
> }
>
> // reg is now a Result<HPETRegister, hwaddr>
> // convert the Err case into HPETRegister as well
> let reg = reg.unwrap_or_else(HPETRegister::Unknown);
> HPETAddrDecode { shift, len, reg }
> }
>
> (untested). The read and write functions then can do something like
>
> let val = match decoded.reg {
> Timer(timer, reg) => timer.borrow_mut().read(decoded),
> Global(GlobalRegister::CAP) => self.capability.get(),
> Global(GlobalRegister::CFG) => self.config.get(),
> ...
> }
> val >> decoded.shift
>
> and for write:
>
> match decoded.reg {
> Timer(timer, reg) => timer.borrow_mut().write(decoded, value),
> Global(GlobalRegister::CAP) => {}, // read-only
> Global(GlobalRegister::CFG) => self.set_cfg_reg(decoded, value),
> ...
> }
>
>
> The above could be a scheme that new devices could copy. Overall I think it
> would be shorter code than what is there in rust/hw/timer/hpet (which is IMO
> already better than C, mind!).
Yes, I also like your way. It's a bit more abstract than current hpet,
but is also more simple than pl011 without bilge.
I also found that there's another example to abstract register fields
without bilge [2]. However, examples such as hpet without any abstraction
also exit [3].
>From the experiences of other Rust VMMs, the handling of registers varies
greatly :-(, and there doesn't seem to be a unified approach. Developers
create abstractions according to their preferences, which I think is quite
different from C devices (in C, most people will hardcode everything
like hpet).
[2]:
https://github.com/cloud-hypervisor/cloud-hypervisor/blob/main/devices/src/tpm.rs
[3]: https://github.com/google/crosvm/blob/main/devices/src/cmos.rs
> The honest question for people with less experience is whether this is
> readable at all; whether seeing it helps you learn the language or
> discourages you.
If you think this conversion is not urgent, perhaps this case could
become as a "good first issue" to encourage people practice and get
familiar with RustInQemu.
Regards,
Zhao
