Thank you for reviewing.
The patch fixed by you can't solve my problem.
However, it is able to do this with minor modification.
I will send a new version patch.
________________________________
差出人: Paolo Bonzini <pbonz...@redhat.com>
送信日時: 2024年7月10日 19:01
宛先: TaiseiIto <taisei1...@outlook.jp>; qemu-devel@nongnu.org
<qemu-devel@nongnu.org>
CC: m...@redhat.com <m...@redhat.com>
件名: Re: [PATCH] hw/timer/hpet: Fix wrong HPET interrupts
Hello! Thanks for looking after the HPET, which is not a very well
maintained device.
I am not sure your patch needs to mask the comparator with
timer->cmp &= 0xffffffffULL;
I think that's a bug in the "case HPET_TN_CMP + 4" part of
hpet_ram_write. The logic was changed in "hpet: Fix emulation of
HPET_TN_SETVAL" but the commit forgot to apply it to the high bits of
the comparator.
The whole handling of writes to the comparator is very messy. I can
see two more bugs:
* Idon't think it's correct that HPET_TN_CMP+4 does not clear
HPET_TN_SETVAL
* the clamping should take into account that new_val is shifted by 32
Can you check that this also fixes the problem:
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 01efe4885db..11df272fe87 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -552,6 +552,10 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
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);
@@ -562,7 +566,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
if (!timer_is_periodic(timer)
|| (timer->config & HPET_TN_SETVAL)) {
timer->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
- } else {
+ }
+ if (timer_is_periodic(timer)) {
/*
* FIXME: Clamp period to reasonable min value?
* Clamp period to reasonable max value
@@ -562,20 +566,21 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
if (!timer_is_periodic(timer)
|| (timer->config & HPET_TN_SETVAL)) {
timer->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
- } else {
+ }
+ if (timer_is_periodic(timer)) {
/*
* FIXME: Clamp period to reasonable min value?
* Clamp period to reasonable max value
*/
- 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)) {
- hpet_set_timer(timer);
- }
- break;
+ }
+ timer->config &= ~HPET_TN_SETVAL;
+ if (hpet_enabled(s)) {
+ hpet_set_timer(timer);
+ }
+ break;
case HPET_TN_ROUTE:
timer->fsb = (timer->fsb & 0xffffffff00000000ULL) | new_val;
break;
@@ -605,7 +605,7 @@ 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) {
+ if (timer_enabled(timer)) {
hpet_set_timer(&s->timer[i]);
}
}
(the final part is taken from your patch)?
Thanks,
Paolo
On 6/18/24 15:10, TaiseiIto wrote:
> Before this commit, there are 3 problems about HPET timer interrupts. First,
> HPET periodic timers cause a too early interrupt before HPET main counter
> value reaches a value written its comparator value register. Second,
> disabled HPET timers whose comparator value register is not
> 0xffffffffffffffff cause wrong interrupts. Third, enabled HPET timers whose
> comparator value register is 0xffffffffffffffff don't cause any interrupts.
> About the first one, for example, an HPET driver writes 0x00000000aaaaaaaa
> to an HPET periodic timer comparator value register. As a result, the
> register becomes 0xffffffffaaaaaaaa because writing to the higher 32 bits
> of the register doesn't affect itself in periodic mode. (see
> "case HPET_TN_CMP + 4" of "hpet_ram_write" function.) And "timer->period"
> which means interrupt period in periodic mode becomes 0xaaaaaaaa. Next, the
> HPET driver sets the HPET_CFG_ENABLE flag to start the main counter. The
> comparator value register (0xffffffffaaaaaaaa) indicate the next interrupt
> time. The period (0xaaaaaaaa) is added to the comparator value register at
> "hpet_timer" function because "hpet_time_after64" function returns true when
> the main counter is small. So, the first interrupt is planned when the main
> counter is 0x0000000055555554, but the first interrupt should occur when the
> main counter is 0x00000000aaaaaaaa. To solve this problem, I fix the code to
> clear the higher 32 bits of comparator value registers of periodic mode
> timers when HPET starts the main counter. About the other two problems, it
> was decided by comparator value whether each timer is enabled, but it should
> be decided by "timer_enabled" function which confirm "HPET_TN_ENABLE" flag.
> To solve these problems, I fix the code to decide correctly whether each
> timer is enabled. After this commit, the 3 problems are solved. First, HPET
> periodic timers cause the first interrupt when the main counter value
> reaches a value written its comparator value register. Second, disabled HPET
> timers never cause any interrupt. Third, enabled HPET timers cause
> interrupts correctly even if an HPET driver writes 0xffffffff to its
> comparator value register.
>
> Signed-off-by: TaiseiIto <taisei1...@outlook.jp>
> ---
> hw/timer/hpet.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 01efe4885d..2dcefa7049 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -599,8 +599,12 @@ 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]);
> + HPETTimer *timer = &s->timer[i];
> + if (timer_enabled(timer)) {
> + if (timer_is_periodic(timer)) {
> + timer->cmp &= 0xffffffffULL;
> + }
> + hpet_set_timer(timer);
> }
> }
> } else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE))
> {
