Kernel: Fix HPET timer not firing in Bochs

* Change the register structures to use the volatile keyword explicitly
  on the register values. This avoids accidentally omitting it as any
  access will be guaranteed volatile.
* Don't assume we can read/write 64 bit value to the main counter and
  the comparator. Not all HPET implementations may support this. So,
  just use 32 bit words to access the registers. This ultimately works
  around a bug in Bochs 2.6.11 that loses 32 bits of a 64 bit write to
  a timer's comparator register (it internally writes one half and
  clears the Tn_VAL_SET_CNF bit, and then because it's cleared it
  fails to write the second half).
* Properly calculate the tick duration in calculate_ticks_in_nanoseconds
* As per specification, changing the frequency of one periodic timer
  requires a restart of all periodic timers as it requires the main
  counter to be reset.

4 files changed, 155 insertions, 92 deletions

diff --git a/Kernel/Time/HPET.cpp b/Kernel/Time/HPET.cpp
index b0295072cd..ed4c5a5e1c 100644
--- a/Kernel/Time/HPET.cpp
+++ b/Kernel/Time/HPET.cpp
@@ -48,58 +48,78 @@ enum class Attributes {
 };
 
 enum class Configuration {
-    Enable = 0x1,
-    LegacyReplacementRoute = 0x2
+    Enable = 1 << 0,
+    LegacyReplacementRoute = 1 << 1
 };
 
 enum class TimerConfiguration : u32 {
-    InterruptType = 1 << 1,
+    LevelTriggered = 1 << 1,
     InterruptEnable = 1 << 2,
-    TimerType = 1 << 3,
+    GeneratePeriodicInterrupt = 1 << 3,
     PeriodicInterruptCapable = 1 << 4,
     Timer64BitsCapable = 1 << 5,
     ValueSet = 1 << 6,
-    Force32BitMode = 1 << 7,
+    Force32BitMode = 1 << 8,
     FSBInterruptEnable = 1 << 14,
     FSBInterruptDelivery = 1 << 15
 };
 };
 
-struct [[gnu::packed]] TimerStructure
+struct [[gnu::packed]] HPETRegister
 {
-    u64 configuration_and_capability;
-    u64 comparator_value;
-    u64 fsb_interrupt_route;
-    u64 reserved;
+    volatile u32 low;
+    volatile u32 high;
 };
 
-struct [[gnu::packed]] HPETCapabilityRegister
+struct [[gnu::packed]] TimerStructure
 {
-    u32 attributes; // Note: We must do a 32 bit access to offsets 0x0, or 0x4 only, according to HPET spec.
-    u32 main_counter_tick_period;
+    volatile u32 capabilities;
+    volatile u32 interrupt_routing;
+    HPETRegister comparator_value;
+    volatile u64 fsb_interrupt_route;
     u64 reserved;
 };
 
-struct [[gnu::packed]] HPETRegister
+struct [[gnu::packed]] HPETCapabilityRegister
 {
-    u64 reg;
+    // Note: We must do a 32 bit access to offsets 0x0, or 0x4 only, according to HPET spec.
+    volatile u32 attributes;
+    volatile u32 main_counter_tick_period;
     u64 reserved;
 };
 
 struct [[gnu::packed]] HPETRegistersBlock
 {
-    union {
-        HPETCapabilityRegister capabilities;
-        HPETRegister raw_capabilites;
-    };
+    HPETCapabilityRegister capabilities;
     HPETRegister configuration;
+    u64 reserved1;
     HPETRegister interrupt_status;
-    u8 reserved[0xF0 - 48];
+    u8 reserved2[0xF0 - 0x28];
     HPETRegister main_counter_value;
+    u64 reserved3;
     TimerStructure timers[3];
-    u8 reserved2[0x400 - 0x160];
+    u8 reserved4[0x400 - 0x160];
 };
 
+static_assert(__builtin_offsetof(HPETRegistersBlock, main_counter_value) == 0xf0);
+static_assert(__builtin_offsetof(HPETRegistersBlock, timers[0]) == 0x100);
+static_assert(__builtin_offsetof(HPETRegistersBlock, timers[1]) == 0x120);
+
+static u64 read_register_safe64(const HPETRegister& reg)
+{
+    // As per 2.4.7 this reads the 64 bit value in a consistent manner
+    // using only 32 bit reads
+    u32 low, high = reg.high;
+    for (;;) {
+        low = reg.low;
+        u32 new_high = reg.high;
+        if (new_high == high)
+            break;
+        high = new_high;
+    }
+    return ((u64)high << 32) | (u64)low;
+}
+
 static HPET* s_hpet;
 static bool hpet_initialized { false };
 
@@ -135,7 +155,7 @@ bool HPET::test_and_initialize()
             return false;
         }
     }
-    s_hpet = new HPET(PhysicalAddress(hpet));
+    new HPET(PhysicalAddress(hpet));
     return true;
 }
 
@@ -147,11 +167,11 @@ bool HPET::check_for_exisiting_periodic_timers()
 
     auto sdt = map_typed<ACPI::Structures::HPET>(hpet);
     ASSERT(sdt->event_timer_block.address_space == 0);
-    auto registers = map_typed<volatile HPETRegistersBlock>(PhysicalAddress(sdt->event_timer_block.address));
+    auto registers = map_typed<HPETRegistersBlock>(PhysicalAddress(sdt->event_timer_block.address));
 
-    size_t timers_count = ((registers->raw_capabilites.reg >> 8) & 0x1f) + 1;
+    size_t timers_count = ((registers->capabilities.attributes >> 8) & 0x1f) + 1;
     for (size_t index = 0; index < timers_count; index++) {
-        if (registers->timers[index].configuration_and_capability & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable)
+        if (registers->timers[index].capabilities & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable)
             return true;
     }
     return false;
@@ -159,30 +179,67 @@ bool HPET::check_for_exisiting_periodic_timers()
 
 void HPET::global_disable()
 {
-    registers().configuration.reg = registers().configuration.reg & ~(u32)HPETFlags::Configuration::Enable;
+    auto& regs = registers();
+    regs.configuration.low = regs.configuration.low & ~(u32)HPETFlags::Configuration::Enable;
 }
 void HPET::global_enable()
 {
-    registers().configuration.reg = registers().configuration.reg | (u32)HPETFlags::Configuration::Enable;
+    auto& regs = registers();
+    regs.configuration.low = regs.configuration.low | (u32)HPETFlags::Configuration::Enable;
 }
 
-void HPET::set_periodic_comparator_value(const HPETComparator& comparator, u64 value)
+void HPET::update_periodic_comparator_value()
 {
-    disable(comparator);
-    ASSERT(comparator.is_periodic());
-    ASSERT(comparator.comparator_number() <= m_comparators.size());
-    volatile auto& timer = registers().timers[comparator.comparator_number()];
-    timer.configuration_and_capability = timer.configuration_and_capability | (u32)HPETFlags::TimerConfiguration::ValueSet;
-    timer.comparator_value = value;
-    enable(comparator);
+    // According to 2.3.9.2.2 the only safe way to change the periodic timer frequency
+    // is to disable all periodic timers, reset the main counter and each timer's comparator value.
+    // This introduces time drift, so it should be avoided unless absolutely necessary.
+    global_disable();
+    auto& regs = registers();
+
+    u64 previous_main_value = (u64)regs.main_counter_value.low | ((u64)regs.main_counter_value.high << 32);
+    regs.main_counter_value.low = 0;
+    regs.main_counter_value.high = 0;
+    for (auto& comparator : m_comparators) {
+        auto& timer = regs.timers[comparator.comparator_number()];
+        if (comparator.is_periodic()) {
+            // Note that this means we're restarting all periodic timers. There is no
+            // way to resume periodic timers properly because we reset the main counter
+            // and we can only write the period into the comparator value...
+            timer.capabilities = timer.capabilities | (u32)HPETFlags::TimerConfiguration::ValueSet;
+            u64 value = frequency() / comparator.ticks_per_second();
+#ifdef HPET_DEBUG
+            dbg() << "HPET: Update periodic comparator " << comparator.comparator_number() << " comparator value to " << value << " main value was: " << previous_main_value;
+#endif
+            timer.comparator_value.low = (u32)value;
+            timer.capabilities = timer.capabilities | (u32)HPETFlags::TimerConfiguration::ValueSet;
+            timer.comparator_value.high = (u32)(value >> 32);
+        } else {
+            // Set the new target comparator value to the delta to the remaining ticks
+            u64 current_value = (u64)timer.comparator_value.low | ((u64)timer.comparator_value.high << 32);
+            u64 value = current_value - previous_main_value;
+#ifdef HPET_DEBUG
+            dbg() << "HPET: Update non-periodic comparator " << comparator.comparator_number() << " comparator value from " << current_value << " to " << value << " main value was: " << previous_main_value;
+#endif
+            timer.comparator_value.low = (u32)value;
+            timer.comparator_value.high = (u32)(value >> 32);
+        }
+    }
+
+    global_enable();
 }
 
-void HPET::set_non_periodic_comparator_value(const HPETComparator& comparator, u64 value)
+void HPET::update_non_periodic_comparator_value(const HPETComparator& comparator)
 {
     ASSERT_INTERRUPTS_DISABLED();
     ASSERT(!comparator.is_periodic());
     ASSERT(comparator.comparator_number() <= m_comparators.size());
-    registers().timers[comparator.comparator_number()].comparator_value = main_counter_value() + value;
+    auto& regs = registers();
+    auto& timer = regs.timers[comparator.comparator_number()];
+    u64 value = frequency() / comparator.ticks_per_second();
+    // NOTE: If the main counter passes this new value before we finish writing it, we will never receive an interrupt!
+    u64 new_counter_value = read_register_safe64(regs.main_counter_value) + value;
+    timer.comparator_value.high = (u32)(new_counter_value >> 32);
+    timer.comparator_value.low = (u32)new_counter_value;
 }
 
 void HPET::enable_periodic_interrupt(const HPETComparator& comparator)
@@ -192,10 +249,10 @@ void HPET::enable_periodic_interrupt(const HPETComparator& comparator)
 #endif
     disable(comparator);
     ASSERT(comparator.comparator_number() <= m_comparators.size());
-    volatile auto& timer = registers().timers[comparator.comparator_number()];
-    auto configuration_and_capability = timer.configuration_and_capability;
-    ASSERT(configuration_and_capability & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable);
-    timer.configuration_and_capability = configuration_and_capability | (u32)HPETFlags::TimerConfiguration::TimerType;
+    auto& timer = registers().timers[comparator.comparator_number()];
+    auto capabilities = timer.capabilities;
+    ASSERT(capabilities & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable);
+    timer.capabilities = capabilities | (u32)HPETFlags::TimerConfiguration::GeneratePeriodicInterrupt;
     enable(comparator);
 }
 void HPET::disable_periodic_interrupt(const HPETComparator& comparator)
@@ -205,10 +262,10 @@ void HPET::disable_periodic_interrupt(const HPETComparator& comparator)
 #endif
     disable(comparator);
     ASSERT(comparator.comparator_number() <= m_comparators.size());
-    auto volatile& timer = registers().timers[comparator.comparator_number()];
-    auto configuration_and_capability = timer.configuration_and_capability;
-    ASSERT(configuration_and_capability & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable);
-    timer.configuration_and_capability = configuration_and_capability & ~(u32)HPETFlags::TimerConfiguration::TimerType;
+    auto& timer = registers().timers[comparator.comparator_number()];
+    auto capabilities = timer.capabilities;
+    ASSERT(capabilities & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable);
+    timer.capabilities = capabilities & ~(u32)HPETFlags::TimerConfiguration::GeneratePeriodicInterrupt;
     enable(comparator);
 }
 
@@ -218,8 +275,8 @@ void HPET::disable(const HPETComparator& comparator)
     klog() << "HPET: Disable comparator " << comparator.comparator_number() << ".";
 #endif
     ASSERT(comparator.comparator_number() <= m_comparators.size());
-    volatile auto& timer = registers().timers[comparator.comparator_number()];
-    timer.configuration_and_capability = timer.configuration_and_capability & ~(u32)HPETFlags::TimerConfiguration::InterruptEnable;
+    auto& timer = registers().timers[comparator.comparator_number()];
+    timer.capabilities = timer.capabilities & ~(u32)HPETFlags::TimerConfiguration::InterruptEnable;
 }
 void HPET::enable(const HPETComparator& comparator)
 {
@@ -227,13 +284,8 @@ void HPET::enable(const HPETComparator& comparator)
     klog() << "HPET: Enable comparator " << comparator.comparator_number() << ".";
 #endif
     ASSERT(comparator.comparator_number() <= m_comparators.size());
-    volatile auto& timer = registers().timers[comparator.comparator_number()];
-    timer.configuration_and_capability = timer.configuration_and_capability | (u32)HPETFlags::TimerConfiguration::InterruptEnable;
-}
-
-u64 HPET::main_counter_value() const
-{
-    return registers().main_counter_value.reg;
+    auto& timer = registers().timers[comparator.comparator_number()];
+    timer.capabilities = timer.capabilities | (u32)HPETFlags::TimerConfiguration::InterruptEnable;
 }
 
 u64 HPET::frequency() const
@@ -245,8 +297,8 @@ Vector<unsigned> HPET::capable_interrupt_numbers(const HPETComparator& comparato
 {
     ASSERT(comparator.comparator_number() <= m_comparators.size());
     Vector<unsigned> capable_interrupts;
-    auto& comparator_registers = (const volatile TimerStructure&)registers().timers[comparator.comparator_number()];
-    u32 interrupt_bitfield = comparator_registers.configuration_and_capability >> 32;
+    auto& comparator_registers = registers().timers[comparator.comparator_number()];
+    u32 interrupt_bitfield = comparator_registers.interrupt_routing;
     for (size_t index = 0; index < 32; index++) {
         if (interrupt_bitfield & 1)
             capable_interrupts.append(index);
@@ -259,8 +311,8 @@ Vector<unsigned> HPET::capable_interrupt_numbers(u8 comparator_number)
 {
     ASSERT(comparator_number <= m_comparators.size());
     Vector<unsigned> capable_interrupts;
-    auto& comparator_registers = (const volatile TimerStructure&)registers().timers[comparator_number];
-    u32 interrupt_bitfield = comparator_registers.configuration_and_capability >> 32;
+    auto& comparator_registers = registers().timers[comparator_number];
+    u32 interrupt_bitfield = comparator_registers.interrupt_routing;
     for (size_t index = 0; index < 32; index++) {
         if (interrupt_bitfield & 1)
             capable_interrupts.append(index);
@@ -272,15 +324,15 @@ Vector<unsigned> HPET::capable_interrupt_numbers(u8 comparator_number)
 void HPET::set_comparator_irq_vector(u8 comparator_number, u8 irq_vector)
 {
     ASSERT(comparator_number <= m_comparators.size());
-    auto& comparator_registers = (volatile TimerStructure&)registers().timers[comparator_number];
-    comparator_registers.configuration_and_capability = comparator_registers.configuration_and_capability | (irq_vector << 9);
+    auto& comparator_registers = registers().timers[comparator_number];
+    comparator_registers.capabilities = comparator_registers.capabilities | (irq_vector << 9);
 }
 
 bool HPET::is_periodic_capable(u8 comparator_number) const
 {
     ASSERT(comparator_number <= m_comparators.size());
-    auto& comparator_registers = (const volatile TimerStructure&)registers().timers[comparator_number];
-    return comparator_registers.configuration_and_capability & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable;
+    auto& comparator_registers = registers().timers[comparator_number];
+    return comparator_registers.capabilities & (u32)HPETFlags::TimerConfiguration::PeriodicInterruptCapable;
 }
 
 void HPET::set_comparators_to_optimal_interrupt_state(size_t)
@@ -296,19 +348,20 @@ PhysicalAddress HPET::find_acpi_hpet_registers_block()
     return PhysicalAddress(sdt->event_timer_block.address);
 }
 
-const volatile HPETRegistersBlock& HPET::registers() const
+const HPETRegistersBlock& HPET::registers() const
 {
-    return *(const volatile HPETRegistersBlock*)m_hpet_mmio_region->vaddr().offset(m_physical_acpi_hpet_registers.offset_in_page()).as_ptr();
+    return *(const HPETRegistersBlock*)m_hpet_mmio_region->vaddr().offset(m_physical_acpi_hpet_registers.offset_in_page()).as_ptr();
 }
 
-volatile HPETRegistersBlock& HPET::registers()
+HPETRegistersBlock& HPET::registers()
 {
-    return *(volatile HPETRegistersBlock*)m_hpet_mmio_region->vaddr().offset(m_physical_acpi_hpet_registers.offset_in_page()).as_ptr();
+    return *(HPETRegistersBlock*)m_hpet_mmio_region->vaddr().offset(m_physical_acpi_hpet_registers.offset_in_page()).as_ptr();
 }
 
 u64 HPET::calculate_ticks_in_nanoseconds() const
 {
-    return ABSOLUTE_MAXIMUM_COUNTER_TICK_PERIOD / registers().capabilities.main_counter_tick_period;
+    // ABSOLUTE_MAXIMUM_COUNTER_TICK_PERIOD == 100 nanoseconds
+    return ((u64)registers().capabilities.main_counter_tick_period * 100ull) / ABSOLUTE_MAXIMUM_COUNTER_TICK_PERIOD;
 }
 
 HPET::HPET(PhysicalAddress acpi_hpet)
@@ -316,27 +369,36 @@ HPET::HPET(PhysicalAddress acpi_hpet)
     , m_physical_acpi_hpet_registers(find_acpi_hpet_registers_block())
     , m_hpet_mmio_region(MM.allocate_kernel_region(m_physical_acpi_hpet_registers.page_base(), PAGE_SIZE, "HPET MMIO", Region::Access::Read | Region::Access::Write))
 {
+    s_hpet = this; // Make available as soon as possible so that IRQs can use it
+
     auto sdt = map_typed<const volatile ACPI::Structures::HPET>(m_physical_acpi_hpet_table);
     m_vendor_id = sdt->pci_vendor_id;
     m_minimum_tick = sdt->mininum_clock_tick;
     klog() << "HPET: Minimum clock tick - " << m_minimum_tick;
 
+    auto& regs = registers();
+
     // Note: We must do a 32 bit access to offsets 0x0, or 0x4 only.
-    size_t timers_count = ((registers().raw_capabilites.reg >> 8) & 0x1f) + 1;
+    size_t timers_count = ((regs.capabilities.attributes >> 8) & 0x1f) + 1;
     klog() << "HPET: Timers count - " << timers_count;
+    klog() << "HPET: Main counter size: " << ((regs.capabilities.attributes & (u32)HPETFlags::Attributes::Counter64BitCapable) ? "64 bit" : "32 bit");
+    for (size_t i = 0; i < timers_count; i++) {
+        bool capable_64_bit = regs.timers[i].capabilities & (u32)HPETFlags::TimerConfiguration::Timer64BitsCapable;
+        klog() << "HPET: Timer[" << i << "] comparator size: " << (capable_64_bit ? "64 bit" : "32 bit") << " mode: " << ((!capable_64_bit || (regs.timers[i].capabilities & (u32)HPETFlags::TimerConfiguration::Force32BitMode)) ? "32 bit" : "64 bit");
+    }
     ASSERT(timers_count >= 2);
-    auto* capabilities_register = (const volatile HPETCapabilityRegister*)&registers().raw_capabilites.reg;
 
     global_disable();
 
     m_frequency = NANOSECOND_PERIOD_TO_HERTZ(calculate_ticks_in_nanoseconds());
-    klog() << "HPET: frequency " << m_frequency << " Hz (" << MEGAHERTZ_TO_HERTZ(m_frequency) << " MHz)";
-    ASSERT(capabilities_register->main_counter_tick_period <= ABSOLUTE_MAXIMUM_COUNTER_TICK_PERIOD);
+    klog() << "HPET: frequency " << m_frequency << " Hz (" << MEGAHERTZ_TO_HERTZ(m_frequency) << " MHz) resolution: " << calculate_ticks_in_nanoseconds() << "ns";
+    ASSERT(regs.capabilities.main_counter_tick_period <= ABSOLUTE_MAXIMUM_COUNTER_TICK_PERIOD);
 
     // Reset the counter, just in case...
-    registers().main_counter_value.reg = 0;
-    if (registers().raw_capabilites.reg & (u32)HPETFlags::Attributes::LegacyReplacementRouteCapable)
-        registers().configuration.reg = registers().configuration.reg | (u32)HPETFlags::Configuration::LegacyReplacementRoute;
+    regs.main_counter_value.high = 0;
+    regs.main_counter_value.low = 0;
+    if (regs.capabilities.attributes & (u32)HPETFlags::Attributes::LegacyReplacementRouteCapable)
+        regs.configuration.low = regs.configuration.low | (u32)HPETFlags::Configuration::LegacyReplacementRoute;
 
     m_comparators.append(HPETComparator::create(0, 0, is_periodic_capable(0)));
     m_comparators.append(HPETComparator::create(1, 8, is_periodic_capable(1)));
diff --git a/Kernel/Time/HPET.h b/Kernel/Time/HPET.h
index 1a048fb13a..d60adc4839 100644
--- a/Kernel/Time/HPET.h
+++ b/Kernel/Time/HPET.h
@@ -45,15 +45,14 @@ public:
     static bool check_for_exisiting_periodic_timers();
     static HPET& the();
 
-    u64 main_counter_value() const;
     u64 frequency() const;
 
     const NonnullRefPtrVector<HPETComparator>& comparators() const { return m_comparators; }
     void disable(const HPETComparator&);
     void enable(const HPETComparator&);
 
-    void set_periodic_comparator_value(const HPETComparator& comparator, u64 value);
-    void set_non_periodic_comparator_value(const HPETComparator& comparator, u64 value);
+    void update_periodic_comparator_value();
+    void update_non_periodic_comparator_value(const HPETComparator& comparator);
 
     void set_comparator_irq_vector(u8 comparator_number, u8 irq_vector);
 
@@ -64,8 +63,8 @@ public:
     Vector<unsigned> capable_interrupt_numbers(const HPETComparator&);
 
 private:
-    const volatile HPETRegistersBlock& registers() const;
-    volatile HPETRegistersBlock& registers();
+    const HPETRegistersBlock& registers() const;
+    HPETRegistersBlock& registers();
 
     void global_disable();
     void global_enable();
diff --git a/Kernel/Time/HPETComparator.cpp b/Kernel/Time/HPETComparator.cpp
index b2e68ce583..f902b92ea8 100644
--- a/Kernel/Time/HPETComparator.cpp
+++ b/Kernel/Time/HPETComparator.cpp
@@ -48,14 +48,14 @@ HPETComparator::HPETComparator(u8 number, u8 irq, bool periodic_capable)
 void HPETComparator::set_periodic()
 {
     ASSERT(m_periodic_capable);
-    HPET::the().enable_periodic_interrupt(*this);
     m_periodic = true;
+    HPET::the().enable_periodic_interrupt(*this);
 }
 void HPETComparator::set_non_periodic()
 {
     ASSERT(m_periodic_capable);
-    HPET::the().disable_periodic_interrupt(*this);
     m_periodic = false;
+    HPET::the().disable_periodic_interrupt(*this);
 }
 
 void HPETComparator::handle_irq(const RegisterState& regs)
@@ -69,7 +69,7 @@ void HPETComparator::set_new_countdown()
 {
     ASSERT_INTERRUPTS_DISABLED();
     ASSERT(m_frequency <= HPET::the().frequency());
-    HPET::the().set_non_periodic_comparator_value(*this, HPET::the().frequency() / m_frequency);
+    HPET::the().update_non_periodic_comparator_value(*this);
 }
 
 size_t HPETComparator::ticks_per_second() const
@@ -91,20 +91,21 @@ bool HPETComparator::try_to_set_frequency(size_t frequency)
     InterruptDisabler disabler;
     if (!is_capable_of_frequency(frequency))
         return false;
-    disable_irq();
+    if (m_frequency == frequency)
+        return true;
     auto hpet_frequency = HPET::the().frequency();
     ASSERT(frequency <= hpet_frequency);
+    m_frequency = frequency;
 #ifdef HPET_COMPARATOR_DEBUG
-    dbg() << "HPET Comparator: Max frequency - " << hpet_frequency << " Hz, want to set " << frequency << " Hz";
+    dbg() << "HPET Comparator: Max frequency " << hpet_frequency << " Hz, want to set " << frequency << " Hz, periodic: " << is_periodic();
 #endif
-    if (is_periodic())
-        HPET::the().set_periodic_comparator_value(*this, hpet_frequency / frequency);
-    else {
-        HPET::the().set_non_periodic_comparator_value(*this, hpet_frequency / frequency);
-        HPET::the().enable(*this);
+
+    if (is_periodic()) {
+        HPET::the().update_periodic_comparator_value();
+    } else {
+        HPET::the().update_non_periodic_comparator_value(*this);
     }
-    m_frequency = frequency;
-    enable_irq();
+    enable_irq(); // Enable if we haven't already
     return true;
 }
 bool HPETComparator::is_capable_of_frequency(size_t frequency) const
diff --git a/Kernel/Time/TimeManagement.cpp b/Kernel/Time/TimeManagement.cpp
index c04f096073..7d67605250 100644
--- a/Kernel/Time/TimeManagement.cpp
+++ b/Kernel/Time/TimeManagement.cpp
@@ -225,9 +225,10 @@ bool TimeManagement::probe_and_set_non_legacy_hardware_timers()
     }
 
     m_system_timer->set_callback(Scheduler::timer_tick);
+    m_time_keeper_timer->set_callback(TimeManagement::update_time);
+
     dbg() << "Reset timers";
     m_system_timer->try_to_set_frequency(m_system_timer->calculate_nearest_possible_frequency(1024));
-    m_time_keeper_timer->set_callback(TimeManagement::update_time);
     m_time_keeper_timer->try_to_set_frequency(OPTIMAL_TICKS_PER_SECOND_RATE);
 
     return true;