[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 04/15/14 12:14, Jan Beulich wrote:
On 15.04.14 at 17:53, <dslutz@xxxxxxxxxxx> wrote:
On 04/15/14 03:05, Jan Beulich wrote:
On 14.04.14 at 21:50, <dslutz@xxxxxxxxxxx> 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




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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.