From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled. Date: Tue, 15 Apr 2014 14:11:22 -0400 Message-ID: <534D764A.7010606@terremark.com> References: <1396967094-29484-1-git-send-email-dslutz@verizon.com> <1396967094-29484-7-git-send-email-dslutz@verizon.com> <534C15E902000078000088C4@nat28.tlf.novell.com> <534C3C1A.7090002@terremark.com> <534CF6440200007800008CBF@nat28.tlf.novell.com> <534D560F.4000709@terremark.com> <534D771C02000078000091EE@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <534D771C02000078000091EE@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Don Slutz Cc: Keir Fraser , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 04/15/14 12:14, Jan Beulich wrote: >>>> On 15.04.14 at 17:53, wrote: >> On 04/15/14 03:05, Jan Beulich wrote: >>>>>> On 14.04.14 at 21:50, 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 > >