Re: [Xen-devel] [PATCH v3 11/11] hvm/hpet: handle 1st period special

["Slutz, Donald Christopher" <dslutz@xxxxxxxxxxx>]

On 04/25/14 08:32, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43, <dslutz@xxxxxxxxxxx> wrote:
>> v3:
>>    Better commit message.
>>    More setting of first_mc64 & first_enabled when needed.
>>    Switch to bool_t.
>>
>>   xen/arch/x86/hvm/hpet.c       | 83 
>> ++++++++++++++++++++++++++++++++++++-------
>>   xen/include/asm-x86/hvm/vpt.h |  2 ++
>>   2 files changed, 73 insertions(+), 12 deletions(-)
> I still can't help myself thinking that this must be achievable with less
> special casing - I just can't imagine that hardware also has this huge
> an amount of special case logic just for the first period.

The hardware is simple. And the code to simulate it:

do_count()
{
     mc++;
     if ( mc == comparator )
         handle_int();
         if ( periodic )
             comparator += period;
    }
}

is also simple.  However this way has a very high cost in that you have
to call do_count at the frequency of the master clock.  Instead of the
simple simulation, the current code uses arithmetic to compute the
current state of mc and comparator.  This is where the complexity comes
in.  During answering of comments on #8 (signed divide) and #10
(prevent mc equal), I noticed that what I coded here only works for
expected case where mc is close to zero and comparator starts
out > mc in unsigned.  I will be sending a v4 with this issue fixed.
New code (untested):

+        if ( h->hpet.first_enabled[tn] )
+        {
+            int64_t mc_diff = mc - h->hpet.first_mc64[tn];
+
+            if ( mc_diff >= 0 && mc_diff < comparator - h->hpet.first_mc64[tn] 
)


was:

+            if ( mc >= h->hpet.first_mc64[tn] && mc < comparator )


>> @@ -208,12 +216,18 @@ static void hpet_stop_timer(HPETState *h, unsigned int 
>> tn,
>>    * 1/(2^10) second, namely, 0.9765625 milliseconds */
>>   #define  HPET_TINY_TIME_SPAN  ((h->stime_freq >> 10) / STIME_PER_HPET_TICK)
>>   
>> +static void hpet_time_fired(struct vcpu *v, void *priv)
>> +{
>> +    bool_t *first_enabled_p = (bool_t *)priv;
> Pointless cast.

Will drop.

    -Don Slutz

> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel