hw/timer/hpet.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
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 <taisei1212@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)) {
--
2.34.1Hello! 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 <taisei1212@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)) {
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 <pbonzini@redhat.com>
送信日時: 2024年7月10日 19:01
宛先: TaiseiIto <taisei1212@outlook.jp>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>
CC: mst@redhat.com <mst@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 <taisei1212@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)) {
On Tue, Jun 18, 2024 at 01:10:44PM +0000, 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 <taisei1212@outlook.jp>
I didn't get this patch previously. Donnu why.
Tagged now.
> ---
> 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)) {
> --
> 2.34.1
>
>
TaiseiIto <taisei1212@outlook.jp> writes:
(widen CC list to include machine pc maintainers which use HPET)
> 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 <taisei1212@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)) {
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
This is a ping for the patch below.
https://lore.kernel.org/qemu-devel/TY0PR0101MB4285838139BC56DEC3D1CCFDA4CE2@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
________________________________
差出人: TaiseiIto <taisei1212@outlook.jp>
送信日時: 2024年6月18日 22:10
宛先: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
CC: mst@redhat.com <mst@redhat.com>; pbonzini@redhat.com <pbonzini@redhat.com>; TaiseiIto <taisei1212@outlook.jp>
件名: [PATCH] hw/timer/hpet: Fix wrong HPET interrupts
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 <taisei1212@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)) {
--
2.34.1
© 2016 - 2024 Red Hat, Inc.