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

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

On Tue, Feb 18, 2025 at 09:53:00AM +0100, Paolo Bonzini wrote:
> Date: Tue, 18 Feb 2025 09:53:00 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH] hw/timer/hpet: Detect invalid access to TN registers
> 
> 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(). In C you don't have the same thing.

Yes.

> If anything you could do something like this:
 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index ccb97b68066..7c011204971 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -425,6 +425,7 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
>      int shift = (addr & 4) * 8;
>      uint64_t cur_tick;
> +    addr &= ~4;
>      trace_hpet_ram_read(addr);
>      /*address range of all TN regs*/
> @@ -437,7 +438,7 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
>              return 0;
>          }
> -        switch (addr & 0x18) {
> +        switch (addr & 0x1f) {
>          case HPET_TN_CFG: // including interrupt capabilities
>              return timer->config >> shift;
>          case HPET_TN_CMP: // comparator register
> @@ -449,7 +450,7 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
>              break;
>          }
>      } else {
> -        switch (addr & ~4) {
> +        switch (addr) {
>          case HPET_ID: // including HPET_PERIOD
>              return s->capability >> shift;
>          case HPET_CFG:
> 
> and the same in the write function, 

Thanks! Your example is clearer!

> but that's also different from the Rust code.

At least, user could know the invalid access by trace in C side. Rust
hasn't supported trace, but it will be in the future.

There will be some differences in code between the two, and we can make
sure that the debug ability to be as consistent as possible.

Thanks,
Zhao