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

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

On Tue, Feb 18, 2025 at 10:07:18AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Tue, 18 Feb 2025 10:07:18 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH] hw/timer/hpet: Detect invalid access to TN registers
> 
> 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.

Range of general control region is from 0x00 to 0xff.

> 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?
> 
> My 2 cents looking at QDev modelling to avoid these address
> manipulations.

I think it's a good idea! Pls give me some time to try applying
memory_region_add_subregion() to this HPET case. :-)

Thanks,
Zhao