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

[Paolo Bonzini]

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.  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, but that's also different from the Rust
code.

Paolo

Signed-off-by: Zhao Liu <zhao1....@intel.com>
---
  * Note: This patch applies the changes from Rust HPET [*] to C HPET to
    ensure logic consistency.
    [*]: The original comment should use "(0x1f & ~4)":
         https://lore.kernel.org/qemu-devel/z6edkxyfza6su...@intel.com/
---
  hw/timer/hpet.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index dcff18a9871d..0f786047e54f 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -455,7 +455,7 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
              return 0;
          }
  
-        switch (addr & 0x18) {
+        switch (addr & (0x1f & ~4)) {
          case HPET_TN_CFG: // including interrupt capabilities
              return timer->config >> shift;
          case HPET_TN_CMP: // comparator register
@@ -511,7 +511,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
              trace_hpet_timer_id_out_of_range(timer_id);
              return;
          }
-        switch (addr & 0x18) {
+
+        switch (addr & (0x1f & ~4)) {
          case HPET_TN_CFG:
              trace_hpet_ram_write_tn_cfg(addr & 4);
              old_val = timer->config;