Re: [Xen-devel] [PATCH v4 1/3] x86: Use native RDTSC(P) execution when guest and host frequencies are the same

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.04.14 at 17:33, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 04/16/2014 10:37 AM, Jan Beulich wrote:
>>>>> On 16.04.14 at 16:28, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 04/16/2014 07:38 AM, Jan Beulich wrote:
>>>>>>> On 16.04.14 at 03:27, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> @@ -1889,10 +1890,14 @@ void tsc_set_info(struct domain *d,
>>>>>            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 )
>>>>> +        /*
>>>>> +         * Use native TSC if initial host has safe TSC and either has not
>>>>> +         * migrated yet or tsc_khz == cpu_khz (either "naturally" or via
>>>>> +         * TSC scaling)
>>>>> +         */
>>>>> +        if ( host_tsc_is_safe() &&
>>>>> +             (incarnation == 0 || d->arch.tsc_khz == cpu_khz ||
>>>>> +              cpu_has_tsc_ratio) )
>>>> Doesn't this cpu_has_tsc_ratio check also need to be qualified with
>>>> is_pv_domain()? And is the change from && in the old condition to ||
>>>> actually valid for PV guests?
>>> Hmm, I haven't thought about PV here.
>>>
>>> So then the condition should be
>>>
>>> if ( host_tsc_is_safe() )
>>> {
>>>       if ( (is_hvm_domain() && (arch.tsc_khz == cpu_khz || 
>>> cpu_has_tsc_ratio)) 
> ||
>>>             (incarnation == 0 && d->arch.tsc_khz == cpu_khz) )
>>>                 d->arch.vtsc = 0;
>>> }
>> Almost - to include PVH you need to either use !is_pv_domain() or
>> has_hvm_container_domain().
> 
> PVH never makes here, it is forced to use TSC_MODE_NEVER_EMULATE above 
> (see pvhfixme above).

Please sort this out with Mukesh - I would generally have thought that
time handling should be HVM-like for PVH, and was surprised (in the
sense that I didn't recall) to find that fixme comment there when
reviewing your patch.

> PVH need to be looked at anyway. For example, there is a is_hvm_domain() 
> check at the bottom which I suspect needs to be PVH-safe.

Indeed this all looks rather suspicious...

Jan


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