* [pve-devel] [PATCH qemu] revert hpet changes to fix performance regression
@ 2025-03-19 16:39 Fiona Ebner
2025-03-26 9:10 ` [pve-devel] applied: " Thomas Lamprecht
0 siblings, 1 reply; 2+ messages in thread
From: Fiona Ebner @ 2025-03-19 16:39 UTC (permalink / raw)
To: pve-devel
As reported in the community forum [0][1], QEMU processes for Linux
guests would consume more CPU on the host after an update to QEMU 9.2.
The issue was reproduced and bisecting pointed to QEMU commit
f0ccf77078 ("hpet: fix and cleanup persistence of interrupt status").
Some quick experimentation suggests that in particular the last part
is responsible for the issue:
> - the timer must be kept running even if not enabled, in
> order to set the ISR flag, so writes to HPET_TN_CFG must
> not call hpet_del_timer()
Users confirmed that setting the hpet=off machine flag works around
the issue[0]. For Windows (7 or later) guests, the flag is already
disabled, because of issues in the past [2].
Upstream suggested reverting the relevant patches for now [3], because
other issues were reported too. All except commit 5895879aca ("hpet:
remove unnecessary variable "index"") are actually dependent on each
other for cleanly reverting f0ccf77078, and while not strictly
required, that one was reverted too for completeness.
[0]: https://forum.proxmox.com/threads/163694/
[1]: https://forum.proxmox.com/threads/161849/post-756793
[2]: https://lists.proxmox.com/pipermail/pve-devel/2012-December/004958.html
[3]: https://lore.kernel.org/qemu-devel/CABgObfaKJ5NFVKmYLFmu4C0iZZLJJtcWksLCzyA0tBoz0koZ4A@mail.gmail.com/
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
...void-timer-storms-on-periodic-timers.patch | 50 ++++
...e-full-64-bit-target-value-of-the-co.patch | 203 +++++++++++++
...-hpet-accept-64-bit-reads-and-writes.patch | 281 ++++++++++++++++++
...e-read-only-bits-directly-in-new_val.patch | 64 ++++
...et-remove-unnecessary-variable-index.patch | 68 +++++
...re-high-bits-of-comparator-in-32-bit.patch | 40 +++
...and-cleanup-persistence-of-interrupt.patch | 120 ++++++++
debian/patches/series | 7 +
8 files changed, 833 insertions(+)
create mode 100644 debian/patches/pve/0051-Revert-hpet-avoid-timer-storms-on-periodic-timers.patch
create mode 100644 debian/patches/pve/0052-Revert-hpet-store-full-64-bit-target-value-of-the-co.patch
create mode 100644 debian/patches/pve/0053-Revert-hpet-accept-64-bit-reads-and-writes.patch
create mode 100644 debian/patches/pve/0054-Revert-hpet-place-read-only-bits-directly-in-new_val.patch
create mode 100644 debian/patches/pve/0055-Revert-hpet-remove-unnecessary-variable-index.patch
create mode 100644 debian/patches/pve/0056-Revert-hpet-ignore-high-bits-of-comparator-in-32-bit.patch
create mode 100644 debian/patches/pve/0057-Revert-hpet-fix-and-cleanup-persistence-of-interrupt.patch
diff --git a/debian/patches/pve/0051-Revert-hpet-avoid-timer-storms-on-periodic-timers.patch b/debian/patches/pve/0051-Revert-hpet-avoid-timer-storms-on-periodic-timers.patch
new file mode 100644
index 0000000..3ff1ee4
--- /dev/null
+++ b/debian/patches/pve/0051-Revert-hpet-avoid-timer-storms-on-periodic-timers.patch
@@ -0,0 +1,50 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Wed, 19 Mar 2025 17:31:05 +0100
+Subject: [PATCH] Revert "hpet: avoid timer storms on periodic timers"
+
+This reverts commit 7c912ffb59e8137091894d767433e65c3df8b0bf.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/timer/hpet.c | 13 ++-----------
+ 1 file changed, 2 insertions(+), 11 deletions(-)
+
+diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
+index 5399f1b2a3..8ccc421cbb 100644
+--- a/hw/timer/hpet.c
++++ b/hw/timer/hpet.c
+@@ -59,7 +59,6 @@ typedef struct HPETTimer { /* timers */
+ uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
+ * mode. Next pop will be actual timer expiration.
+ */
+- uint64_t last; /* last value armed, to avoid timer storms */
+ } HPETTimer;
+
+ struct HPETState {
+@@ -267,7 +266,6 @@ static int hpet_post_load(void *opaque, int version_id)
+ for (i = 0; i < s->num_timers; i++) {
+ HPETTimer *t = &s->timer[i];
+ t->cmp64 = hpet_calculate_cmp64(t, s->hpet_counter, t->cmp);
+- t->last = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - NANOSECONDS_PER_SECOND;
+ }
+ /* Recalculate the offset between the main counter and guest time */
+ if (!s->hpet_offset_saved) {
+@@ -366,15 +364,8 @@ static const VMStateDescription vmstate_hpet = {
+
+ static void hpet_arm(HPETTimer *t, uint64_t tick)
+ {
+- uint64_t ns = hpet_get_ns(t->state, tick);
+-
+- /* Clamp period to reasonable min value (1 us) */
+- if (timer_is_periodic(t) && ns - t->last < 1000) {
+- ns = t->last + 1000;
+- }
+-
+- t->last = ns;
+- timer_mod(t->qemu_timer, ns);
++ /* FIXME: Clamp period to reasonable min value? */
++ timer_mod(t->qemu_timer, hpet_get_ns(t->state, tick));
+ }
+
+ /*
diff --git a/debian/patches/pve/0052-Revert-hpet-store-full-64-bit-target-value-of-the-co.patch b/debian/patches/pve/0052-Revert-hpet-store-full-64-bit-target-value-of-the-co.patch
new file mode 100644
index 0000000..482940c
--- /dev/null
+++ b/debian/patches/pve/0052-Revert-hpet-store-full-64-bit-target-value-of-the-co.patch
@@ -0,0 +1,203 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Wed, 19 Mar 2025 17:31:08 +0100
+Subject: [PATCH] Revert "hpet: store full 64-bit target value of the counter"
+
+This reverts commit 242d665396407f83a6acbffc804882eeb21cfdad.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/timer/hpet.c | 111 +++++++++++++++++++++++++++---------------------
+ 1 file changed, 62 insertions(+), 49 deletions(-)
+
+diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
+index 8ccc421cbb..415a9433f1 100644
+--- a/hw/timer/hpet.c
++++ b/hw/timer/hpet.c
+@@ -54,7 +54,6 @@ typedef struct HPETTimer { /* timers */
+ uint64_t cmp; /* comparator */
+ uint64_t fsb; /* FSB route */
+ /* Hidden register state */
+- uint64_t cmp64; /* comparator (extended to counter width) */
+ uint64_t period; /* Last value written to comparator */
+ uint8_t wrap_flag; /* timer pop will indicate wrap for one-shot 32-bit
+ * mode. Next pop will be actual timer expiration.
+@@ -116,6 +115,11 @@ static uint32_t timer_enabled(HPETTimer *t)
+ }
+
+ static uint32_t hpet_time_after(uint64_t a, uint64_t b)
++{
++ return ((int32_t)(b - a) < 0);
++}
++
++static uint32_t hpet_time_after64(uint64_t a, uint64_t b)
+ {
+ return ((int64_t)(b - a) < 0);
+ }
+@@ -152,32 +156,27 @@ static uint64_t hpet_get_ticks(HPETState *s)
+ return ns_to_ticks(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->hpet_offset);
+ }
+
+-static uint64_t hpet_get_ns(HPETState *s, uint64_t tick)
+-{
+- return ticks_to_ns(tick) - s->hpet_offset;
+-}
+-
+ /*
+- * calculate next value of the general counter that matches the
+- * target (either entirely, or the low 32-bit only depending on
+- * the timer mode).
++ * calculate diff between comparator value and current ticks
+ */
+-static uint64_t hpet_calculate_cmp64(HPETTimer *t, uint64_t cur_tick, uint64_t target)
++static inline uint64_t hpet_calculate_diff(HPETTimer *t, uint64_t current)
+ {
++
+ if (t->config & HPET_TN_32BIT) {
+- uint64_t result = deposit64(cur_tick, 0, 32, target);
+- if (result < cur_tick) {
+- result += 0x100000000ULL;
+- }
+- return result;
++ uint32_t diff, cmp;
++
++ cmp = (uint32_t)t->cmp;
++ diff = cmp - (uint32_t)current;
++ diff = (int32_t)diff > 0 ? diff : (uint32_t)1;
++ return (uint64_t)diff;
+ } else {
+- return target;
+- }
+-}
++ uint64_t diff, cmp;
+
+-static uint64_t hpet_next_wrap(uint64_t cur_tick)
+-{
+- return (cur_tick | 0xffffffffU) + 1;
++ cmp = t->cmp;
++ diff = cmp - current;
++ diff = (int64_t)diff > 0 ? diff : (uint64_t)1;
++ return diff;
++ }
+ }
+
+ static void update_irq(struct HPETTimer *timer, int set)
+@@ -261,12 +260,7 @@ static bool hpet_validate_num_timers(void *opaque, int version_id)
+ static int hpet_post_load(void *opaque, int version_id)
+ {
+ HPETState *s = opaque;
+- int i;
+
+- for (i = 0; i < s->num_timers; i++) {
+- HPETTimer *t = &s->timer[i];
+- t->cmp64 = hpet_calculate_cmp64(t, s->hpet_counter, t->cmp);
+- }
+ /* Recalculate the offset between the main counter and guest time */
+ if (!s->hpet_offset_saved) {
+ s->hpet_offset = ticks_to_ns(s->hpet_counter)
+@@ -362,10 +356,14 @@ static const VMStateDescription vmstate_hpet = {
+ }
+ };
+
+-static void hpet_arm(HPETTimer *t, uint64_t tick)
++static void hpet_arm(HPETTimer *t, uint64_t ticks)
+ {
+- /* FIXME: Clamp period to reasonable min value? */
+- timer_mod(t->qemu_timer, hpet_get_ns(t->state, tick));
++ if (ticks < ns_to_ticks(INT64_MAX / 2)) {
++ timer_mod(t->qemu_timer,
++ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ticks_to_ns(ticks));
++ } else {
++ timer_del(t->qemu_timer);
++ }
+ }
+
+ /*
+@@ -374,44 +372,54 @@ static void hpet_arm(HPETTimer *t, uint64_t tick)
+ static void hpet_timer(void *opaque)
+ {
+ HPETTimer *t = opaque;
++ uint64_t diff;
++
+ uint64_t period = t->period;
+ uint64_t cur_tick = hpet_get_ticks(t->state);
+
+ if (timer_is_periodic(t) && period != 0) {
+- while (hpet_time_after(cur_tick, t->cmp64)) {
+- t->cmp64 += period;
+- }
+ if (t->config & HPET_TN_32BIT) {
+- t->cmp = (uint32_t)t->cmp64;
++ while (hpet_time_after(cur_tick, t->cmp)) {
++ t->cmp = (uint32_t)(t->cmp + t->period);
++ }
+ } else {
+- t->cmp = t->cmp64;
++ while (hpet_time_after64(cur_tick, t->cmp)) {
++ t->cmp += period;
++ }
++ }
++ diff = hpet_calculate_diff(t, cur_tick);
++ hpet_arm(t, diff);
++ } else if (t->config & HPET_TN_32BIT && !timer_is_periodic(t)) {
++ if (t->wrap_flag) {
++ diff = hpet_calculate_diff(t, cur_tick);
++ hpet_arm(t, diff);
++ t->wrap_flag = 0;
+ }
+- hpet_arm(t, t->cmp64);
+- } else if (t->wrap_flag) {
+- t->wrap_flag = 0;
+- hpet_arm(t, t->cmp64);
+ }
+ update_irq(t, 1);
+ }
+
+ static void hpet_set_timer(HPETTimer *t)
+ {
++ uint64_t diff;
++ uint32_t wrap_diff; /* how many ticks until we wrap? */
+ uint64_t cur_tick = hpet_get_ticks(t->state);
+
++ /* whenever new timer is being set up, make sure wrap_flag is 0 */
+ t->wrap_flag = 0;
+- t->cmp64 = hpet_calculate_cmp64(t, cur_tick, t->cmp);
+- if (t->config & HPET_TN_32BIT) {
+-
+- /* hpet spec says in one-shot 32-bit mode, generate an interrupt when
+- * counter wraps in addition to an interrupt with comparator match.
+- */
+- if (!timer_is_periodic(t) && t->cmp64 > hpet_next_wrap(cur_tick)) {
++ diff = hpet_calculate_diff(t, cur_tick);
++
++ /* hpet spec says in one-shot 32-bit mode, generate an interrupt when
++ * counter wraps in addition to an interrupt with comparator match.
++ */
++ if (t->config & HPET_TN_32BIT && !timer_is_periodic(t)) {
++ wrap_diff = 0xffffffff - (uint32_t)cur_tick;
++ if (wrap_diff < (uint32_t)diff) {
++ diff = wrap_diff;
+ t->wrap_flag = 1;
+- hpet_arm(t, hpet_next_wrap(cur_tick));
+- return;
+ }
+ }
+- hpet_arm(t, t->cmp64);
++ hpet_arm(t, diff);
+ }
+
+ static void hpet_del_timer(HPETTimer *t)
+@@ -542,7 +550,12 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ timer->cmp = deposit64(timer->cmp, shift, len, value);
+ }
+ if (timer_is_periodic(timer)) {
+- timer->period = deposit64(timer->period, shift, len, value);
++ /*
++ * FIXME: Clamp period to reasonable min value?
++ * Clamp period to reasonable max value
++ */
++ new_val = deposit64(timer->period, shift, len, value);
++ timer->period = MIN(new_val, (timer->config & HPET_TN_32BIT ? ~0u : ~0ull) >> 1);
+ }
+ timer->config &= ~HPET_TN_SETVAL;
+ if (hpet_enabled(s)) {
diff --git a/debian/patches/pve/0053-Revert-hpet-accept-64-bit-reads-and-writes.patch b/debian/patches/pve/0053-Revert-hpet-accept-64-bit-reads-and-writes.patch
new file mode 100644
index 0000000..b47b823
--- /dev/null
+++ b/debian/patches/pve/0053-Revert-hpet-accept-64-bit-reads-and-writes.patch
@@ -0,0 +1,281 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Wed, 19 Mar 2025 17:31:09 +0100
+Subject: [PATCH] Revert "hpet: accept 64-bit reads and writes"
+
+This reverts commit c2366567378dd8fb89329816003801f54e30e6f3.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/timer/hpet.c | 137 +++++++++++++++++++++++++++++-------------
+ hw/timer/trace-events | 3 +-
+ 2 files changed, 96 insertions(+), 44 deletions(-)
+
+diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
+index 415a9433f1..e1ac877759 100644
+--- a/hw/timer/hpet.c
++++ b/hw/timer/hpet.c
+@@ -437,7 +437,6 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
+ unsigned size)
+ {
+ HPETState *s = opaque;
+- int shift = (addr & 4) * 8;
+ uint64_t cur_tick;
+
+ trace_hpet_ram_read(addr);
+@@ -452,33 +451,52 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
+ return 0;
+ }
+
+- switch (addr & 0x18) {
+- case HPET_TN_CFG: // including interrupt capabilities
+- return timer->config >> shift;
++ switch ((addr - 0x100) % 0x20) {
++ case HPET_TN_CFG:
++ return timer->config;
++ case HPET_TN_CFG + 4: // Interrupt capabilities
++ return timer->config >> 32;
+ case HPET_TN_CMP: // comparator register
+- return timer->cmp >> shift;
++ return timer->cmp;
++ case HPET_TN_CMP + 4:
++ return timer->cmp >> 32;
+ case HPET_TN_ROUTE:
+- return timer->fsb >> shift;
++ return timer->fsb;
++ case HPET_TN_ROUTE + 4:
++ return timer->fsb >> 32;
+ default:
+ trace_hpet_ram_read_invalid();
+ break;
+ }
+ } else {
+- switch (addr & ~4) {
+- case HPET_ID: // including HPET_PERIOD
+- return s->capability >> shift;
++ switch (addr) {
++ case HPET_ID:
++ return s->capability;
++ case HPET_PERIOD:
++ return s->capability >> 32;
+ case HPET_CFG:
+- return s->config >> shift;
++ return s->config;
++ case HPET_CFG + 4:
++ trace_hpet_invalid_hpet_cfg(4);
++ return 0;
+ case HPET_COUNTER:
+ if (hpet_enabled(s)) {
+ cur_tick = hpet_get_ticks(s);
+ } else {
+ cur_tick = s->hpet_counter;
+ }
+- trace_hpet_ram_read_reading_counter(addr & 4, cur_tick);
+- return cur_tick >> shift;
++ trace_hpet_ram_read_reading_counter(0, cur_tick);
++ return cur_tick;
++ case HPET_COUNTER + 4:
++ if (hpet_enabled(s)) {
++ cur_tick = hpet_get_ticks(s);
++ } else {
++ cur_tick = s->hpet_counter;
++ }
++ trace_hpet_ram_read_reading_counter(4, cur_tick);
++ return cur_tick >> 32;
+ case HPET_STATUS:
+- return s->isr >> shift;
++ return s->isr;
+ default:
+ trace_hpet_ram_read_invalid();
+ break;
+@@ -492,11 +510,11 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ {
+ int i;
+ HPETState *s = opaque;
+- int shift = (addr & 4) * 8;
+- int len = MIN(size * 8, 64 - shift);
+ uint64_t old_val, new_val, cleared;
+
+ trace_hpet_ram_write(addr, value);
++ old_val = hpet_ram_read(opaque, addr, 4);
++ new_val = value;
+
+ /*address range of all TN regs*/
+ if (addr >= 0x100 && addr <= 0x3ff) {
+@@ -508,12 +526,9 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ trace_hpet_timer_id_out_of_range(timer_id);
+ return;
+ }
+- switch (addr & 0x18) {
++ switch ((addr - 0x100) % 0x20) {
+ case HPET_TN_CFG:
+- trace_hpet_ram_write_tn_cfg(addr & 4);
+- old_val = timer->config;
+- new_val = deposit64(old_val, shift, len, value);
+- new_val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
++ trace_hpet_ram_write_tn_cfg();
+ if (deactivating_bit(old_val, new_val, HPET_TN_TYPE_LEVEL)) {
+ /*
+ * Do this before changing timer->config; otherwise, if
+@@ -521,7 +536,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ */
+ update_irq(timer, 0);
+ }
+- timer->config = new_val;
++ new_val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
++ timer->config = (timer->config & 0xffffffff00000000ULL) | new_val;
+ if (activating_bit(old_val, new_val, HPET_TN_ENABLE)
+ && (s->isr & (1 << timer_id))) {
+ update_irq(timer, 1);
+@@ -534,28 +550,56 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ hpet_set_timer(timer);
+ }
+ break;
++ case HPET_TN_CFG + 4: // Interrupt capabilities
++ trace_hpet_ram_write_invalid_tn_cfg(4);
++ break;
+ case HPET_TN_CMP: // comparator register
++ trace_hpet_ram_write_tn_cmp(0);
+ if (timer->config & HPET_TN_32BIT) {
+- /* High 32-bits are zero, leave them untouched. */
+- if (shift) {
+- trace_hpet_ram_write_invalid_tn_cmp();
+- break;
++ new_val = (uint32_t)new_val;
++ }
++ if (!timer_is_periodic(timer)
++ || (timer->config & HPET_TN_SETVAL)) {
++ timer->cmp = (timer->cmp & 0xffffffff00000000ULL) | new_val;
++ }
++ if (timer_is_periodic(timer)) {
++ /*
++ * FIXME: Clamp period to reasonable min value?
++ * Clamp period to reasonable max value
++ */
++ if (timer->config & HPET_TN_32BIT) {
++ new_val = MIN(new_val, ~0u >> 1);
+ }
+- len = 64;
+- value = (uint32_t) value;
++ timer->period =
++ (timer->period & 0xffffffff00000000ULL) | new_val;
++ }
++ /*
++ * FIXME: on a 64-bit write, HPET_TN_SETVAL should apply to the
++ * high bits part as well.
++ */
++ timer->config &= ~HPET_TN_SETVAL;
++ if (hpet_enabled(s)) {
++ hpet_set_timer(timer);
+ }
+- trace_hpet_ram_write_tn_cmp(addr & 4);
++ break;
++ case HPET_TN_CMP + 4: // comparator register high order
++ if (timer->config & HPET_TN_32BIT) {
++ trace_hpet_ram_write_invalid_tn_cmp();
++ break;
++ }
++ trace_hpet_ram_write_tn_cmp(4);
+ if (!timer_is_periodic(timer)
+ || (timer->config & HPET_TN_SETVAL)) {
+- timer->cmp = deposit64(timer->cmp, shift, len, value);
++ timer->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
+ }
+ if (timer_is_periodic(timer)) {
+ /*
+ * FIXME: Clamp period to reasonable min value?
+ * Clamp period to reasonable max value
+ */
+- new_val = deposit64(timer->period, shift, len, value);
+- timer->period = MIN(new_val, (timer->config & HPET_TN_32BIT ? ~0u : ~0ull) >> 1);
++ new_val = MIN(new_val, ~0u >> 1);
++ timer->period =
++ (timer->period & 0xffffffffULL) | new_val << 32;
+ }
+ timer->config &= ~HPET_TN_SETVAL;
+ if (hpet_enabled(s)) {
+@@ -563,7 +607,10 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ }
+ break;
+ case HPET_TN_ROUTE:
+- timer->fsb = deposit64(timer->fsb, shift, len, value);
++ timer->fsb = (timer->fsb & 0xffffffff00000000ULL) | new_val;
++ break;
++ case HPET_TN_ROUTE + 4:
++ timer->fsb = (new_val << 32) | (timer->fsb & 0xffffffff);
+ break;
+ default:
+ trace_hpet_ram_write_invalid();
+@@ -571,14 +618,12 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ }
+ return;
+ } else {
+- switch (addr & ~4) {
++ switch (addr) {
+ case HPET_ID:
+ return;
+ case HPET_CFG:
+- old_val = s->config;
+- new_val = deposit64(old_val, shift, len, value);
+ new_val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
+- s->config = new_val;
++ s->config = (s->config & 0xffffffff00000000ULL) | new_val;
+ if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
+ /* Enable main counter and interrupt generation. */
+ s->hpet_offset =
+@@ -608,8 +653,10 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ qemu_set_irq(s->irqs[RTC_ISA_IRQ], s->rtc_irq_level);
+ }
+ break;
++ case HPET_CFG + 4:
++ trace_hpet_invalid_hpet_cfg(4);
++ break;
+ case HPET_STATUS:
+- new_val = value << shift;
+ cleared = new_val & s->isr;
+ for (i = 0; i < s->num_timers; i++) {
+ if (cleared & (1 << i)) {
+@@ -621,7 +668,15 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ if (hpet_enabled(s)) {
+ trace_hpet_ram_write_counter_write_while_enabled();
+ }
+- s->hpet_counter = deposit64(s->hpet_counter, shift, len, value);
++ s->hpet_counter =
++ (s->hpet_counter & 0xffffffff00000000ULL) | value;
++ trace_hpet_ram_write_counter_written(0, value, s->hpet_counter);
++ break;
++ case HPET_COUNTER + 4:
++ trace_hpet_ram_write_counter_write_while_enabled();
++ s->hpet_counter =
++ (s->hpet_counter & 0xffffffffULL) | (((uint64_t)value) << 32);
++ trace_hpet_ram_write_counter_written(4, value, s->hpet_counter);
+ break;
+ default:
+ trace_hpet_ram_write_invalid();
+@@ -635,11 +690,7 @@ static const MemoryRegionOps hpet_ram_ops = {
+ .write = hpet_ram_write,
+ .valid = {
+ .min_access_size = 4,
+- .max_access_size = 8,
+- },
+- .impl = {
+- .min_access_size = 4,
+- .max_access_size = 8,
++ .max_access_size = 4,
+ },
+ .endianness = DEVICE_NATIVE_ENDIAN,
+ };
+diff --git a/hw/timer/trace-events b/hw/timer/trace-events
+index 5cfc369fba..219747df2f 100644
+--- a/hw/timer/trace-events
++++ b/hw/timer/trace-events
+@@ -114,7 +114,8 @@ hpet_ram_read_reading_counter(uint8_t reg_off, uint64_t cur_tick) "reading count
+ hpet_ram_read_invalid(void) "invalid hpet_ram_readl"
+ hpet_ram_write(uint64_t addr, uint64_t value) "enter hpet_ram_writel at 0x%" PRIx64 " = 0x%" PRIx64
+ hpet_ram_write_timer_id(uint64_t timer_id) "hpet_ram_writel timer_id = 0x%" PRIx64
+-hpet_ram_write_tn_cfg(uint8_t reg_off) "hpet_ram_writel HPET_TN_CFG + %" PRIu8
++hpet_ram_write_tn_cfg(void) "hpet_ram_writel HPET_TN_CFG"
++hpet_ram_write_invalid_tn_cfg(uint8_t reg_off) "invalid HPET_TN_CFG + %" PRIu8 " write"
+ hpet_ram_write_tn_cmp(uint8_t reg_off) "hpet_ram_writel HPET_TN_CMP + %" PRIu8
+ hpet_ram_write_invalid_tn_cmp(void) "invalid HPET_TN_CMP + 4 write"
+ hpet_ram_write_invalid(void) "invalid hpet_ram_writel"
diff --git a/debian/patches/pve/0054-Revert-hpet-place-read-only-bits-directly-in-new_val.patch b/debian/patches/pve/0054-Revert-hpet-place-read-only-bits-directly-in-new_val.patch
new file mode 100644
index 0000000..85b0989
--- /dev/null
+++ b/debian/patches/pve/0054-Revert-hpet-place-read-only-bits-directly-in-new_val.patch
@@ -0,0 +1,64 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Wed, 19 Mar 2025 17:31:10 +0100
+Subject: [PATCH] Revert "hpet: place read-only bits directly in "new_val""
+
+This reverts commit ba88935b0fac2588b0a739f810b58dfabf7f92c8.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/timer/hpet.c | 15 ++++++++-------
+ 1 file changed, 8 insertions(+), 7 deletions(-)
+
+diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
+index e1ac877759..b12bbaf10d 100644
+--- a/hw/timer/hpet.c
++++ b/hw/timer/hpet.c
+@@ -510,7 +510,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ {
+ int i;
+ HPETState *s = opaque;
+- uint64_t old_val, new_val, cleared;
++ uint64_t old_val, new_val, val;
+
+ trace_hpet_ram_write(addr, value);
+ old_val = hpet_ram_read(opaque, addr, 4);
+@@ -536,12 +536,13 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ */
+ update_irq(timer, 0);
+ }
+- new_val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
+- timer->config = (timer->config & 0xffffffff00000000ULL) | new_val;
++ val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
++ timer->config = (timer->config & 0xffffffff00000000ULL) | val;
+ if (activating_bit(old_val, new_val, HPET_TN_ENABLE)
+ && (s->isr & (1 << timer_id))) {
+ update_irq(timer, 1);
+ }
++
+ if (new_val & HPET_TN_32BIT) {
+ timer->cmp = (uint32_t)timer->cmp;
+ timer->period = (uint32_t)timer->period;
+@@ -622,8 +623,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ case HPET_ID:
+ return;
+ case HPET_CFG:
+- new_val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
+- s->config = (s->config & 0xffffffff00000000ULL) | new_val;
++ val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
++ s->config = (s->config & 0xffffffff00000000ULL) | val;
+ if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
+ /* Enable main counter and interrupt generation. */
+ s->hpet_offset =
+@@ -657,9 +658,9 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ trace_hpet_invalid_hpet_cfg(4);
+ break;
+ case HPET_STATUS:
+- cleared = new_val & s->isr;
++ val = new_val & s->isr;
+ for (i = 0; i < s->num_timers; i++) {
+- if (cleared & (1 << i)) {
++ if (val & (1 << i)) {
+ update_irq(&s->timer[i], 0);
+ }
+ }
diff --git a/debian/patches/pve/0055-Revert-hpet-remove-unnecessary-variable-index.patch b/debian/patches/pve/0055-Revert-hpet-remove-unnecessary-variable-index.patch
new file mode 100644
index 0000000..c8aa6ad
--- /dev/null
+++ b/debian/patches/pve/0055-Revert-hpet-remove-unnecessary-variable-index.patch
@@ -0,0 +1,68 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Wed, 19 Mar 2025 17:31:11 +0100
+Subject: [PATCH] Revert "hpet: remove unnecessary variable "index""
+
+This reverts commit 5895879aca252f4ebb2d1078eaf836c61ec54e9b.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/timer/hpet.c | 15 ++++++++-------
+ 1 file changed, 8 insertions(+), 7 deletions(-)
+
+diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
+index b12bbaf10d..6f83d88516 100644
+--- a/hw/timer/hpet.c
++++ b/hw/timer/hpet.c
+@@ -437,12 +437,12 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
+ unsigned size)
+ {
+ HPETState *s = opaque;
+- uint64_t cur_tick;
++ uint64_t cur_tick, index;
+
+ trace_hpet_ram_read(addr);
+-
++ index = addr;
+ /*address range of all TN regs*/
+- if (addr >= 0x100 && addr <= 0x3ff) {
++ if (index >= 0x100 && index <= 0x3ff) {
+ uint8_t timer_id = (addr - 0x100) / 0x20;
+ HPETTimer *timer = &s->timer[timer_id];
+
+@@ -469,7 +469,7 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
+ break;
+ }
+ } else {
+- switch (addr) {
++ switch (index) {
+ case HPET_ID:
+ return s->capability;
+ case HPET_PERIOD:
+@@ -510,14 +510,15 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ {
+ int i;
+ HPETState *s = opaque;
+- uint64_t old_val, new_val, val;
++ uint64_t old_val, new_val, val, index;
+
+ trace_hpet_ram_write(addr, value);
++ index = addr;
+ old_val = hpet_ram_read(opaque, addr, 4);
+ new_val = value;
+
+ /*address range of all TN regs*/
+- if (addr >= 0x100 && addr <= 0x3ff) {
++ if (index >= 0x100 && index <= 0x3ff) {
+ uint8_t timer_id = (addr - 0x100) / 0x20;
+ HPETTimer *timer = &s->timer[timer_id];
+
+@@ -619,7 +620,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ }
+ return;
+ } else {
+- switch (addr) {
++ switch (index) {
+ case HPET_ID:
+ return;
+ case HPET_CFG:
diff --git a/debian/patches/pve/0056-Revert-hpet-ignore-high-bits-of-comparator-in-32-bit.patch b/debian/patches/pve/0056-Revert-hpet-ignore-high-bits-of-comparator-in-32-bit.patch
new file mode 100644
index 0000000..0fde8dd
--- /dev/null
+++ b/debian/patches/pve/0056-Revert-hpet-ignore-high-bits-of-comparator-in-32-bit.patch
@@ -0,0 +1,40 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Wed, 19 Mar 2025 17:31:12 +0100
+Subject: [PATCH] Revert "hpet: ignore high bits of comparator in 32-bit mode"
+
+This reverts commit 9eb7fad3546a89ee7cf0e90f5b1daccf89725cea.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/timer/hpet.c | 4 ----
+ hw/timer/trace-events | 1 -
+ 2 files changed, 5 deletions(-)
+
+diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
+index 6f83d88516..509986c0a9 100644
+--- a/hw/timer/hpet.c
++++ b/hw/timer/hpet.c
+@@ -585,10 +585,6 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ }
+ break;
+ case HPET_TN_CMP + 4: // comparator register high order
+- if (timer->config & HPET_TN_32BIT) {
+- trace_hpet_ram_write_invalid_tn_cmp();
+- break;
+- }
+ trace_hpet_ram_write_tn_cmp(4);
+ if (!timer_is_periodic(timer)
+ || (timer->config & HPET_TN_SETVAL)) {
+diff --git a/hw/timer/trace-events b/hw/timer/trace-events
+index 219747df2f..b86870fb22 100644
+--- a/hw/timer/trace-events
++++ b/hw/timer/trace-events
+@@ -117,7 +117,6 @@ hpet_ram_write_timer_id(uint64_t timer_id) "hpet_ram_writel timer_id = 0x%" PRIx
+ hpet_ram_write_tn_cfg(void) "hpet_ram_writel HPET_TN_CFG"
+ hpet_ram_write_invalid_tn_cfg(uint8_t reg_off) "invalid HPET_TN_CFG + %" PRIu8 " write"
+ hpet_ram_write_tn_cmp(uint8_t reg_off) "hpet_ram_writel HPET_TN_CMP + %" PRIu8
+-hpet_ram_write_invalid_tn_cmp(void) "invalid HPET_TN_CMP + 4 write"
+ hpet_ram_write_invalid(void) "invalid hpet_ram_writel"
+ hpet_ram_write_counter_write_while_enabled(void) "Writing counter while HPET enabled!"
+ hpet_ram_write_counter_written(uint8_t reg_off, uint64_t value, uint64_t counter) "HPET counter + %" PRIu8 "written. crt = 0x%" PRIx64 " -> 0x%" PRIx64
diff --git a/debian/patches/pve/0057-Revert-hpet-fix-and-cleanup-persistence-of-interrupt.patch b/debian/patches/pve/0057-Revert-hpet-fix-and-cleanup-persistence-of-interrupt.patch
new file mode 100644
index 0000000..0c58bae
--- /dev/null
+++ b/debian/patches/pve/0057-Revert-hpet-fix-and-cleanup-persistence-of-interrupt.patch
@@ -0,0 +1,120 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Wed, 19 Mar 2025 17:31:13 +0100
+Subject: [PATCH] Revert "hpet: fix and cleanup persistence of interrupt
+ status"
+
+This reverts commit f0ccf770789e48b7a73497b465fdc892d28c1339.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ hw/timer/hpet.c | 60 ++++++++++++++++---------------------------------
+ 1 file changed, 19 insertions(+), 41 deletions(-)
+
+diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
+index 509986c0a9..402cc960f0 100644
+--- a/hw/timer/hpet.c
++++ b/hw/timer/hpet.c
+@@ -196,31 +196,21 @@ static void update_irq(struct HPETTimer *timer, int set)
+ }
+ s = timer->state;
+ mask = 1 << timer->tn;
+-
+- if (set && (timer->config & HPET_TN_TYPE_LEVEL)) {
+- /*
+- * If HPET_TN_ENABLE bit is 0, "the timer will still operate and
+- * generate appropriate status bits, but will not cause an interrupt"
+- */
+- s->isr |= mask;
+- } else {
++ if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) {
+ s->isr &= ~mask;
+- }
+-
+- if (set && timer_enabled(timer) && hpet_enabled(s)) {
+- if (timer_fsb_route(timer)) {
+- address_space_stl_le(&address_space_memory, timer->fsb >> 32,
+- timer->fsb & 0xffffffff, MEMTXATTRS_UNSPECIFIED,
+- NULL);
+- } else if (timer->config & HPET_TN_TYPE_LEVEL) {
+- qemu_irq_raise(s->irqs[route]);
+- } else {
+- qemu_irq_pulse(s->irqs[route]);
+- }
+- } else {
+ if (!timer_fsb_route(timer)) {
+ qemu_irq_lower(s->irqs[route]);
+ }
++ } else if (timer_fsb_route(timer)) {
++ address_space_stl_le(&address_space_memory, timer->fsb >> 32,
++ timer->fsb & 0xffffffff, MEMTXATTRS_UNSPECIFIED,
++ NULL);
++ } else if (timer->config & HPET_TN_TYPE_LEVEL) {
++ s->isr |= mask;
++ qemu_irq_raise(s->irqs[route]);
++ } else {
++ s->isr &= ~mask;
++ qemu_irq_pulse(s->irqs[route]);
+ }
+ }
+
+@@ -424,13 +414,8 @@ static void hpet_set_timer(HPETTimer *t)
+
+ static void hpet_del_timer(HPETTimer *t)
+ {
+- HPETState *s = t->state;
+ timer_del(t->qemu_timer);
+-
+- if (s->isr & (1 << t->tn)) {
+- /* For level-triggered interrupt, this leaves ISR set but lowers irq. */
+- update_irq(t, 1);
+- }
++ update_irq(t, 0);
+ }
+
+ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
+@@ -530,26 +515,20 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ switch ((addr - 0x100) % 0x20) {
+ case HPET_TN_CFG:
+ trace_hpet_ram_write_tn_cfg();
+- if (deactivating_bit(old_val, new_val, HPET_TN_TYPE_LEVEL)) {
+- /*
+- * Do this before changing timer->config; otherwise, if
+- * HPET_TN_FSB is set, update_irq will not lower the qemu_irq.
+- */
++ if (activating_bit(old_val, new_val, HPET_TN_FSB_ENABLE)) {
+ update_irq(timer, 0);
+ }
+ val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
+ timer->config = (timer->config & 0xffffffff00000000ULL) | val;
+- if (activating_bit(old_val, new_val, HPET_TN_ENABLE)
+- && (s->isr & (1 << timer_id))) {
+- update_irq(timer, 1);
+- }
+-
+ if (new_val & HPET_TN_32BIT) {
+ timer->cmp = (uint32_t)timer->cmp;
+ timer->period = (uint32_t)timer->period;
+ }
+- if (hpet_enabled(s)) {
++ if (activating_bit(old_val, new_val, HPET_TN_ENABLE) &&
++ hpet_enabled(s)) {
+ hpet_set_timer(timer);
++ } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE)) {
++ hpet_del_timer(timer);
+ }
+ break;
+ case HPET_TN_CFG + 4: // Interrupt capabilities
+@@ -627,10 +606,9 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
+ s->hpet_offset =
+ ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ for (i = 0; i < s->num_timers; i++) {
+- if (timer_enabled(&s->timer[i]) && (s->isr & (1 << i))) {
+- update_irq(&s->timer[i], 1);
++ if ((&s->timer[i])->cmp != ~0ULL) {
++ hpet_set_timer(&s->timer[i]);
+ }
+- hpet_set_timer(&s->timer[i]);
+ }
+ } else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
+ /* Halt main counter and disable interrupt generation. */
diff --git a/debian/patches/series b/debian/patches/series
index b780c1f..c077e88 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -65,3 +65,10 @@ pve/0047-PVE-backup-factor-out-setting-up-snapshot-access-for.patch
pve/0048-PVE-backup-save-device-name-in-device-info-structure.patch
pve/0049-PVE-backup-include-device-name-in-error-when-setting.patch
pve/0050-adapt-machine-version-deprecation-for-Proxmox-VE.patch
+pve/0051-Revert-hpet-avoid-timer-storms-on-periodic-timers.patch
+pve/0052-Revert-hpet-store-full-64-bit-target-value-of-the-co.patch
+pve/0053-Revert-hpet-accept-64-bit-reads-and-writes.patch
+pve/0054-Revert-hpet-place-read-only-bits-directly-in-new_val.patch
+pve/0055-Revert-hpet-remove-unnecessary-variable-index.patch
+pve/0056-Revert-hpet-ignore-high-bits-of-comparator-in-32-bit.patch
+pve/0057-Revert-hpet-fix-and-cleanup-persistence-of-interrupt.patch
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* [pve-devel] applied: [PATCH qemu] revert hpet changes to fix performance regression
2025-03-19 16:39 [pve-devel] [PATCH qemu] revert hpet changes to fix performance regression Fiona Ebner
@ 2025-03-26 9:10 ` Thomas Lamprecht
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2025-03-26 9:10 UTC (permalink / raw)
To: Proxmox VE development discussion, Fiona Ebner
Am 19.03.25 um 17:39 schrieb Fiona Ebner:
> As reported in the community forum [0][1], QEMU processes for Linux
> guests would consume more CPU on the host after an update to QEMU 9.2.
> The issue was reproduced and bisecting pointed to QEMU commit
> f0ccf77078 ("hpet: fix and cleanup persistence of interrupt status").
>
> Some quick experimentation suggests that in particular the last part
> is responsible for the issue:
>> - the timer must be kept running even if not enabled, in
>> order to set the ISR flag, so writes to HPET_TN_CFG must
>> not call hpet_del_timer()
>
> Users confirmed that setting the hpet=off machine flag works around
> the issue[0]. For Windows (7 or later) guests, the flag is already
> disabled, because of issues in the past [2].
>
> Upstream suggested reverting the relevant patches for now [3], because
> other issues were reported too. All except commit 5895879aca ("hpet:
> remove unnecessary variable "index"") are actually dependent on each
> other for cleanly reverting f0ccf77078, and while not strictly
> required, that one was reverted too for completeness.
>
> [0]: https://forum.proxmox.com/threads/163694/
> [1]: https://forum.proxmox.com/threads/161849/post-756793
> [2]: https://lists.proxmox.com/pipermail/pve-devel/2012-December/004958.html
> [3]: https://lore.kernel.org/qemu-devel/CABgObfaKJ5NFVKmYLFmu4C0iZZLJJtcWksLCzyA0tBoz0koZ4A@mail.gmail.com/
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> ...void-timer-storms-on-periodic-timers.patch | 50 ++++
> ...e-full-64-bit-target-value-of-the-co.patch | 203 +++++++++++++
> ...-hpet-accept-64-bit-reads-and-writes.patch | 281 ++++++++++++++++++
> ...e-read-only-bits-directly-in-new_val.patch | 64 ++++
> ...et-remove-unnecessary-variable-index.patch | 68 +++++
> ...re-high-bits-of-comparator-in-32-bit.patch | 40 +++
> ...and-cleanup-persistence-of-interrupt.patch | 120 ++++++++
> debian/patches/series | 7 +
> 8 files changed, 833 insertions(+)
> create mode 100644 debian/patches/pve/0051-Revert-hpet-avoid-timer-storms-on-periodic-timers.patch
> create mode 100644 debian/patches/pve/0052-Revert-hpet-store-full-64-bit-target-value-of-the-co.patch
> create mode 100644 debian/patches/pve/0053-Revert-hpet-accept-64-bit-reads-and-writes.patch
> create mode 100644 debian/patches/pve/0054-Revert-hpet-place-read-only-bits-directly-in-new_val.patch
> create mode 100644 debian/patches/pve/0055-Revert-hpet-remove-unnecessary-variable-index.patch
> create mode 100644 debian/patches/pve/0056-Revert-hpet-ignore-high-bits-of-comparator-in-32-bit.patch
> create mode 100644 debian/patches/pve/0057-Revert-hpet-fix-and-cleanup-persistence-of-interrupt.patch
>
>
applied, thanks!
nit: would have moved the patches in another folder, potentially a new
dedicated hpet one, as this isn't a pve downstream adaption/extension,
but no biggie.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-03-26 9:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-03-19 16:39 [pve-devel] [PATCH qemu] revert hpet changes to fix performance regression Fiona Ebner
2025-03-26 9:10 ` [pve-devel] applied: " Thomas Lamprecht
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal