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

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 09/27/2017 11:44 PM, Boris Ostrovsky wrote:
> 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()?

I didn't say we shouldn't have this flag there - I was just pointing out a
matter of taste on whether to put on xen_time_init() or in
xen_setup_vsyscall_time_info() (which is called from xen_time_init btw) so
there's no functional change.

> It seems to me that it would be useful not just for
> vDSO but for xen_clocksource_read()->pvclock_clocksource_read() as well.

Right - That's what I mentioned by "allowing xen clocksource to use/check that
bit (consequently speeding up sched_clock)". The above chunk is really focused
on enabling the flag on pvclock_clocksource_read().

>>
>>>>  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