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é <phi...@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
