Re: [PATCH] hw/timer/hpet: Detect invalid access to TN registers

[Zhao Liu <zhao1.liu@intel.com>]

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 <pbonzini@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