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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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?
No clue what is between 0x020-0x0ff.

My 2 cents looking at QDev modelling to avoid these address
manipulations.

Regards,

Phil.