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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.04.14 at 00:53, <dslutz@xxxxxxxxxxx> wrote:
> 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.
> 
> 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


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