* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-02-22 4:37 [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled Matt Lupfer
@ 2014-02-22 9:01 ` Paolo Bonzini
2014-03-26 20:20 ` Matt Lupfer
2014-02-22 9:03 ` Alex Bligh
2014-03-27 12:19 ` Michael S. Tsirkin
2 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2014-02-22 9:01 UTC (permalink / raw)
To: Matt Lupfer, QEMU Developers; +Cc: Alex Bligh
Il 22/02/2014 05:37, Matt Lupfer ha scritto:
> A HPET timer can be started when HPET is not yet
> enabled. This will not generate an interrupt
> to the guest, but causes problems when HPET is later
> enabled.
>
> A timer that is created and expires at least once before
> HPET is enabled will have an initialized comparator based
> on a hpet_offset of 0 (uninitialized). When HPET is
> enabled, hpet_set_timer() is called a second time, which
> modifies the timer expiry to a time based on the
> difference between current ticks (measured with the
> newly initialized hpet_offset) and the timer's
> comparator (which was generated before hpet_offset was
> initialized). This results in a long period of no HPET
> timer ticks.
>
> When this occurs with a CentOS 5.x guest, the guest
> may not receive timer interrupts during its narrow
> timer check window and panic on boot.
>
> Signed-off-by: Matt Lupfer <mlupfer@ddn.com>
> ---
> hw/timer/hpet.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 1264dfd..e15d6bc 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -506,7 +506,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
> timer->cmp = (uint32_t)timer->cmp;
> timer->period = (uint32_t)timer->period;
> }
> - if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
> + 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);
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-02-22 9:01 ` Paolo Bonzini
@ 2014-03-26 20:20 ` Matt Lupfer
2014-03-27 12:17 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Matt Lupfer @ 2014-03-26 20:20 UTC (permalink / raw)
To: Paolo Bonzini, QEMU Developers; +Cc: Alex Bligh
On 02/22/2014 02:01 AM, Paolo Bonzini wrote:
> Il 22/02/2014 05:37, Matt Lupfer ha scritto:
>> A HPET timer can be started when HPET is not yet
>> enabled. This will not generate an interrupt
>> to the guest, but causes problems when HPET is later
>> enabled.
>>
>> A timer that is created and expires at least once before
>> HPET is enabled will have an initialized comparator based
>> on a hpet_offset of 0 (uninitialized). When HPET is
>> enabled, hpet_set_timer() is called a second time, which
>> modifies the timer expiry to a time based on the
>> difference between current ticks (measured with the
>> newly initialized hpet_offset) and the timer's
>> comparator (which was generated before hpet_offset was
>> initialized). This results in a long period of no HPET
>> timer ticks.
>>
>> When this occurs with a CentOS 5.x guest, the guest
>> may not receive timer interrupts during its narrow
>> timer check window and panic on boot.
>>
>> Signed-off-by: Matt Lupfer <mlupfer@ddn.com>
>> ---
>> hw/timer/hpet.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>> index 1264dfd..e15d6bc 100644
>> --- a/hw/timer/hpet.c
>> +++ b/hw/timer/hpet.c
>> @@ -506,7 +506,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>> timer->cmp = (uint32_t)timer->cmp;
>> timer->period = (uint32_t)timer->period;
>> }
>> - if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
>> + 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);
>>
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Ping? Now that 1.7.1 is out, I hope this small patch will be considered
for the 2.0 release.
http://patchwork.ozlabs.org/patch/323121/
Matt
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-03-26 20:20 ` Matt Lupfer
@ 2014-03-27 12:17 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2014-03-27 12:17 UTC (permalink / raw)
To: Matt Lupfer, QEMU Developers; +Cc: Alex Bligh, Michael S. Tsirkin
Il 26/03/2014 21:20, Matt Lupfer ha scritto:
> On 02/22/2014 02:01 AM, Paolo Bonzini wrote:
>> Il 22/02/2014 05:37, Matt Lupfer ha scritto:
>>> A HPET timer can be started when HPET is not yet
>>> enabled. This will not generate an interrupt
>>> to the guest, but causes problems when HPET is later
>>> enabled.
>>>
>>> A timer that is created and expires at least once before
>>> HPET is enabled will have an initialized comparator based
>>> on a hpet_offset of 0 (uninitialized). When HPET is
>>> enabled, hpet_set_timer() is called a second time, which
>>> modifies the timer expiry to a time based on the
>>> difference between current ticks (measured with the
>>> newly initialized hpet_offset) and the timer's
>>> comparator (which was generated before hpet_offset was
>>> initialized). This results in a long period of no HPET
>>> timer ticks.
>>>
>>> When this occurs with a CentOS 5.x guest, the guest
>>> may not receive timer interrupts during its narrow
>>> timer check window and panic on boot.
>>>
>>> Signed-off-by: Matt Lupfer <mlupfer@ddn.com>
>>> ---
>>> hw/timer/hpet.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>>> index 1264dfd..e15d6bc 100644
>>> --- a/hw/timer/hpet.c
>>> +++ b/hw/timer/hpet.c
>>> @@ -506,7 +506,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>>> timer->cmp = (uint32_t)timer->cmp;
>>> timer->period = (uint32_t)timer->period;
>>> }
>>> - if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
>>> + 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);
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Ping? Now that 1.7.1 is out, I hope this small patch will be considered
> for the 2.0 release.
>
> http://patchwork.ozlabs.org/patch/323121/
Oops. Michael, can you take care of this one?
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-02-22 4:37 [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled Matt Lupfer
2014-02-22 9:01 ` Paolo Bonzini
@ 2014-02-22 9:03 ` Alex Bligh
2014-02-22 10:55 ` Paolo Bonzini
2014-03-27 12:19 ` Michael S. Tsirkin
2 siblings, 1 reply; 9+ messages in thread
From: Alex Bligh @ 2014-02-22 9:03 UTC (permalink / raw)
To: Matt Lupfer; +Cc: pbonzini, QEMU Developers, Alex Bligh
On 22 Feb 2014, at 04:37, Matt Lupfer wrote:
> A HPET timer can be started when HPET is not yet
> enabled. This will not generate an interrupt
> to the guest, but causes problems when HPET is later
> enabled.
>
> A timer that is created and expires at least once before
> HPET is enabled will have an initialized comparator based
> on a hpet_offset of 0 (uninitialized). When HPET is
> enabled, hpet_set_timer() is called a second time, which
> modifies the timer expiry to a time based on the
> difference between current ticks (measured with the
> newly initialized hpet_offset) and the timer's
> comparator (which was generated before hpet_offset was
> initialized). This results in a long period of no HPET
> timer ticks.
>
> When this occurs with a CentOS 5.x guest, the guest
> may not receive timer interrupts during its narrow
> timer check window and panic on boot.
>
> Signed-off-by: Matt Lupfer <mlupfer@ddn.com>
> ---
> hw/timer/hpet.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 1264dfd..e15d6bc 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -506,7 +506,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
> timer->cmp = (uint32_t)timer->cmp;
> timer->period = (uint32_t)timer->period;
> }
> - if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
> + 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);
> --
> 1.8.5.3
>
>
Thanks
I am unfamiliar with the HPET code but symmetry suggests perhaps the 'else'
condition should be changed too:
} else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE) ||
!hpet_enabled(s)) {
hpet_del_timer(timer);
}
--
Alex Bligh
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-02-22 9:03 ` Alex Bligh
@ 2014-02-22 10:55 ` Paolo Bonzini
2014-02-22 12:25 ` Alex Bligh
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2014-02-22 10:55 UTC (permalink / raw)
To: Alex Bligh, Matt Lupfer; +Cc: QEMU Developers
Il 22/02/2014 10:03, Alex Bligh ha scritto:
> I am unfamiliar with the HPET code but symmetry suggests perhaps the 'else'
> condition should be changed too:
>
> } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE) ||
> !hpet_enabled(s)) {
> hpet_del_timer(timer);
> }
>
> --
I thought the same, but there is no need for that. When the enabled bit goes
from set to clear, all timers are disabled:
} else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
/* Halt main counter and disable interrupt generation. */
s->hpet_counter = hpet_get_ticks(s);
for (i = 0; i < s->num_timers; i++) {
hpet_del_timer(&s->timer[i]);
}
}
So hpet_del_timer would be a nop if !hpet_enabled(s).
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-02-22 10:55 ` Paolo Bonzini
@ 2014-02-22 12:25 ` Alex Bligh
2014-02-22 14:39 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Alex Bligh @ 2014-02-22 12:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: QEMU Developers, Alex Bligh, Matt Lupfer
Paolo,
On 22 Feb 2014, at 10:55, Paolo Bonzini wrote:
> Il 22/02/2014 10:03, Alex Bligh ha scritto:
>> I am unfamiliar with the HPET code but symmetry suggests perhaps the 'else'
>> condition should be changed too:
>>
>> } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE) ||
>> !hpet_enabled(s)) {
>> hpet_del_timer(timer);
>> }
>>
>> --
>
> I thought the same, but there is no need for that. When the enabled bit goes
> from set to clear, all timers are disabled:
>
> } else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
> /* Halt main counter and disable interrupt generation. */
> s->hpet_counter = hpet_get_ticks(s);
> for (i = 0; i < s->num_timers; i++) {
> hpet_del_timer(&s->timer[i]);
> }
> }
>
> So hpet_del_timer would be a nop if !hpet_enabled(s).
Thanks. I didn't know whether HPET_CFG_ENABLE and HPET_TN_ENABLE were equivalent.
--
Alex Bligh
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-02-22 12:25 ` Alex Bligh
@ 2014-02-22 14:39 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2014-02-22 14:39 UTC (permalink / raw)
To: Alex Bligh; +Cc: QEMU Developers, Matt Lupfer
Il 22/02/2014 13:25, Alex Bligh ha scritto:
> Paolo,
>
> On 22 Feb 2014, at 10:55, Paolo Bonzini wrote:
>
>> Il 22/02/2014 10:03, Alex Bligh ha scritto:
>>> I am unfamiliar with the HPET code but symmetry suggests perhaps the 'else'
>>> condition should be changed too:
>>>
>>> } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE) ||
>>> !hpet_enabled(s)) {
>>> hpet_del_timer(timer);
>>> }
>>>
>>> --
>>
>> I thought the same, but there is no need for that. When the enabled bit goes
>> from set to clear, all timers are disabled:
>>
>> } else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
>> /* Halt main counter and disable interrupt generation. */
>> s->hpet_counter = hpet_get_ticks(s);
>> for (i = 0; i < s->num_timers; i++) {
>> hpet_del_timer(&s->timer[i]);
>> }
>> }
>>
>> So hpet_del_timer would be a nop if !hpet_enabled(s).
>
> Thanks. I didn't know whether HPET_CFG_ENABLE and HPET_TN_ENABLE were equivalent.
No, they are not, but HPET_CFG_ENABLE is equivalent to hpet_enabled.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled
2014-02-22 4:37 [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled Matt Lupfer
2014-02-22 9:01 ` Paolo Bonzini
2014-02-22 9:03 ` Alex Bligh
@ 2014-03-27 12:19 ` Michael S. Tsirkin
2 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-03-27 12:19 UTC (permalink / raw)
To: Matt Lupfer; +Cc: pbonzini, QEMU Developers, Alex Bligh
On Fri, Feb 21, 2014 at 09:37:23PM -0700, Matt Lupfer wrote:
> A HPET timer can be started when HPET is not yet
> enabled. This will not generate an interrupt
> to the guest, but causes problems when HPET is later
> enabled.
>
> A timer that is created and expires at least once before
> HPET is enabled will have an initialized comparator based
> on a hpet_offset of 0 (uninitialized). When HPET is
> enabled, hpet_set_timer() is called a second time, which
> modifies the timer expiry to a time based on the
> difference between current ticks (measured with the
> newly initialized hpet_offset) and the timer's
> comparator (which was generated before hpet_offset was
> initialized). This results in a long period of no HPET
> timer ticks.
>
> When this occurs with a CentOS 5.x guest, the guest
> may not receive timer interrupts during its narrow
> timer check window and panic on boot.
>
> Signed-off-by: Matt Lupfer <mlupfer@ddn.com>
Queued for 2.0, thanks everyone.
> ---
> hw/timer/hpet.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 1264dfd..e15d6bc 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -506,7 +506,8 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
> timer->cmp = (uint32_t)timer->cmp;
> timer->period = (uint32_t)timer->period;
> }
> - if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
> + 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);
> --
> 1.8.5.3
^ permalink raw reply [flat|nested] 9+ messages in thread