From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both. Date: Tue, 15 Apr 2014 07:59:58 +0100 Message-ID: <534CF50E0200007800008CAE@nat28.tlf.novell.com> References: <1396967094-29484-1-git-send-email-dslutz@verizon.com> <1396967094-29484-4-git-send-email-dslutz@verizon.com> <534C13B6020000780000889D@nat28.tlf.novell.com> <534C66EE.2050303@terremark.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <534C66EE.2050303@terremark.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Don Slutz Cc: Keir Fraser , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 15.04.14 at 00:53, wrote: > On 04/14/14 10:58, Jan Beulich wrote: >>>>> On 08.04.14 at 16:24, wrote: > >> The if() is clearly unnecessary here, and with it removed I don't see >> any difference left with the inner if() path above. Hence I guess >> they should be folded. Once that's done, new_val as calculated >> before the outer if() isn't needed in the inner "else" path, and >> hence its truncation could be moved inside the if(). Which in turn >> allows you to fix the comparator64 related bug here too: All other >> code stores the non-truncated value there, just the code here >> doesn't. > > If I understand this correctly, this leads to the following change > (Hopefully not line wrapped): Yes, with a minor comment. > --- a/xen/arch/x86/hvm/hpet.c > +++ b/xen/arch/x86/hvm/hpet.c > @@ -404,20 +404,8 @@ static int hpet_write( > case HPET_Tn_CMP(1): > case HPET_Tn_CMP(2): > tn = HPET_TN(CMP, addr); > - if ( timer_is_32bit(h, tn) ) > - new_val = (uint32_t)new_val; > - h->hpet.timers[tn].cmp = new_val; > - if ( h->hpet.timers[tn].config & HPET_TN_SETVAL ) > - /* > - * When SETVAL is one, software is able to "directly set a periodic > - * timer's accumulator." That is, set the comparator without > - * adjusting the period. Much the same as just setting the > - * comparator on an enabled one-shot timer. > - * > - * This configuration bit clears when the comparator is written. > - */ > - h->hpet.timers[tn].config &= ~HPET_TN_SETVAL; > - else if ( timer_is_periodic(h, tn) ) > + if ( timer_is_periodic(h, tn) && > + !(h->hpet.timers[tn].config & HPET_TN_SETVAL) ) > { > /* > * Clamp period to reasonable min/max values: > @@ -429,7 +417,25 @@ static int hpet_write( > new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1; > h->hpet.period[tn] = new_val; > } > - h->hpet.comparator64[tn] = new_val; > + else > + { > + /* > + * When SETVAL is one, software is able to "directly set > + * a periodic timer's accumulator." That is, set the > + * comparator without adjusting the period. Much the > + * same as just setting the comparator on an enabled > + * one-shot timer. > + * > + * This configuration bit clears when the comparator is > + * written. > + */ > + h->hpet.timers[tn].config &= ~HPET_TN_SETVAL; > + h->hpet.comparator64[tn] = new_val; > + if ( timer_is_32bit(h, tn) ) > + h->hpet.timers[tn].cmp = (uint32_t)new_val; > + else > + h->hpet.timers[tn].cmp = new_val; I'd continue to do the truncation on new_val, and write just once (as the old code did). Jan