Implement QOM & QAPI support for HPET device.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* Merge qdev.rs to hpet.rs.
* Apply memory and Resettable bindings.
* Consolidate inmutable &self and QOM casting.
* Prefer timer iterator over loop.
* Move init_mmio() and init_irq() to post_init().
---
rust/hw/timer/hpet/src/fw_cfg.rs | 2 -
rust/hw/timer/hpet/src/hpet.rs | 283 ++++++++++++++++++++++++++++++-
rust/hw/timer/hpet/src/lib.rs | 4 +
3 files changed, 279 insertions(+), 10 deletions(-)
diff --git a/rust/hw/timer/hpet/src/fw_cfg.rs b/rust/hw/timer/hpet/src/fw_cfg.rs
index 2f72bf854a66..5223629576a1 100644
--- a/rust/hw/timer/hpet/src/fw_cfg.rs
+++ b/rust/hw/timer/hpet/src/fw_cfg.rs
@@ -2,8 +2,6 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-#![allow(dead_code)]
-
use qemu_api::{cell::bql_locked, impl_zeroable, zeroable::Zeroable};
// Each HPETState represents a Event Timer Block. The v1 spec supports
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 0884a2ac73c4..7a717dbcfdd0 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -2,21 +2,30 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-#![allow(dead_code)]
-
-use core::ptr::{null_mut, NonNull};
+use core::ptr::{addr_of_mut, null_mut, NonNull};
+use std::{ffi::CStr, slice::from_ref};
use qemu_api::{
- bindings::{address_space_memory, address_space_stl_le, MemoryRegion, SCALE_NS},
+ bindings::{
+ address_space_memory, address_space_stl_le, qdev_prop_bit, qdev_prop_bool,
+ qdev_prop_uint32, qdev_prop_uint8, SCALE_NS,
+ },
+ c_str,
cell::{BqlCell, BqlRefCell},
irq::InterruptSource,
- memory::MEMTXATTRS_UNSPECIFIED,
+ memory::{
+ hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder, MEMTXATTRS_UNSPECIFIED,
+ },
prelude::*,
- qom::ParentField,
- sysbus::SysBusDevice,
+ qdev::{DeviceImpl, DeviceMethods, DeviceState, Property, ResetType, ResettablePhasesImpl},
+ qom::{ClassInitImpl, ObjectImpl, ObjectType, ParentField},
+ qom_isa,
+ sysbus::{SysBusDevice, SysBusDeviceClass},
timer::{QEMUTimer, CLOCK_VIRTUAL},
};
+use crate::fw_cfg::HPETFwConfig;
+
// Register space for each timer block. (HPET_BASE isn't defined here.)
const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
@@ -451,13 +460,43 @@ fn callback(&mut self) {
}
self.update_irq(true);
}
+
+ fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
+ let shift: u64 = (addr & 4) * 8;
+
+ match addr {
+ HPET_TN_CFG_REG => self.config >> shift, // including interrupt capabilities
+ HPET_TN_CMP_REG => self.cmp >> shift, // comparator register
+ HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_read_invalid()
+ // Reserved.
+ 0
+ }
+ }
+ }
+
+ fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
+ let shift = ((addr & 4) * 8) as u32;
+ let len = std::cmp::min(size * 8, 64 - shift);
+
+ match addr {
+ HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),
+ HPET_TN_CMP_REG => self.set_tn_cmp_reg(shift, len, value),
+ HPET_TN_FSB_ROUTE_REG => self.set_tn_fsb_route_reg(shift, len, value),
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid()
+ // Reserved.
+ }
+ }
+ }
}
/// HPET Event Timer Block Abstraction
/// Note: Wrap all items that may be changed in the callback called by C
/// into the BqlCell/BqlRefCell.
#[repr(C)]
-#[derive(qemu_api_macros::offsets)]
+#[derive(qemu_api_macros::Object, qemu_api_macros::offsets)]
pub struct HPETState {
parent_obj: ParentField<SysBusDevice>,
iomem: MemoryRegion,
@@ -630,4 +669,232 @@ fn set_counter_reg(&self, shift: u32, len: u32, val: u64) {
self.counter
.set(self.counter.get().deposit(shift, len, val));
}
+
+ unsafe fn init(&mut self) {
+ static HPET_RAM_OPS: MemoryRegionOps<HPETState> =
+ MemoryRegionOpsBuilder::<HPETState>::new()
+ .read(&HPETState::read)
+ .write(&HPETState::write)
+ .native_endian()
+ .valid_sizes(4, 8)
+ .impl_sizes(4, 8)
+ .build();
+
+ // SAFETY:
+ // self and self.iomem are guaranteed to be valid at this point since callers
+ // must make sure the `self` reference is valid.
+ MemoryRegion::init_io(
+ unsafe { &mut *addr_of_mut!(self.iomem) },
+ addr_of_mut!(*self),
+ &HPET_RAM_OPS,
+ "hpet",
+ HPET_REG_SPACE_LEN,
+ );
+ }
+
+ fn post_init(&self) {
+ self.init_mmio(&self.iomem);
+ for irq in self.irqs.iter() {
+ self.init_irq(irq);
+ }
+ }
+
+ fn realize(&self) {
+ if self.int_route_cap == 0 {
+ // TODO: Add error binding: warn_report()
+ println!("Hpet's hpet-intcap property not initialized");
+ }
+
+ self.hpet_id.set(HPETFwConfig::assign_hpet_id());
+
+ if self.num_timers.get() < HPET_MIN_TIMERS {
+ self.num_timers.set(HPET_MIN_TIMERS);
+ } else if self.num_timers.get() > HPET_MAX_TIMERS {
+ self.num_timers.set(HPET_MAX_TIMERS);
+ }
+
+ self.init_timer();
+ // 64-bit General Capabilities and ID Register; LegacyReplacementRoute.
+ self.capability.set(
+ HPET_CAP_REV_ID_VALUE << HPET_CAP_REV_ID_SHIFT |
+ 1 << HPET_CAP_COUNT_SIZE_CAP_SHIFT |
+ 1 << HPET_CAP_LEG_RT_CAP_SHIFT |
+ HPET_CAP_VENDER_ID_VALUE << HPET_CAP_VENDER_ID_SHIFT |
+ ((self.num_timers.get() - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
+ (HPET_CLK_PERIOD * FS_PER_NS) << HPET_CAP_CNT_CLK_PERIOD_SHIFT, // 10 ns
+ );
+
+ self.init_gpio_in(2, HPETState::handle_legacy_irq);
+ self.init_gpio_out(from_ref(&self.pit_enabled));
+ }
+
+ fn reset_hold(&self, _type: ResetType) {
+ let sbd = self.upcast::<SysBusDevice>();
+
+ for timer in self.timers.iter().take(self.num_timers.get()) {
+ timer.borrow_mut().reset();
+ }
+
+ self.counter.set(0);
+ self.config.set(0);
+ self.pit_enabled.set(true);
+ self.hpet_offset.set(0);
+
+ HPETFwConfig::update_hpet_cfg(
+ self.hpet_id.get(),
+ Some(self.capability.get() as u32),
+ Some((*sbd).mmio[0].addr),
+ );
+
+ // to document that the RTC lowers its output on reset as well
+ self.rtc_irq_level.set(0);
+ }
+
+ fn timer_and_addr(&self, addr: hwaddr) -> Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
+ let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
+
+ // TODO: Add trace point - trace_hpet_ram_[read|write]_timer_id(timer_id)
+ if timer_id > self.num_timers.get() {
+ // TODO: Add trace point - trace_hpet_timer_id_out_of_range(timer_id)
+ None
+ } else {
+ Some((&self.timers[timer_id], addr & 0x18))
+ }
+ }
+
+ fn read(&self, addr: hwaddr, size: u32) -> u64 {
+ let shift: u64 = (addr & 4) * 8;
+
+ // address range of all TN regs
+ // TODO: Add trace point - trace_hpet_ram_read(addr)
+ if (0x100..=0x3ff).contains(&addr) {
+ match self.timer_and_addr(addr) {
+ None => 0, // Reserved,
+ Some((timer, addr)) => timer.borrow_mut().read(addr, size),
+ }
+ } else {
+ match addr & !4 {
+ HPET_CAP_REG => self.capability.get() >> shift, /* including HPET_PERIOD 0x004 */
+ // (CNT_CLK_PERIOD field)
+ HPET_CFG_REG => self.config.get() >> shift,
+ HPET_COUNTER_REG => {
+ let cur_tick: u64 = if self.is_hpet_enabled() {
+ self.get_ticks()
+ } else {
+ self.counter.get()
+ };
+
+ // TODO: Add trace point - trace_hpet_ram_read_reading_counter(addr & 4,
+ // cur_tick)
+ cur_tick >> shift
+ }
+ HPET_INT_STATUS_REG => self.int_status.get() >> shift,
+ _ => {
+ // TODO: Add trace point- trace_hpet_ram_read_invalid()
+ // Reserved.
+ 0
+ }
+ }
+ }
+ }
+
+ fn write(&self, addr: hwaddr, value: u64, size: u32) {
+ let shift = ((addr & 4) * 8) as u32;
+ let len = std::cmp::min(size * 8, 64 - shift);
+
+ // TODO: Add trace point - trace_hpet_ram_write(addr, value)
+ if (0x100..=0x3ff).contains(&addr) {
+ match self.timer_and_addr(addr) {
+ None => return, // Reserved.
+ Some((timer, addr)) => timer.borrow_mut().write(addr, value, size),
+ }
+ } else {
+ match addr & !0x4 {
+ HPET_CAP_REG => {} // General Capabilities and ID Register: Read Only
+ HPET_CFG_REG => self.set_cfg_reg(shift, len, value),
+ HPET_INT_STATUS_REG => self.set_int_status_reg(shift, len, value),
+ HPET_COUNTER_REG => self.set_counter_reg(shift, len, value),
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid()
+ // Reserved.
+ }
+ }
+ }
+ }
+}
+
+qom_isa!(HPETState: SysBusDevice, DeviceState, Object);
+
+// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
+#[repr(C)]
+pub struct HPETClass {
+ parent_class: <SysBusDevice as ObjectType>::Class,
+}
+
+unsafe impl ObjectType for HPETState {
+ type Class = HPETClass;
+ const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
+}
+
+impl ClassInitImpl<HPETClass> for HPETState {
+ fn class_init(klass: &mut HPETClass) {
+ <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
+ }
+}
+
+impl ObjectImpl for HPETState {
+ type ParentType = SysBusDevice;
+
+ const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+ const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
+}
+
+// TODO: Make these properties user-configurable!
+qemu_api::declare_properties! {
+ HPET_PROPERTIES,
+ qemu_api::define_property!(
+ c_str!("timers"),
+ HPETState,
+ num_timers,
+ unsafe { &qdev_prop_uint8 },
+ u8,
+ default = HPET_MIN_TIMERS
+ ),
+ qemu_api::define_property!(
+ c_str!("msi"),
+ HPETState,
+ flags,
+ unsafe { &qdev_prop_bit },
+ u32,
+ bit = HPET_FLAG_MSI_SUPPORT_SHIFT as u8,
+ default = false,
+ ),
+ qemu_api::define_property!(
+ c_str!("hpet-intcap"),
+ HPETState,
+ int_route_cap,
+ unsafe { &qdev_prop_uint32 },
+ u32,
+ default = 0
+ ),
+ qemu_api::define_property!(
+ c_str!("hpet-offset-saved"),
+ HPETState,
+ hpet_offset_saved,
+ unsafe { &qdev_prop_bool },
+ bool,
+ default = true
+ ),
+}
+
+impl DeviceImpl for HPETState {
+ fn properties() -> &'static [Property] {
+ &HPET_PROPERTIES
+ }
+
+ const REALIZE: Option<fn(&Self)> = Some(Self::realize);
+}
+
+impl ResettablePhasesImpl for HPETState {
+ const HOLD: Option<fn(&Self, ResetType)> = Some(Self::reset_hold);
}
diff --git a/rust/hw/timer/hpet/src/lib.rs b/rust/hw/timer/hpet/src/lib.rs
index 027f7f83349a..25251112a86d 100644
--- a/rust/hw/timer/hpet/src/lib.rs
+++ b/rust/hw/timer/hpet/src/lib.rs
@@ -10,5 +10,9 @@
#![deny(unsafe_op_in_unsafe_fn)]
+use qemu_api::c_str;
+
pub mod fw_cfg;
pub mod hpet;
+
+pub const TYPE_HPET: &::std::ffi::CStr = c_str!("hpet");
--
2.34.1
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
This can be &self.
> let shift: u64 = (addr & 4) * 8;
>
> + match addr {
> + HPET_TN_CFG_REG => self.config >> shift, // including interrupt capabilities
This needs to be "match addr & !4".
> + HPET_TN_CMP_REG => self.cmp >> shift, // comparator register
> + HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
> + _ => {
> + // TODO: Add trace point - trace_hpet_ram_read_invalid()
> + // Reserved.
> + 0
> + }
> + }
> + }
> +
> + fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
> + let shift = ((addr & 4) * 8) as u32;
> + let len = std::cmp::min(size * 8, 64 - shift);
> +
> + match addr {
> + HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),
Likewise.
> + fn write(&self, addr: hwaddr, value: u64, size: u32) {
> + let shift = ((addr & 4) * 8) as u32;
> + let len = std::cmp::min(size * 8, 64 - shift);
> +
> + // TODO: Add trace point - trace_hpet_ram_write(addr, value)
> + if (0x100..=0x3ff).contains(&addr
) {
> + match self.timer_and_addr(addr) {
> + None => return, // Reserved.
Clippy complains about an unnecessary return, just replace it with "()".
> + Some((timer, addr)) => timer.borrow_mut().write(addr, value, size),
> + }
> +
> + fn reset_hold(&self, _type: ResetType) {
> + let sbd = self.upcast::<SysBusDevice>();
> +
> + for timer in self.timers.iter().take(self.num_timers.get()) {
> + timer.borrow_mut().reset();
> + }
> +
> + self.counter.set(0);
> + self.config.set(0);
> + self.pit_enabled.set(true);
> + self.hpet_offset.set(0);
> +
> + HPETFwConfig::update_hpet_cfg(
> + self.hpet_id.get(),
> + Some(self.capability.get() as u32),
> + Some((*sbd).mmio[0].addr),
> + );
This can be simply sbd.mmio[0].addr, without the (*...).
Also, you can change update_hpet_cfg to take arguments without the
Option<> around them.
> +// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
> +#[repr(C)]
> +pub struct HPETClass {
> + parent_class: <SysBusDevice as ObjectType>::Class,
> +}
> +
> +unsafe impl ObjectType for HPETState {
> + type Class = HPETClass;
> + const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> +}
No need for HPETClass (and for ClassInitImpl<HPETClass>), just do
unsafe impl ObjectType for HPETState {
type Class = <SysBusDevice as ObjectType>::Class;
const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
}
which is indeed more similar to OBJECT_DECLARE_SIMPLE_TYPE().
Paolo
On Wed, Jan 29, 2025 at 11:58:14AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:58:14 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
>
>
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
>
> This can be &self.
Done.
> > let shift: u64 = (addr & 4) * 8;
> >
> > + match addr {
> > + HPET_TN_CFG_REG => self.config >> shift, // including interrupt capabilities
>
> This needs to be "match addr & !4".
I understand it's not necessary:
In timer_and_addr(), I've masked the address with 0x18.
fn timer_and_addr(&self, addr: hwaddr) -> Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
if timer_id > self.num_timers.get() {
None
} else {
Some((&self.timers[timer_id], addr & 0x18))
}
}
And in HPETState::read(), I pass the masked address to
HPETTimer::read():
fn read(&self, addr: hwaddr, size: u32) -> u64 {
let shift: u64 = (addr & 4) * 8;
if (0x100..=0x3ff).contains(&addr) {
match self.timer_and_addr(addr) {
None => 0, // Reserved,
Some((timer, addr)) => timer.borrow_mut().read(addr, size),
}
}
Ah, the `addr` variable in Some() is misleading, and I should use
anothor name like `tn_addr`.
0x18 is a subset of !0x4, so the bitwise & operation with !0x4 can be
omitted (I understand that this is why you also omitted this in the C
side in the commit c2366567378dd :-) ).
But `match addr & !4` is also reasonable, which explicitly emphasize
`& !4` makes the code clearer. I would do this.
> > + HPET_TN_CMP_REG => self.cmp >> shift, // comparator register
> > + HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
> > + _ => {
> > + // TODO: Add trace point - trace_hpet_ram_read_invalid()
> > + // Reserved.
> > + 0
> > + }
> > + }
> > + }
> > +
> > + fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
> > + let shift = ((addr & 4) * 8) as u32;
> > + let len = std::cmp::min(size * 8, 64 - shift);
> > +
> > + match addr {
> > + HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),
>
> Likewise.
Done. Thanks!
> > + fn write(&self, addr: hwaddr, value: u64, size: u32) {
> > + let shift = ((addr & 4) * 8) as u32;
> > + let len = std::cmp::min(size * 8, 64 - shift);
> > +
> > + // TODO: Add trace point - trace_hpet_ram_write(addr, value)
> > + if (0x100..=0x3ff).contains(&addr
> ) {
> > + match self.timer_and_addr(addr) {
> > + None => return, // Reserved.
>
> Clippy complains about an unnecessary return, just replace it with "()".
Fixed.
> > + Some((timer, addr)) => timer.borrow_mut().write(addr, value, size),
> > + }
>
> > +
> > + fn reset_hold(&self, _type: ResetType) {
> > + let sbd = self.upcast::<SysBusDevice>();
> > +
> > + for timer in self.timers.iter().take(self.num_timers.get()) {
> > + timer.borrow_mut().reset();
> > + }
> > +
> > + self.counter.set(0);
> > + self.config.set(0);
> > + self.pit_enabled.set(true);
> > + self.hpet_offset.set(0);
> > +
> > + HPETFwConfig::update_hpet_cfg(
> > + self.hpet_id.get(),
> > + Some(self.capability.get() as u32),
> > + Some((*sbd).mmio[0].addr),
> > + );
>
> This can be simply sbd.mmio[0].addr, without the (*...).
>
> Also, you can change update_hpet_cfg to take arguments without the Option<>
> around them.
Good idea! Done.
> > +// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
> > +#[repr(C)]
> > +pub struct HPETClass {
> > + parent_class: <SysBusDevice as ObjectType>::Class,
> > +}
> > +
> > +unsafe impl ObjectType for HPETState {
> > + type Class = HPETClass;
> > + const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> > +}
>
> No need for HPETClass (and for ClassInitImpl<HPETClass>), just do
>
> unsafe impl ObjectType for HPETState {
Pls let me add a comment here:
// No need for HPETClass. Just like OBJECT_DECLARE_SIMPLE_TYPE in C.
Then this can be "grep", as a reference.
> type Class = <SysBusDevice as ObjectType>::Class;
> const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> }
>
> which is indeed more similar to OBJECT_DECLARE_SIMPLE_TYPE().
Awesome! Thanks.
Regards,
Zhao
Il sab 8 feb 2025, 11:36 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> On Wed, Jan 29, 2025 at 11:58:14AM +0100, Paolo Bonzini wrote:
> > Date: Wed, 29 Jan 2025 11:58:14 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
> >
> >
> >
> > On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > > fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
> >
> > This can be &self.
>
> Done.
>
> > > let shift: u64 = (addr & 4) * 8;
> > >
> > > + match addr {
> > > + HPET_TN_CFG_REG => self.config >> shift, // including
> interrupt capabilities
> >
> > This needs to be "match addr & !4".
>
> I understand it's not necessary:
>
> In timer_and_addr(), I've masked the address with 0x18.
>
> fn timer_and_addr(&self, addr: hwaddr) ->
> Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
> let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
>
> if timer_id > self.num_timers.get() {
> None
> } else {
> Some((&self.timers[timer_id], addr & 0x18))
>
Ah, this should be 0x1C (or perhaps 0x1F). Otherwise there is a bug in
accessing the high 32 bits of a 64-bit register; shift will always be 0 in
HPETTimer::read and write.
// No need for HPETClass. Just like OBJECT_DECLARE_SIMPLE_TYPE in C.
>
> Then this can be "grep", as a reference.
>
Sure.
Thanks,
Paolo
> > type Class = <SysBusDevice as ObjectType>::Class;
> > const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> > }
> >
> > which is indeed more similar to OBJECT_DECLARE_SIMPLE_TYPE().
>
> Awesome! Thanks.
>
> Regards,
> Zhao
>
>
> > > This needs to be "match addr & !4".
> >
> > I understand it's not necessary:
> >
> > In timer_and_addr(), I've masked the address with 0x18.
> >
> > fn timer_and_addr(&self, addr: hwaddr) ->
> > Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
> > let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
> >
> > if timer_id > self.num_timers.get() {
> > None
> > } else {
> > Some((&self.timers[timer_id], addr & 0x18))
> >
>
> Ah, this should be 0x1C (or perhaps 0x1F). Otherwise there is a bug in
> accessing the high 32 bits of a 64-bit register; shift will always be 0 in
> HPETTimer::read and write.
Yes, you're right. Then we should use 0x1F so that invalid access could
detected (or loged in future) and ignored.
Based on the similar reason, C side also need to use "addr & (0x1f | ~4)"
instead of 0x18 to catch invalid access. If I'm right, I can submit a
fix.
Thanks,
Zhao
© 2016 - 2025 Red Hat, Inc.