Re: [Xen-devel] [PATCH] x86: reduce redundancy in tsc_[gs]et_info()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.04.14 at 16:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 30/04/14 14:53, Jan Beulich wrote:
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1786,26 +1786,22 @@ void tsc_get_info(struct domain *d, uint
>>  
>>      switch ( *tsc_mode )
>>      {
>> +        uint64_t tsc;
>> +
>>      case TSC_MODE_NEVER_EMULATE:
>> -        *elapsed_nsec =  *gtsc_khz = 0;
>> +        *elapsed_nsec = *gtsc_khz = 0;
>>          break;
>> -    case TSC_MODE_ALWAYS_EMULATE:
>> -        *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
>> -        *gtsc_khz =  d->arch.tsc_khz;
>> -         break;
>>      case TSC_MODE_DEFAULT:
>>          if ( d->arch.vtsc )
>>          {
>> +    case TSC_MODE_ALWAYS_EMULATE:
>>              *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
>>              *gtsc_khz =  d->arch.tsc_khz;
> 
> double space after =

I left them untouched on lines I didn't need to touch anyway, but of
course I can as well clean them up uniformly (there were two or three
more of them).

>> @@ -1815,10 +1811,9 @@ void tsc_get_info(struct domain *d, uint
>>          }
>>          else
>>          {
>> -            uint64_t tsc = 0;
>>              rdtscll(tsc);
>> -            *elapsed_nsec = (scale_delta(tsc,&d->arch.vtsc_to_ns) -
>> -                             d->arch.vtsc_offset);
>> +            *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns) -
>> +                            d->arch.vtsc_offset;
>>              *gtsc_khz = 0; /* ignored by tsc_set_info */
>>          }
>>          break;
> 
> There is a coverity complaint that following this switch statement,
> there is a read of *elapsed_nsec which might be uninitialised if
> tsc_mode missed on of the case statements.  While I believe this is
> currently impossible, it might we wise to have a "default: BUG();" in
> case half a new tsc_mode appears.

I'm not in favor of such constructs when it's sufficiently obvious that
all cases are being handled.

>> @@ -1875,28 +1870,24 @@ void tsc_set_info(struct domain *d,
>>  
>>      switch ( d->arch.tsc_mode = tsc_mode )
>>      {
>> -    case TSC_MODE_NEVER_EMULATE:
>> -        d->arch.vtsc = 0;
>> -        break;
>> -    case TSC_MODE_ALWAYS_EMULATE:
>> -        d->arch.vtsc = 1;
>> -        d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
>> -        d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
>> -        set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
>> -        d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
>> -        break;
>>      case TSC_MODE_DEFAULT:
>> -        d->arch.vtsc = 1;
>> +    case TSC_MODE_ALWAYS_EMULATE:
>>          d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
>> -        d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
>> -        set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
>> -        /* use native TSC if initial host has safe TSC, has not migrated
>> -         * yet and tsc_khz == cpu_khz */
>> -        if ( host_tsc_is_safe() && incarnation == 0 &&
>> -                d->arch.tsc_khz == cpu_khz )
> 
> This wont apply cleanly on top of staging.  Boris made some changed in
> this area with his HVM guest TSC series.

Right - I forgot I need to re-sync before submitting. Will be a v2 with
the above formatting changes also taken care of.

Jan


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