Re: [Xen-devel] [PATCH v4 2/3] x86/xen/time: setup vcpu 0 time info page

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/27/2017 04:57 PM, Joao Martins wrote:
> On 09/27/2017 09:22 PM, Boris Ostrovsky wrote:
>> On 09/27/2017 11:26 AM, Joao Martins wrote:
>>> On 09/27/2017 03:40 PM, Boris Ostrovsky wrote:
>>>>> +static void xen_setup_vsyscall_time_info(void)
>>>>> +{
>>>>> + struct vcpu_register_time_memory_area t;
>>>>> + struct pvclock_vsyscall_time_info *ti;
>>>>> + struct pvclock_vcpu_time_info *pvti;
>>>>> + int ret;
>>>>> +
>>>>> + pvti = &__this_cpu_read(xen_vcpu)->time;
>>>>> +
>>>>> + /*
>>>>> +  * We check ahead on the primary time info if this
>>>>> +  * bit is supported hence speeding up Xen clocksource.
>>>>> +  */
>>>>> + if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))
>>>>> +         return;
>>>>> +
>>>>> + pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>>>> Is it OK to have this flag set if anything below fails?
>>>>
>>> Yes - if anything below fails it will only affect userspace mapped page.
>> Then should it be set somewhere else, like in xen_time_init()?
>>
> Hm, I could move it if you think it's better - but given the importance of the
> bit we are checking and its direct correlation to whether or not we can setup
> VCLOCK_PVCLOCK then I find it cleaner to have it here in the same routine. One
> thing I failed to mention before is that checking ahead like above, let us 
> also
> avoid allocating a page plus an hypercall to register the pvti just to check 
> the
> one bit of info we need for using VCLOCK_PVCLOCK.
>
> It is very unlikely with current Xen code that 1) the secondary copy register
> below fails, or 2) master and secondary don't have the same bits set. So in 
> case
> you're reconsidering the "shortcut" check above I can move it like we had in 
> v1
> and have pvclock_set_flags right before pvclock_set_pvti_cpu0_va().

I think it would be more logical to move it to the end like in v1.

But can you explain again why this flag should not be set in
xen_time_init()? It seems to me that it would be useful not just for
vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.

-boris


>
>>>  What I
>>> do above is just allowing xen clocksource to use/check that bit 
>>> (consequently
>>> speeding up sched_clock) given the necessary support is there in the master
>>> copy. The secondary copy (i.e. what's being set up below, mapped/used in 
>>> vdso)
>>> has the same data from the master copy, just separate memory regions. The 
>>> checks
>>> below are just for the unlikely cases of failing to register the secondary 
>>> copy
>>> or if its content were to differ from master copy in future releases - and
>>> therefore we handle those more gracefully.
>>>
>>>> (I can see in the changelog that apparently at some point I've asked
>>>> about this at v1 but I can't remember/find what exactly it was)
>>>>
>>>>> +
>>>>> + ti = (struct pvclock_vsyscall_time_info *)get_zeroed_page(GFP_KERNEL);
>>>>> + if (!ti)
>>>>> +         return;
>>>>> +
>>>>> + t.addr.v = &ti->pvti;
>>>>> +
>>>>> + ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>>>>> + if (ret) {
>>>>> +         pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret);
>>>>> +         free_page((unsigned long)ti);
>>>>> +         return;
>>>>> + }
>>>>> +
>>>>> + /*
>>>>> +  * If the check above succedded this one should too since it's the
>>>>> +  * same data on both primary and secondary time infos just different
>>>>> +  * memory regions. But we still check it in case hypervisor is buggy.
>>>>> +  */
>>>>> + pvti = &ti->pvti;
>>>>> + if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) {
>>>>> +         t.addr.v = NULL;
>>>>> +         ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
>>>>> +                                  0, &t);
>>>>> +         if (!ret)
>>>>> +                 free_page((unsigned long)ti);
>>>>> +
>>>>> +         pr_notice("xen: VCLOCK_PVCLOCK not supported (tsc unstable)\n");
>>>>> +         return;
>>>>> + }
>>>>> +
>>>>> + xen_clock = ti;
>>>>> + pvclock_set_pvti_cpu0_va(xen_clock);
>>>>> +
>>>>> + xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK;
>>>>> +}
>>>>> +


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