[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.
On 04/14/14 10:58, Jan Beulich wrote: On 08.04.14 at 16:24, <dslutz@xxxxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |