hw/timer/hpet.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 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
"case HPET_TN_CMP + 4" of "hpet_ram_write" function to ensure that writings
to higher 32 bits of a comparator value register reflect itself even if in
periodic mode. 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 0xffffffffffffffff to its comparator value
register.
Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
---
Changes in v2:
- Reflect writings to higher 32 bits of a comparator value register rather
than clearing these bits.
- Fix wrong indents.
- Link to v1: https://lore.kernel.org/qemu-devel/TY0PR0101MB4285838139BC56DEC3D1CCFDA4CE2@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
hw/timer/hpet.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 01efe4885d..4b6352e257 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,20 +566,22 @@ 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->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
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;
@@ -599,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 (hpet_enabled(s)) {
hpet_set_timer(&s->timer[i]);
}
}
--
2.34.1On 7/13/24 13:54, 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
> "case HPET_TN_CMP + 4" of "hpet_ram_write" function to ensure that writings
> to higher 32 bits of a comparator value register reflect itself even if in
> periodic mode. 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 0xffffffffffffffff to its comparator value
> register.
>
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
>
> Changes in v2:
> - Reflect writings to higher 32 bits of a comparator value register rather
> than clearing these bits.
> - Fix wrong indents.
> - Link to v1: https://lore.kernel.org/qemu-devel/TY0PR0101MB4285838139BC56DEC3D1CCFDA4CE2@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
>
> hw/timer/hpet.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 01efe4885d..4b6352e257 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,20 +566,22 @@ 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->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
This seems wrong to me. The comparator must be reset using
HPET_TN_SETVAL, otherwise it just keeps running. From the specification:
"If software wants to change the periodic rate, it should write a new
value to the comparator value register. At the point when the timer’s
comparator indicates a match, this new value will be added to derive the
next matching point. So as to avoid race conditions where the new value
is written just as a match occurs, either the main counter should be
halted or the comparator disabled when the new periodic rate is written".
The sentence "at the point when the timer’s comparator indicates a
match" indicates that the comparator register is not written.
I suspect you're hitting the other issue, and HPET_TN_SETVAL is cleared
incorrectly by a 64-bit write. I'll try sending out a patch to fix that.
> - if ((&s->timer[i])->cmp != ~0ULL) {
> + if (hpet_enabled(s)) {
> hpet_set_timer(&s->timer[i]);
> }
> }
This is incorrect too. "hpet_enabled(s)" is always true here; I think
the bug is that timer_enabled() should only affect generation of
interrupts. Are you perhaps using level-triggered interrupts? If so,
it makes sense that you want the timer to be running - and then when
setting HPET_TN_ENABLE the interrupt will fire.
However, we're making progress; you're just finding more bugs.
I have some ideas on what needs to be fixed next, and I placed them at a
branch "hpet" of https://gitlab.com/bonzini/qemu.git. Unfortunately, I
don't have a way to test them (writing testcases is on my list but I
don't have time right now) and as the branch proceeds things get more
experimental; but let me know if it helps or if you can make small
changes to the patches that fix your testcase.
Paolo
Thank you for preparing the hpet branch. I tried to use the branch. Your
modification was not enough to solve my problem but I confirmed that the
problem is completely solved by adding the following 2 patches on your hpet
branch.
https://lore.kernel.org/qemu-devel/TY0PR0101MB42850337F8917D1F514107FBA4D52@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
https://lore.kernel.org/qemu-devel/TY0PR0101MB4285D5A3587179A5788F3356A4AE2@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
Taisei
________________________________
差出人: Paolo Bonzini <pbonzini@redhat.com>
送信日時: 2024年7月16日 19:51
宛先: TaiseiIto <taisei1212@outlook.jp>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>
CC: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>
件名: Re: [PATCH v2] hw/timer/hpet: Fix wrong HPET interrupts
On 7/13/24 13:54, 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
> "case HPET_TN_CMP + 4" of "hpet_ram_write" function to ensure that writings
> to higher 32 bits of a comparator value register reflect itself even if in
> periodic mode. 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 0xffffffffffffffff to its comparator value
> register.
>
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
>
> Changes in v2:
> - Reflect writings to higher 32 bits of a comparator value register rather
> than clearing these bits.
> - Fix wrong indents.
> - Link to v1: https://lore.kernel.org/qemu-devel/TY0PR0101MB4285838139BC56DEC3D1CCFDA4CE2@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
>
> hw/timer/hpet.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 01efe4885d..4b6352e257 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,20 +566,22 @@ 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->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
This seems wrong to me. The comparator must be reset using
HPET_TN_SETVAL, otherwise it just keeps running. From the specification:
"If software wants to change the periodic rate, it should write a new
value to the comparator value register. At the point when the timer’s
comparator indicates a match, this new value will be added to derive the
next matching point. So as to avoid race conditions where the new value
is written just as a match occurs, either the main counter should be
halted or the comparator disabled when the new periodic rate is written".
The sentence "at the point when the timer’s comparator indicates a
match" indicates that the comparator register is not written.
I suspect you're hitting the other issue, and HPET_TN_SETVAL is cleared
incorrectly by a 64-bit write. I'll try sending out a patch to fix that.
> - if ((&s->timer[i])->cmp != ~0ULL) {
> + if (hpet_enabled(s)) {
> hpet_set_timer(&s->timer[i]);
> }
> }
This is incorrect too. "hpet_enabled(s)" is always true here; I think
the bug is that timer_enabled() should only affect generation of
interrupts. Are you perhaps using level-triggered interrupts? If so,
it makes sense that you want the timer to be running - and then when
setting HPET_TN_ENABLE the interrupt will fire.
However, we're making progress; you're just finding more bugs.
I have some ideas on what needs to be fixed next, and I placed them at a
branch "hpet" of https://gitlab.com/bonzini/qemu.git. Unfortunately, I
don't have a way to test them (writing testcases is on my list but I
don't have time right now) and as the branch proceeds things get more
experimental; but let me know if it helps or if you can make small
changes to the patches that fix your testcase.
Paolo
I disassemble my HPET driver and confirm it does once 64bit writing to the
comparator register. However, QEMU does twice 32bit writings to the
comparator register. As a result, writing to the higher 32bit is rejected
because HPET_TN_SETVAL is cleared when lower 32bit writing. I think this
64bit writing should not be divided into twice 32bit writings. Anyway, I
will continue investigation about this bug. Also, please check the following
patch which affects HPET interruptions.
https://lore.kernel.org/qemu-devel/TY0PR0101MB42850337F8917D1F514107FBA4D52@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
Taisei
________________________________
差出人: Paolo Bonzini <pbonzini@redhat.com>
送信日時: 2024年7月16日 19:51
宛先: TaiseiIto <taisei1212@outlook.jp>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>
CC: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>
件名: Re: [PATCH v2] hw/timer/hpet: Fix wrong HPET interrupts
On 7/13/24 13:54, 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
> "case HPET_TN_CMP + 4" of "hpet_ram_write" function to ensure that writings
> to higher 32 bits of a comparator value register reflect itself even if in
> periodic mode. 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 0xffffffffffffffff to its comparator value
> register.
>
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
>
> Changes in v2:
> - Reflect writings to higher 32 bits of a comparator value register rather
> than clearing these bits.
> - Fix wrong indents.
> - Link to v1: https://lore.kernel.org/qemu-devel/TY0PR0101MB4285838139BC56DEC3D1CCFDA4CE2@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
>
> hw/timer/hpet.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 01efe4885d..4b6352e257 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,20 +566,22 @@ 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->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
This seems wrong to me. The comparator must be reset using
HPET_TN_SETVAL, otherwise it just keeps running. From the specification:
"If software wants to change the periodic rate, it should write a new
value to the comparator value register. At the point when the timer’s
comparator indicates a match, this new value will be added to derive the
next matching point. So as to avoid race conditions where the new value
is written just as a match occurs, either the main counter should be
halted or the comparator disabled when the new periodic rate is written".
The sentence "at the point when the timer’s comparator indicates a
match" indicates that the comparator register is not written.
I suspect you're hitting the other issue, and HPET_TN_SETVAL is cleared
incorrectly by a 64-bit write. I'll try sending out a patch to fix that.
> - if ((&s->timer[i])->cmp != ~0ULL) {
> + if (hpet_enabled(s)) {
> hpet_set_timer(&s->timer[i]);
> }
> }
This is incorrect too. "hpet_enabled(s)" is always true here; I think
the bug is that timer_enabled() should only affect generation of
interrupts. Are you perhaps using level-triggered interrupts? If so,
it makes sense that you want the timer to be running - and then when
setting HPET_TN_ENABLE the interrupt will fire.
However, we're making progress; you're just finding more bugs.
I have some ideas on what needs to be fixed next, and I placed them at a
branch "hpet" of https://gitlab.com/bonzini/qemu.git. Unfortunately, I
don't have a way to test them (writing testcases is on my list but I
don't have time right now) and as the branch proceeds things get more
experimental; but let me know if it helps or if you can make small
changes to the patches that fix your testcase.
Paolo
© 2016 - 2024 Red Hat, Inc.