Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Don Slutz <dslutz@verizon.com>]

On 04/15/14 12:14, Jan Beulich wrote:
>>>> On 15.04.14 at 17:53, <dslutz@verizon.com> wrote:
>> On 04/15/14 03:05, Jan Beulich wrote:
>>>>>> On 14.04.14 at 21:50, <dslutz@verizon.com> wrote:
>>>> On 04/14/14 11:07, Jan Beulich wrote:
>>>>> As to the change - I'm not sure: The quoted description from the
>>>>> specification cal also be read to mean that interrupt generation is
>>>>> optional, but comparator increment will always happen. As long as
>>>>> this can't be clarified, I'd prefer to stay with the code as is.
>>>> I think the code needs to change to match the spec.
>>>>
>>>> #define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE)
>>>>
>>>> vs
>>>>
>>>> #define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE)
>>>>
>>>>
>>>> The change uses hpet_enabled() (I.E. Overall Enable).
>>> Correct. But do we really need this? When the HPET is globally
>>> disabled, hpet_read_maincounter() returns a fixed value, and
>>> hence - due to the comparators not changing either -
>>> hpet_get_comparator() will too even without the addition. So if
>>> at all, the change would be mostly for documentation purposes.
>> We do need this.  hpet_get_comparator() does not return a
>> fixed value.  Without this change it will always adjust to
>> hpet_read_maincounter().
> But as said, hpet_read_maincounter() returns a fixed value in that
> case too (or at least it is supposed to be).
>
> And no, I'm not fully trusting the test program, so please explain
> things relative to the source code rather than by pointing at the
> test program showing something.

Ok,

Since this is all about when master clock is not enabled, I will
work with hpet_read_maincounter() returning a fix value. I will
also only talk about timer 0 (tn==0).

Since master clock, comparator, and period can all be set by
the guest, I am picking values:

master clock = 67752 (0x108a8)
comparator = 255252 (0x3e514)
period = 62500 (0xf424)

Using code as of:

commit d2b4c27c0718f27deba00a16317a8ba04c1a2cb7
     xen/arm32: __cmpxchg_mb should be marked always_inline


86:static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn)
87:{
88:    uint64_t comparator;
89:    uint64_t elapsed;
91:    comparator = h->hpet.comparator64[tn];
92:    if ( timer_is_periodic(h, tn) )
93:    {
94:        /* update comparator by number of periods elapsed since last update */
95:        uint64_t period = h->hpet.period[tn];
96:        if (period)
97:        {
98:            elapsed = hpet_read_maincounter(h) + period - 1 - comparator;
99:            comparator += (elapsed / period) * period;
100:            h->hpet.comparator64[tn] = comparator;
101:        }
102:    }
104:    /* truncate if timer is in 32 bit mode */
105:    if ( timer_is_32bit(h, tn) )
106:        comparator = (uint32_t)comparator;
107:    h->hpet.timers[tn].cmp = comparator;
108:    return comparator;
109:}

so on line 98:

elapsed = 67752 + 62500 - 1 - 255252

bc
bc 1.06.95
Copyright 1991-1994, 1997, 1998, 2000, 2004, 2006 Free Software Foundation, Inc.
This is free software with ABSOLUTELY NO WARRANTY.
For details type `warranty'.
67752 + 62500 - 1 - 255252
-125001

Or in hex: 0xFFFFFFFFFFFE17B7

Next:

-125001/62500
-2
-2*62500
-125000


So on line 99 the comparator is changed by -125000.

I.E. the value written is not the value read.

    -Don Slutz

> Jan
>
>