From f0ccf770789e48b7a73497b465fdc892d28c1339 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 16 Jul 2024 11:36:44 +0200 Subject: hpet: fix and cleanup persistence of interrupt status There are several bugs in the handling of the ISR register: - switching level->edge was not lowering the interrupt and clearing ISR - switching on the enable bit was not raising a level-triggered interrupt if the timer had fired - 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() Signed-off-by: Paolo Bonzini --- hw/timer/hpet.c | 60 +++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 19 deletions(-) (limited to 'hw/timer/hpet.c') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index 4cb5393c0b..58073df02b 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -196,21 +196,31 @@ static void update_irq(struct HPETTimer *timer, int set) } s = timer->state; mask = 1 << timer->tn; - if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) { + + 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 { 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]); } } @@ -414,8 +424,13 @@ static void hpet_set_timer(HPETTimer *t) static void hpet_del_timer(HPETTimer *t) { + HPETState *s = t->state; timer_del(t->qemu_timer); - update_irq(t, 0); + + if (s->isr & (1 << t->tn)) { + /* For level-triggered interrupt, this leaves ISR set but lowers irq. */ + update_irq(t, 1); + } } static uint64_t hpet_ram_read(void *opaque, hwaddr addr, @@ -515,20 +530,26 @@ static void hpet_ram_write(void *opaque, hwaddr addr, switch ((addr - 0x100) % 0x20) { case HPET_TN_CFG: trace_hpet_ram_write_tn_cfg(); - if (activating_bit(old_val, new_val, HPET_TN_FSB_ENABLE)) { + 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. + */ 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 (activating_bit(old_val, new_val, HPET_TN_ENABLE) && - hpet_enabled(s)) { + if (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 @@ -606,9 +627,10 @@ 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 ((&s->timer[i])->cmp != ~0ULL) { - hpet_set_timer(&s->timer[i]); + if (timer_enabled(&s->timer[i]) && (s->isr & (1 << i))) { + update_irq(&s->timer[i], 1); } + hpet_set_timer(&s->timer[i]); } } else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) { /* Halt main counter and disable interrupt generation. */ -- cgit 1.4.1 From 9eb7fad3546a89ee7cf0e90f5b1daccf89725cea Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 10 Jul 2024 10:53:05 +0200 Subject: hpet: ignore high bits of comparator in 32-bit mode Signed-off-by: Paolo Bonzini --- hw/timer/hpet.c | 4 ++++ hw/timer/trace-events | 1 + 2 files changed, 5 insertions(+) (limited to 'hw/timer/hpet.c') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index 58073df02b..bbb1e5f089 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -585,6 +585,10 @@ 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 de769f4b71..a5fafbc679 100644 --- a/hw/timer/trace-events +++ b/hw/timer/trace-events @@ -111,6 +111,7 @@ 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 -- cgit 1.4.1 From 5895879aca252f4ebb2d1078eaf836c61ec54e9b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 10 Jul 2024 10:31:48 +0200 Subject: hpet: remove unnecessary variable "index" Signed-off-by: Paolo Bonzini --- hw/timer/hpet.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) (limited to 'hw/timer/hpet.c') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index bbb1e5f089..380e272fbe 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, index; + uint64_t cur_tick; trace_hpet_ram_read(addr); - index = addr; + /*address range of all TN regs*/ - if (index >= 0x100 && index <= 0x3ff) { + if (addr >= 0x100 && addr <= 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 (index) { + switch (addr) { case HPET_ID: return s->capability; case HPET_PERIOD: @@ -510,15 +510,14 @@ static void hpet_ram_write(void *opaque, hwaddr addr, { int i; HPETState *s = opaque; - uint64_t old_val, new_val, val, index; + uint64_t old_val, new_val, val; 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 (index >= 0x100 && index <= 0x3ff) { + if (addr >= 0x100 && addr <= 0x3ff) { uint8_t timer_id = (addr - 0x100) / 0x20; HPETTimer *timer = &s->timer[timer_id]; @@ -620,7 +619,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr, } return; } else { - switch (index) { + switch (addr) { case HPET_ID: return; case HPET_CFG: -- cgit 1.4.1 From ba88935b0fac2588b0a739f810b58dfabf7f92c8 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 10 Jul 2024 10:55:13 +0200 Subject: hpet: place read-only bits directly in "new_val" The variable "val" is used for two different purposes. As an intermediate value when writing configuration registers, and to store the cleared bits when writing ISR. Use "new_val" for the former, and rename the variable so that it is clearer for the latter case. Signed-off-by: Paolo Bonzini --- hw/timer/hpet.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) (limited to 'hw/timer/hpet.c') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index 380e272fbe..831e5a95b0 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, val; + uint64_t old_val, new_val, cleared; trace_hpet_ram_write(addr, value); old_val = hpet_ram_read(opaque, addr, 4); @@ -536,13 +536,12 @@ static void hpet_ram_write(void *opaque, hwaddr addr, */ update_irq(timer, 0); } - val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK); - timer->config = (timer->config & 0xffffffff00000000ULL) | 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); } - if (new_val & HPET_TN_32BIT) { timer->cmp = (uint32_t)timer->cmp; timer->period = (uint32_t)timer->period; @@ -623,8 +622,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr, case HPET_ID: return; case HPET_CFG: - val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK); - s->config = (s->config & 0xffffffff00000000ULL) | val; + new_val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK); + 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 = @@ -658,9 +657,9 @@ static void hpet_ram_write(void *opaque, hwaddr addr, trace_hpet_invalid_hpet_cfg(4); break; case HPET_STATUS: - val = new_val & s->isr; + cleared = new_val & s->isr; for (i = 0; i < s->num_timers; i++) { - if (val & (1 << i)) { + if (cleared & (1 << i)) { update_irq(&s->timer[i], 0); } } -- cgit 1.4.1 From c2366567378dd8fb89329816003801f54e30e6f3 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 10 Jul 2024 09:58:01 +0200 Subject: hpet: accept 64-bit reads and writes Declare the MemoryRegionOps so that 64-bit reads and writes to the HPET are received directly. This makes it possible to unify the code to process low and high parts: for 32-bit reads, extract the desired word; for 32-bit writes, just merge the desired part into the old value and proceed as with a 64-bit write. Signed-off-by: Paolo Bonzini --- hw/timer/hpet.c | 137 ++++++++++++++++---------------------------------- hw/timer/trace-events | 3 +- 2 files changed, 44 insertions(+), 96 deletions(-) (limited to 'hw/timer/hpet.c') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index 831e5a95b0..ac55dd1ebd 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -437,6 +437,7 @@ 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); @@ -451,52 +452,33 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr, return 0; } - switch ((addr - 0x100) % 0x20) { - case HPET_TN_CFG: - return timer->config; - case HPET_TN_CFG + 4: // Interrupt capabilities - return timer->config >> 32; + switch (addr & 0x18) { + case HPET_TN_CFG: // including interrupt capabilities + return timer->config >> shift; case HPET_TN_CMP: // comparator register - return timer->cmp; - case HPET_TN_CMP + 4: - return timer->cmp >> 32; + return timer->cmp >> shift; case HPET_TN_ROUTE: - return timer->fsb; - case HPET_TN_ROUTE + 4: - return timer->fsb >> 32; + return timer->fsb >> shift; default: trace_hpet_ram_read_invalid(); break; } } else { - switch (addr) { - case HPET_ID: - return s->capability; - case HPET_PERIOD: - return s->capability >> 32; + switch (addr & ~4) { + case HPET_ID: // including HPET_PERIOD + return s->capability >> shift; case HPET_CFG: - return s->config; - case HPET_CFG + 4: - trace_hpet_invalid_hpet_cfg(4); - return 0; + return s->config >> shift; 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(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; + trace_hpet_ram_read_reading_counter(addr & 4, cur_tick); + return cur_tick >> shift; case HPET_STATUS: - return s->isr; + return s->isr >> shift; default: trace_hpet_ram_read_invalid(); break; @@ -510,11 +492,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) { @@ -526,9 +508,12 @@ static void hpet_ram_write(void *opaque, hwaddr addr, trace_hpet_timer_id_out_of_range(timer_id); return; } - switch ((addr - 0x100) % 0x20) { + switch (addr & 0x18) { case HPET_TN_CFG: - trace_hpet_ram_write_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); if (deactivating_bit(old_val, new_val, HPET_TN_TYPE_LEVEL)) { /* * Do this before changing timer->config; otherwise, if @@ -536,8 +521,7 @@ 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; + timer->config = new_val; if (activating_bit(old_val, new_val, HPET_TN_ENABLE) && (s->isr & (1 << timer_id))) { update_irq(timer, 1); @@ -550,56 +534,28 @@ 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) { - 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); + /* High 32-bits are zero, leave them untouched. */ + if (shift) { + trace_hpet_ram_write_invalid_tn_cmp(); + break; } - 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); + len = 64; + value = (uint32_t) value; } - 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); + trace_hpet_ram_write_tn_cmp(addr & 4); if (!timer_is_periodic(timer) || (timer->config & HPET_TN_SETVAL)) { - timer->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32; + timer->cmp = deposit64(timer->cmp, shift, len, value); } if (timer_is_periodic(timer)) { /* * FIXME: Clamp period to reasonable min value? * Clamp period to reasonable max value */ - new_val = MIN(new_val, ~0u >> 1); - timer->period = - (timer->period & 0xffffffffULL) | new_val << 32; + 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)) { @@ -607,10 +563,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr, } break; case HPET_TN_ROUTE: - timer->fsb = (timer->fsb & 0xffffffff00000000ULL) | new_val; - break; - case HPET_TN_ROUTE + 4: - timer->fsb = (new_val << 32) | (timer->fsb & 0xffffffff); + timer->fsb = deposit64(timer->fsb, shift, len, value); break; default: trace_hpet_ram_write_invalid(); @@ -618,12 +571,14 @@ static void hpet_ram_write(void *opaque, hwaddr addr, } return; } else { - switch (addr) { + switch (addr & ~4) { 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 = (s->config & 0xffffffff00000000ULL) | new_val; + s->config = new_val; if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) { /* Enable main counter and interrupt generation. */ s->hpet_offset = @@ -653,10 +608,8 @@ 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)) { @@ -668,15 +621,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr, if (hpet_enabled(s)) { trace_hpet_ram_write_counter_write_while_enabled(); } - 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); + s->hpet_counter = deposit64(s->hpet_counter, shift, len, value); break; default: trace_hpet_ram_write_invalid(); @@ -690,7 +635,11 @@ static const MemoryRegionOps hpet_ram_ops = { .write = hpet_ram_write, .valid = { .min_access_size = 4, - .max_access_size = 4, + .max_access_size = 8, + }, + .impl = { + .min_access_size = 4, + .max_access_size = 8, }, .endianness = DEVICE_NATIVE_ENDIAN, }; diff --git a/hw/timer/trace-events b/hw/timer/trace-events index a5fafbc679..f48a712801 100644 --- a/hw/timer/trace-events +++ b/hw/timer/trace-events @@ -108,8 +108,7 @@ 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(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_cfg(uint8_t reg_off) "hpet_ram_writel HPET_TN_CFG + %" PRIu8 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" -- cgit 1.4.1 From 242d665396407f83a6acbffc804882eeb21cfdad Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 16 Jul 2024 12:26:36 +0200 Subject: hpet: store full 64-bit target value of the counter Store the full 64-bit value at which the timer should fire. This makes it possible to skip the imprecise hpet_calculate_diff() step, and to remove the clamping of the period to 31 or 63 bits. Signed-off-by: Paolo Bonzini --- hw/timer/hpet.c | 111 +++++++++++++++++++++++++------------------------------- 1 file changed, 49 insertions(+), 62 deletions(-) (limited to 'hw/timer/hpet.c') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index ac55dd1ebd..1654b7cb8b 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -54,6 +54,7 @@ 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. @@ -115,11 +116,6 @@ 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); } @@ -156,29 +152,34 @@ 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 diff between comparator value and current ticks + * calculate next value of the general counter that matches the + * target (either entirely, or the low 32-bit only depending on + * the timer mode). */ -static inline uint64_t hpet_calculate_diff(HPETTimer *t, uint64_t current) +static uint64_t hpet_calculate_cmp64(HPETTimer *t, uint64_t cur_tick, uint64_t target) { - if (t->config & HPET_TN_32BIT) { - 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; + uint64_t result = deposit64(cur_tick, 0, 32, target); + if (result < cur_tick) { + result += 0x100000000ULL; + } + return result; } else { - uint64_t diff, cmp; - - cmp = t->cmp; - diff = cmp - current; - diff = (int64_t)diff > 0 ? diff : (uint64_t)1; - return diff; + return target; } } +static uint64_t hpet_next_wrap(uint64_t cur_tick) +{ + return (cur_tick | 0xffffffffU) + 1; +} + static void update_irq(struct HPETTimer *timer, int set) { uint64_t mask; @@ -260,7 +261,12 @@ 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) @@ -356,14 +362,10 @@ static const VMStateDescription vmstate_hpet = { } }; -static void hpet_arm(HPETTimer *t, uint64_t ticks) +static void hpet_arm(HPETTimer *t, uint64_t 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); - } + /* FIXME: Clamp period to reasonable min value? */ + timer_mod(t->qemu_timer, hpet_get_ns(t->state, tick)); } /* @@ -372,54 +374,44 @@ static void hpet_arm(HPETTimer *t, uint64_t ticks) 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) { - while (hpet_time_after(cur_tick, t->cmp)) { - t->cmp = (uint32_t)(t->cmp + t->period); - } + t->cmp = (uint32_t)t->cmp64; } else { - 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; + t->cmp = t->cmp64; } + 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; - 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->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)) { t->wrap_flag = 1; + hpet_arm(t, hpet_next_wrap(cur_tick)); + return; } } - hpet_arm(t, diff); + hpet_arm(t, t->cmp64); } static void hpet_del_timer(HPETTimer *t) @@ -550,12 +542,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr, timer->cmp = deposit64(timer->cmp, shift, len, value); } 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); + timer->period = deposit64(timer->period, shift, len, value); } timer->config &= ~HPET_TN_SETVAL; if (hpet_enabled(s)) { -- cgit 1.4.1 From 7c912ffb59e8137091894d767433e65c3df8b0bf Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 16 Jul 2024 12:35:39 +0200 Subject: hpet: avoid timer storms on periodic timers If the period is set to a value that is too low, there could be no time left to run the rest of QEMU. Do not trigger interrupts faster than 1 MHz. Signed-off-by: Paolo Bonzini --- hw/timer/hpet.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 'hw/timer/hpet.c') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index 1654b7cb8b..471950adef 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -59,6 +59,7 @@ 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 { @@ -266,6 +267,7 @@ 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) { @@ -364,8 +366,15 @@ static const VMStateDescription vmstate_hpet = { static void hpet_arm(HPETTimer *t, uint64_t tick) { - /* FIXME: Clamp period to reasonable min value? */ - timer_mod(t->qemu_timer, hpet_get_ns(t->state, 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); } /* -- cgit 1.4.1