[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |