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 <address@hidden>
> ---
> 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