Re: [Xen-devel] [PATCH v2] x86/time: Don't use virtual TSC if host and guest frequencies are equal

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.17 at 16:13, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 03/17/2017 10:56 AM, Jan Beulich wrote:
>>>>> On 17.03.17 at 15:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 03/17/2017 10:24 AM, Jan Beulich wrote:
>>>>>>> On 17.03.17 at 14:36, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> On 03/17/2017 03:48 AM, Jan Beulich wrote:
>>>>>>>>> On 16.03.17 at 20:35, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>>>> --- a/xen/arch/x86/time.c
>>>>>>> +++ b/xen/arch/x86/time.c
>>>>>>> @@ -2051,17 +2051,12 @@ void tsc_set_info(struct domain *d,
>>>>>>>          d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
>>>>>>>          d->arch.tsc_khz = gtsc_khz ?: cpu_khz;
>>>>>>>          set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000);
>>>>>>> -        /*
>>>>>>> -         * In default mode use native TSC if the host has safe TSC and:
>>>>>>> -         *  HVM/PVH: host and guest frequencies are the same (either
>>>>>>> -         *           "naturally" or via TSC scaling)
>>>>>>> -         *  PV: guest has not migrated yet (and thus arch.tsc_khz == 
>>> cpu_khz)
>>>>>>> -         */
>>>>>>> +
>>>>>>> +        ASSERT(incarnation || d->arch.tsc_khz == cpu_khz);
>>>>>> Hmm, is this valid for other than TSC_MODE_DEFAULT?
>>>>> It is valid for all modes but I thought that the ASSERT is really only
>>>>> "interesting" for DEFAULT and ALWAYS_EMULATE since this is when we
>>>>> decide whether or not to set vtsc.
>>>>>
>>>>> Since I need to rebase this anyway (due to PVH1 removal) I can move this
>>>>> down right after the switch if you feel it would be useful.
>>>> Actually I think the other way around: For ALWAYS_EMULATE as
>>>> well as for PVRDTSCP I don't think the assertion is valid, the more
>>>> that d->arch.tsc_khz gets set from input to the function. That last
>>>> fact actually makes the ASSERT() dubious in all cases, I'm afraid.
>>> It is valid (in the sense that it will evaluate to true) because we
>>> always first call tsc_set_info with DEFAULT mode and with gtsc_khz=0
>>> from arch_domain_create(). So d->arch.tsc_khz will be primed to cpu_khz.
>> It is valid for this specific call. A malicious tool stack could easily
>> pass incarnation zero to the domctl together with a random
>> gtsc_khz.
> 
> 
> So how do you want to go about this then? Original (but rebased) patch,
> remove incarnation check and add a comment stating that there is no need
> to check it?

v2 patch with ASSERT() changed to comment, I would say.

Jan


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