What''s the meaning of "division by zero" in the change log of changeset 15943 : c0d1825f5189 (Don''t count "missed ticks" on one-shot timers.)? I found the c/s breaks Linux 2.6.23-rc4 when ACPI=1 in HVM config file. I don''t think the 2 lines below are correct for one_shot vpt: pt->enabled = 0; list_del(&pt->list); because i.e., it may drop one-shot local timer interrupt wrongly (this breaks ACPI Linux 2.6.23-rc4...): 1) an one-shot timer interrupt is triggered in pt_timer_fn(), then c/s 15943 sets pt->enabled to 0, and removes the vpt from the tm_list; 2) in vmx_intr_assit() - > pt_update_irq(), we can''t find the pt in the tm_list, so the timer interrupt is dropped... Actually we don''t need to remove an one_shot vpt from tm_list, since pt_update_irq() ignores a vpt if pt->pending_intr_nr == 0. Am I missing something? -- Dexuan> # Node ID c0d1825f51899b329495efb2078dd15e0fb3b479 > # Parent d17532dc1725e58b787329f64ce2e4e0e79516f0 > [HVM] Don''t count "missed ticks" on one-shot timers. > It''s not clear what it would mean, and it leads to division by zero. > Signed-off-by: Tim Deegan <Tim.Deegan@xensource.com > <mailto:Tim.Deegan@xensource.com> > --- > xen/arch/x86/hvm/vpt.c | 19 ++++++++++++++----- > 1 files changed, 14 insertions(+), 5 deletions(-) > > diff -r d17532dc1725 -r c0d1825f5189 xen/arch/x86/hvm/vpt.c > --- a/xen/arch/x86/hvm/vpt.c Sun Sep 23 12:56:11 2007 +0100 > +++ b/xen/arch/x86/hvm/vpt.c Mon Sep 24 13:44:29 2007 +0100 > @@ -46,6 +46,9 @@ static void missed_ticks(struct periodic > { > s_time_t missed_ticks; > > + if ( unlikely(pt->one_shot) ) > + return; > + > missed_ticks = NOW() - pt->scheduled; > if ( missed_ticks <= 0 ) > return; > @@ -111,12 +114,18 @@ static void pt_timer_fn(void *data) > pt_lock(pt); > > pt->pending_intr_nr++; > - pt->scheduled += pt->period; > - > - missed_ticks(pt); > - > - if ( !pt->one_shot ) > + > + if ( unlikely(pt->one_shot) ) > + { > + pt->enabled = 0; > + list_del(&pt->list); > + } > + else > + { > + pt->scheduled += pt->period; > + missed_ticks(pt); > set_timer(&pt->timer, pt->scheduled); > + } > > vcpu_kick(pt->vcpu); >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi, At 18:53 +0800 on 17 Oct (1192647209), Cui, Dexuan wrote:> What''s the meaning of "division by zero" in the change log of changeset > 15943 : c0d1825f5189 (Don''t count "missed ticks" on one-shot timers.)?An OS that set up a one-shot ACPI timer could cause the timer to fire with pt->period set to zero, which crashes Xen in the missed_ticks calculation. (vpt.c:56 missed_ticks = missed_ticks / (s_time_t) pt->period + 1;) Also, it''s surely wrong to calculate "missed" ticks on a non-repeating timer.> I found the c/s breaks Linux 2.6.23-rc4 when ACPI=1 in HVM config file. > > I don''t think the 2 lines below are correct for one_shot vpt: > pt->enabled = 0; > list_del(&pt->list); > because i.e., it may drop one-shot local timer interrupt wrongly (this > breaks ACPI Linux 2.6.23-rc4...): > 1) an one-shot timer interrupt is triggered in pt_timer_fn(), then > c/s 15943 > sets pt->enabled to 0, and removes the vpt from the tm_list; > 2) in vmx_intr_assit() - > pt_update_irq(), we can''t find the pt in > the tm_list, so the timer > interrupt is dropped...Ah, I see. Yes, those lines need to move to after the interrupt is delivered. :)> Actually we don''t need to remove an one_shot vpt from tm_list, since > pt_update_irq() ignores a vpt if pt->pending_intr_nr == 0.We should do it, though, because otherwise we''re just making pt_update_irq''s list walk more expensive for no benefit. Cheers, Tim. -- Tim Deegan <Tim.Deegan@xensource.com>, XenSource UK Limited Registered office c/o EC2Y 5EB, UK; company number 05334508 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yes, the newly-checked-in patch fixes the 2.6.23 issue. Thanks! -- Dexuan Tim Deegan wrote:> Hi, > > At 18:53 +0800 on 17 Oct (1192647209), Cui, Dexuan wrote: >> What''s the meaning of "division by zero" in the change log of >> changeset 15943 : c0d1825f5189 (Don''t count "missed ticks" on >> one-shot timers.)? > > An OS that set up a one-shot ACPI timer could cause the timer to fire > with pt->period set to zero, which crashes Xen in the missed_ticks > calculation. (vpt.c:56 missed_ticks = missed_ticks / (s_time_t) > pt->period + 1;) Also, it''s surely wrong to calculate "missed" ticks > on a non-repeating timer. > >> I found the c/s breaks Linux 2.6.23-rc4 when ACPI=1 in HVM config >> file. >> >> I don''t think the 2 lines below are correct for one_shot vpt: >> pt->enabled = 0; list_del(&pt->list); >> because i.e., it may drop one-shot local timer interrupt wrongly >> (this breaks ACPI Linux 2.6.23-rc4...): >> 1) an one-shot timer interrupt is triggered in pt_timer_fn(), then >> c/s 15943 >> sets pt->enabled to 0, and removes the vpt from the tm_list; >> 2) in vmx_intr_assit() - > pt_update_irq(), we can''t find the pt in >> the tm_list, so the timer interrupt is dropped... > > Ah, I see. Yes, those lines need to move to after the interrupt is > delivered. :) > >> Actually we don''t need to remove an one_shot vpt from tm_list, since >> pt_update_irq() ignores a vpt if pt->pending_intr_nr == 0. > > We should do it, though, because otherwise we''re just making > pt_update_irq''s list walk more expensive for no benefit. > > Cheers, > > Tim._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel