Re: [Xen-devel] [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.04.14 at 16:24, <dslutz@xxxxxxxxxxx> wrote:
> +        if ( timer_is_periodic(h, tn) )
> +        {
> +            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;
> +                h->hpet.timers[tn].cmp = new_val;
> +                h->hpet.comparator64[tn] = new_val;
> +            }
> +            else
> +            {
> +                /*
> +                 * Clamp period to reasonable min/max values:
> +                 *  - minimum is 100us, same as timers controlled by vpt.c
> +                 *  - maximum is to prevent overflow in time_after()
> +                 *    calculations
> +                 */
> +                if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
> +                    new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
> +                new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
> +                h->hpet.period[tn] = new_val;
> +            }
> +        }
> +        else
>          {
>              /*
> -             * Clamp period to reasonable min/max values:
> -             *  - minimum is 100us, same as timers controlled by vpt.c
> -             *  - maximum is to prevent overflow in time_after() calculations
> +             * The spec says "do not use", but clear if specified.
>               */
> -            if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
> -                new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
> -            new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
> -            h->hpet.period[tn] = new_val;
> +            if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
> +                h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel