[Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Matt Lupfer]

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

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Paolo Bonzini]

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>

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[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

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Alex Bligh]

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

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Matt Lupfer]

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

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Don't enable a HPET timer if HPET is disabled

[Michael S. Tsirkin]

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