| Message ID | 53082983.4090000@ddn.com |
|---|---|
| State | New |
| Headers | show |
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>
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); }
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
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.
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
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
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
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
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);
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(-)