RE: [Xen-devel] [PATCH]Fixes for overflowed calculation in vHPET

["Shan, Haitao" <haitao.shan@xxxxxxxxx>]

Yes. That's why I say normally it is OK.

In fact, this change is in close relation with another patch I sent (you can see the attached). Correct behavior of HPET should be that maincounter and timer are all enabled when HPET is globally enabled. And the timer period following a reset is 0xffff_ffff_ffff_ffff. If guest just enables HPET to use maincounter, that large value will be used to set timer to update the status. At that time, the period will be forced to 0.

Current vHPET uses per timer interrupt control bit as per timer enable control bit. And timer interrupts are disable by default. So, luckily the above scheme won't happen in current implementation, since that large value won't be used to set timer.

 

As long as no one uses HPET like that, I think there is no problem and the patch can be ignored. The question is whether we should make device model strictly following the specifications, given that current vHPET does not.

Best Regards
Haitao Shan

 

---

From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Keir Fraser
Sent: 2008年1月9日 16:37
To: Shan, Haitao; xen-devel@xxxxxxxxxxxxxxxxxxx
Cc: Mark McLoughlin; Cui, Dexuan
Subject: Re: [Xen-devel] [PATCH]Fixes for overflowed calculation in vHPET

It sounds like a theoretical problem to me. You’d have 
to set the period, or single-shot timeout, to many years to have it wrap around 
in the 64th bit and appear negative. Noone will do that.

 -- 
Keir

On 9/1/08 01:19, "Shan, Haitao" <haitao.shan@xxxxxxxxx> 
wrote:

I think it is OK for normal usage and for 32bit timer operation.
But if a timer is programmed at 64bit mode, and the period programmed is sufficiently large, say 0xf000_0000_0000_0000, the code introduces trouble. Actually the timer should never be fired. However, (int64_s)0xf000_0000_0000_0000 < 0, then the period is forced to 0 and the timer is fired immediately.
Best Regards
Haitao Shan

 

---

From: Keir Fraser [mailto:Keir.Fraser@xxxxxxxxxxxx]
Sent: 2008年1月8日 22:15
To: Shan, Haitao; xen-devel@xxxxxxxxxxxxxxxxxxx
Cc: Mark McLoughlin; Cui, Dexuan
Subject: Re: [Xen-devel] [PATCH]Fixes for overflowed calculation in vHPET

On 4/1/08 03:21, "Shan, Haitao" <haitao.shan@xxxxxxxxx> wrote:

This patch will  fix the bugs in hpet_set_timer. Currently in hpet_tick_to_ns, the approach is  multiplying first, which easily causes overflow when tick is quite large. The  patch cannot handle arbitrate large ticks duo to the precision requirement and  64bit's value range. But by optimize the equation, a larger ticks than current  code can be supported. Also an overflow check is added before the calculation.  
This patch will also fix the wrong handling of wrap around case when timer  is in 64bit mode.  

What’s wrong with the handling of the wrap-around case? It looks okay to me.

 -- Keir

--- Begin Message ---

• To: <xen-devel@xxxxxxxxxxxxxxxxxxx>
• From: "Shan, Haitao" <haitao.shan@xxxxxxxxx>
• Date: Fri, 4 Jan 2008 11:21:06 +0800
• Cc: "Mark McLoughlin" <markmc@xxxxxxxxxx>, "Cui, Dexuan" <dexuan.cui@xxxxxxxxx>
• Thread-index: AchIXkjedwVeiEy4RPSClG5JXO2lGgGIZGWw
• Thread-topic: [PATCH]Fixes for wrong emulated behavior of HPET

This patch will fix the wrong emulated behavior in current vHPET's code.
The enable/disable per timer interrupt bit is wrongly used as per timer enable/disable. According to spec, comparator value should constantly increasing when HPET is globally enabled , no matter whether the timer interrupt is enabled or not.  This little patch will fix that.

Best Regards
Haitao Shan

Attachment: hpet1.patch
Description: hpet1.patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

--- End Message ---

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel