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

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

[Zhao Liu]

"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.

Signed-off-by: Zhao Liu <zhao1.liu@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/Z6edKxYFzA6suDlj@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;
-- 
2.34.1

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.liu@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/Z6edKxYFzA6suDlj@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;

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

[Philippe Mathieu-Daudé]

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.

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

[Zhao Liu]

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

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

[Zhao Liu]

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

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

[Paolo Bonzini]

On 2/18/25 10:07, Philippe Mathieu-Daudé wrote:
> 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?

You would have to come up with a way to get the index though.  It seems 
to be adding churn for no particular reason.

I'd rather look into how to make decoding code *easy* without making 
everything MemoryRegions.  As I explained yesterday, while I'm not yet 
sure that Rust is going to stay in QEMU, I'd like to have as many 
examples as possible to help tilting the balance one way or the other. 
And indeed in the Rust version of HPET, timer_and_addr() could be 
extended to something like this:

// Start with the same "enum for registers" pattern that PL011 uses:
#[derive(qemu_api_macros::TryInto)]
#[repr(u64)]
enum TimerRegister {
     CFG = 0,
     CMP = 8,
     ROUTE = 16,
}

#[derive(qemu_api_macros::TryInto)]
#[repr(u64)]
enum GlobalRegister {
     CAP = 0,
     CFG = 0x10,
     INT_STATUS = 0x20,
     COUNTER = 0xF0,
}

// Go one step further and define types for all possible outcomes:
#[derive(Copy)]
enum HPETRegister {
     Timer(&BqlRefCell<HPETTimer>, TimerRegister),
     Global(GlobalRegister),
     Unknown(hwaddr),
}

struct HPETAddrDecode {
     u32 shift,
     u32 len,
     HPETRegister reg,
}

fn decode(&self, addr: hwaddr, size: u32) -> HPETAddrDecode {
     let shift = ((addr & 4) * 8) as u32;
     let len = std::cmp::min(size * 8, 64 - shift);

     addr &= !4;
     let reg = if (0x100..=0x3ff).contains(&addr) {
         let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
         TimerRegister::try_from(addr)
             .map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg))
     } else {
         GlobalRegister::try_from(addr)
             .map(HPETRegister::Global)
     }

     // reg is now a Result<HPETRegister, hwaddr>
     // convert the Err case into HPETRegister as well
     let reg = reg.unwrap_or_else(HPETRegister::Unknown);
     HPETAddrDecode { shift, len, reg }
}

(untested).  The read and write functions then can do something like

     let val = match decoded.reg {
         Timer(timer, reg) => timer.borrow_mut().read(decoded),
         Global(GlobalRegister::CAP) => self.capability.get(),
         Global(GlobalRegister::CFG) => self.config.get(),
         ...
     }
     val >> decoded.shift

and for write:

     match decoded.reg {
         Timer(timer, reg) => timer.borrow_mut().write(decoded, value),
         Global(GlobalRegister::CAP) => {}, // read-only
         Global(GlobalRegister::CFG) => self.set_cfg_reg(decoded, value),
         ...
     }


The above could be a scheme that new devices could copy.  Overall I 
think it would be shorter code than what is there in rust/hw/timer/hpet 
(which is IMO already better than C, mind!).

The honest question for people with less experience is whether this is 
readable at all; whether seeing it helps you learn the language or 
discourages you.

Paolo

> No clue what is between 0x020-0x0ff.
> 
> My 2 cents looking at QDev modelling to avoid these address
> manipulations.
> 
> Regards,
> 
> Phil.
> 
> 

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

[Zhao Liu]

On Wed, Feb 19, 2025 at 10:25:40AM +0100, Paolo Bonzini wrote:
> Date: Wed, 19 Feb 2025 10:25:40 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH] hw/timer/hpet: Detect invalid access to TN registers
> 
> On 2/18/25 10:07, Philippe Mathieu-Daudé wrote:
> > 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?
> 
> You would have to come up with a way to get the index though.  It seems to
> be adding churn for no particular reason.
> 
> I'd rather look into how to make decoding code *easy* without making
> everything MemoryRegions.

I aslo met an register space implementation [1] which stores registers
with range (I guess for QEMU, it could map each register to a memory
region) and register specific callbacks.

I didn't choose this way since it's too complex to quickly develop...

[1]: https://github.com/google/crosvm/blob/main/devices/src/register_space/mod.rs

> As I explained yesterday, while I'm not yet sure
> that Rust is going to stay in QEMU, I'd like to have as many examples as
> possible to help tilting the balance one way or the other. And indeed in the
> Rust version of HPET, timer_and_addr() could be extended to something like
> this:
> 
> // Start with the same "enum for registers" pattern that PL011 uses:
> #[derive(qemu_api_macros::TryInto)]
> #[repr(u64)]
> enum TimerRegister {
>     CFG = 0,
>     CMP = 8,
>     ROUTE = 16,
> }
> 
> #[derive(qemu_api_macros::TryInto)]
> #[repr(u64)]
> enum GlobalRegister {
>     CAP = 0,
>     CFG = 0x10,
>     INT_STATUS = 0x20,
>     COUNTER = 0xF0,
> }
> 
> // Go one step further and define types for all possible outcomes:
> #[derive(Copy)]
> enum HPETRegister {
>     Timer(&BqlRefCell<HPETTimer>, TimerRegister),
>     Global(GlobalRegister),
>     Unknown(hwaddr),
> }
> 
> struct HPETAddrDecode {
>     u32 shift,
>     u32 len,
>     HPETRegister reg,
> }
> 
> fn decode(&self, addr: hwaddr, size: u32) -> HPETAddrDecode {
>     let shift = ((addr & 4) * 8) as u32;
>     let len = std::cmp::min(size * 8, 64 - shift);
> 
>     addr &= !4;
>     let reg = if (0x100..=0x3ff).contains(&addr) {
>         let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
>         TimerRegister::try_from(addr)
>             .map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg))
>     } else {
>         GlobalRegister::try_from(addr)
>             .map(HPETRegister::Global)
>     }
> 
>     // reg is now a Result<HPETRegister, hwaddr>
>     // convert the Err case into HPETRegister as well
>     let reg = reg.unwrap_or_else(HPETRegister::Unknown);
>     HPETAddrDecode { shift, len, reg }
> }
> 
> (untested).  The read and write functions then can do something like
> 
>     let val = match decoded.reg {
>         Timer(timer, reg) => timer.borrow_mut().read(decoded),
>         Global(GlobalRegister::CAP) => self.capability.get(),
>         Global(GlobalRegister::CFG) => self.config.get(),
>         ...
>     }
>     val >> decoded.shift
> 
> and for write:
> 
>     match decoded.reg {
>         Timer(timer, reg) => timer.borrow_mut().write(decoded, value),
>         Global(GlobalRegister::CAP) => {}, // read-only
>         Global(GlobalRegister::CFG) => self.set_cfg_reg(decoded, value),
>         ...
>     }
> 
> 
> The above could be a scheme that new devices could copy.  Overall I think it
> would be shorter code than what is there in rust/hw/timer/hpet (which is IMO
> already better than C, mind!).

Yes, I also like your way. It's a bit more abstract than current hpet,
but is also more simple than pl011 without bilge.

I also found that there's another example to abstract register fields
without bilge [2]. However, examples such as hpet without any abstraction
also exit [3].

From the experiences of other Rust VMMs, the handling of registers varies
greatly :-(, and there doesn't seem to be a unified approach. Developers
create abstractions according to their preferences, which I think is quite
different from C devices (in C, most people will hardcode everything
like hpet).

[2]: https://github.com/cloud-hypervisor/cloud-hypervisor/blob/main/devices/src/tpm.rs
[3]: https://github.com/google/crosvm/blob/main/devices/src/cmos.rs

> The honest question for people with less experience is whether this is
> readable at all; whether seeing it helps you learn the language or
> discourages you.

If you think this conversion is not urgent, perhaps this case could
become as a "good first issue" to encourage people practice and get
familiar with RustInQemu.

Regards,
Zhao