From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both. Date: Mon, 14 Apr 2014 18:53:34 -0400 Message-ID: <534C66EE.2050303@terremark.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <534C13B6020000780000889D@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/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. > > Jan If I understand this correctly, this leads to the following change (Hopefully not line wrapped): diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c index c3f286f..913beb1 100644 --- 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; + } if ( hpet_enabled(h) && timer_enabled(h, tn) ) set_restart_timer(tn); break; I am still testing that this re-write still does the "right thing". Which is why it is yet to be a v3. This preview is sent to check that I did correctly do the requested change. -Don Slutz