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

[Paolo Bonzini <pbonzini@redhat.com>]

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.  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!).

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.

Paolo

> No clue what is between 0x020-0x0ff.
> 
> My 2 cents looking at QDev modelling to avoid these address
> manipulations.
> 
> Regards,
> 
> Phil.
> 
>