Re: [Xen-devel] [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator.

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

On 04/23/14 11:45, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43, <dslutz@xxxxxxxxxxx> wrote:
>> This is done so that when comparator is less then or equal to one
>> period it does not change.
>>
>> The code lines:
>>
>>      elapsed = hpet_read_maincounter(h, guest_time) +
>>          period - 1 - comparator;
>>      comparator += (elapsed / period) * period;
>>
>> are what matter here.  For almost all cases where
>> "hpet_read_maincounter() + period - 1" is greater then
>> "comparator", a signed divide will produce the same answer as an
>> unsigned divide.  One of the problem areas is when
>> "hpet_read_maincounter() + period - 1" needs more then 64 bits to
>> represent it.  It includes cases where hpet_read_maincounter() has
> "It includes" implies there are other cases, yet I can't see any.
> (Also twice s/then/than/ I think.)

The boundary cases of 2's complement show up here (8 bits is just
nice small numbers: -128 (signed) vs 128 (unsigned)).
Ok.

>> wrapped (I.E. needs more then 64 bits to correctly represent it),
>> but "comparator" has not wrapped.
> But can this happen in practice? Main counter and comparator
> shouldn't be very far apart, and hence other than
> "hpet_read_maincounter() + period - 1",
> "hpet_read_maincounter() + period - 1 - comparator" shouldn't
> ever require more than 64 bits to correctly represent it. All you
> really need to deal with is the case where
> hpet_read_maincounter() + period == comparator, i.e. the 1
> being subtracted makes the expression degenerate. So you could
> simply make the computation and update dependent upon
> hpet_read_maincounter() + period > comparator.
> And then again I don't really see why the subtraction of 1 is needed
> here in the first place.

As you saw in patch #10, the -1 is an issue.

Doing patch #10 1st, removes the normal Linux case.

After more thinking on it, the last patch (#11) does make this patch 
(#8) not needed.  The case
that is overlapping is:

If the 1st period's cmp is > 1 period by less then 2 periods (using 
numbers from #8 adjusted):

hpet_read_maincounter(h, guest_time) = 67752
period = 62500
comparator = 130253 == 67752 + 62500 + 1

what               unsigned               signed

comparator       : 130253                  130253
elapsed          : 18446744073709551615    -1
elapsed/period   : 295147905179352         0
comparator_delta : 18446744073709500000    0
new comparator   : 78637                   130253


and

hpet_read_maincounter(h, guest_time) = 67752
period = 62500
comparator = 161502 == 67752 + 62500 + 31250

what               unsigned               signed

comparator       : 161502                 161502
elapsed          : 18446744073709520366   -31250
elapsed/period   : 295147905179352        0
comparator_delta : 18446744073709500000   0
new comparator   : 109886                 161502

It does the right thing, but does not handle the general case.


But since the last patch also handles this case, I will be dropping this 
patch in v4.

     -Don Slutz
>
> Jan
>

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